fix(security): prevent git import symlink path traversal → arbitrary file read/write (GHSA-fqwc-g9wm-5895)#41993
Open
subrata71 wants to merge 1 commit into
Open
Conversation
…file read/write (GHSA-fqwc-g9wm-5895) (#9250) ## Description fix(security): prevent git import symlink path traversal → arbitrary file read/write → RCE (GHSA-fqwc-g9wm-5895) - **Primary fix:** clone imported repos with symlink materialization disabled (`core.symlinks=false`) so malicious symlink tree entries (git mode `120000`) are checked out as inert regular files instead of real filesystem symlinks. - **Defense-in-depth:** harden `validatePathIsWithinGitRoot` to resolve the real (symlink-free) path before the Git-root containment check, and route the low-level write sink (`writeStringToFile`) through that guard so the concrete file path — not just its parent directory — is validated. - **Test coverage:** a symlink whose lexical path is inside the Git root but resolves outside is now rejected on read; a cloned symlink entry is verified to be checked out as a regular file; legitimate paths and the prior path-traversal regression tests still pass. Fixes https://linear.app/appsmith/issue/APP-15328/security-critical-git-import-symlink-path-traversal-arbitrary-file ### Vulnerability | Field | Value | |-------|-------| | **GHSA** | [GHSA-fqwc-g9wm-5895](https://github.com/appsmithorg/appsmith/security/advisories/GHSA-fqwc-g9wm-5895) | | **CVE** | Not assigned | | **CVSS** | 9.9 (critical) | | **CWE** | CWE-59 (Improper Link Resolution Before File Access) | | **Affected component** | `com.appsmith:appsmith-git` — Git import/commit file I/O | ### Exposure Analysis - **Who can exploit:** Any authenticated user. Signup is enabled by default in Appsmith CE, so an unauthenticated attacker can self-register and exploit immediately. Only workspace-level access (auto-granted on signup) is required — no instance-admin privileges. - **What an attacker can achieve:** Commit a symlink inside a Git repository, then import it. On import the symlink is materialized inside the Git root and the import reader follows it, leaking arbitrary server files (e.g. `/proc/self/environ` → `APPSMITH_ENCRYPTION_PASSWORD`/`SALT`, MongoDB/Redis credentials) into a JS Object body returned via the API. On commit, the serializer writes attacker-controlled content through the symlink, overwriting arbitrary files (e.g. `/opt/appsmith/run-rts.sh`) → **remote code execution as root** on the next process restart. - **In the wild:** Reported by Hacktron AI (Rahul Maini); the file-read primitive was confirmed against Appsmith Cloud test instances. 30-day disclosure deadline. - **Blast radius:** Full instance compromise on self-hosted deployments. On shared/multi-tenant infrastructure a single exploitation can expose the shared encryption key and DB credentials across tenants. ### Fix - **Root cause:** the containment guard used purely lexical `Path.normalize()`, which collapses `../` but does **not** resolve symbolic links, while every file I/O sink (`FileReader`, `FileUtils.readFileToString`, `Files.newBufferedWriter`) follows them. JGit also materialized committed symlink entries as real OS symlinks because `core.symlinks=false` was never set on clone. - **Fix strategy (defense-in-depth, all in the shared `appsmith-git` module):** 1. `GitExecutorCEImpl` — clone with `setNoCheckout(true)` then check out via `checkoutWithSymlinksDisabled()`, which sets `core.symlinks=false`. This neutralizes the vector at the source: no hostile symlink is ever created inside the Git root, so every downstream read/write is automatically safe. 2. `FileUtilsCEImpl.validatePathIsWithinGitRoot` — add a symlink-aware containment check that resolves the real path of the longest existing path prefix (via `toRealPath`) and re-appends not-yet-created segments, then verifies it stays within the real Git root. Fails closed on I/O error. Covers any symlink that may already exist (e.g. repos cloned before this fix). 3. `FileUtilsCEImpl.writeStringToFile` — validate the concrete file path before writing, closing the write path that previously only validated the parent directory. - **Intentionally not changed:** `SshTransportConfigCallback` (its `SshTransport` cast is correct for production SSH transport); `FileOperationsCEv2Impl` sinks (made safe by fix #1 removing symlinks, with fix #2 guarding the validated entry points). ### CE/EE sync EE-first flow for a Critical (CVSS 9.9) advisory: this EE PR lands first to keep public CE commit history clean until the fix is ready for disclosure. The three touched files are identical in CE and EE; the equivalent CE PR will be opened and merged coordinated with disclosure/release. ### Disclosure > **Do not merge until the advisory is ready for disclosure coordination.** > > After merge: > 1. Confirm fix is in the release branch > 2. Coordinate with the security team on disclosure timeline > 3. Open/merge the equivalent CE PR > 4. Update advisory with patched version and publish; notify reporter ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith-ee/actions/runs/28449518676> > Commit: 0522864fa2b862abf1b7b3793002d360e8d7a506 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=28449518676&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Tue, 30 Jun 2026 14:36:52 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Follow-ups - Open the equivalent CE PR coordinated with disclosure (identical change to `appsmith-git`). - E2E coverage for malicious-symlink Git import is deferred (requires an attacker-controlled remote fixture); covered by unit/integration tests in this PR. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hardened Git import and checkout flows against symlink-based path traversal and filesystem escape. * Added stricter validation so writes reject symlink paths that could escape the expected Git root. * Clone/check-out now disables symlink materialization and cleans up any symlinks post-clone, ensuring regular-file output. * **Tests** * Added regression tests for symlink-escape behavior and for ensuring normal files remain unchanged during import/clone. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
subrata71
added
Security
Contributor
WalkthroughGit repository path validation now resolves symlinks, concrete file writes are checked, and clone flows remove worktree symlinks before checkout. Regression tests cover escaped metadata links, regular files, and cloned symlink materialization. ChangesSymlink security controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CloneFlow
participant JGit
participant SymlinkRemediation
participant FileSystem
CloneFlow->>JGit: clone repository
JGit-->>CloneFlow: cloned Git handle
CloneFlow->>SymlinkRemediation: removeSymlinksAfterClone
SymlinkRemediation->>JGit: disable core.symlinks
SymlinkRemediation->>FileSystem: remove worktree symlinks outside .git
SymlinkRemediation->>JGit: checkout all paths
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java (1)
49-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the public clone flows instead of only the helper.
Calling
removeSymlinksAfterClonedirectly will not catch removal of the production invocation, and it leavesFSGitHandlerCEImpl.cloneRemoteIntoArtifactRepountested. Add coverage through both public clone implementations using the local repository URI.🤖 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 `@app/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java` around lines 49 - 92, Update cloneRepoWithSymlinkEntry_isCheckedOutAsRegularFile_GHSA_fqwc to exercise both public clone implementations, including FSGitHandlerCEImpl.cloneRemoteIntoArtifactRepo, with the local repository URI instead of invoking removeSymlinksAfterClone directly. Assert that each public flow materializes the symlink entry as a regular file while preserving normal-file behavior, and retain any control checkout needed to validate the vulnerable baseline.
🤖 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
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`:
- Around line 870-873: Update saveActionCollection and saveActions to call
validatePathIsWithinGitRoot(metadataPath) immediately before each
fileOperations.writeToFile metadata write, while retaining the existing
parent-directory validation and leaving other write paths unchanged.
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java`:
- Around line 339-365: Make symlink remediation fail closed in
removeSymlinksAfterClone: propagate Files.delete failures instead of logging and
continuing, so the clone is not reported as successfully remediated and is
removed by the existing clone-failure cleanup path. Apply the same behavior to
the corresponding symlink-remediation flow in
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
lines 430-452; both sites must stop swallowing deletion errors.
In
`@app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java`:
- Around line 336-370: Update
reconstructMetadata_symlinkEscapesGitRoot_throwsSecurityException_GHSA_fqwc to
assert AppsmithPluginException instead of the broad RuntimeException, and verify
the exception message identifies the path traversal or containment failure.
Preserve the existing symlink setup and cleanup.
---
Nitpick comments:
In
`@app/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java`:
- Around line 49-92: Update
cloneRepoWithSymlinkEntry_isCheckedOutAsRegularFile_GHSA_fqwc to exercise both
public clone implementations, including
FSGitHandlerCEImpl.cloneRemoteIntoArtifactRepo, with the local repository URI
instead of invoking removeSymlinksAfterClone directly. Assert that each public
flow materializes the symlink entry as a regular file while preserving
normal-file behavior, and retain any control checkout needed to validate the
vulnerable baseline.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c463f6ab-40b0-42b3-b08c-a87e330529ee
📒 Files selected for processing (6)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.javaapp/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.javaapp/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.javaapp/server/appsmith-git/src/main/resources/git.shapp/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.javaapp/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java
Comment on lines
+870
to
+873
| // Validate the concrete file path (not just its parent directory) so that a symlink at this | ||
| // path pointing outside the Git root is rejected before any write follows it | ||
| // (GHSA-fqwc-g9wm-5895). | ||
| validatePathIsWithinGitRoot(path); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Validate every concrete write target, not only string-body files.
saveActionCollection and saveActions still pass metadataPath directly to fileOperations.writeToFile after validating only the parent directory. An existing metadata.json symlink can therefore redirect those writes outside the Git root. Apply validatePathIsWithinGitRoot(metadataPath) before both metadata writes.
🤖 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
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`
around lines 870 - 873, Update saveActionCollection and saveActions to call
validatePathIsWithinGitRoot(metadataPath) immediately before each
fileOperations.writeToFile metadata write, while retaining the existing
parent-directory validation and leaving other write paths unchanged.
Comment on lines
+339
to
+365
| /** | ||
| * Disables symlink support and re-materializes any symlinks as regular files | ||
| * (GHSA-fqwc-g9wm-5895). | ||
| * | ||
| * <p>After a normal clone, any git mode {@code 120000} entries are real OS symlinks on disk. | ||
| * Setting {@code core.symlinks=false} and then deleting and re-checking-out those symlinks | ||
| * converts them to regular files containing the link target text. | ||
| */ | ||
| protected void removeSymlinksAfterClone(Git git) throws GitAPIException, IOException { | ||
| StoredConfig config = git.getRepository().getConfig(); | ||
| config.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, ConfigConstants.CONFIG_KEY_SYMLINKS, false); | ||
| config.save(); | ||
|
|
||
| Path workTree = git.getRepository().getWorkTree().toPath(); | ||
| try (Stream<Path> paths = Files.walk(workTree)) { | ||
| paths.filter(Files::isSymbolicLink) | ||
| .filter(p -> !p.startsWith(workTree.resolve(".git"))) | ||
| .forEach(p -> { | ||
| try { | ||
| Files.delete(p); | ||
| } catch (IOException e) { | ||
| log.warn("Failed to delete symlink: {}", p, e); | ||
| } | ||
| }); | ||
| } | ||
| git.checkout().setAllPaths(true).call(); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Symlink remediation must fail closed. Both clone implementations ignore deletion failures and then report a successfully remediated clone.
app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java#L339-L365: propagate anyFiles.deletefailure and remove the unsafe clone.app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java#L430-L452: apply the same fail-closed behavior.
📍 Affects 2 files
app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java#L339-L365(this comment)app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java#L430-L452
🤖 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
`@app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java`
around lines 339 - 365, Make symlink remediation fail closed in
removeSymlinksAfterClone: propagate Files.delete failures instead of logging and
continuing, so the clone is not reported as successfully remediated and is
removed by the existing clone-failure cleanup path. Apply the same behavior to
the corresponding symlink-remediation flow in
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java
lines 430-452; both sites must stop swallowing deletion errors.
Comment on lines
+336
to
+370
| /** | ||
| * GHSA-fqwc-g9wm-5895: Symlink path traversal in Git import. | ||
| * A file inside the git root whose lexical path is contained in the git root, but which is a | ||
| * symbolic link pointing OUTSIDE the git root, must be rejected by validatePathIsWithinGitRoot. | ||
| * The lexical Path.normalize() guard does NOT resolve symlinks, so before the fix the read | ||
| * follows the symlink and leaks the target file's content. After the fix the real-path | ||
| * containment check detects the escape and throws. | ||
| */ | ||
| @Test | ||
| public void reconstructMetadata_symlinkEscapesGitRoot_throwsSecurityException_GHSA_fqwc() throws IOException { | ||
| // A "secret" file located OUTSIDE the git root. | ||
| Path externalDir = Files.createTempDirectory("ghsa-fqwc-external"); | ||
| try { | ||
| Path secretFile = externalDir.resolve("secret.json"); | ||
| Files.writeString(secretFile, "{\"fileFormatVersion\": 99, \"leaked\": \"top-secret\"}"); | ||
|
|
||
| // Set up a repo directory inside the git root, then replace metadata.json with a symlink | ||
| // pointing to the external secret. The symlink's own path is lexically inside the git root. | ||
| Path repoSuffix = Path.of("workspace-symlink", "app", "repo"); | ||
| Path fullRepoPath = localTestDirectoryPath.resolve(repoSuffix); | ||
| Files.createDirectories(fullRepoPath); | ||
| Path metadataPath = fullRepoPath.resolve("metadata.json"); | ||
| Files.createSymbolicLink(metadataPath, secretFile); | ||
|
|
||
| // Sanity: the symlink lexically resolves inside the git root (this is what fools the | ||
| // old lexical guard), but its real target is outside. | ||
| Assertions.assertTrue(Files.isSymbolicLink(metadataPath)); | ||
|
|
||
| Assertions.assertThrows(RuntimeException.class, () -> { | ||
| fileUtils.reconstructMetadataFromGitRepository(repoSuffix).block(); | ||
| }); | ||
| } finally { | ||
| FileUtils.deleteDirectory(externalDir.toFile()); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the specific containment exception.
RuntimeException allows this security regression to pass for unrelated failures. Assert AppsmithPluginException and, preferably, the traversal error message.
🤖 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
`@app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java`
around lines 336 - 370, Update
reconstructMetadata_symlinkEscapesGitRoot_throwsSecurityException_GHSA_fqwc to
assert AppsmithPluginException instead of the broad RuntimeException, and verify
the exception message identifies the path traversal or containment failure.
Preserve the existing symlink setup and cleanup.
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
CE port of EE PR (already merged and deployed in v2.2).
Fixes a Critical (CVSS 9.9) symlink path traversal in the Git import/commit feature that allows any authenticated user to read arbitrary server files and achieve RCE via symlink tree entries (git mode 120000).
Defense-in-depth fix across all three clone paths:
FileUtilsCEImpl.validatePathIsWithinGitRoot— symlink-aware real-path containment check (toRealPath), fails closedFileUtilsCEImpl.writeStringToFile— validates the concrete file path before writingGitExecutorCEImpl+FSGitHandlerCEImpl— post-clone symlink removal (core.symlinks=false+ delete symlinks + re-checkout)git.sh—git config core.symlinks falseafter clone in the bash/in-memory git pathFixes APP-15328
Vulnerability
EE PR
Merged: https://github.com/appsmithorg/appsmith-ee/pull/9250
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/29332548734
Commit: ddc60f8
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 14 Jul 2026 12:47:00 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Security
Bug Fixes