Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.io.StringWriter;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.MessageDigest;
Expand Down Expand Up @@ -671,17 +672,65 @@ protected Set<String> updateEntitiesInRepo(ApplicationGitReference applicationGi
* @throws AppsmithPluginException if the path escapes the Git root directory
*/
protected void validatePathIsWithinGitRoot(Path targetPath) {
Path normalizedTarget = targetPath.toAbsolutePath().normalize();
Path gitRoot =
Paths.get(gitServiceConfig.getGitRootPath()).toAbsolutePath().normalize();

// 1. Lexical containment check — blocks "../" traversal in crafted resource names.
Path normalizedTarget = targetPath.toAbsolutePath().normalize();
if (!normalizedTarget.startsWith(gitRoot)) {
String errorMessage = "SECURITY: Path traversal detected. Attempted to access " + normalizedTarget
+ " which is outside the Git root " + gitRoot;
log.error(errorMessage);
throwPathTraversal(normalizedTarget, gitRoot);
}

// 2. Symlink-aware containment check (GHSA-fqwc-g9wm-5895) — blocks symbolic links committed
// inside a repository that point outside the Git root. Path.normalize() above is purely
// lexical and does NOT resolve symlinks, whereas every downstream file I/O sink follows
// them. Resolve the real (symlink-free) path before comparing. Fails closed on I/O error.
try {
Path realGitRoot = toRealPathResolvingExistingPrefix(gitRoot);
Path realTarget = toRealPathResolvingExistingPrefix(normalizedTarget);
if (!realTarget.startsWith(realGitRoot)) {
throwPathTraversal(realTarget, realGitRoot);
}
} catch (IOException e) {
String errorMessage = "SECURITY: Unable to resolve real path for " + normalizedTarget
+ " while validating Git root containment";
log.error(errorMessage, e);
throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, errorMessage);
}
}

private void throwPathTraversal(Path attemptedPath, Path gitRoot) {
String errorMessage = "SECURITY: Path traversal detected. Attempted to access " + attemptedPath
+ " which is outside the Git root " + gitRoot;
log.error(errorMessage);
throw new AppsmithPluginException(AppsmithPluginError.PLUGIN_ERROR, errorMessage);
}

/**
* Resolves symbolic links in the longest existing prefix of {@code path} and re-appends the
* remaining (not-yet-created) path segments lexically. {@link Path#toRealPath} cannot be used
* directly because it requires the whole path to exist, while file writes legitimately target
* paths that do not exist yet. By resolving the deepest existing ancestor we still detect any
* symlink along the existing portion (including when {@code path} itself is a symlink) while
* supporting yet-to-be-created files.
*/
private Path toRealPathResolvingExistingPrefix(Path path) throws IOException {
Path absolute = path.toAbsolutePath().normalize();
Path existing = absolute;
while (existing != null && !Files.exists(existing, LinkOption.NOFOLLOW_LINKS)) {
existing = existing.getParent();
}
if (existing == null) {
// No component of the path exists; no symlink can be involved.
return absolute;
}
Path realExisting = existing.toRealPath();
if (existing.equals(absolute)) {
return realExisting;
}
return realExisting.resolve(existing.relativize(absolute)).normalize();
}

