fix: arbitrary file write outside repo scope vulnerability#41565
Merged
ashit-rath merged 1 commit intoreleasefrom Feb 17, 2026
Merged
fix: arbitrary file write outside repo scope vulnerability#41565ashit-rath merged 1 commit intoreleasefrom
ashit-rath merged 1 commit intoreleasefrom
Conversation
Contributor
WalkthroughThe changes introduce multi-layer path traversal prevention by adding validation helpers to check that file paths remain within the Git root and that resource names don't contain unsafe path characters before write operations and serialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~18 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 4
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ActionCollectionCE_DTO.java (2)
129-131: Duplicate path-safety logic — consider extracting a shared utility.This exact check (
..,/,\\,\0) is repeated inCommonGitFileUtilsCE.validateGitSerializableName. While duplication across layers is acceptable for defense-in-depth, extracting the predicate into a small shared utility would keep the two sites in sync if the ruleset evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ActionCollectionCE_DTO.java` around lines 129 - 131, Extract the duplicated path-safety predicate into a shared utility method and replace the local checks with calls to it: create a small utility (e.g., PathSafetyUtils.isUnsafePath(String)) that encapsulates the logic currently in containsUnsafePathCharacters and the predicate used in CommonGitFileUtilsCE.validateGitSerializableName (checking "..", "/", "\\", and '\0'), then update containsUnsafePathCharacters to delegate to that utility (or remove it and call the new utility directly) and modify CommonGitFileUtilsCE.validateGitSerializableName to use the same utility so both sites share a single authoritative rule.
104-108: Same error message for two distinct validation failures.Both the empty-name and unsafe-characters branches report
INVALID_PARAMETER(NAME). Consider using a more specific error for the path-safety check so callers/logs can distinguish between "missing name" and "name contains unsafe characters."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ActionCollectionCE_DTO.java` around lines 104 - 108, The validation currently adds the same error for empty names and unsafe-path names; update the else-if branch that calls containsUnsafePathCharacters(this.name) to add a distinct, specific error (e.g., AppsmithError.INVALID_PATH_NAME or a new error constant) instead of AppsmithError.INVALID_PARAMETER.getMessage(FieldName.NAME) so callers can distinguish missing name vs unsafe characters; modify the ActionCollectionCE_DTO validation logic to use the new/specific AppsmithError enum value (or create one) when adding to validationErrors and keep the empty-name branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`:
- Around line 671-681: In validatePathIsWithinGitRoot, keep logging the detailed
error (including normalizedTarget and gitRoot) via log.error, but change the
thrown AppsmithPluginException to use a generic error message (e.g., "SECURITY:
Path traversal detected") so internal paths are not leaked; ensure you still use
AppsmithPluginError.PLUGIN_ERROR when constructing the exception and do not
remove the detailed log call for diagnostics.
- Around line 731-733: The span
(observationHelper.createSpan(GitSpan.FILE_WRITE)) is created before the path
validation, so if validatePathIsWithinGitRoot(path) throws the span is never
closed; move the validatePathIsWithinGitRoot(path) call to occur before calling
observationHelper.createSpan(...) inside saveActionCollection, or alternatively
include both the validation and span creation inside the same try block so the
existing finally block always runs to end the span; reference
saveActionCollection, observationHelper.createSpan(GitSpan.FILE_WRITE), and
validatePathIsWithinGitRoot to locate where to reorder or wrap them.
- Around line 767-769: The span in saveActions is created before
validatePathIsWithinGitRoot, so if validation throws the span is never ended;
move the observationHelper.createSpan(GitSpan.FILE_WRITE) call to after
validatePathIsWithinGitRoot(path) in saveActions, and ensure the created Span is
closed/ended in a finally block (matching the pattern used elsewhere) so the
span is always ended even on exceptions.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java`:
- Around line 797-807: The log call in validateGitSerializableName currently
prints the raw resourceName (in log.error(..., resourceType, resourceName))
which can contain newlines or malicious log sequences; sanitize and truncate
resourceName into a safe value (e.g., remove control characters including \n and
\r, escape/replace percentage/format tokens like '%' and '${', and cap length to
a reasonable max) and use that safeName in the log.error call; keep the
AppsmithException message unchanged (or ensure it does not include raw
resourceName) and update references in validateGitSerializableName to use the
sanitized variable when logging.
---
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ActionCollectionCE_DTO.java`:
- Around line 129-131: Extract the duplicated path-safety predicate into a
shared utility method and replace the local checks with calls to it: create a
small utility (e.g., PathSafetyUtils.isUnsafePath(String)) that encapsulates the
logic currently in containsUnsafePathCharacters and the predicate used in
CommonGitFileUtilsCE.validateGitSerializableName (checking "..", "/", "\\", and
'\0'), then update containsUnsafePathCharacters to delegate to that utility (or
remove it and call the new utility directly) and modify
CommonGitFileUtilsCE.validateGitSerializableName to use the same utility so both
sites share a single authoritative rule.
- Around line 104-108: The validation currently adds the same error for empty
names and unsafe-path names; update the else-if branch that calls
containsUnsafePathCharacters(this.name) to add a distinct, specific error (e.g.,
AppsmithError.INVALID_PATH_NAME or a new error constant) instead of
AppsmithError.INVALID_PARAMETER.getMessage(FieldName.NAME) so callers can
distinguish missing name vs unsafe characters; modify the ActionCollectionCE_DTO
validation logic to use the new/specific AppsmithError enum value (or create
one) when adding to validationErrors and keep the empty-name branch unchanged.
Comment on lines
+671
to
+681
| private void validatePathIsWithinGitRoot(Path targetPath) { | ||
| Path normalizedTarget = targetPath.toAbsolutePath().normalize(); | ||
| Path gitRoot = | ||
| Paths.get(gitServiceConfig.getGitRootPath()).toAbsolutePath().normalize(); | ||
| if (!normalizedTarget.startsWith(gitRoot)) { | ||
| String errorMessage = "SECURITY: Path traversal detected. Attempted to write to " + normalizedTarget | ||
| + " which is outside the Git root " + gitRoot; | ||
| log.error(errorMessage); | ||
| throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, errorMessage); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid leaking internal paths in the thrown exception message.
The error message embedded in AppsmithPluginException includes the full normalized target path and the Git root path. If this exception propagates to an API response, it exposes internal directory structure to the caller. Log the detailed message, but throw with a generic one.
Proposed fix
private void validatePathIsWithinGitRoot(Path targetPath) {
Path normalizedTarget = targetPath.toAbsolutePath().normalize();
Path gitRoot =
Paths.get(gitServiceConfig.getGitRootPath()).toAbsolutePath().normalize();
if (!normalizedTarget.startsWith(gitRoot)) {
- String errorMessage = "SECURITY: Path traversal detected. Attempted to write to " + normalizedTarget
- + " which is outside the Git root " + gitRoot;
- log.error(errorMessage);
- throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, errorMessage);
+ log.error("SECURITY: Path traversal detected. Attempted to write to {} which is outside the Git root {}", normalizedTarget, gitRoot);
+ throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, "Path traversal detected: write target is outside the permitted directory");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void validatePathIsWithinGitRoot(Path targetPath) { | |
| Path normalizedTarget = targetPath.toAbsolutePath().normalize(); | |
| Path gitRoot = | |
| Paths.get(gitServiceConfig.getGitRootPath()).toAbsolutePath().normalize(); | |
| if (!normalizedTarget.startsWith(gitRoot)) { | |
| String errorMessage = "SECURITY: Path traversal detected. Attempted to write to " + normalizedTarget | |
| + " which is outside the Git root " + gitRoot; | |
| log.error(errorMessage); | |
| throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, errorMessage); | |
| } | |
| } | |
| private void validatePathIsWithinGitRoot(Path targetPath) { | |
| Path normalizedTarget = targetPath.toAbsolutePath().normalize(); | |
| Path gitRoot = | |
| Paths.get(gitServiceConfig.getGitRootPath()).toAbsolutePath().normalize(); | |
| if (!normalizedTarget.startsWith(gitRoot)) { | |
| log.error("SECURITY: Path traversal detected. Attempted to write to {} which is outside the Git root {}", normalizedTarget, gitRoot); | |
| throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, "Path traversal detected: write target is outside the permitted directory"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`
around lines 671 - 681, In validatePathIsWithinGitRoot, keep logging the
detailed error (including normalizedTarget and gitRoot) via log.error, but
change the thrown AppsmithPluginException to use a generic error message (e.g.,
"SECURITY: Path traversal detected") so internal paths are not leaked; ensure
you still use AppsmithPluginError.PLUGIN_ERROR when constructing the exception
and do not remove the detailed log call for diagnostics.
Comment on lines
731
to
+733
| private boolean saveActionCollection(Object sourceEntity, String body, String resourceName, Path path) { | ||
| Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); | ||
| validatePathIsWithinGitRoot(path); |
Contributor
There was a problem hiding this comment.
Span is created before the validation check — minor resource waste on rejection.
observationHelper.createSpan(GitSpan.FILE_WRITE) on Line 732 allocates a tracing span before the path is validated. If the path is rejected, the span is never ended (the finally block on Line 750 would be skipped since the exception is thrown before try). Consider moving validatePathIsWithinGitRoot before createSpan, or wrapping both in the try/finally.
Proposed fix
private boolean saveActionCollection(Object sourceEntity, String body, String resourceName, Path path) {
- Span span = observationHelper.createSpan(GitSpan.FILE_WRITE);
validatePathIsWithinGitRoot(path);
+ Span span = observationHelper.createSpan(GitSpan.FILE_WRITE);
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private boolean saveActionCollection(Object sourceEntity, String body, String resourceName, Path path) { | |
| Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); | |
| validatePathIsWithinGitRoot(path); | |
| private boolean saveActionCollection(Object sourceEntity, String body, String resourceName, Path path) { | |
| validatePathIsWithinGitRoot(path); | |
| Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); | |
| try { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`
around lines 731 - 733, The span
(observationHelper.createSpan(GitSpan.FILE_WRITE)) is created before the path
validation, so if validatePathIsWithinGitRoot(path) throws the span is never
closed; move the validatePathIsWithinGitRoot(path) call to occur before calling
observationHelper.createSpan(...) inside saveActionCollection, or alternatively
include both the validation and span creation inside the same try block so the
existing finally block always runs to end the span; reference
saveActionCollection, observationHelper.createSpan(GitSpan.FILE_WRITE), and
validatePathIsWithinGitRoot to locate where to reorder or wrap them.
Comment on lines
767
to
+769
| private boolean saveActions(Object sourceEntity, String body, String resourceName, Path path) { | ||
| Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); | ||
| validatePathIsWithinGitRoot(path); |
Contributor
There was a problem hiding this comment.
Same span-before-validation issue in saveActions.
Same as saveActionCollection — the span is created before the validation, and won't be ended if the validation throws.
Proposed fix
private boolean saveActions(Object sourceEntity, String body, String resourceName, Path path) {
- Span span = observationHelper.createSpan(GitSpan.FILE_WRITE);
validatePathIsWithinGitRoot(path);
+ Span span = observationHelper.createSpan(GitSpan.FILE_WRITE);
try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`
around lines 767 - 769, The span in saveActions is created before
validatePathIsWithinGitRoot, so if validation throws the span is never ended;
move the observationHelper.createSpan(GitSpan.FILE_WRITE) call to after
validatePathIsWithinGitRoot(path) in saveActions, and ensure the created Span is
closed/ended in a finally block (matching the pattern used elsewhere) so the
span is always ended even on exceptions.
Comment on lines
+797
to
+807
| protected static void validateGitSerializableName(String resourceName, String resourceType) { | ||
| if (resourceName != null | ||
| && (resourceName.contains("..") | ||
| || resourceName.contains("/") | ||
| || resourceName.contains("\\") | ||
| || resourceName.indexOf('\0') >= 0)) { | ||
| log.error("SECURITY: Path traversal attempt detected in {} name: '{}'", resourceType, resourceName); | ||
| throw new AppsmithException( | ||
| AppsmithError.GIT_FILE_SYSTEM_ERROR, "Path traversal detected in " + resourceType + " name"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential log injection via resourceName.
Line 803 logs the raw resourceName which may contain newline characters or log format sequences (e.g., %n, ${jndi:...}). While most modern logging frameworks (Logback/Log4j2 with recent patches) mitigate JNDI attacks, consider sanitizing or truncating the name before logging it.
Proposed fix
- log.error("SECURITY: Path traversal attempt detected in {} name: '{}'", resourceType, resourceName);
+ log.error("SECURITY: Path traversal attempt detected in {} name: '{}'",
+ resourceType, resourceName.replaceAll("[\\r\\n]", "_"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected static void validateGitSerializableName(String resourceName, String resourceType) { | |
| if (resourceName != null | |
| && (resourceName.contains("..") | |
| || resourceName.contains("/") | |
| || resourceName.contains("\\") | |
| || resourceName.indexOf('\0') >= 0)) { | |
| log.error("SECURITY: Path traversal attempt detected in {} name: '{}'", resourceType, resourceName); | |
| throw new AppsmithException( | |
| AppsmithError.GIT_FILE_SYSTEM_ERROR, "Path traversal detected in " + resourceType + " name"); | |
| } | |
| } | |
| protected static void validateGitSerializableName(String resourceName, String resourceType) { | |
| if (resourceName != null | |
| && (resourceName.contains("..") | |
| || resourceName.contains("/") | |
| || resourceName.contains("\\") | |
| || resourceName.indexOf('\0') >= 0)) { | |
| log.error("SECURITY: Path traversal attempt detected in {} name: '{}'", | |
| resourceType, resourceName.replaceAll("[\\r\\n]", "_")); | |
| throw new AppsmithException( | |
| AppsmithError.GIT_FILE_SYSTEM_ERROR, "Path traversal detected in " + resourceType + " name"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java`
around lines 797 - 807, The log call in validateGitSerializableName currently
prints the raw resourceName (in log.error(..., resourceType, resourceName))
which can contain newlines or malicious log sequences; sanitize and truncate
resourceName into a safe value (e.g., remove control characters including \n and
\r, escape/replace percentage/format tokens like '%' and '${', and cap length to
a reasonable max) and use that safeName in the log.error call; keep the
AppsmithException message unchanged (or ensure it does not include raw
resourceName) and update references in validateGitSerializableName to use the
sanitized variable when logging.
sondermanish
approved these changes
Feb 17, 2026
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
3 participants
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
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes https://linear.app/appsmith/issue/APP-14882/fix-arbitrary-file-write-vulnerability-in-git-integration
Fixes https://github.com/appsmithorg/appsmith/security/advisories/GHSA-ffr8-rvj3-ff3g
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/22088106918
Commit: e4c0c0e
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 17 Feb 2026 06:52:38 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit