-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix: arbitrary file write outside repo scope vulnerability #41565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -659,6 +659,27 @@ protected Set<String> updateEntitiesInRepo(ApplicationGitReference applicationGi | |||||||||||||||
| return validPages; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * Validates that the given target path, after normalization, is still contained within | ||||||||||||||||
| * the configured Git root directory. This is a defense-in-depth measure to prevent | ||||||||||||||||
| * path traversal attacks where crafted resource names could cause file writes outside | ||||||||||||||||
| * the repository. | ||||||||||||||||
| * | ||||||||||||||||
| * @param targetPath the resolved path intended for a file write | ||||||||||||||||
| * @throws AppsmithPluginException if the path escapes the Git root directory | ||||||||||||||||
| */ | ||||||||||||||||
| 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); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * This method will be used to store the DB resource to JSON file | ||||||||||||||||
| * | ||||||||||||||||
|
|
@@ -667,6 +688,7 @@ protected Set<String> updateEntitiesInRepo(ApplicationGitReference applicationGi | |||||||||||||||
| * @return if the file operation is successful | ||||||||||||||||
| */ | ||||||||||||||||
| protected boolean saveResource(Object sourceEntity, Path path) { | ||||||||||||||||
| validatePathIsWithinGitRoot(path); | ||||||||||||||||
| try { | ||||||||||||||||
| Files.createDirectories(path.getParent()); | ||||||||||||||||
| return fileOperations.writeToFile(sourceEntity, path); | ||||||||||||||||
|
|
@@ -678,6 +700,7 @@ protected boolean saveResource(Object sourceEntity, Path path) { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| protected void saveResourceCommon(Object sourceEntity, Path path) { | ||||||||||||||||
| validatePathIsWithinGitRoot(path); | ||||||||||||||||
| try { | ||||||||||||||||
| Files.createDirectories(path.getParent()); | ||||||||||||||||
| if (sourceEntity instanceof String s) { | ||||||||||||||||
|
|
@@ -707,6 +730,7 @@ protected void saveResourceCommon(Object sourceEntity, Path path) { | |||||||||||||||
| */ | ||||||||||||||||
| private boolean saveActionCollection(Object sourceEntity, String body, String resourceName, Path path) { | ||||||||||||||||
| Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); | ||||||||||||||||
| validatePathIsWithinGitRoot(path); | ||||||||||||||||
|
Comment on lines
731
to
+733
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Span is created before the validation check — minor resource waste on rejection.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| try { | ||||||||||||||||
| Files.createDirectories(path); | ||||||||||||||||
| if (StringUtils.hasText(body)) { | ||||||||||||||||
|
|
@@ -742,6 +766,7 @@ private boolean saveActionCollection(Object sourceEntity, String body, String re | |||||||||||||||
| */ | ||||||||||||||||
| private boolean saveActions(Object sourceEntity, String body, String resourceName, Path path) { | ||||||||||||||||
| Span span = observationHelper.createSpan(GitSpan.FILE_WRITE); | ||||||||||||||||
| validatePathIsWithinGitRoot(path); | ||||||||||||||||
|
Comment on lines
767
to
+769
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same span-before-validation issue in Same as 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 |
||||||||||||||||
| try { | ||||||||||||||||
| Files.createDirectories(path); | ||||||||||||||||
| // Write the user written query to .txt file to make conflict handling easier | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -292,6 +292,7 @@ protected void setArtifactIndependentResources( | |||||||||||||||||||||||||||||||||||||||||||||||
| if (datasourceList != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| datasourceList.forEach(datasource -> { | ||||||||||||||||||||||||||||||||||||||||||||||||
| removeUnwantedFieldsFromDatasource(datasource); | ||||||||||||||||||||||||||||||||||||||||||||||||
| validateGitSerializableName(datasource.getName(), "Datasource"); | ||||||||||||||||||||||||||||||||||||||||||||||||
| final String filePath = DATASOURCE_DIRECTORY + DELIMITER_PATH + datasource.getName() + JSON_EXTENSION; | ||||||||||||||||||||||||||||||||||||||||||||||||
| GitResourceIdentity identity = | ||||||||||||||||||||||||||||||||||||||||||||||||
| new GitResourceIdentity(GitResourceType.DATASOURCE_CONFIG, datasource.getGitSyncId(), filePath); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -369,6 +370,7 @@ protected void setNewActionsInResourceMap( | |||||||||||||||||||||||||||||||||||||||||||||||
| .forEach(newAction -> { | ||||||||||||||||||||||||||||||||||||||||||||||||
| ActionDTO action = newAction.getUnpublishedAction(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| final String actionFileName = action.getUserExecutableName().replace(".", "-"); | ||||||||||||||||||||||||||||||||||||||||||||||||
| validateGitSerializableName(actionFileName, "Action"); | ||||||||||||||||||||||||||||||||||||||||||||||||
| final String filePathPrefix = getContextDirectoryByType(action.getContextType()) | ||||||||||||||||||||||||||||||||||||||||||||||||
| + DELIMITER_PATH | ||||||||||||||||||||||||||||||||||||||||||||||||
| + action.calculateContextId() | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -429,6 +431,7 @@ protected void setActionCollectionsInResourceMap( | |||||||||||||||||||||||||||||||||||||||||||||||
| .forEach(actionCollection -> { | ||||||||||||||||||||||||||||||||||||||||||||||||
| ActionCollectionDTO collection = actionCollection.getUnpublishedCollection(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| final String collectionName = collection.getUserExecutableName(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| validateGitSerializableName(collectionName, "ActionCollection"); | ||||||||||||||||||||||||||||||||||||||||||||||||
| final String filePathPrefix = getContextDirectoryByType(collection.getContextType()) | ||||||||||||||||||||||||||||||||||||||||||||||||
| + DELIMITER_PATH | ||||||||||||||||||||||||||||||||||||||||||||||||
| + collection.calculateContextId() | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -782,6 +785,27 @@ public Mono<Boolean> checkIfDirectoryIsEmpty(Path baseRepoSuffix) throws IOExcep | |||||||||||||||||||||||||||||||||||||||||||||||
| .onErrorResume(e -> Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e))); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Validates that a resource name intended for use as a Git file path segment does not contain | ||||||||||||||||||||||||||||||||||||||||||||||||
| * path traversal sequences or directory separator characters. This is a defense-in-depth measure | ||||||||||||||||||||||||||||||||||||||||||||||||
| * that prevents arbitrary file writes even if input validation at the API layer is bypassed. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param resourceName the name to validate | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @param resourceType a human-readable description of the resource type (for error messages) | ||||||||||||||||||||||||||||||||||||||||||||||||
| * @throws AppsmithException if the name contains path traversal or directory separator characters | ||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||
| 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"); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+797
to
+807
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential log injection via Line 803 logs the raw 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| public static void removeUnwantedFieldsFromBaseDomain(BaseDomain baseDomain) { | ||||||||||||||||||||||||||||||||||||||||||||||||
| baseDomain.setPolicies(null); | ||||||||||||||||||||||||||||||||||||||||||||||||
| baseDomain.setUserPermissions(null); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid leaking internal paths in the thrown exception message.
The error message embedded in
AppsmithPluginExceptionincludes 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
🤖 Prompt for AI Agents