protected Object readFileValidated(Path filePath) {
validatePathIsWithinGitRoot(filePath);
return fileOperations.readFile(filePath);
Expand Down Expand Up @@ -818,6 +867,10 @@ private boolean saveActions(Object sourceEntity, String body, String resourceNam
}

private void writeStringToFile(String sourceEntity, Path path) throws IOException {
// 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);
Comment on lines +870 to +873

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.

try (BufferedWriter fileWriter = Files.newBufferedWriter(path, StandardCharsets.UTF_8)) {
fileWriter.write(sourceEntity);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.BranchTrackingStatus;
import org.eclipse.jgit.lib.ConfigConstants;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
Expand All @@ -62,6 +63,7 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
Expand Down Expand Up @@ -408,6 +410,11 @@ public Mono<String> cloneRemoteIntoArtifactRepo(
.call()) {
branchName = git.getRepository().getBranch();

// SECURITY (GHSA-fqwc-g9wm-5895): disable symlink materialization and
// re-checkout any symlinks as regular files. A malicious repo can commit
// symlink entries (git mode 120000) pointing outside the Git root.
removeSymlinksAfterClone(git);

repositoryHelper.updateRemoteBranchTrackingConfig(branchName, git);
}
processStopwatch.stopAndLogTimeInMillis();
Expand All @@ -420,6 +427,30 @@ public Mono<String> cloneRemoteIntoArtifactRepo(
.subscribeOn(scheduler));
}

/**
* Disables symlink support and re-materializes any symlinks as regular files
* (GHSA-fqwc-g9wm-5895).
*/
private 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();
}

@Override
public Mono<String> createAndCheckoutToBranch(Path repoSuffix, String branchName) {
// We can safely assume that repo has been already initialised either in commit or clone flow and can directly
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.eclipse.jgit.api.errors.CheckoutConflictException;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.lib.BranchTrackingStatus;
import org.eclipse.jgit.lib.ConfigConstants;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.StoredConfig;
Expand All @@ -53,6 +54,7 @@

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
Expand Down Expand Up @@ -317,6 +319,11 @@ public Mono<String> cloneRemoteIntoArtifactRepo(
.call()) {
branchName = git.getRepository().getBranch();

// SECURITY (GHSA-fqwc-g9wm-5895): disable symlink materialization and
// re-checkout any symlinks as regular files. A malicious repo can commit
// symlink entries (git mode 120000) pointing outside the Git root.
removeSymlinksAfterClone(git);

repositoryHelper.updateRemoteBranchTrackingConfig(branchName, git);
}
processStopwatch.stopAndLogTimeInMillis();
Expand All @@ -329,6 +336,34 @@ public Mono<String> cloneRemoteIntoArtifactRepo(
.subscribeOn(scheduler);
}

/**
* 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();
}
Comment on lines +339 to +365

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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 any Files.delete failure 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.


@Override
public Mono<String> createAndCheckoutToBranch(Path repoSuffix, String branchName) {
// We can safely assume that repo has been already initialised either in commit or clone flow and can directly
Expand Down
1 change: 1 addition & 0 deletions app/server/appsmith-git/src/main/resources/git.sh
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ git_clone_and_checkout() {
mkdir -p "$target_folder"

git_clone "$private_key" "$remote_url" "$target_folder" "$git_root"
git -C "$target_folder" config core.symlinks false
git -C "$target_folder" config user.name "$author_name"
git -C "$target_folder" config user.email "$author_email"
git -C "$target_folder" config fetch.parallel 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,58 @@ public void saveApplicationRef_legitimateWidgetName_doesNotThrow_GHSA_r553() thr
Assertions.assertTrue(Files.exists(widgetFile), "Legitimate widget file should exist");
}

/**
* 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());
}
}
Comment on lines +336 to +370

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.


/**
* GHSA-fqwc-g9wm-5895: A legitimate (non-symlink) file inside the git root must still be readable.
* Regression guard ensuring the symlink-aware check does not break normal reads.
*/
@Test
public void reconstructMetadata_regularFileWithinGitRoot_doesNotThrow_GHSA_fqwc() throws IOException {
Path repoSuffix = Path.of("workspace-regular", "app", "repo");
Path fullRepoPath = localTestDirectoryPath.resolve(repoSuffix);
Files.createDirectories(fullRepoPath);
Files.writeString(fullRepoPath.resolve("metadata.json"), "{\"fileFormatVersion\": 5}");

Object result =
fileUtils.reconstructMetadataFromGitRepository(repoSuffix).block();
Assertions.assertNotNull(result);
}

/**
* This will delete localTestDirectory and its contents after the test is executed.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package com.appsmith.git.service.ce;

import com.appsmith.external.helpers.ObservationHelper;
import com.appsmith.git.configurations.GitServiceConfig;
import io.micrometer.observation.ObservationRegistry;
import org.apache.commons.io.FileUtils;
import org.eclipse.jgit.api.Git;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.io.File;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.Path;

/**
* GHSA-fqwc-g9wm-5895: Git import symlink path traversal.
*
* <p>A malicious repository can commit a symbolic link (git mode 120000) pointing at an arbitrary
* server path. When cloned, JGit's default behaviour materializes it as a real filesystem symlink
* inside the Git root; subsequent file reads/writes follow it, enabling arbitrary file read/write.
* The fix disables symlink support ({@code core.symlinks=false}) before checkout so the entry is
* written as an inert regular file.
*/
public class GitExecutorSymlinkCloneTest {

private GitExecutorCEImpl newGitExecutor(Path gitRoot) {
GitServiceConfig gitServiceConfig = new GitServiceConfig();
gitServiceConfig.setGitRootPath(gitRoot.toString());
return new GitExecutorCEImpl(gitServiceConfig, null, ObservationRegistry.NOOP, ObservationHelper.NOOP, null);
}

private String buildSourceRepoWithSymlink(Path srcDir) throws Exception {
try (Git srcGit = Git.init().setDirectory(srcDir.toFile()).call()) {
Files.writeString(srcDir.resolve("normal.txt"), "hello");
// Symlink committed as git mode 120000; target need not exist for the entry to be stored.
Files.createSymbolicLink(srcDir.resolve("evil.txt"), Path.of("/etc/passwd"));
srcGit.add().addFilepattern(".").call();
srcGit.commit()
.setMessage("init")
.setSign(false)
.setAuthor("tester", "tester@appsmith.test")
.setCommitter("tester", "tester@appsmith.test")
.call();
}
return srcDir.toUri().toString();
}

@Test
public void cloneRepoWithSymlinkEntry_isCheckedOutAsRegularFile_GHSA_fqwc() throws Exception {
Path srcDir = Files.createTempDirectory("ghsa-fqwc-src");
Path controlRoot = Files.createTempDirectory("ghsa-fqwc-control");
Path fixRoot = Files.createTempDirectory("ghsa-fqwc-fix");
try {
String uri = buildSourceRepoWithSymlink(srcDir);

// Control: a default checkout materializes the entry as a REAL symlink (the vulnerability).
File controlDest = controlRoot.resolve("clone").toFile();
try (Git control =
Git.cloneRepository().setURI(uri).setDirectory(controlDest).call()) {
Path controlEvil = controlDest.toPath().resolve("evil.txt");
Assertions.assertTrue(
Files.isSymbolicLink(controlEvil),
"Sanity check: without hardening JGit materializes a real symlink");
}

// Fix: normal clone, then removeSymlinksAfterClone converts them to regular files.
File dest = fixRoot.resolve("clone").toFile();
try (Git cloned =
Git.cloneRepository().setURI(uri).setDirectory(dest).call()) {

newGitExecutor(fixRoot).removeSymlinksAfterClone(cloned);

Path evil = dest.toPath().resolve("evil.txt");
Assertions.assertTrue(Files.exists(evil, LinkOption.NOFOLLOW_LINKS), "The entry should be checked out");
Assertions.assertFalse(
Files.isSymbolicLink(evil),
"With core.symlinks=false the entry must be a regular file, not a symlink");
Assertions.assertTrue(Files.isRegularFile(evil, LinkOption.NOFOLLOW_LINKS));
Assertions.assertEquals(
"/etc/passwd",
Files.readString(evil).trim(),
"The symlink target text is stored as plain file content");
// Legitimate regular files are unaffected.
Assertions.assertEquals("hello", Files.readString(dest.toPath().resolve("normal.txt")));
}
} finally {
FileUtils.deleteDirectory(srcDir.toFile());
FileUtils.deleteDirectory(controlRoot.toFile());
FileUtils.deleteDirectory(fixRoot.toFile());
}
}
}
Loading