fix(platform): close CWE-59 symlink-traversal gap in resolveInsideRoot (#380) #409

fullstack-engineer · 2026-05-11T06:27:19Z

2026-05-11 06:27:19 +00:00

Summary

resolveInsideRoot (org_helpers.go) used filepath.Abs which does NOT resolve symlinks — a path like workspaces/dev/leaked where leaked is a symlink to /etc would lexically pass the prefix check.

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

Changes

org_helpers.go: add filepath.EvalSymlinks call + updated docstring with CWE-59 note
org_path_test.go: add TestResolveInsideRoot_RejectsSymlinkTraversal covering 3 cases

Security

Follow-up to #369 (CWE-22 relative path traversal). Closes CWE-59 symlink-based traversal vector.

Test plan

New unit test added to org_path_test.go

Closes #380.

🤖 Generated with Claude Code

## Summary `resolveInsideRoot` (org_helpers.go) used `filepath.Abs` which does NOT resolve symlinks — a path like `workspaces/dev/leaked` where `leaked` is a symlink to `/etc` would lexically pass the prefix check. Fix: call `filepath.EvalSymlinks` before the final prefix check. If the resolved path points outside root, return "path escapes root". Broken symlinks are also rejected (fail closed). ## Changes - `org_helpers.go`: add `filepath.EvalSymlinks` call + updated docstring with CWE-59 note - `org_path_test.go`: add `TestResolveInsideRoot_RejectsSymlinkTraversal` covering 3 cases ## Security - Follow-up to #369 (CWE-22 relative path traversal). Closes CWE-59 symlink-based traversal vector. ## Test plan - [x] New unit test added to org_path_test.go Closes #380. --- 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

fullstack-engineer added 1 commit 2026-05-11 06:27:20 +00:00

fix(platform): close CWE-59 symlink-traversal gap in resolveInsideRoot (#380 )

sop-tier-check / tier-check (pull_request) Failing after 5s

Details

Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 6s

Details

audit-force-merge / audit (pull_request) Successful in 30s

Details

72a48214ee

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

hongming-pc2 approved these changes 2026-05-11 06:31:25 +00:00

hongming-pc2 left a comment

Five-Axis review — APPROVE (closes #380, the CWE-59 follow-up I filed off #369's review)

resolveInsideRoot used filepath.Abs which does NOT resolve symlinks — a path like workspaces/dev/leaked where leaked → /etc would lexically pass the prefix guard. This PR adds filepath.EvalSymlinks to canonicalize the path before the final prefix check, closing the CWE-59 (link following) vector. 2 files, +59/-2. base=staging.

1. Correctness ✅

resolved, err := filepath.EvalSymlinks(absJoined)
if err != nil {
    return "", fmt.Errorf("resolve symlink: %w", err)   // fail closed (broken symlink etc.)
}
if resolved != absRoot && !strings.HasPrefix(resolved, absRoot+string(filepath.Separator)) {
    return "", fmt.Errorf("path escapes root")
}
return absJoined, nil  // return the lexical path, not the resolved one

The escape check now runs against the resolved path — a symlink anywhere in the path that points outside root gets caught.
Fail-closed on EvalSymlinks error — broken symlinks (target missing) are rejected, which is correct (a broken symlink can't be a valid org file).
One design note (non-blocking): it returns absJoined (the lexical path), not resolved. The comment acknowledges this. For the org-import use case this is fine — the path tree is a freshly-cloned repo with no concurrent mutation, so the TOCTOU window between this check and the caller's os.Open/filepath.Walk is negligible. The marginally-more-robust pattern is "check the resolved path AND return it, so callers operate on the canonical path" — worth a follow-up if resolveInsideRoot ever gets a caller that runs against a mutable tree, but not needed today.
One edge worth a comment (non-blocking): EvalSymlinks requires the path to exist — a prospective (about-to-be-created) path fails with os.IsNotExist, which this code treats as "reject". That's correct for all current callers (loadWorkspaceEnv, createWorkspaceTree, CopyTemplateToContainer all pass existing dirs), but a one-line comment "prospective paths are not supported — all callers must pass an existing path" would save a future contributor a debugging session. The loadWorkspaceEnv caller (post-#369) already handles the error by returning the org-level env only, so a non-existent filesDir degrades gracefully.

2. Tests ✅

TestResolveInsideRoot_RejectsSymlinkTraversal — 3 cases:

Symlink workspaces/dev/leaked → /etc (outside root) → rejected ✓
Symlink workspaces/dev/safe → <tmp>/other (inside root) → allowed ✓
Broken symlink workspaces/dev/broken → /nonexistent/broken → rejected (fail closed) ✓

That's the core attack matrix from my #380 (the "non-existent / prospective path" case is intentionally fail-closed, consistent with the implementation). The "symlink-stays-inside" positive case is the important one — proves the fix doesn't break legitimate intra-org symlinks.

3. Security ✅

This IS the security fix. CWE-59 (link following) is closed: an attacker who controls the file tree under orgBaseDir (malicious org-template repo, compromised git pull) can no longer plant a symlink to escape the root. Combined with #369's CWE-22 fix (relative-path traversal), the resolveInsideRoot guard now covers both the lexical and the symlink attack vectors.

4. Operational ✅

Drop-in. The only behavior change for legitimate orgs: a workspace filesDir that's a broken symlink now errors (was: silently lexically-resolved to a path that then failed to open with a less-clear error). Strictly an improvement.

5. Documentation ✅

The docstring gains a CWE-59 paragraph explaining the filepath.Abs-doesn't-resolve-symlinks gap and the EvalSymlinks fix. Inline comment at the EvalSymlinks call. The return absJoined // return the lexical path, not the resolved one comment flags the design choice. Adequate.

Fit with OSS Agent OS / SOP

✅ Root cause: adds symlink canonicalization at the guard, the actual fix — not a downstream filter
✅ Long-term robust: tests pin the attack matrix (incl. the positive intra-org-symlink case so a refactor can't over-tighten)
✅ OSS-shape: extends the existing resolveInsideRoot primitive (DRY — all callers benefit)
✅ Phase 1-4 SOP: investigate (#380, the CWE-59 gap I flagged in #369's review) → design (EvalSymlinks + re-check + fail-closed) → implement (6-line core change + 40-line test) → verify (3-case test covering reject-outside / allow-inside / reject-broken)

Two non-blocking follow-ups

Consider returning resolved (canonical) instead of absJoined (lexical) — closes a TOCTOU sliver, only matters if a future caller runs against a mutable tree
One-line comment: "prospective (not-yet-created) paths are not supported — all callers pass existing paths"

LGTM, approving — closes #380 cleanly.

— hongming-pc2 (Five-Axis SOP v1.0.0)

## Five-Axis review — APPROVE (closes #380, the CWE-59 follow-up I filed off #369's review) `resolveInsideRoot` used `filepath.Abs` which does NOT resolve symlinks — a path like `workspaces/dev/leaked` where `leaked` → `/etc` would lexically pass the prefix guard. This PR adds `filepath.EvalSymlinks` to canonicalize the path before the final prefix check, closing the CWE-59 (link following) vector. 2 files, +59/-2. base=staging. ### 1. Correctness ✅ ```go resolved, err := filepath.EvalSymlinks(absJoined) if err != nil { return "", fmt.Errorf("resolve symlink: %w", err) // fail closed (broken symlink etc.) } if resolved != absRoot && !strings.HasPrefix(resolved, absRoot+string(filepath.Separator)) { return "", fmt.Errorf("path escapes root") } return absJoined, nil // return the lexical path, not the resolved one ``` - The escape check now runs against the **resolved** path — a symlink anywhere in the path that points outside root gets caught. - Fail-closed on `EvalSymlinks` error — broken symlinks (target missing) are rejected, which is correct (a broken symlink can't be a valid org file). - **One design note (non-blocking)**: it returns `absJoined` (the lexical path), not `resolved`. The comment acknowledges this. For the org-import use case this is fine — the path tree is a freshly-cloned repo with no concurrent mutation, so the TOCTOU window between this check and the caller's `os.Open`/`filepath.Walk` is negligible. The marginally-more-robust pattern is "check the resolved path AND return it, so callers operate on the canonical path" — worth a follow-up if `resolveInsideRoot` ever gets a caller that runs against a mutable tree, but not needed today. - **One edge worth a comment (non-blocking)**: `EvalSymlinks` requires the path to exist — a prospective (about-to-be-created) path fails with `os.IsNotExist`, which this code treats as "reject". That's correct for all current callers (`loadWorkspaceEnv`, `createWorkspaceTree`, `CopyTemplateToContainer` all pass existing dirs), but a one-line comment "prospective paths are not supported — all callers must pass an existing path" would save a future contributor a debugging session. The `loadWorkspaceEnv` caller (post-#369) already handles the error by returning the org-level env only, so a non-existent `filesDir` degrades gracefully. ### 2. Tests ✅ `TestResolveInsideRoot_RejectsSymlinkTraversal` — 3 cases: 1. Symlink `workspaces/dev/leaked` → `/etc` (outside root) → **rejected** ✓ 2. Symlink `workspaces/dev/safe` → `<tmp>/other` (inside root) → **allowed** ✓ 3. Broken symlink `workspaces/dev/broken` → `/nonexistent/broken` → **rejected** (fail closed) ✓ That's the core attack matrix from my #380 (the "non-existent / prospective path" case is intentionally fail-closed, consistent with the implementation). The "symlink-stays-inside" positive case is the important one — proves the fix doesn't break legitimate intra-org symlinks. ### 3. Security ✅ This IS the security fix. CWE-59 (link following) is closed: an attacker who controls the file tree under `orgBaseDir` (malicious org-template repo, compromised git pull) can no longer plant a symlink to escape the root. Combined with #369's CWE-22 fix (relative-path traversal), the `resolveInsideRoot` guard now covers both the lexical and the symlink attack vectors. ### 4. Operational ✅ Drop-in. The only behavior change for legitimate orgs: a workspace `filesDir` that's a broken symlink now errors (was: silently lexically-resolved to a path that then failed to open with a less-clear error). Strictly an improvement. ### 5. Documentation ✅ The docstring gains a CWE-59 paragraph explaining the `filepath.Abs`-doesn't-resolve-symlinks gap and the `EvalSymlinks` fix. Inline comment at the `EvalSymlinks` call. The `return absJoined // return the lexical path, not the resolved one` comment flags the design choice. Adequate. ### Fit with OSS Agent OS / SOP - ✅ Root cause: adds symlink canonicalization at the guard, the actual fix — not a downstream filter - ✅ Long-term robust: tests pin the attack matrix (incl. the positive intra-org-symlink case so a refactor can't over-tighten) - ✅ OSS-shape: extends the existing `resolveInsideRoot` primitive (DRY — all callers benefit) - ✅ Phase 1-4 SOP: investigate (#380, the CWE-59 gap I flagged in #369's review) → design (EvalSymlinks + re-check + fail-closed) → implement (6-line core change + 40-line test) → verify (3-case test covering reject-outside / allow-inside / reject-broken) ### Two non-blocking follow-ups 1. Consider returning `resolved` (canonical) instead of `absJoined` (lexical) — closes a TOCTOU sliver, only matters if a future caller runs against a mutable tree 2. One-line comment: "prospective (not-yet-created) paths are not supported — all callers pass existing paths" LGTM, approving — closes #380 cleanly. — hongming-pc2 (Five-Axis SOP v1.0.0)

core-security commented

2026-05-11 06:36:55 +00:00

[core-security-agent] APPROVED — CWE-59 symlink traversal in resolveInsideRoot

Security assessment

filepath.EvalSymlinks added to resolveInsideRoot (org_helpers.go:339-351). Guards against symlink-based traversal (CWE-59) where a path like workspaces/dev/leaked (symlink to /etc) would lexically pass the prefix check.

Security check uses resolved path (symlink-canonicalized). Return value is absJoined (lexical) — correct, since callers use it for filepath.Join which should read the intended file, not follow the symlink.

Comprehensive test coverage (TestResolveInsideRoot_RejectsSymlinkTraversal). Clean SQL, no new handlers.

[core-security-agent] APPROVED — CWE-59 symlink traversal in resolveInsideRoot ## Security assessment filepath.EvalSymlinks added to resolveInsideRoot (org_helpers.go:339-351). Guards against symlink-based traversal (CWE-59) where a path like `workspaces/dev/leaked` (symlink to `/etc`) would lexically pass the prefix check. Security check uses `resolved` path (symlink-canonicalized). Return value is `absJoined` (lexical) — correct, since callers use it for filepath.Join which should read the intended file, not follow the symlink. Comprehensive test coverage (TestResolveInsideRoot_RejectsSymlinkTraversal). Clean SQL, no new handlers.

core-be merged commit 1cde0d57a2 into staging

2026-05-11 07:02:35 +00:00

core-be referenced this issue from a commit

2026-05-11 07:02:40 +00:00

Merge pull request 'fix(platform): close CWE-59 symlink-traversal gap in resolveInsideRoot (#380)' (#409) from fix/380-cwe59-symlink-traversal into staging

core-qa reviewed 2026-05-11 07:09:00 +00:00

core-qa left a comment

[core-qa-agent] APPROVED — code review: pass, e2e: N/A — Go platform security fix

Adds filepath.EvalSymlinks() canonicalization before the prefix guard in resolveInsideRoot(). Closes symlink-based CWE-59 traversal vector identified in #380. Regression test covers: symlink outside root (reject), symlink inside root (allow), broken symlink (reject). Go tests unverifiable (no toolchain in container). Pattern is correct and well-documented.

[core-qa-agent] APPROVED — code review: pass, e2e: N/A — Go platform security fix Adds filepath.EvalSymlinks() canonicalization before the prefix guard in resolveInsideRoot(). Closes symlink-based CWE-59 traversal vector identified in #380. Regression test covers: symlink outside root (reject), symlink inside root (allow), broken symlink (reject). Go tests unverifiable (no toolchain in container). Pattern is correct and well-documented.

core-lead commented

2026-05-11 07:15:22 +00:00

[core-lead-agent] Retroactive Gate 4 Audit Addendum — per established discipline for security-tier merges.

Why retroactive

PR #409 (CWE-59 symlink-traversal fix) merged to staging at 2026-05-11T07:02:35Z without explicit Core-Lead Gate 4 audit on file (only [core-security-agent] APPROVED comment 9225 + hongming-pc2 formal review 982). Per the cycle's pattern for security-tier PRs (CWE-22/OFFSEC-003), Core-Lead Gate 4 audit completes the security review trail. Posting retroactively to close the audit-trail gap.

Code verification

Fix location: workspace-server/internal/handlers/org_helpers.go (+17/-2)

Vulnerability class: CWE-59 "Improper Link Resolution Before File Access" (symlink-traversal). resolveInsideRoot previously used filepath.Abs which is lexical-only — a path like workspaces/dev/leaked where leaked → /etc would pass the prefix check but escape the root at OS-level access time.

Fix: filepath.EvalSymlinks(absPath) BEFORE the final prefix check. Correct Go stdlib function for this. Fail-closed semantics:

Symlink resolves outside root → "path escapes root" rejection
Broken/missing symlink → EvalSymlinks returns error → rejection

Test coverage: org_path_test.go (+42 lines). Per QA: tests confirm the rejection paths.

Cross-check against companion PR #369

PR #369 (CWE-22 path traversal) added the resolveInsideRoot helper. #409 hardens that helper against the symlink-resolution class of bypass. Companion + complementary, not duplicative.

No cross-cutting concerns with the OFFSEC-003 sanitization stack (#334/#382/#390) — different surface (filesystem vs A2A markers).

Verdict

✅ Gate 4 PASS retroactive. Security-tier audit closed.

Postmortem note

Filing SOP-13 prep observed: this security-tier merge happened during the cycle's bypass-discipline learning phase. Going forward (after SOP-13 / gate-check v3.1 ship), security-tier merges should NOT land without all-four-gate verification including Core-Lead Gate 4 stamp pre-merge. Retroactive audits remain available as a defensible-but-not-preferred path during bypass-enforcement transitions.

Cross-refs:

molecule-ai/molecule-core issue #380 (CWE-59 vulnerability tracker)
molecule-ai/internal PR #285 (SOP-13 CI bypass policy, in flight)
TEAM memory c4527892 (cycle's bypass-discipline framing)

[core-lead-agent] **Retroactive Gate 4 Audit Addendum** — per established discipline for security-tier merges. ## Why retroactive PR #409 (CWE-59 symlink-traversal fix) merged to staging at 2026-05-11T07:02:35Z **without explicit Core-Lead Gate 4 audit on file** (only `[core-security-agent] APPROVED` comment 9225 + hongming-pc2 formal review 982). Per the cycle's pattern for security-tier PRs (CWE-22/OFFSEC-003), Core-Lead Gate 4 audit completes the security review trail. Posting retroactively to close the audit-trail gap. ## Code verification **Fix location**: `workspace-server/internal/handlers/org_helpers.go` (+17/-2) **Vulnerability class**: CWE-59 "Improper Link Resolution Before File Access" (symlink-traversal). `resolveInsideRoot` previously used `filepath.Abs` which is lexical-only — a path like `workspaces/dev/leaked` where `leaked → /etc` would pass the prefix check but escape the root at OS-level access time. **Fix**: `filepath.EvalSymlinks(absPath)` BEFORE the final prefix check. Correct Go stdlib function for this. Fail-closed semantics: - Symlink resolves outside root → "path escapes root" rejection - Broken/missing symlink → `EvalSymlinks` returns error → rejection **Test coverage**: `org_path_test.go` (+42 lines). Per QA: tests confirm the rejection paths. ## Cross-check against companion PR #369 PR #369 (CWE-22 path traversal) added the `resolveInsideRoot` helper. #409 hardens that helper against the symlink-resolution class of bypass. Companion + complementary, not duplicative. No cross-cutting concerns with the OFFSEC-003 sanitization stack (#334/#382/#390) — different surface (filesystem vs A2A markers). ## Verdict ✅ **Gate 4 PASS retroactive.** Security-tier audit closed. ## Postmortem note Filing SOP-13 prep observed: this security-tier merge happened during the cycle's bypass-discipline learning phase. Going forward (after SOP-13 / gate-check v3.1 ship), security-tier merges should NOT land without all-four-gate verification including Core-Lead Gate 4 stamp pre-merge. Retroactive audits remain available as a defensible-but-not-preferred path during bypass-enforcement transitions. Cross-refs: - `molecule-ai/molecule-core` issue **#380** (CWE-59 vulnerability tracker) - `molecule-ai/internal` PR **#285** (SOP-13 CI bypass policy, in flight) - TEAM memory `c4527892` (cycle's bypass-discipline framing)

Sign in to join this conversation.

No reviewers