Load secrets from _FILE environment variables#2019
Conversation
Added processing of _FILE environment variables to load secrets from files.
WalkthroughThe Docker entrypoint now resolves environment variables ending in ChangesDocker entrypoint environment handling
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
🤖 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 `@packaging/docker/entrypoint.sh`:
- Around line 38-42: Update the environment-variable discovery loop in the
entrypoint script to extract and validate variable names before processing them,
accepting only names whose suffix is exactly _FILE. Ensure names such as
CACHE_FILE_BACKUP are excluded, while preserving the existing file existence
check and export behavior for valid variables.
- Around line 38-39: Replace the eval-based lookup in the environment-variable
loop with a safe lookup using the variable name as data, such as printenv
"$var". Validate that each var matches the expected environment-variable naming
pattern before using it, then preserve the existing export behavior without
evaluating environment-derived shell code.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbd83c43-801c-49c7-80dc-5a47dd40a5f9
📒 Files selected for processing (1)
packaging/docker/entrypoint.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
grimmory-tools/grimmory-docs(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This project is being developed using current and future-facing technologies:
- Java 25 with --enable-preview (preview features are INTENTIONAL and encouraged)
- Spring Boot 4 (latest major version, check APIs accordingly)
- Jackson 3 (new package: tools.jackson.* instead of com.fasterxml.jackson.*)
- Hibernate 7.3.x (Jakarta Persistence 3.2, new APIs; avoid deprecated Hibernate 5/6 patterns)
- Angular 21 (signals-based reactivity, no NgModules unless legacy)
Grimmory Internal Tools
- epub4j and pdfium4j are our own internal tools developed by the Grimmory team.
- Always verify behavior and API changes against the upstream repositories:
- If you encounter issues with these libraries, check if a fix exists in the upstream grimmory-tools organization.
Metadata Standards and Compliance
- For all metadata writing and parsing logic, double-check against Dublin Core and ANSI standards to ensure perfect official compliance.
- We strictly follow the widespread and official XML-compliant methods for EPUB2, EPUB3, CBX, and PDF formats.
General Java and Spring rules
- ALWAYS prefer modern, idiomatic Java 25 constructs over legacy patterns.
- Preview features (--enable-preview) are enabled and intentional; do NOT flag them as risky unless there is a concrete runtime issue.
- Prefer: records, sealed classes/interfaces, pattern matching (switch expressions, instanceof), structured concurrency (StructuredTaskScope), scoped values, string templates, unnamed patterns/variables.
- Prefer virtual threads (Thread.ofVirtual(), Executors.newVirtualThreadPerTaskExecutor()) over platform threads for I/O-bound work.
- Prefer the new Sequenced Collections API (SequencedCollection, SequencedMap) where applicable.
- Prefer
varfor local variables when the type is obvious from context.- Use stream().toList() instead of stream().collect(Collectors.toList()) for imm...
Files:
packaging/docker/entrypoint.sh
🪛 ast-grep (0.44.1)
packaging/docker/entrypoint.sh
[error] 38-38: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval echo $$var
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🪛 Shellcheck (0.11.0)
packaging/docker/entrypoint.sh
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 42-42: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (1)
packaging/docker/entrypoint.sh (1)
42-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winQuote the complete export assignment.
Because the command-substitution result is unquoted, file contents containing spaces or glob characters can be split or expanded. Use:
- export "$var_name"=$(cat "$file_path") + export "$var_name=$(cat "$file_path")"> Likely an incorrect or invalid review comment.Source: Linters/SAST tools
| for var in $(env | grep '_FILE=' | cut -d= -f1); do | ||
| file_path=$(eval echo \$$var) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove eval from environment-variable lookup.
eval echo \$$var reparses the environment-derived name as shell code. A crafted environment variable name can therefore execute arbitrary commands before su-exec drops privileges. Use a safe lookup such as printenv "$var" and validate the variable name before exporting it.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 38-38: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval echo $$var
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🪛 Shellcheck (0.11.0)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@packaging/docker/entrypoint.sh` around lines 38 - 39, Replace the eval-based
lookup in the environment-variable loop with a safe lookup using the variable
name as data, such as printenv "$var". Validate that each var matches the
expected environment-variable naming pattern before using it, then preserve the
existing export behavior without evaluating environment-derived shell code.
Source: Linters/SAST tools
| for var in $(env | grep '_FILE=' | cut -d= -f1); do | ||
| file_path=$(eval echo \$$var) | ||
| if [ -n "$file_path" ] && [ -f "$file_path" ]; then | ||
| var_name=${var%_FILE} | ||
| export "$var_name"=$(cat "$file_path") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict discovery to variable names ending in _FILE.
The grep '_FILE=' pattern matches names such as CACHE_FILE_BACKUP, while ${var%_FILE} leaves those names unchanged. The script can then overwrite unrelated configuration variables with file contents. Extract and validate names first, then process only an exact _FILE suffix.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 38-38: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval echo $$var
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🪛 Shellcheck (0.11.0)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 42-42: Quote this to prevent word splitting.
(SC2046)
🤖 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 `@packaging/docker/entrypoint.sh` around lines 38 - 42, Update the
environment-variable discovery loop in the entrypoint script to extract and
validate variable names before processing them, accepting only names whose
suffix is exactly _FILE. Ensure names such as CACHE_FILE_BACKUP are excluded,
while preserving the existing file existence check and export behavior for valid
variables.
|
Which environment variables are these for? Doesn't this suffer from the same problems that normal environment variables do - that the secrets are stored in the clear where other processes can easily read them? |
Trivial implementation using the same of wide used https://hub.docker.com/_/mariadb#secrets LGTM |
|
@balazs-szucs I checked this one, safe and important for security reasons. |
I get that other docker images have used this. I am asking what makes this safer, and when should people be using it? I would understand it being safer if the process itself read the These are still available in Where was this discussed?
I also checked it, given this is also an implementation that uses |
Technical Debt: https://docs.docker.com/compose/how-tos/use-secrets/ Please, reopen. |
This problem also applies to the implementation here. I will not reopen this. Thanks for understanding! |
3 participants
Added processing of _FILE environment variables to load secrets from files.
Description
Linked Issue
Changes
Manual Testing Steps
Screenshots (Optional)
Additional Context (Optional)
AI Disclosure
Checklist
just ui checkandjust api check.Summary by CodeRabbit
_FILEsuffix.