Make entrypoint .env parsing safer - #2463
Conversation
The .env loader used unanchored greps, so a comment containing a variable name, or another variable that merely contains the name (e.g. SOME_APP_KEY_BACKUP), matched too. With multiple matching lines the export received a multiline string, failed, and 'ash -e' killed the container at boot. - match only real assignments anchored at the start of a line - let container environment variables take precedence over .env for the entrypoint's own vars, matching Laravel's precedence and the comment's stated intent - generate APP_KEY in the standard base64: format (32 random bytes) instead of a hand-rolled alphanumeric string - add a SKIP_MIGRATIONS escape hatch for multi-replica deployments where automatic 'migrate --force' on every boot would race
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Docker entrypoint selectively loads sanitized Laravel environment variables, generates a ChangesEntrypoint startup behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/entrypoint.sh (1)
52-54: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve
APP_INSTALLEDif it is passed via the container environment.If a user provides
APP_INSTALLED=truevia a container environment variable during a fully automated fresh start (where no.envfile exists), this line will unconditionally appendAPP_INSTALLED=falseto the newly generated.envfile. While Laravel's environment precedence handles this gracefully at runtime, it leaves the.envfile in a contradictory state.Consider mirroring the fallback behavior used for
APP_KEYabove to keep the.envfile consistent with the container environment.💡 Proposed fix
# enable installer - echo "APP_INSTALLED=false" >> /pelican-data/.env + echo "APP_INSTALLED=${APP_INSTALLED:-false}" >> /pelican-data/.env🤖 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/entrypoint.sh` around lines 52 - 54, Update the APP_INSTALLED initialization in the docker entrypoint flow to preserve an existing container environment value when generating the .env file, mirroring the fallback behavior used for APP_KEY. Only write APP_INSTALLED=false when no APP_INSTALLED value was provided, keeping the generated file consistent with the container environment.
🤖 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 `@docker/entrypoint.sh`:
- Around line 33-34: Update the export command in the .env loading loop to make
its sed transformation remove double quotes, single quotes, and carriage returns
before exporting each variable. Preserve the existing LINE-based parsing and
export behavior while ensuring values from CRLF files and single-quoted entries
are cleaned.
---
Outside diff comments:
In `@docker/entrypoint.sh`:
- Around line 52-54: Update the APP_INSTALLED initialization in the docker
entrypoint flow to preserve an existing container environment value when
generating the .env file, mirroring the fallback behavior used for APP_KEY. Only
write APP_INSTALLED=false when no APP_INSTALLED value was provided, keeping the
generated file consistent with the container environment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
A .env edited on Windows leaves a trailing \r in exported values, which breaks the APP_INSTALLED comparison and the DB_HOST wait, and single quoted values kept their quotes. Strip both along with double quotes.
1 participant
The entrypoint grepped .env without anchoring, so a comment line or another variable containing the same name could match and crash the container on boot. Now it only matches real assignments, container env wins over .env, generated keys use the normal
base64:format, and there's aSKIP_MIGRATIONSflag for people running more than one replica.