Tuan - #132
Conversation
These files are auto-generated by 'flutter pub get' and only re-introduce noise diffs (especially CRLF warnings on Windows). Add explicit patterns to .gitignore and remove from index.
Without explicit jniLibs.srcDirs, AGP mergeReleaseNativeLibs only picks up libflutter.so, causing the release APK to crash on launch with 'VM snapshot invalid' / SIGSEGV. Also bump version 1.0.0+1 -> 1.0.0+2. Refs: docs/issues.md (2026-07-14 incident)
Added patterns to ignore keystore and signing properties files to prevent accidental commits of sensitive information.
Added new OAuth client configurations in google-services.json for enhanced authentication support. Updated appId in firebase_options.dart to align with the new client settings.
Google sign-in (FirebaseAuth.VerifyIdTokenAsync) and FCM push notifications were silently failing on production because the firebase-adminsdk.json file was never mounted into the API container. Program.cs has a File.Exists guard that skips FirebaseApp.Create() when the credential path is missing, so no error was surfaced. Changes: - docker-compose.prod.yml: mount the host JSON to /etc/secrets/firebase-adminsdk.json (ro) and pass Firebase__CredentialPath via env (with hardcoded fallback). - backend/scripts/deploy-server.sh: on every deploy, read the FIREBASE_CREDENTIALS_JSON Doppler secret (full JSON body, kept out of git because it holds a private_key) and write it to $APP_DIR/firebase-adminsdk.json with mode 600. Validate JSON parses and contains a private_key before pulling the new image, so a bad secret aborts the deploy without disrupting traffic. Same block added inside perform_rollback() so the rolled-back container also has working Firebase. - docs/issues.md: record the diagnosis and fix attempt log. Tested locally (YAML/shell lint clean). Next: push to Tuan to trigger backend-cd.yml. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughFlutter Android release packaging and Firebase configuration were updated, while production deployment now materializes and mounts validated Firebase credentials, retries image pulls, and filters multiline secrets. Backend CI branch triggers and Docker attestation settings were also changed. ChangesMobile release configuration
Production Firebase credential delivery
Backend CI workflow settings
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Doppler
participant DeployScript
participant Compose
participant API
Doppler->>DeployScript: Provide Firebase credentials
DeployScript->>DeployScript: Validate and write credential file
DeployScript->>Compose: Start production service
Compose->>API: Mount file and set credential path
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/lib/firebase_options.dart (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid manually editing generated files.
firebase_options.dartis typically generated by theflutterfire configureCLI tool. Any manual changes made here, such as customizing theUnsupportedErrormessage, will be overwritten the next time the tool is run.If you need a custom message or behavior, consider wrapping the initialization logic in a separate configuration class or service rather than modifying this generated file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/lib/firebase_options.dart` around lines 17 - 19, Revert the manual customization in the generated firebase_options.dart file, including the localized UnsupportedError message, and restore the output expected from flutterfire configure. If custom initialization behavior is required, implement it in a separate configuration class or service rather than modifying the generated file.docker-compose.prod.yml (1)
29-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a relative path for the host volume mount.
Hardcoding the absolute host path
/home/ubuntu/apps/menugreen/firebase-adminsdk.jsoncouples thedocker-compose.prod.ymlfile to a specific server directory structure and username.Since
deploy-server.shplaces the file in$APP_DIRalongside the compose file, you can use a relative path. This ensures the compose file remains portable and behaves correctly regardless of the server's absolute directory path.🛠 Proposed fix
volumes: # Firebase Admin SDK credentials — read-only mount from the host. # File is materialized at deploy time by deploy-server.sh from the # Doppler secret FIREBASE_CREDENTIALS_JSON, so it's never stored in git. # Without this mount, FirebaseAuth.VerifyIdTokenAsync (Google sign-in) # and FirebaseMessaging.SendAsync (FCM push notifications) silently # fail because FirebaseApp.DefaultInstance == null. - - /home/ubuntu/apps/menugreen/firebase-adminsdk.json:/etc/secrets/firebase-adminsdk.json:ro + - ./firebase-adminsdk.json:/etc/secrets/firebase-adminsdk.json:ro🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.prod.yml` around lines 29 - 36, Update the Firebase Admin SDK volume entry in the compose service to use the credential file’s relative path alongside the compose file instead of the hardcoded /home/ubuntu/apps/menugreen host path, while preserving the container destination and read-only mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/scripts/deploy-server.sh`:
- Around line 196-198: Update both Firebase credential materialization sites in
backend/scripts/deploy-server.sh at lines 196-198 and 548-550: replace echo with
printf '%s\n' to preserve JSON and private_key formatting, and change the
root-owned 600 permissions to a mode or ownership that allows the non-root .NET
container user to read the file.
---
Nitpick comments:
In `@docker-compose.prod.yml`:
- Around line 29-36: Update the Firebase Admin SDK volume entry in the compose
service to use the credential file’s relative path alongside the compose file
instead of the hardcoded /home/ubuntu/apps/menugreen host path, while preserving
the container destination and read-only mode.
In `@frontend/lib/firebase_options.dart`:
- Around line 17-19: Revert the manual customization in the generated
firebase_options.dart file, including the localized UnsupportedError message,
and restore the output expected from flutterfire configure. If custom
initialization behavior is required, implement it in a separate configuration
class or service rather than modifying the generated file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef97497d-5705-4576-a226-50c526cac93a
📒 Files selected for processing (15)
.gitignorebackend/scripts/deploy-server.shdocker-compose.prod.ymldocs/issues.mdfrontend/.gitignorefrontend/android/app/build.gradle.ktsfrontend/android/app/google-services.jsonfrontend/lib/firebase_options.dartfrontend/linux/flutter/generated_plugin_registrant.ccfrontend/linux/flutter/generated_plugin_registrant.hfrontend/linux/flutter/generated_plugins.cmakefrontend/pubspec.yamlfrontend/windows/flutter/generated_plugin_registrant.ccfrontend/windows/flutter/generated_plugin_registrant.hfrontend/windows/flutter/generated_plugins.cmake
💤 Files with no reviewable changes (6)
- frontend/windows/flutter/generated_plugin_registrant.h
- frontend/linux/flutter/generated_plugins.cmake
- frontend/linux/flutter/generated_plugin_registrant.h
- frontend/windows/flutter/generated_plugin_registrant.cc
- frontend/windows/flutter/generated_plugins.cmake
- frontend/linux/flutter/generated_plugin_registrant.cc
…o 'main' and 'Tuan'
The previous deploy died at docker compose up with: failed to read .env: line 22: unexpected character "}" in variable name "}=" Root cause: the while-read loop that copies Doppler secrets into $APP_DIR/.env has no skip rule for FIREBASE_CREDENTIALS_JSON, so each continuation line of the multi-line JSON (spaces, commas, the closing '}') gets echoed verbatim into .env. Docker compose then refuses to parse the file and never starts the container. Service went DOWN because the rollback path also had no local rollback tag (cold deploy) and no :previous / :main-<oldsha> image on Hub — script exited 1. Fix: skip FIREBASE_CREDENTIALS_JSON= in both the main path and the rollback path of the .env loop. The Firebase JSON is handled by the materialize block above (echo > $APP_DIR/firebase-adminsdk.json), not via env var — the .NET SDK only accepts a file path. This commit alone should bring the service back up because the image on Hub still contains the previously working code; the only broken thing was the new deploy script. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/backend-cd.yml:
- Around line 58-66: Prevent command injection in the Resolve SHA step by
mapping inputs.sha and github.event.workflow_run.head_sha to environment
variables, using those variables in the shell, and using the built-in GITHUB_SHA
variable for the fallback. In .github/workflows/backend-cd.yml lines 84-85, also
map steps.resolve_sha.outputs.SHA to an environment variable before echoing it
to GITHUB_ENV.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e2a683a9-3fc3-4baa-9f0e-7797a84ed679
📒 Files selected for processing (3)
.github/workflows/backend-cd.yml.github/workflows/backend-ci.ymlbackend/scripts/deploy-server.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/scripts/deploy-server.sh
| run: | | ||
| if [ "${{ github.event_name }}" = "workflow_dispatch" ] \ | ||
| && [ -n "${{ inputs.sha }}" ]; then | ||
| echo "SHA=${{ inputs.sha }}" >> "$GITHUB_OUTPUT" | ||
| elif [ "${{ github.event_name }}" = "workflow_run" ]; then | ||
| echo "SHA=${{ github.event.workflow_run.head_sha }}" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "SHA=${{ github.sha }}" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent command injection by using environment variables.
Using GitHub Actions ${{ ... }} expression syntax inside a run block allows template injection. If an attacker triggers the workflow_dispatch event with a specially crafted sha input (e.g., "; malicious command; echo "), or if upstream outputs contain malicious characters, it could result in arbitrary code execution on the runner.
Instead of directly interpolating these values into the script, pass them safely as environment variables.
.github/workflows/backend-cd.yml#L58-L66: Mapinputs.shaandgithub.event.workflow_run.head_shato environment variables, and use the built-in$GITHUB_SHAfor the fallback..github/workflows/backend-cd.yml#L84-L85: Mapsteps.resolve_sha.outputs.SHAto an environment variable before echoing it to$GITHUB_ENV.
🔒️ Proposed fixes
For the Resolve SHA step:
- run: |
- if [ "${{ github.event_name }}" = "workflow_dispatch" ] \
- && [ -n "${{ inputs.sha }}" ]; then
- echo "SHA=${{ inputs.sha }}" >> "$GITHUB_OUTPUT"
- elif [ "${{ github.event_name }}" = "workflow_run" ]; then
- echo "SHA=${{ github.event.workflow_run.head_sha }}" >> "$GITHUB_OUTPUT"
- else
- echo "SHA=${{ github.sha }}" >> "$GITHUB_OUTPUT"
- fi
+ env:
+ INPUT_SHA: ${{ inputs.sha }}
+ RUN_SHA: ${{ github.event.workflow_run.head_sha }}
+ run: |
+ if [ "${{ github.event_name }}" = "workflow_dispatch" ] \
+ && [ -n "$INPUT_SHA" ]; then
+ echo "SHA=$INPUT_SHA" >> "$GITHUB_OUTPUT"
+ elif [ "${{ github.event_name }}" = "workflow_run" ]; then
+ echo "SHA=$RUN_SHA" >> "$GITHUB_OUTPUT"
+ else
+ echo "SHA=$GITHUB_SHA" >> "$GITHUB_OUTPUT"
+ fiFor the Export SHA to environment step:
- name: Export SHA to environment
- run: echo "SHA=${{ steps.resolve_sha.outputs.SHA }}" >> "$GITHUB_ENV"
+ env:
+ RESOLVED_SHA: ${{ steps.resolve_sha.outputs.SHA }}
+ run: echo "SHA=$RESOLVED_SHA" >> "$GITHUB_ENV"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| if [ "${{ github.event_name }}" = "workflow_dispatch" ] \ | |
| && [ -n "${{ inputs.sha }}" ]; then | |
| echo "SHA=${{ inputs.sha }}" >> "$GITHUB_OUTPUT" | |
| elif [ "${{ github.event_name }}" = "workflow_run" ]; then | |
| echo "SHA=${{ github.event.workflow_run.head_sha }}" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "SHA=${{ github.sha }}" >> "$GITHUB_OUTPUT" | |
| fi | |
| env: | |
| INPUT_SHA: ${{ inputs.sha }} | |
| RUN_SHA: ${{ github.event.workflow_run.head_sha }} | |
| run: | | |
| if [ "${{ github.event_name }}" = "workflow_dispatch" ] \ | |
| && [ -n "$INPUT_SHA" ]; then | |
| echo "SHA=$INPUT_SHA" >> "$GITHUB_OUTPUT" | |
| elif [ "${{ github.event_name }}" = "workflow_run" ]; then | |
| echo "SHA=$RUN_SHA" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "SHA=$GITHUB_SHA" >> "$GITHUB_OUTPUT" | |
| fi |
🧰 Tools
🪛 zizmor (1.26.1)
[error] 60-60: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 61-61: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 63-63: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 1 file
.github/workflows/backend-cd.yml#L58-L66(this comment).github/workflows/backend-cd.yml#L84-L85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/backend-cd.yml around lines 58 - 66, Prevent command
injection in the Resolve SHA step by mapping inputs.sha and
github.event.workflow_run.head_sha to environment variables, using those
variables in the shell, and using the built-in GITHUB_SHA variable for the
fallback. In .github/workflows/backend-cd.yml lines 84-85, also map
steps.resolve_sha.outputs.SHA to an environment variable before echoing it to
GITHUB_ENV.
Source: Linters/SAST tools
The previous deploy died at:
>>> FATAL: firebase-adminsdk.json is missing or invalid
>>> (no private_key field).
Root cause: Doppler --format env exports the JSON as a multi-line
secret. The materialize block used:
FIREBASE_JSON="$(grep '^FIREBASE_CREDENTIALS_JSON=' /tmp/doppler_raw.env | cut -d= -f2-)"
`cut -d= -f2-` only keeps the part after the first '=' on the
matching line — i.e. just the leading "{". The file written to
$APP_DIR/firebase-adminsdk.json was therefore "{", which the
python sanity check correctly rejected as missing private_key.
Fix: use python to capture the multi-line value between the
FIREBASE_CREDENTIALS_JSON= header and the next KEY= line (or EOF).
Applied to both the main materialize block and the rollback block
so a future rollback still has a valid Firebase JSON.
This unblocks the deploy pipeline — once pushed to main, CD will
re-run with this commit, materialize the full JSON, start the
container, and (hopefully) pass the health check.
Co-authored-by: Cursor <cursoragent@cursor.com>
The previous python-regex fix (commit 62afaea) didn't work because Doppler --format env emits multi-line values like this: FIREBASE_CREDENTIALS_JSON="{ \"type\": \"service_account\",\n ... }" — quoted, with \" escaping inner quotes and REAL newlines inside the quoted block. A regex that grabs everything between KEY=" and the next KEY= line captures the closing \" too, leaving the surrounding double-quotes in place; json.loads then bails with 'Expecting value: line 1 column 2 (char 1)'. Approach: walk the file line-by-line starting at the header line, collect subsequent lines, and stop when the cumulative unescaped quote count is even (i.e. we've reached the closing quote of the value). Strip the surrounding double-quotes and unescape \" → ". Verified locally against the real firebase-adminsdk.json from the repo (private_key contains a multi-line PEM block). Output JSON parses cleanly and contains the real newlines that FirebaseAdmin needs in the PEM. Co-authored-by: Cursor <cursoragent@cursor.com>
…ilure The deploy pipeline reached 'Pull latest image' for the first time (commit 0054c33 made the Firebase JSON materialize step succeed), but then died with: failed commit on ref 'attestation-sha256:...': rename /var/lib/containerd/io.containerd.content.v1.content/ingest/... /var/lib/containerd/io.containerd.content.v1.content/blobs/... no such file or directory Failed to pull image ***/menugreensystem:main Root cause: Docker Buildx v6 pushes provenance + SBOM attestations by default. On the deploy host, `docker pull` then commits those attestation manifests into containerd's blob store, and the rename fails because the ingest directory gets cleared by `docker image prune` running earlier in the deploy (step 6). Two changes: 1. .github/workflows/backend-ci.yml: disable provenance and sbom in the build-push-action. We don't use these attestations downstream (no cosign verify, no policy check), so turning them off keeps the next `docker pull` clean. 2. backend/scripts/deploy-server.sh: wrap the pull in a 3-attempt retry with a 5s delay between attempts, and if those fail restart containerd/docker once and retry a final time. This makes the pipeline self-heal if a stale attestation from an older CI build is still sitting in the registry. Co-authored-by: Cursor <cursoragent@cursor.com>
The deploy reached 'Start API container' for the first time (commit 4420084 made Docker pull work), then died with: failed to read /home/***/apps/menugreen/.env: line 22: unexpected character '}' in variable name '}='=' Root cause: the .env loop used: [[ $key =~ ^(FIREBASE_CREDENTIALS_JSON=) ]] && continue That only matches the very first line of the multi-line value ('FIREBASE_CREDENTIALS_JSON="{'). All continuation lines (' "type": ...' or the closing '}"') still fell through with their first 'word' becoming $key. Once IFS='=' split the closing '}"' line, key was literally '}', raw_value was empty, and the loop wrote '= ' into .env — which Docker compose rejects with the error above. Fix: instead of blacklisting a single key, whitelist what a real env variable name looks like — anything that isn't an ALL-CAPS identifier (continuation of a quoted multi-line value, blank line, comment, etc.) is dropped. That catches both FIREBASE_CREDENTIALS_JSON AND any future multi-line secret we might add to Doppler without us having to remember to blacklist each one. Verified locally with a mocked Doppler env containing a multi-line JSON secret — generated .env contains exactly the expected 8 keys, no JSON fragments leaked through, LIGHTSAIL_SSH_KEY still filtered. Co-authored-by: Cursor <cursoragent@cursor.com>
…ebase JSON The deploy reached 'Container menugreen_api Created/Started' for the first time (commit ae7521f fixed .env parsing), but the container then crash-looped with: System.ArgumentException: PKCS8 data must be contained within '-----BEGIN PRIVATE KEY-----' and '-----END PRIVATE KEY-----'. at Google.Apis.Auth.OAuth2.Pkcs8.DecodeRsaParameters at Program.<Main>$ ... in MenuGreen.API/Program.cs:line 35 (repeating ~6 times — one per health-check interval during the 30-attempt wait window — until the deploy timed out.) Root cause: the Doppler --format env extractor in deploy-server.sh captures the FIREBASE_CREDENTIALS_JSON value but does NOT normalise the embedded newline encoding of the PEM. Whether Doppler stores the private_key field with real newlines (\n as one char) or literal \n (two chars: backslash + n) depends on how the secret was ingested originaly, and either form will pass a naive json.loads sanity check (string 'starts with -----BEGIN'). But GoogleCredential.FromFile() hands the private_key string to RSA crypto, which expects real newlines between the BEGIN line and the END line. If the file has the literal \n encoding, the PEM body smashes into one giant line and Pkcs8.DecodeRsaParameters throws the ArgumentException above. Fix: 1. Strengthen the sanity check: json.load() the file, find the BEGIN and END markers in private_key, and require a real '\n' between them. If absent, abort the deploy with a clear error. 2. Re-serialise the parsed JSON with json.dump(indent=2) so the on-disk file is canonical and the PEM is always human-readable (one line at a time). This catches all 3 broken variants (literal \n, no newline at all, missing field) at deploy time, and fixes the most common one (literal \n -> real \n) automatically by writing the JSON back from the parsed dict. Verified locally with a 4-case test (real \n PASS, literal \n REJECTED, no-newline REJECTED, missing-field REJECTED). Co-authored-by: Cursor <cursoragent@cursor.com>
The previous fix (3546f1a) used: sudo python3 - <<PYEOF ... PYEOF But that broke at runtime: /tmp/.../deploy-server.sh: line 248: pk: command not found File '<stdin>', line 1 import json, sys IndentationError: unexpected indent Two compounding bugs: 1. The Doppler env-extraction earlier in the same script set pk as a bare shell variable. Even though that block ended, the trailing content of the previous python3 -c heredoc was bleeding back into the script context, so bash was treating 'pk:' as a command. 2. sudo python3 - (stdin-based) is unreliable across sudo versions — some builds drop stdin entirely, some leave the heredoc un-delimited, and the leading 6-space indent on each line gets passed as-is to python3 -c which then chokes with IndentationError. Fix: write the Python script to /tmp/firebase_pem_check.py via bash heredoc (UNQUOTED, so $APP_DIR still expands), then run it via 'sudo python3 /tmp/firebase_pem_check.py'. That has no stdin ambiguity, no shell-globals bleed-through, and the script can be debugged on disk if anything goes wrong in the future. Also corrected the heredoc tag: 'PYSCRIPT' (quoted) would skip variable expansion and leave a literal '$APP_DIR' in the Python script — now bare PYSCRIPT so $APP_DIR expands to /home/<user>/apps/menugreen as expected. Verified locally with 4-case smoke test (real \n PASS, literal \n REJECTED, no-newline REJECTED, missing-field REJECTED). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/backend-ci.yml (1)
5-5: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep CI and CD branch filters aligned. Removing
Tuanxhere stopsBackend CI - Build & Pushfrom completing on that branch, while.github/workflows/backend-cd.ymlstill listens forworkflow_runevents fromTuanx. Either keepTuanxhere or remove it from CD if the branch is retired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml at line 5, Align the branch filters between the backend CI workflow and backend CD workflow: either restore Tuanx in the CI branches list so its build completes, or remove Tuanx from the CD workflow if the branch is retired. Ensure both workflows use the same branch set.
🧹 Nitpick comments (2)
.github/workflows/backend-ci.yml (1)
116-127: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftPreserve supply-chain metadata or document the security trade-off.
provenance: falseandsbom: falseremove provenance and SBOM attestations from every pushed image. Confirm that no vulnerability scanner, registry policy, or release process depends on them; preferably fix the deploy-host pull failure or publish these attestations through a separate step.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/backend-ci.yml around lines 116 - 127, Revisit the Docker Buildx configuration around provenance and sbom in the workflow: preserve these attestations by fixing the deploy-host pull failure, or document and validate that scanners, registry policies, and release processes do not depend on them before disabling them. If they must remain disabled, add a separate attestation publishing step and record the security trade-off in the workflow documentation.backend/scripts/deploy-server.sh (1)
200-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated multi-line JSON extraction logic between deploy and rollback paths.
This exact python heredoc extractor (and the paired ALL-CAPS whitelist regex at Line 325) is copy-pasted verbatim into
perform_rollbackat Lines 683-705/726. This duplication is precisely the kind of drift risk that already produced the earlier bug where a fix landed on the main path but not the rollback path. Extracting this into a single shared helper (a small script file or bash function parameterized by the input env path) would prevent the two copies from diverging again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/scripts/deploy-server.sh` around lines 200 - 221, The multi-line FIREBASE_CREDENTIALS_JSON extraction and ALL-CAPS whitelist logic is duplicated between deployment and perform_rollback, allowing the paths to diverge. Extract both into one shared helper, script, or parameterized Bash function that accepts the input environment path, then have both paths reuse it while preserving their current extraction and filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/scripts/deploy-server.sh`:
- Around line 513-517: Remove the host-wide containerd/docker restart from the
PULL_OK recovery branch in deploy-server.sh. Scope recovery to this deployment
only, such as clearing the stale image metadata or failing the pull so CI can
retry, while preserving the final failure handling for unsuccessful docker
pulls.
- Around line 257-296: Replace the predictable /tmp/firebase_pem_check.py path
in the PEM validation block with a securely created temporary file using mktemp,
and execute that generated path with sudo. Add trap-based cleanup immediately
after creation so the temporary script is removed on both successful and failed
validation, eliminating the existing rm-based cleanup dependency; preserve the
current validation behavior and failure messages.
- Around line 504-518: Ensure the pull retry condition around the docker pull
pipeline reflects the docker command’s exit status rather than tail’s status.
Enable pipefail before the loop or explicitly inspect the pull command status
via PIPESTATUS, so failed pulls trigger retries and the existing containerd
restart fallback.
---
Outside diff comments:
In @.github/workflows/backend-ci.yml:
- Line 5: Align the branch filters between the backend CI workflow and backend
CD workflow: either restore Tuanx in the CI branches list so its build
completes, or remove Tuanx from the CD workflow if the branch is retired. Ensure
both workflows use the same branch set.
---
Nitpick comments:
In @.github/workflows/backend-ci.yml:
- Around line 116-127: Revisit the Docker Buildx configuration around provenance
and sbom in the workflow: preserve these attestations by fixing the deploy-host
pull failure, or document and validate that scanners, registry policies, and
release processes do not depend on them before disabling them. If they must
remain disabled, add a separate attestation publishing step and record the
security trade-off in the workflow documentation.
In `@backend/scripts/deploy-server.sh`:
- Around line 200-221: The multi-line FIREBASE_CREDENTIALS_JSON extraction and
ALL-CAPS whitelist logic is duplicated between deployment and perform_rollback,
allowing the paths to diverge. Extract both into one shared helper, script, or
parameterized Bash function that accepts the input environment path, then have
both paths reuse it while preserving their current extraction and filtering
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0464ea35-6ce8-43eb-a10c-cd929f003a81
📒 Files selected for processing (2)
.github/workflows/backend-ci.ymlbackend/scripts/deploy-server.sh
| sudo tee /tmp/firebase_pem_check.py > /dev/null <<PYSCRIPT | ||
| import json, sys | ||
| path = "$APP_DIR/firebase-adminsdk.json" | ||
| try: | ||
| with open(path) as f: | ||
| data = json.load(f) | ||
| except Exception as e: | ||
| sys.stderr.write("failed to parse JSON: %s\n" % e) | ||
| sys.exit(2) | ||
| pk = data.get("private_key", "") | ||
| if "-----BEGIN" not in pk or "-----END" not in pk: | ||
| sys.stderr.write("FATAL: private_key is missing BEGIN/END markers\n") | ||
| sys.exit(3) | ||
| if "-----BEGIN PRIVATE KEY-----" not in pk: | ||
| sys.stderr.write("FATAL: private_key does not contain '-----BEGIN PRIVATE KEY-----'\n") | ||
| sys.exit(4) | ||
| # Verify real newlines sit between the BEGIN/END markers — otherwise | ||
| # GoogleCredential will crash with the exact same ArgumentException. | ||
| begin_idx = pk.index("-----BEGIN PRIVATE KEY-----") | ||
| end_idx = pk.index("-----END PRIVATE KEY-----") | ||
| if begin_idx >= end_idx: | ||
| sys.stderr.write("FATAL: BEGIN marker must appear before END marker in private_key\n") | ||
| sys.exit(5) | ||
| body = pk[begin_idx:end_idx] | ||
| if "\n" not in body: | ||
| sys.stderr.write( | ||
| "FATAL: private_key PEM has no real newlines between BEGIN and END - " | ||
| "GoogleCredential needs real \\n in the PEM body, not literal \\\\n.\n" | ||
| ) | ||
| sys.exit(6) | ||
| # Re-serialize canonically so on-disk JSON is consistent and PEM is human-readable. | ||
| with open(path, "w") as f: | ||
| json.dump(data, f, indent=2) | ||
| PYSCRIPT | ||
| if ! sudo python3 /tmp/firebase_pem_check.py; then | ||
| echo ">>> FATAL: $APP_DIR/firebase-adminsdk.json failed sanity check (exit $?)" | ||
| echo ">>> Aborting deploy before pulling new image." | ||
| exit 1 | ||
| fi | ||
| rm -f /tmp/firebase_pem_check.py |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Predictable /tmp path for the sudo-executed PEM check script.
/tmp/firebase_pem_check.py is a hardcoded, predictable path, written via sudo tee and then executed via sudo python3 — a classic TOCTOU/symlink hazard (flagged by static analysis, CWE-377). A local actor able to pre-create that path (e.g. as a symlink) before this deploy step runs could hijack or redirect the write, or have malicious content executed with root privileges. Also note rm -f at Line 296 is skipped if the check fails and the script exits at Line 294, leaving the file behind.
🛡️ Proposed fix using mktemp + trap cleanup
-sudo tee /tmp/firebase_pem_check.py > /dev/null <<PYSCRIPT
+PEM_CHECK_SCRIPT="$(mktemp /tmp/firebase_pem_check.XXXXXX.py)"
+trap 'rm -f "$PEM_CHECK_SCRIPT"' RETURN
+sudo tee "$PEM_CHECK_SCRIPT" > /dev/null <<PYSCRIPT
import json, sys
...
PYSCRIPT
-if ! sudo python3 /tmp/firebase_pem_check.py; then
+if ! sudo python3 "$PEM_CHECK_SCRIPT"; then
echo ">>> FATAL: $APP_DIR/firebase-adminsdk.json failed sanity check (exit $?)"
echo ">>> Aborting deploy before pulling new image."
exit 1
fi
-rm -f /tmp/firebase_pem_check.py📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo tee /tmp/firebase_pem_check.py > /dev/null <<PYSCRIPT | |
| import json, sys | |
| path = "$APP_DIR/firebase-adminsdk.json" | |
| try: | |
| with open(path) as f: | |
| data = json.load(f) | |
| except Exception as e: | |
| sys.stderr.write("failed to parse JSON: %s\n" % e) | |
| sys.exit(2) | |
| pk = data.get("private_key", "") | |
| if "-----BEGIN" not in pk or "-----END" not in pk: | |
| sys.stderr.write("FATAL: private_key is missing BEGIN/END markers\n") | |
| sys.exit(3) | |
| if "-----BEGIN PRIVATE KEY-----" not in pk: | |
| sys.stderr.write("FATAL: private_key does not contain '-----BEGIN PRIVATE KEY-----'\n") | |
| sys.exit(4) | |
| # Verify real newlines sit between the BEGIN/END markers — otherwise | |
| # GoogleCredential will crash with the exact same ArgumentException. | |
| begin_idx = pk.index("-----BEGIN PRIVATE KEY-----") | |
| end_idx = pk.index("-----END PRIVATE KEY-----") | |
| if begin_idx >= end_idx: | |
| sys.stderr.write("FATAL: BEGIN marker must appear before END marker in private_key\n") | |
| sys.exit(5) | |
| body = pk[begin_idx:end_idx] | |
| if "\n" not in body: | |
| sys.stderr.write( | |
| "FATAL: private_key PEM has no real newlines between BEGIN and END - " | |
| "GoogleCredential needs real \\n in the PEM body, not literal \\\\n.\n" | |
| ) | |
| sys.exit(6) | |
| # Re-serialize canonically so on-disk JSON is consistent and PEM is human-readable. | |
| with open(path, "w") as f: | |
| json.dump(data, f, indent=2) | |
| PYSCRIPT | |
| if ! sudo python3 /tmp/firebase_pem_check.py; then | |
| echo ">>> FATAL: $APP_DIR/firebase-adminsdk.json failed sanity check (exit $?)" | |
| echo ">>> Aborting deploy before pulling new image." | |
| exit 1 | |
| fi | |
| rm -f /tmp/firebase_pem_check.py | |
| PEM_CHECK_SCRIPT="$(mktemp /tmp/firebase_pem_check.XXXXXX.py)" | |
| trap 'rm -f "$PEM_CHECK_SCRIPT"' RETURN | |
| sudo tee "$PEM_CHECK_SCRIPT" > /dev/null <<PYSCRIPT | |
| import json, sys | |
| path = "$APP_DIR/firebase-adminsdk.json" | |
| try: | |
| with open(path) as f: | |
| data = json.load(f) | |
| except Exception as e: | |
| sys.stderr.write("failed to parse JSON: %s\n" % e) | |
| sys.exit(2) | |
| pk = data.get("private_key", "") | |
| if "-----BEGIN" not in pk or "-----END" not in pk: | |
| sys.stderr.write("FATAL: private_key is missing BEGIN/END markers\n") | |
| sys.exit(3) | |
| if "-----BEGIN PRIVATE KEY-----" not in pk: | |
| sys.stderr.write("FATAL: private_key does not contain '-----BEGIN PRIVATE KEY-----'\n") | |
| sys.exit(4) | |
| # Verify real newlines sit between the BEGIN/END markers — otherwise | |
| # GoogleCredential will crash with the exact same ArgumentException. | |
| begin_idx = pk.index("-----BEGIN PRIVATE KEY-----") | |
| end_idx = pk.index("-----END PRIVATE KEY-----") | |
| if begin_idx >= end_idx: | |
| sys.stderr.write("FATAL: BEGIN marker must appear before END marker in private_key\n") | |
| sys.exit(5) | |
| body = pk[begin_idx:end_idx] | |
| if "\n" not in body: | |
| sys.stderr.write( | |
| "FATAL: private_key PEM has no real newlines between BEGIN and END - " | |
| "GoogleCredential needs real \\n in the PEM body, not literal \\\\n.\n" | |
| ) | |
| sys.exit(6) | |
| # Re-serialize canonically so on-disk JSON is consistent and PEM is human-readable. | |
| with open(path, "w") as f: | |
| json.dump(data, f, indent=2) | |
| PYSCRIPT | |
| if ! sudo python3 "$PEM_CHECK_SCRIPT"; then | |
| echo ">>> FATAL: $APP_DIR/firebase-adminsdk.json failed sanity check (exit $?)" | |
| echo ">>> Aborting deploy before pulling new image." | |
| exit 1 | |
| fi |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 290-290: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/firebase_pem_check.py
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 295-295: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/firebase_pem_check.py
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🪛 Betterleaks (1.6.1)
[high] 270-271: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/deploy-server.sh` around lines 257 - 296, Replace the
predictable /tmp/firebase_pem_check.py path in the PEM validation block with a
securely created temporary file using mktemp, and execute that generated path
with sudo. Add trap-based cleanup immediately after creation so the temporary
script is removed on both successful and failed validation, eliminating the
existing rm-based cleanup dependency; preserve the current validation behavior
and failure messages.
Source: Linters/SAST tools
| PULL_OK=0 | ||
| for attempt in 1 2 3; do | ||
| if sudo docker pull "$IMAGE:main" 2>&1 | tail -5; then | ||
| PULL_OK=1 | ||
| break | ||
| fi | ||
| echo " ! Pull attempt $attempt failed, retrying in 5s..." | ||
| sleep 5 | ||
| done | ||
| if [ "$PULL_OK" = "0" ]; then | ||
| echo " ! Pull still failing after 3 attempts — restarting containerd and trying once more" | ||
| sudo systemctl restart containerd 2>/dev/null || sudo systemctl restart docker 2>/dev/null || true | ||
| sleep 5 | ||
| sudo docker pull "$IMAGE:main" || { echo "Failed to pull image $IMAGE:main"; exit 1; } | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
grep -n 'set -e\|pipefail' backend/scripts/deploy-server.sh | head -20Repository: EXE201-MenuGreen/MenuGreenSystem
Length of output: 396
🏁 Script executed:
#!/bin/bash
set -e
printf '\n[Top of backend/scripts/deploy-server.sh]\n'
sed -n '1,30p' backend/scripts/deploy-server.sh | cat -n
printf '\n[Retry block around lines 504-518]\n'
sed -n '504,518p' backend/scripts/deploy-server.sh | cat -n
printf '\n[Search for pipefail]\n'
grep -n 'pipefail' backend/scripts/deploy-server.sh || trueRepository: EXE201-MenuGreen/MenuGreenSystem
Length of output: 2600
🏁 Script executed:
#!/bin/bash
bash -lc '
set -e
false | tail -5
echo "pipeline_status=$?"
if false | tail -5; then echo IF_OK; else echo IF_FAIL; fi
'Repository: EXE201-MenuGreen/MenuGreenSystem
Length of output: 194
Retry loop needs pipefail or explicit pull status handling. With only set -e, if sudo docker pull ... | tail -5; then tests tail’s exit code, so a failed pull can still set PULL_OK=1 and skip the retry/fallback path. Add set -o pipefail or check ${PIPESTATUS[0]} directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/deploy-server.sh` around lines 504 - 518, Ensure the pull
retry condition around the docker pull pipeline reflects the docker command’s
exit status rather than tail’s status. Enable pipefail before the loop or
explicitly inspect the pull command status via PIPESTATUS, so failed pulls
trigger retries and the existing containerd restart fallback.
| if [ "$PULL_OK" = "0" ]; then | ||
| echo " ! Pull still failing after 3 attempts — restarting containerd and trying once more" | ||
| sudo systemctl restart containerd 2>/dev/null || sudo systemctl restart docker 2>/dev/null || true | ||
| sleep 5 | ||
| sudo docker pull "$IMAGE:main" || { echo "Failed to pull image $IMAGE:main"; exit 1; } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Restarting containerd/docker on pull failure can take down unrelated containers on the host.
sudo systemctl restart containerd 2>/dev/null || sudo systemctl restart docker 2>/dev/null || true restarts the shared container runtime daemon. Restarting dockerd without live-restore enabled typically stops running containers host-wide, and even a containerd restart can disrupt other workloads sharing this host — turning a single image-pull hiccup into a broader outage for services unrelated to this deploy.
Consider scoping recovery to this deploy only (e.g., clear the stale attestation blob under the containerd content store, or just fail fast and let CI retry) rather than restarting the daemon that every other container on the box depends on.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/scripts/deploy-server.sh` around lines 513 - 517, Remove the
host-wide containerd/docker restart from the PULL_OK recovery branch in
deploy-server.sh. Scope recovery to this deployment only, such as clearing the
stale image metadata or failing the pull so CI can retry, while preserving the
final failure handling for unsuccessful docker pulls.
Deploy reached the Firebase sanity-check (commit 20ebdce) which correctly rejected the on-disk PEM with: FATAL: private_key PEM has no real newlines between BEGIN and END - GoogleCredential needs real \n in the PEM body, not literal \n. Root cause: the multi-line FIREBASE_CREDENTIALS_JSON secret was extracted from Doppler's bulk '--format env' download. Doppler wraps multi-line values in double quotes, escapes embedded double quotes as backslash-quote, and (depending on Doppler CLI version) either preserves or escapes newlines. The Python extractor in deploy-server.sh attempted to un-escape double-quotes but had no robust handling for newlines - any line that lost its real newlines (or kept the literal backslash-n 2-char sequence) ended up as a one-line PEM body. The sanity check's 'no \n between BEGIN and END' assertion caught this and aborted the deploy. Per Doppler docs (https://docs.doppler.com/docs/accessing-secrets#mounting), the recommended way to fetch a multi-line secret for a file-on-disk use case is: doppler secrets get <NAME> --plain > /etc/path/to/secret This emits the raw secret value byte-for-byte, with NO quote-wrapping and NO escape sequences. If the secret was ingested as a raw JSON file (which Firebase Admin SDK expects), the output IS the JSON file content - exactly what GoogleCredential.FromFile wants to read. Fix: replace the in-script Python extractor with a single 'doppler secrets get FIREBASE_CREDENTIALS_JSON --plain' call. The sanity check (json.load + PEM newline verification + canonical json.dump(indent=2) re-serialize) stays in place as a safety net for the unlikely case where Doppler CLI has a bug or the secret is ingested in an unexpected encoding. The previous 4 commits on this same issue were all attempts to teach the --format env extractor to un-escape the value correctly. They are superseded by this approach, which avoids the escape dance entirely. Verified locally with a 4-case test: - Real \n in PEM -> PASS (the actual --plain output form) - Literal \n -> REJECTED (safety net catches it) - No newlines -> REJECTED (safety net catches it) - Missing field -> REJECTED (safety net catches it) Co-authored-by: Cursor <cursoragent@cursor.com>
The file was created by 'sudo tee' (owned by root), but 'rm' was
running without sudo. The first deploy creates the file; every
subsequent deploy then fails to remove it with:
rm: cannot remove '/tmp/firebase_pem_check.py': Operation not permitted
That non-zero exit propagates out of deploy-server.sh and aborts
the CD workflow even though the Firebase sanity check itself passed.
Add 'sudo' so the rm uses the same privilege as the original tee.
Confirmed by log run on 2026-07-15T20:13:24: the previous commit
('doppler secrets get --plain') succeeded materializing the JSON,
the sanity check exited 0, and only the cleanup 'rm' failed with
exit code 1, killing the whole deploy.
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores