diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java index abf326a644fe..96340710f8f8 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java @@ -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; @@ -671,17 +672,65 @@ protected Set 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); @@ -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); try (BufferedWriter fileWriter = Files.newBufferedWriter(path, StandardCharsets.UTF_8)) { fileWriter.write(sourceEntity); } diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java index d822041a8aec..d7074b913175 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java @@ -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; @@ -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; @@ -408,6 +410,11 @@ public Mono 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(); @@ -420,6 +427,30 @@ public Mono 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 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 createAndCheckoutToBranch(Path repoSuffix, String branchName) { // We can safely assume that repo has been already initialised either in commit or clone flow and can directly diff --git a/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java b/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java index 47f8181958c0..42641c6887ca 100644 --- a/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java +++ b/app/server/appsmith-git/src/main/java/com/appsmith/git/service/ce/GitExecutorCEImpl.java @@ -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; @@ -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; @@ -317,6 +319,11 @@ public Mono 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(); @@ -329,6 +336,34 @@ public Mono cloneRemoteIntoArtifactRepo( .subscribeOn(scheduler); } + /** + * Disables symlink support and re-materializes any symlinks as regular files + * (GHSA-fqwc-g9wm-5895). + * + *

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 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 createAndCheckoutToBranch(Path repoSuffix, String branchName) { // We can safely assume that repo has been already initialised either in commit or clone flow and can directly diff --git a/app/server/appsmith-git/src/main/resources/git.sh b/app/server/appsmith-git/src/main/resources/git.sh index 41b2c29331b1..fbb8227f3a19 100644 --- a/app/server/appsmith-git/src/main/resources/git.sh +++ b/app/server/appsmith-git/src/main/resources/git.sh @@ -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 diff --git a/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java b/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java index 4832780665d8..11dac2fe40fc 100644 --- a/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java +++ b/app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java @@ -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()); + } + } + + /** + * 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. */ diff --git a/app/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java b/app/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java new file mode 100644 index 000000000000..56226e3ce454 --- /dev/null +++ b/app/server/appsmith-git/src/test/java/com/appsmith/git/service/ce/GitExecutorSymlinkCloneTest.java @@ -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. + * + *

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()); + } + } +}