fix(security): add path traversal validation to widget save path (GHSA-r553-q33m-v7pf)#41834
Open
subrata71 wants to merge 2 commits into
Open
fix(security): add path traversal validation to widget save path (GHSA-r553-q33m-v7pf)#41834subrata71 wants to merge 2 commits into
subrata71 wants to merge 2 commits into
Conversation
…-v7pf) Adds regression tests that verify: - Widget names containing path traversal sequences (e.g., "../../../../tmp/evil") are rejected during Git serialization with a security exception - Legitimate widget names continue to serialize correctly
…A-r553-q33m-v7pf) The widget serialization path in FileUtilsCEImpl.updateEntitiesInRepo() wrote files using user-controlled widgetName values without calling validatePathIsWithinGitRoot(), while all other Git write helpers in the same class did. An authenticated user with edit access to a Git-connected application could craft a widgetName containing path traversal sequences to write arbitrary JSON files outside the Git repository on the server filesystem. Add validatePathIsWithinGitRoot() calls on both the widget directory path and the final file path before calling saveWidgets(), consistent with how saveResource(), saveActions(), and saveActionCollection() already validate their paths.
subrata71
added
the
Security
Contributor
WalkthroughThis PR adds path traversal defense-in-depth to widget file serialization in the Appsmith git module. The implementation validates widget paths against the configured git root before write operations, and new security tests confirm that malicious widget names are rejected while legitimate names serialize correctly. ChangesPath traversal validation in widget serialization (GHSA-r553)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java`:
- Around line 262-271: Ensure the test in FileUtilsImplTest cleans up the
traversal artifact and asserts the precise exception: before calling
fileUtils.saveApplicationToGitRepo(Path.of(""), appRef, "branch",
false).block(), delete the file at "/tmp/ghsa-r553-traversal-test.json" if it
exists so the assertion is not affected by prior state; replace
Assertions.assertThrows(RuntimeException.class, ...) with
Assertions.assertThrows(AppsmithPluginException.class, ...) so the test expects
the specific exception thrown by saveApplicationToGitRepo rather than any
RuntimeException.
🪄 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: 68639b5f-9ffd-4d09-a567-3368a024217f
📒 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
Comment on lines
+262
to
+271
| Assertions.assertThrows(RuntimeException.class, () -> { | ||
| fileUtils | ||
| .saveApplicationToGitRepo(Path.of(""), appRef, "branch", false) | ||
| .block(); | ||
| }); | ||
|
|
||
| Assertions.assertFalse( | ||
| Files.exists(Path.of("/tmp/ghsa-r553-traversal-test.json")), | ||
| "Path traversal: file was created outside git root"); | ||
| } |
Contributor
There was a problem hiding this comment.
Pre-clean the traversal artifact and tighten the expected exception type.
Two small hygiene gaps in this regression test:
Files.exists(Path.of("/tmp/ghsa-r553-traversal-test.json"))is not guarded against pre-existing state. If a prior run (with a regressed fix, or any unrelated process) left that file on disk, this assertion fails spuriously even when the current code is correct. Delete it before the action so the test is self-contained.assertThrows(RuntimeException.class, ...)is broader than necessary — the fix throwsAppsmithPluginException. Asserting the specific type prevents an unrelatedNullPointerException/RuntimeExceptionfrom masquerading as a passing security test.
🛡️ Proposed tightening
+ Path traversalArtifact = Path.of("/tmp/ghsa-r553-traversal-test.json");
+ Files.deleteIfExists(traversalArtifact);
+
- Assertions.assertThrows(RuntimeException.class, () -> {
+ Assertions.assertThrows(AppsmithPluginException.class, () -> {
fileUtils
.saveApplicationToGitRepo(Path.of(""), appRef, "branch", false)
.block();
});
Assertions.assertFalse(
- Files.exists(Path.of("/tmp/ghsa-r553-traversal-test.json")),
+ Files.exists(traversalArtifact),
"Path traversal: file was created outside git root");🤖 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 262 - 271, Ensure the test in FileUtilsImplTest cleans up the
traversal artifact and asserts the precise exception: before calling
fileUtils.saveApplicationToGitRepo(Path.of(""), appRef, "branch",
false).block(), delete the file at "/tmp/ghsa-r553-traversal-test.json" if it
exists so the assertion is not affected by prior state; replace
Assertions.assertThrows(RuntimeException.class, ...) with
Assertions.assertThrows(AppsmithPluginException.class, ...) so the test expects
the specific exception thrown by saveApplicationToGitRepo rather than any
RuntimeException.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses GHSA-r553-q33m-v7pf by adding path traversal validation to the widget file serialization path during Git repo saves, preventing user-controlled widgetName values from escaping the configured Git root during filesystem writes.
Changes:
- Added
validatePathIsWithinGitRoot(...)checks for both the widget directory path and the final widget JSON file path before writing widget files. - Added regression and positive tests covering malicious vs. legitimate widget names during
saveApplicationToGitReposerialization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java | Adds missing path traversal validation for widget serialization write paths. |
| app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java | Adds tests intended to validate traversal rejection and ensure normal widget serialization still works. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+268
to
+270
| Assertions.assertFalse( | ||
| Files.exists(Path.of("/tmp/ghsa-r553-traversal-test.json")), | ||
| "Path traversal: file was created outside git root"); |
Comment on lines
+262
to
+266
| Assertions.assertThrows(RuntimeException.class, () -> { | ||
| fileUtils | ||
| .saveApplicationToGitRepo(Path.of(""), appRef, "branch", false) | ||
| .block(); | ||
| }); |
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
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
Fixes the path traversal vulnerability in Git widget serialization reported in GHSA-r553-q33m-v7pf.
Root cause:
FileUtilsCEImpl.updateEntitiesInRepo()writes widget files to disk using user-controlledwidgetNamevalues without callingvalidatePathIsWithinGitRoot(). All other Git file write helpers in the same class (saveResource(),saveActions(),saveActionCollection(),saveResourceCommon()) already validate paths, but the widget save path was the only one that skipped this check.Attack: An authenticated user with edit access to a Git-connected application can set
widgetNameto a path traversal payload (e.g.,../../../../tmp/evil) via the layout update API. When Git serialization is triggered (status/commit/auto-commit), the widget JSON is written outside the Git repository to an arbitrary location on the server filesystem.Fix: Add
validatePathIsWithinGitRoot()calls on both the widget directory path and the final file path before callingfileOperations.saveWidgets(), consistent with the established pattern used by every other write helper inFileUtilsCEImpl.Fixes
Testing
saveApplicationRef_pathTraversalInWidgetName_throwsSecurityException_GHSA_r553— constructs a DSL with a path traversal widget name, verifiesAppsmithPluginExceptionis thrown, verifies no file is created outside git rootsaveApplicationRef_legitimateWidgetName_doesNotThrow_GHSA_r553— verifies legitimate widget names still serialize correctlyappsmith-gittest suite: 72/72 tests passCE/EE Impact
CE-only change. The EE
FileUtilsImplcallssuper.updateEntitiesInRepo()so the fix propagates automatically. No EE-specific changes needed.Warning
Tests have not run on the HEAD eff0ba7 yet
Thu, 21 May 2026 14:02:56 UTC
Summary by CodeRabbit
Bug Fixes
Tests