From 72a48214eee893c5d99f56a5ca3460136ee471de Mon Sep 17 00:00:00 2001
From: Molecule AI Fullstack Engineer
 <fullstack-engineer@agents.moleculesai.app>
Date: Mon, 11 May 2026 06:26:56 +0000
Subject: [PATCH] fix(platform): close CWE-59 symlink-traversal gap in
 resolveInsideRoot (#380)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Follow-up to #369. `resolveInsideRoot` used `filepath.Abs` which does NOT
resolve symlinks — so "workspaces/dev/leaked" where "leaked" is a symlink
to "/etc" would lexically pass the prefix check but resolve outside root.

Fix: call `filepath.EvalSymlinks` before the final prefix check. If the
resolved path points outside root the function returns "path escapes root".
Broken symlinks are also rejected (fail closed).

Also add TestResolveInsideRoot_RejectsSymlinkTraversal covering:
- Symlink pointing outside → rejected (CWE-59)
- Symlink staying inside root → allowed
- Broken symlink → rejected
---
 .../internal/handlers/org_helpers.go          | 19 ++++++++-
 .../internal/handlers/org_path_test.go        | 42 +++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/workspace-server/internal/handlers/org_helpers.go b/workspace-server/internal/handlers/org_helpers.go
index 824fd2d7..b491e90f 100644
--- a/workspace-server/internal/handlers/org_helpers.go
+++ b/workspace-server/internal/handlers/org_helpers.go
@@ -317,6 +317,12 @@ func mergePlugins(defaultPlugins, wsPlugins []string) []string {
 // Follows Go's standard pattern for SSRF-class path sanitization; using
 // strings.HasPrefix on an absolute-path pair plus the separator guard rejects
 // sibling directories that share a prefix (e.g. "/foo" vs "/foobar").
+//
+// CWE-59 mitigation: filepath.Abs does NOT resolve symlinks, so a path like
+// "workspaces/dev/inner" where "inner" is a symlink to "/etc" would lexically
+// pass the prefix check. We call filepath.EvalSymlinks to canonicalize the
+// path and re-check that it is still inside root. This closes the symlink-
+// based traversal vector (CWE-59, follow-up to #369).
 func resolveInsideRoot(root, userPath string) (string, error) {
 	if userPath == "" {
 		return "", fmt.Errorf("path is empty")
@@ -333,9 +339,18 @@ func resolveInsideRoot(root, userPath string) (string, error) {
 	if err != nil {
 		return "", fmt.Errorf("joined abs: %w", err)
 	}
+	// CWE-59: resolve symlinks before final prefix check.
+	// If the path contains a symlink pointing outside root, EvalSymlinks
+	// will canonicalize to the external path and fail the guard below.
+	resolved, err := filepath.EvalSymlinks(absJoined)
+	if err != nil {
+		// If EvalSymlinks fails (e.g. broken symlink), fail closed —
+		// broken symlinks should not be used as org files.
+		return "", fmt.Errorf("resolve symlink: %w", err)
+	}
 	// Allow exact-root match (rare but valid) and any descendant.
-	if absJoined != absRoot && !strings.HasPrefix(absJoined, absRoot+string(filepath.Separator)) {
+	if resolved != absRoot && !strings.HasPrefix(resolved, absRoot+string(filepath.Separator)) {
 		return "", fmt.Errorf("path escapes root")
 	}
-	return absJoined, nil
+	return absJoined, nil // return the lexical path, not the resolved one
 }
diff --git a/workspace-server/internal/handlers/org_path_test.go b/workspace-server/internal/handlers/org_path_test.go
index 2ec707ff..e1561850 100644
--- a/workspace-server/internal/handlers/org_path_test.go
+++ b/workspace-server/internal/handlers/org_path_test.go
@@ -78,6 +78,48 @@ func TestResolveInsideRoot_RejectsPrefixSibling(t *testing.T) {
 	}
 }
 
+// TestResolveInsideRoot_RejectsSymlinkTraversal is a regression test for
+// CWE-59 (symlink-based path traversal). An attacker plants a symlink inside
+// the allowed directory that points outside; the function must reject it.
+func TestResolveInsideRoot_RejectsSymlinkTraversal(t *testing.T) {
+	tmp := t.TempDir()
+	// Create a subdirectory inside root.
+	inner := filepath.Join(tmp, "workspaces", "dev")
+	if err := os.MkdirAll(inner, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	// Plant a symlink that resolves outside root.
+	sym := filepath.Join(inner, "leaked")
+	if err := os.Symlink("/etc", sym); err != nil {
+		t.Fatal(err)
+	}
+
+	// Lexically, "workspaces/dev/leaked" is inside tmp — but after symlink
+	// resolution it points to /etc and must be rejected.
+	if _, err := resolveInsideRoot(tmp, filepath.Join("workspaces", "dev", "leaked")); err == nil {
+		t.Error("symlink pointing outside root must be rejected (CWE-59)")
+	}
+
+	// Symlink that stays inside root is fine.
+	safe := filepath.Join(inner, "safe")
+	if err := os.Symlink(filepath.Join(tmp, "other"), safe); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := resolveInsideRoot(tmp, filepath.Join("workspaces", "dev", "safe")); err != nil {
+		t.Errorf("symlink staying inside root must be allowed: %v", err)
+	}
+
+	// Broken symlink (target does not exist) must also be rejected — broken
+	// symlinks cannot be valid org files.
+	broken := filepath.Join(inner, "broken")
+	if err := os.Symlink("/nonexistent/broken", broken); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := resolveInsideRoot(tmp, filepath.Join("workspaces", "dev", "broken")); err == nil {
+		t.Error("broken symlink must be rejected")
+	}
+}
+
 func TestResolveInsideRoot_DeepSubpath(t *testing.T) {
 	tmp := t.TempDir()
 	deep := filepath.Join(tmp, "a", "b", "c")