fix(security): Path Traversal (GHSA-m4hv-9p7g-56vm)#41790
fix(security): Path Traversal (GHSA-m4hv-9p7g-56vm)#41790
Conversation
…SA-m4hv-9p7g-56vm)
…ete operations (GHSA-m4hv-9p7g-56vm) Extend validatePathIsWithinGitRoot to cover readFile, readFiles, readFileAsString, deleteFile, and deleteDirectory via validated wrapper methods. Write operations were already protected; this closes the asymmetric validation gap for reads and deletes.
subrata71
added
Security
WalkthroughThis PR hardens Git repository file operations against path-traversal attacks by introducing a protected validation method and five validated I/O wrapper methods, then systematically replacing all direct file reads and deletes throughout the module with these bounds-checked variants. Two regression tests verify the security boundary. ChangesGit Root Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java (2)
207-214: 💤 Low valueSwallowed
IOExceptionin test cleanup — minor nit.
deleteLocalTestDirectoryPath()silently eats cleanup failures, which can leave stale state and cause flaky cross-test pollution (the new tests both write intolocalTestDirectory). At least log the exception so CI surfaces the cause when cleanup fails.🤖 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 207 - 214, The test cleanup method deleteLocalTestDirectoryPath currently swallows IOException from FileUtils.deleteDirectory; modify the catch block to log the exception instead of ignoring it (e.g., use the test class logger LOGGER.error("Failed to delete local test directory", e) or at minimum e.printStackTrace()) so cleanup failures are visible in CI; update or add a logger field on the test class if needed and reference deleteLocalTestDirectoryPath and FileUtils.deleteDirectory in your change.
176-186: ⚡ Quick winTighten the assertion to the security exception type.
assertThrows(RuntimeException.class, …)passes for any runtime failure (NPE, IO wrapper, etc.), which would mask a regression where the path-traversal gate stops firing but something else throws downstream. Assert onAppsmithPluginException(or assert the message contains"SECURITY: Path traversal detected") so this test fails loudly if the guard is removed.♻️ Proposed tightening
- Assertions.assertThrows(RuntimeException.class, () -> { - fileUtils.reconstructMetadataFromGitRepository(maliciousRepoSuffix).block(); - }); + AppsmithPluginException ex = Assertions.assertThrows(AppsmithPluginException.class, () -> fileUtils + .reconstructMetadataFromGitRepository(maliciousRepoSuffix) + .block()); + Assertions.assertTrue(ex.getMessage().contains("Path traversal"));Don't forget the import for
AppsmithPluginException.🤖 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 176 - 186, The test currently uses Assertions.assertThrows(RuntimeException.class, ...) which is too broad; change it to assert the specific security exception by replacing RuntimeException.class with AppsmithPluginException.class (or assertThrows that also checks the exception message contains "SECURITY: Path traversal detected") for the call to fileUtils.reconstructMetadataFromGitRepository(maliciousRepoSuffix).block(); update the import to include AppsmithPluginException and ensure the assertion checks the exact exception type or message to prevent masking other failures.
🤖 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 683-706: The validator validatePathIsWithinGitRoot currently uses
Path.normalize(), which does not resolve symlinks, so calls like
deleteDirectoryValidated(Path directory) (and the other *Validated methods:
readFileValidated, readFilesValidated, readFileAsStringValidated,
deleteFileValidated) can be tricked by symlinks inside the git root that point
outside; update validatePathIsWithinGitRoot to resolve the input and the git
root via Path.toRealPath() (or explicitly detect and reject symlinks via
Files.isSymbolicLink) before comparing, ensuring any symlink targets are
resolved and that deleteDirectoryValidated cannot accept a symlink that points
outside the repository.
---
Nitpick comments:
In
`@app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java`:
- Around line 207-214: The test cleanup method deleteLocalTestDirectoryPath
currently swallows IOException from FileUtils.deleteDirectory; modify the catch
block to log the exception instead of ignoring it (e.g., use the test class
logger LOGGER.error("Failed to delete local test directory", e) or at minimum
e.printStackTrace()) so cleanup failures are visible in CI; update or add a
logger field on the test class if needed and reference
deleteLocalTestDirectoryPath and FileUtils.deleteDirectory in your change.
- Around line 176-186: The test currently uses
Assertions.assertThrows(RuntimeException.class, ...) which is too broad; change
it to assert the specific security exception by replacing RuntimeException.class
with AppsmithPluginException.class (or assertThrows that also checks the
exception message contains "SECURITY: Path traversal detected") for the call to
fileUtils.reconstructMetadataFromGitRepository(maliciousRepoSuffix).block();
update the import to include AppsmithPluginException and ensure the assertion
checks the exact exception type or message to prevent masking other failures.
🪄 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: bf28cc45-f71c-46df-85e2-cfeccf6a5d22
📒 Files selected for processing (2)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.javaapp/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java
| protected Object readFileValidated(Path filePath) { | ||
| validatePathIsWithinGitRoot(filePath); | ||
| return fileOperations.readFile(filePath); | ||
| } | ||
|
|
||
| protected Map<String, Object> readFilesValidated(Path directoryPath, String keySuffix) { | ||
| validatePathIsWithinGitRoot(directoryPath); | ||
| return fileOperations.readFiles(directoryPath, keySuffix); | ||
| } | ||
|
|
||
| protected String readFileAsStringValidated(Path filePath) { | ||
| validatePathIsWithinGitRoot(filePath); | ||
| return fileOperations.readFileAsString(filePath); | ||
| } | ||
|
|
||
| protected void deleteFileValidated(Path filePath) { | ||
| validatePathIsWithinGitRoot(filePath); | ||
| fileOperations.deleteFile(filePath); | ||
| } | ||
|
|
||
| protected void deleteDirectoryValidated(Path directory) { | ||
| validatePathIsWithinGitRoot(directory); | ||
| fileOperations.deleteDirectory(directory); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how readFiles / deleteDirectory enumerate paths under the validated root.
fd -e java -i 'FileOperations' | xargs -I{} echo {}
ast-grep --pattern $'public $_ readFiles($$$) { $$$ }'
ast-grep --pattern $'public $_ deleteDirectory($$$) { $$$ }'
rg -nP -C3 '\b(Files\.walk|Files\.list|listFiles|walkFileTree)\b' --type=javaRepository: appsmithorg/appsmith
Length of output: 17345
🏁 Script executed:
# Check FileOperations interface and implementations
cat -n app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/operations/FileOperations.java | head -50
# Get full readFiles implementation in FileOperationsCEv2Impl
sed -n '165,190p' app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java
# Find deleteDirectory implementation
rg -n 'deleteDirectory' app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java -A 8Repository: appsmithorg/appsmith
Length of output: 2061
🏁 Script executed:
# Check validatePathIsWithinGitRoot implementation
rg -n 'validatePathIsWithinGitRoot' app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java -A 15 | head -40
# Search for symlink handling in the codebase
rg -n 'symlink|isSymbolic|Links\.read' app/server/appsmith-git/src/main/java --type=java | head -20
# Check if there's any NIO path validation beyond basic directory check
rg -n 'Files\.isSymbolic|BasicFileAttributes|readAttributes' app/server/appsmith-git/src/main/java --type=javaRepository: appsmithorg/appsmith
Length of output: 2173
🌐 Web query:
Apache Commons FileUtils deleteDirectory symlink behavior follows symbolic links
💡 Result:
Apache Commons IO FileUtils.deleteDirectory does not follow symlinks when deleting recursively; it deletes the symlink itself and leaves the link target alone. However, the deletion logic can still end up modifying the target’s permissions in certain cases (a behavior reported in IO-751). [1] Key points: 1) Symlink vs target handling: When FileUtils.deleteDirectory(...) encounters a symlink while recursively deleting, the default behavior is to delete the symlink but leave the symlink target alone (it is not deleted or recursed into). [1] 2) Permission side effects: Despite not deleting the target, FileUtils.deleteDirectory can remove write permissions from the symlink target during the process. [1] 3) Broken symlink edge case: Separate reported issues indicate that FileUtils.deleteDirectory may fail when the directory contains a broken symbolic link (IO-862). [2] 4) Source-level confirmation (current main): FileUtils.deleteDirectory(File) checks whether the directory argument is a symlink via isSymlink(directory); if it is not a symlink it calls cleanDirectory(directory) and then delete(directory). This implies symlink targets are not recursively cleaned through the symlink path; instead the symlink deletion is handled by delete(...). [3] So, your query’s “follows symbolic links” concern is addressed as: it generally does not recurse into symlink targets, but you should be aware of permission side effects and possible failures with broken links. [1][2]
Citations:
- 1: https://issues.apache.org/jira/browse/IO-751
- 2: https://stackoverflow.com/questions/8666420/how-to-recursively-delete-a-folder-without-following-symlinks
- 3: https://issues.apache.org/jira/projects/IO/issues/IO-862
Validation uses normalize(), not toRealPath()—symlinks pass the check undetected.
readFilesValidated and readFileAsStringValidated are safe: they delegate to readFiles(), which uses directory.listFiles() and processes only immediate children—no symlink traversal.
deleteDirectoryValidated has a subtle gap: validatePathIsWithinGitRoot() calls normalize() for syntactic path normalization, which does not resolve symlinks. A symlink within the validated git root pointing outside it will pass validation. When FileUtils.deleteDirectory() is called, it does not recurse into the symlink target (per Apache Commons IO behavior), so the risk is limited—but the symlink itself exists undetected in the validated tree. Prefer toRealPath() in the validator to resolve and verify symlinks, or add explicit symlink checks.
🤖 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 683 - 706, The validator validatePathIsWithinGitRoot currently uses
Path.normalize(), which does not resolve symlinks, so calls like
deleteDirectoryValidated(Path directory) (and the other *Validated methods:
readFileValidated, readFilesValidated, readFileAsStringValidated,
deleteFileValidated) can be tricked by symlinks inside the git root that point
outside; update validatePathIsWithinGitRoot to resolve the input and the git
root via Path.toRealPath() (or explicitly detect and reject symlinks via
Files.isSymbolicLink) before comparing, ensuring any symlink targets are
resolved and that deleteDirectoryValidated cannot accept a symlink that points
outside the repository.
There was a problem hiding this comment.
Verified this concern against the codebase. The symlink scenario is not practically exploitable here, and switching to toRealPath() would introduce functional issues. No change needed for this PR.
Why symlinks can't exist in the git root:
- The git root is populated by JGit's
Git.cloneRepository()and Appsmith's own serialization (writeToFile,saveResource). JGit defaultscore.symlinks=falseand creates text files for symlink entries, not actual OS symlinks. - Zero references to
symlink,isSymbolicLink, orcore.symlinksexist anywhere in the server codebase — symlinks are not part of the application model. - This also holds for in-memory git mode (
/dev/shm), which uses a tmpfs mount but the sameFileUtilsCEImplcode path with the same validation logic.
Why toRealPath() would break things:
toRealPath()throwsIOExceptionwhen the path doesn't exist. Many validation calls happen for paths before files are created (e.g., the write-sidesaveResourcevalidates beforeFiles.createDirectories). Switching would require try-catch wrappers everywhere or conditional existence checks.
Pre-existing behavior:
- The
validatePathIsWithinGitRootmethod usingnormalize()was already approved and merged for write operations. This PR extends that same method to reads — changing the validation algorithm itself is a separate concern, not a gap in this fix.
If symlink hardening is desired as defense-in-depth, an explicit Files.isSymbolicLink() check could be added in a follow-up — it's cheap and doesn't have the IOException problem that toRealPath() has.
There was a problem hiding this comment.
@subrata71, thanks for the thorough analysis — all three arguments hold up:
- JGit
core.symlinks=falseis the decisive architectural constraint. Since JGit materializes symlink entries as plain text files, there's no OS-level symlink to exploit in the first place. toRealPath()breakage is a real concern — it would require wrapping every pre-creation validation call in a try-catch or an existence check, which is disproportionate overhead.- Consistency with approved write-side validation is a fair scoping argument — changing the algorithm itself is orthogonal to this PR's goal.
The Files.isSymbolicLink() suggestion for a follow-up is a good lightweight option — no IOException, no existence requirement, and it adds an explicit signal in the validation log. Would you like me to open a GitHub issue to track that defense-in-depth hardening?
✏️ Learnings added
Learnt from: subrata71
Repo: appsmithorg/appsmith PR: 41790
File: app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java:683-706
Timestamp: 2026-05-11T07:36:39.607Z
Learning: In appsmithorg/appsmith, JGit defaults `core.symlinks=false` when cloning repositories, which means symlink entries in git repos are materialized as plain text files rather than OS-level symlinks. This means symlink-based path traversal attacks are not practically exploitable in the git root managed by FileUtilsCEImpl (app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java). There are zero references to symlink/isSymbolicLink/core.symlinks anywhere in the server codebase.
Learnt from: subrata71
Repo: appsmithorg/appsmith PR: 41790
File: app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java:683-706
Timestamp: 2026-05-11T07:36:39.607Z
Learning: In appsmithorg/appsmith FileUtilsCEImpl (app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java), the `validatePathIsWithinGitRoot` method intentionally uses `Path.normalize()` rather than `Path.toRealPath()` because `toRealPath()` throws `IOException` for paths that don't yet exist. Many validation calls occur before file creation (e.g., `saveResource` validates before `Files.createDirectories`), so switching to `toRealPath()` would require try-catch wrappers or existence checks throughout. The `normalize()`-based approach was approved for write operations and is consistently extended to read operations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: subrata71
Repo: appsmithorg/appsmith PR: 41684
File: app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java:93-97
Timestamp: 2026-04-07T19:40:13.566Z
Learning: For SSRF mitigations involving SSH/Git URL handling, use a dual-validation strategy: validate the target hostname before initiating any network connection (e.g., via a URL/host validator like `validateGitSshUrl`), and then validate the resolved remote IP address after the TCP handshake/connection is established (e.g., in a session factory or connection accept path like `accept()`). Do not rely on a single pre-connection hostname check alone, since DNS rebinding can change the destination between validation and connect. If the underlying library (e.g., JGit) does not allow injecting a pre-resolved `InetAddress`, implement the post-connection IP validation as the second gate to complete the mitigation.
1 participant
Description
fix(security): Path Traversal in git file read/delete operations (GHSA-m4hv-9p7g-56vm)
validatePathIsWithinGitRoot()coverage from write-only to all file operations (reads + deletes) via validated wrapper methods inFileUtilsCEImpl. All 31 directfileOperations.readFile/readFiles/readFileAsStringcall sites now go through path validation.validatePathIsWithinGitRootvisibility fromprivatetoprotectedso EE subclasses can reuse it. Added validated wrappers fordeleteFileanddeleteDirectoryas well.Fixes https://linear.app/appsmith/issue/APP-15180/security-path-traversal-in-file-operations-fileoperationscev2impl-ghsa
Vulnerability
FileOperationsCEv2Impl— git file operationsExposure Analysis
Fix
FileUtilsCEImplhadvalidatePathIsWithinGitRoot()applied to all write operations (saveResource,saveActions, etc.) but not to any read operations (readFile,readFiles,readFileAsString) or delete operations (deleteFile,deleteDirectory).protectedvalidated wrapper methods (readFileValidated,readFilesValidated,readFileAsStringValidated,deleteFileValidated,deleteDirectoryValidated) inFileUtilsCEImplthat validate path containment before delegating tofileOperations. Replaced all 31 direct read call sites and 8 delete call sites with the validated wrappers.FileOperationsCEv2Impl— this is the raw I/O layer that doesn't know about git root paths. Validation belongs inFileUtilsCEImplwhich owns the path policy.scanAndDelete*methods are also unchanged since they operate on boundedFiles.walk()results within an already-validated base directory.protectedso EE'sFileUtilsImpl(which has its own direct read calls) can inherit and use the same wrappers.CE/EE sync
Shadow EE PR needed.
FileUtilsCEImpl.javachange syncs automatically via hourly CE→EE sync. EE'sFileUtilsImpl.javahas ~8 additional directfileOperations.readFile()calls that need to be updated to use the inherited validated wrappers in a separate EE PR.Disclosure
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/25638434147
Commit: 809c50f
Cypress dashboard.
Tags:
@tag.AllSpec:
Sun, 10 May 2026 21:09:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Follow-ups
FileUtilsImpl.javato update EE-specific read/delete calls to use inherited validated wrappersFileOperationsCEv2Impl.saveMetadataResource()calls its ownsaveResource()bypassingFileUtilsCEImplvalidation — pre-existing gap, separate follow-upSummary by CodeRabbit
Bug Fixes
Tests