fix(security): defense-in-depth for env injection RCE (GHSA-h6hh-wqxc-5hw9)#41831
Open
subrata71 wants to merge 1 commit into
Open
fix(security): defense-in-depth for env injection RCE (GHSA-h6hh-wqxc-5hw9)#41831subrata71 wants to merge 1 commit into
subrata71 wants to merge 1 commit into
Conversation
…-5hw9) Replace Bash `source` of docker.env with a safe parser that handles shell quoting without evaluating command substitution. Add startup validation to detect and sanitize poisoned env files from previously exploited instances. - Add safe-env-loader.sh with safe_source_env(), validate_env_file(), and sanitize_env_file() functions - Replace `. "$ENV_PATH"` in entrypoint.sh and run-with-env.sh - Add 18 tests covering quoting variants, injection payloads, and the actual PoC pattern from the advisory Defense-in-depth on top of existing escapeForShell() fix in v1.99.
subrata71
added
the
Security
Contributor
WalkthroughThis PR hardens Docker entrypoint environment loading against shell injection by replacing unsafe ChangesShell-Injection Defense for Environment Files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
deploy/docker/tests/test-safe-env-loader.sh (1)
204-305: 💤 Low valueConsider adding a test for sanitizing values with embedded single quotes.
The
test_sanitize_wraps_dangerous_valuestest verifies basic sanitization works, but doesn't exercise the embedded single-quote escaping logic (line 160 in the loader). A value like$(cmd'with'quotes)would exercise that path.📋 Additional test case
+test_sanitize_handles_embedded_single_quotes() { + cat > "$TEST_TMPDIR/env" <<'EOF' +APPSMITH_INSTANCE_NAME=$(cmd'arg) +EOF + bash -c " + source '$LOADER_SCRIPT' + sanitize_env_file '$TEST_TMPDIR/env' + " 2>/dev/null + # After sanitization, loading should yield the literal value + local result + result=$(bash -c " + source '$LOADER_SCRIPT' + safe_source_env '$TEST_TMPDIR/env' + echo \"\$APPSMITH_INSTANCE_NAME\" + ") + assert_equals "$result" "\$(cmd'arg)" "sanitized value with embedded quote is literal" +}Then add to the run list:
run_test test_ifs_trick_not_executed +run_test test_sanitize_handles_embedded_single_quotes🤖 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 `@deploy/docker/tests/test-safe-env-loader.sh` around lines 204 - 305, Add a new test in deploy/docker/tests/test-safe-env-loader.sh that verifies sanitize_env_file correctly escapes embedded single quotes inside dangerous command-substitution values (e.g., APPSMITH_INSTANCE_NAME=$(cmd'with'quotes)Appsmith); in the test call sanitize_env_file on a temp env file containing that value, then assert the sanitized file passes validate_env_file and that safe_source_env exports the variable with the single quotes preserved (use a new test name like test_sanitize_handles_embedded_single_quotes and reference sanitize_env_file, validate_env_file, and safe_source_env to locate the relevant logic).deploy/docker/fs/opt/appsmith/safe-env-loader.sh (1)
12-14: 💤 Low valueUnused constant is dead code.
_DANGEROUS_UNQUOTED_PATTERNis declared but never referenced. The actual checks on lines 116 and 158 use inline patterns. Either remove the unused constant or refactor to use it consistently.🧹 Remove dead code
-# Pattern matching shell metacharacters that indicate command injection -# when found outside of single quotes in an env value. -readonly _DANGEROUS_UNQUOTED_PATTERN='(\$\(|`|\$\(\(|\$\{[A-Za-z_])' -🤖 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 `@deploy/docker/fs/opt/appsmith/safe-env-loader.sh` around lines 12 - 14, The script defines the readonly variable _DANGEROUS_UNQUOTED_PATTERN but never uses it; update the env-value validation to either remove this dead constant or reference it where the inline regexes are used (the two places that currently embed patterns for command-injection detection in the env checks) so the single source of truth is _DANGEROUS_UNQUOTED_PATTERN; specifically replace the inline regex literals used in the env validation checks with the variable name or delete the constant if you prefer to keep inline patterns.
🤖 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.
Nitpick comments:
In `@deploy/docker/fs/opt/appsmith/safe-env-loader.sh`:
- Around line 12-14: The script defines the readonly variable
_DANGEROUS_UNQUOTED_PATTERN but never uses it; update the env-value validation
to either remove this dead constant or reference it where the inline regexes are
used (the two places that currently embed patterns for command-injection
detection in the env checks) so the single source of truth is
_DANGEROUS_UNQUOTED_PATTERN; specifically replace the inline regex literals used
in the env validation checks with the variable name or delete the constant if
you prefer to keep inline patterns.
In `@deploy/docker/tests/test-safe-env-loader.sh`:
- Around line 204-305: Add a new test in
deploy/docker/tests/test-safe-env-loader.sh that verifies sanitize_env_file
correctly escapes embedded single quotes inside dangerous command-substitution
values (e.g., APPSMITH_INSTANCE_NAME=$(cmd'with'quotes)Appsmith); in the test
call sanitize_env_file on a temp env file containing that value, then assert the
sanitized file passes validate_env_file and that safe_source_env exports the
variable with the single quotes preserved (use a new test name like
test_sanitize_handles_embedded_single_quotes and reference sanitize_env_file,
validate_env_file, and safe_source_env to locate the relevant logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38bbfdd1-5401-4563-be59-67ee737ebdaf
📒 Files selected for processing (4)
deploy/docker/fs/opt/appsmith/entrypoint.shdeploy/docker/fs/opt/appsmith/run-with-env.shdeploy/docker/fs/opt/appsmith/safe-env-loader.shdeploy/docker/tests/test-safe-env-loader.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
TL;DR: Replace Bash
sourceofdocker.envwith a safe parser to prevent RCE via command injection, and add startup validation to detect/sanitize poisoned env files from previously exploited instances.Background
GHSA-h6hh-wqxc-5hw9 (Critical, CVSS 9.1) describes an authenticated RCE where a super-admin injects
$(...)command substitution into env values viaPUT /api/v1/admin/env. The container'sentrypoint.shthen sourcesdocker.envwith. "$ENV_PATH", which evaluates the payload on restart.The core fix (
escapeForShell()inEnvManagerCEImpl) shipped in v1.99. This PR adds defense-in-depth on top of that fix.What changed
New
safe-env-loader.sh— provides three functions:safe_source_env()— parsesKEY=VALUElines with a character-by-character state machine that handles single/double quoting without ever evaluating$(), backticks, or${}validate_env_file()— scans for dangerous shell metacharacters in unquoted value contextssanitize_env_file()— rewrites poisoned values with single-quote wrappingentrypoint.sh— replaced. "$ENV_PATH"and. "$TMP/pre-define.env"withsafe_source_env(). Addedvalidate_env_file || sanitize_env_filebefore loading.run-with-env.sh— same treatment as entrypoint.sh.18 tests covering: quoting variants (single, double, embedded quotes), injection payloads (
$(...), backticks, semicolons,${IFS}trick from the actual PoC), validation detection, and sanitization.Why defense-in-depth matters
escapeForShell()were to regress, thesourcesink no longer exists — the safe parser treats all values as data.docker.envon the persistent volume. The startup validator catches and sanitizes these on first run after upgrade.Linear: APP-15243
Automation
/ok-to-test tags=""
🔍 Cypress test results
Communication
Should the DevRel and Marketing teams inform users about this change?
Warning
Tests have not run on the HEAD 37ccaf0 yet
Wed, 20 May 2026 18:35:48 UTC
Summary by CodeRabbit
Bug Fixes
Chores
Tests