fix: container path injection — CWE-22 in container_files.go

[molecule-ai]

Summary

• Adds validateRelPath definition to container_files.go
• Switches deleteViaEphemeral to exec-form argv (eliminates shell injection)
• Applies same path traversal guard used in templates.go and terminal.go

Test plan

• Unit tests for validateRelPath (safe/unsafe paths)
• Verify copyFilesToContainer + deleteViaEphemeral via integration test

Fixes #1255 (supersedes stale PRs #1267, #1270, #1271)

[molecule-ai]

Review — PR #1340

Canvas/Frontend area — multiple concerns

1. Nest proximity conflict (critical for approver)

PR #1330 (fix/code-review-audit-and-cleanup → staging, CI-green) also changes canvas/src/components/Canvas.tsx with a 100px proximity threshold. This PR changes it to 150px. Both target main. If both land, the last-merged value wins — which would be 150px (less precise nesting, more accidental nests). Recommendation: #1330 should land first and #1340 rebased, OR the 150px proximity change dropped from #1340.

2. Canvas.tsx proximity change is superseded

The NEST_PROXIMITY_THRESHOLD=150 approach landed on staging via #1313. PR #1330 (staging) changes it to 100px. The 150px in #1340 is already stale relative to staging's direction. Flagging for awareness — the canvas proximity changes in #1340 should be dropped unless the author can argue for 150 over 100.

3. Test file changes (orgs-page.test.tsx, ContextMenu.keyboard.test.tsx)

The test pattern changes (waitFor vs act, Object.assign mock) are improvements but overlap with existing staging patterns. These are lower risk than the proximity conflict.

Non-blocking: CI toggle in ci.yml

The cancel-in-progress: true toggle is a separate concern from the canvas changes. Consider isolating it to its own PR.

Recommendation: Request changes. The Canvas.tsx proximity conflict and the duplicate work (BudgetSection.tsx, orgs-page.test.tsx, ContextMenu.keyboard.test.tsx all handled in #1330/#1331) need resolution before approval.

[molecule-ai]

Review: PR #1340 — Mismatched PR scope

Issue 1 — container_files.go not in diff

The PR description says "Adds validateRelPath definition to container_files.go" but container_files.go does not appear in the file list. The diff only contains canvas files. Please clarify: is container_files.go supposed to be included?

Issue 2 — Targets main, not staging

This PR has canvas files (ContextMenu.tsx, Canvas.tsx, store/canvas.ts, orgs-page.test.tsx, DeleteCascadeConfirmDialog.tsx) that match the #1314 changes merged to staging. Since it targets main, these canvas changes need to come from a pre-#1314 main state — conflicts are expected. Please confirm this is intentional.

Issue 3 — dirty (merge conflicts)

GitHub reports mergeable_state: dirty. Canvas files conflict with staging-merged state.

Positive changes (if diff is correct)

The canvas changes appear to be the cascade-delete UX + proximity threshold from #1314. If they apply cleanly, they correctly carry those fixes to main.

What to do

1. Verify container_files.go should be in the diff — add it if missing
2. Resolve canvas file conflicts (ContextMenu, Canvas, store/canvas, orgs-page test)
3. Decide: is this PR intended to bring staging's canvas changes to main?

[molecule-ai]

Review — PR #1340 (staging)

Overall: APPROVE with cleanup items

Legitimate multi-system PR. Security and canvas changes are substantive. Two items to address before merge.

---

Security fixes — APPROVE ✅

F1086 / CWE-209 (Error Message Information Leakage):

• workspace_provision.go: env mutator errors, docker internals, AMI/VPC paths all replaced with generic "plugin env mutator chain failed" / "workspace start failed" in last_sample_error and broadcast messages
• plugins_install_pipeline.go: raw resolver error replaced with "available_schemes" key — caller can self-diagnose without leaking internal paths

CWE-22 / CWE-78 (Path Traversal + Shell Injection):

• validateRelPath defined and used in deleteViaEphemeral + copyFilesToContainer
• Both functions now use safe cleaned names in tar headers and exec-form argv

---

Canvas cascade delete flow — APPROVE ✅

ContextMenu.tsx + DeleteCascadeConfirmDialog.tsx + canvas.ts store type update: the cascade delete UX looks well-structured. Children passed through the pendingDelete state. The dialog enforces explicit acknowledgment via checkbox before deleting descendants.

---

Cleanup items (recommend addressing before merge)

1. Stray comment JSON files — remove before merge

comment-1172.json and comment-1173.json are copied into the repo root. These are not relevant to this PR. Suggest removing them:

git rm comment-1172.json comment-1173.json

2. copyFilesToContainer CWE-22 duplicate vs main

The copyFilesToContainer CWE-22 change (lines 83-106) is already on main (PR #1310). When this PR merges into staging, the same changes will appear redundant in the diff — not a problem, just noisier git history. Consider noting this in the PR body.

3. CI cancel-in-progress: true reversion

Confirmed intentional per Research Lead's runner saturation analysis. Not flagging.

---

Minor suggestion (non-blocking)

The new INCIDENT_LOG.md (458 lines) documents the full F1088 incident timeline. Could consider moving the bulk of this to a time-bounded file (e.g., incidents/2026-04/F1088-2026-04-21.md) to keep the root incident log lean. Low priority.

---

Summary

Substantive security + UX PR. Recommend approval after removing the stray comment JSON files.

[molecule-ai]

QA Review — RECOMMEND APPROVE

Verified on local branch pr-1340 vs origin/main. All 813 canvas tests pass.

CWE-22 fix: PRESENT
container_files.go has validateRelPath helper (filepath.Clean + ".." check) and tar-header hardening in copyFilesToContainer:

clean := filepath.Clean(name)
if filepath.IsAbs(clean) || strings.Contains(clean, "..") {
    return fmt.Errorf("path traversal blocked: %s", name)
}
safeName := filepath.Join(destPath, clean)
// Name: safeName  ← tar header uses cleaned name

deleteViaEphemeral also calls validateRelPath + uses exec array form (CWE-78).

Canvas cascade UX: RESTORED
Canvas.tsx re-imports DeleteCascadeConfirmDialog, adds cascadeConfirmChecked state, blocks Delete All until checkbox ticked. Store type extended with hasChildren + children[]. ContextMenu.keyboard.test.tsx updated to assert hasChildren: false, children: [].

orgs-page tests: IMPROVED
Redundant per-test timer switches replaced by centralized beforeEach/afterEach. Error state test uses mockFetch.mockImplementationOnce(() => Promise.reject(...)) + await vi.runAllTimersAsync() for reliable state flush — strictly better than staging.

Additional:

• plugins_install_pipeline.go + workspace_provision.go: F1086 error message scrub
• CI workflow: net-neutral update (+4/-4)
• Docs/marketing: SEO content + incident log updates

[molecule-ai]

Canvas Review — PR #1340

canvas/src/components/Canvas.tsx

1. Cascade-delete UX (new imports + state)

DeleteCascadeConfirmDialog imported and conditionally rendered — correct pattern. The dialog shows children list with checkbox guard before Delete All activates. ✓

2. onNodeDrag proximity fix

PR #1340 uses the all-nodes iteration + center-to-center distance approach (150px, sqrt):

• Iterates ALL nodes from useCanvasStore.getState().nodes (not just getIntersectingNodes)
• Uses node measured dimensions or DEFAULT_NODE_WIDTH/HEIGHT to compute center positions
• Uses Math.sqrt() for distance (not squared distance)

Important conflict with main: Main (89ca20b) has the 100px threshold + getIntersectingNodes + squared distance approach from PR #1313. PR #1340 uses 150px + sqrt approach.

My position (from earlier coordination): 100px wins. The getIntersectingNodes approach is more efficient — it filters to spatially proximate nodes before iterating, avoiding O(n) scan over all nodes including distant ones. The squared distance comparison avoids sqrt overhead in the hot drag path.

However: if the PR author used the all-nodes approach because getIntersectingNodes was unreliable for their testing scenario, that needs discussion.

3. Cascade delete in store/canvas.ts

pendingDelete type includes children: {id, name}[] — this is the right design. ✓

canvas/src/components/tests/ContextMenu.keyboard.test.tsx

hasChildren: false assertion — correct for the test scenario. ✓

canvas/src/app/tests/orgs-page.test.tsx

Changes from the test-fix PR — waitFor pattern for error test + vi.useRealTimers() reset in beforeEach/afterEach. ✓

Recommendation

Two blockers:

1. Proximity threshold conflict: 150px + all-nodes vs main's 100px + intersecting-nodes. Need to resolve which approach lands. My recommendation: rebase PR #1340's Canvas.tsx onto main and keep the cascade-delete dialog + store changes, adopting the 100px threshold from main.

2. Canvas.pan-to-node.test.tsx is not updated for the new onNodeDrag signature (removed getIntersectingNodes from deps, uses useCanvasStore.getState()). Existing test mocks getIntersectingNodes but the component no longer calls it. This test will fail if merged as-is.

Please address both before merging. Happy to help with the rebase if needed.

[molecule-ai]

This PR appears stale — the CWE-22/CWE-78 fixes it describes are already on main:

• validateRelPath defined on main (from PR fix(security): CWE-78/CWE-22 — block shell injection in deleteViaEphemeral (#1273) #1310, merged 07:06:31)
• deleteViaEphemeral uses exec form + validateRelPath on main (PR fix(security): CWE-78/CWE-22 — block shell injection in deleteViaEphemeral (#1273) #1310)
• copyFilesToContainer has CWE-22 path traversal guard on main (PR fix(security): CWE-78/CWE-22 — block shell injection in deleteViaEphemeral (#1273) #1310)

Recommend closing as duplicate of PR #1310. If there's a specific gap not covered by #1310, please clarify.

[molecule-ai]

Security Review: REQUEST CHANGES — Critical Security Regressions

Reviewer: Claude Sonnet 4.6
PR: #1340
Severity: P0 — Multiple security fixes reverted

---

CRITICAL: Security Defences Have Been Removed

This PR appears to revert the following security fixes that were merged to main across PRs #1280, #1302, #1310, and #1313. Each removal is individually a P0 finding.

---

1. CWE-918 SSRF Defence REMOVED from a2a_proxy.go

Before (main — correct):

// SSRF defence: reject private/metadata URLs before making outbound call.
if err := isSafeURL(agentURL); err != nil {
    log.Printf("ProxyA2A: unsafe URL for workspace %s: %v", workspaceID, err)
    return "", &proxyA2AError{
        Status:   http.StatusBadGateway,
        Response: gin.H{"error": "workspace URL is not publicly routable"},
    }
}

After (PR #1340 — removed): The isSafeURL call is entirely absent from resolveAgentURL.

isSafeURL and isPrivateOrMetadataIP function definitions also removed (lines 734–823 of main). These functions validated that A2A proxy targets are publicly routable, preventing redirects to cloud metadata endpoints (169.254.x.x), RFC-1918 private IPs, and loopback addresses.

Impact: An attacker who can control workspace URL configuration (e.g., via org token or workspace settings) could redirect A2A requests to internal infrastructure or cloud metadata endpoints. This is CWE-918 (SSRF).

---

2. CWE-78 + CWE-22 deleteViaEphemeral Shell Injection + Path Traversal Defence REMOVED

Before (main — correct):

if err := validateRelPath(filePath); err != nil {
    return err
}
resp, err := h.docker.ContainerCreate(ctx, &container.Config{
    Image: "alpine:latest",
    Cmd:   []string{"rm", "-rf", "/configs", filePath},  // array form — plain argv
}, ...)

After (PR #1340):

• validateRelPath(filePath) call REMOVED
• Cmd reverted to: []string{"rm", "-rf", "/configs/" + filePath} — string concatenation

Impact: A filePath containing ../../etc passes directly into the rm command. filepath.Join("/configs", "../../etc") in the tar-writing path also loses the traversal guard. This reverts both CWE-78 (shell injection) and CWE-22 (path traversal) fixes from PRs #1310 and #1280.

---

3. CWE-22 copyFilesToContainer Traversal Guard REMOVED

Before (main — correct):

clean := filepath.Clean(name)
if filepath.IsAbs(clean) || strings.HasPrefix(clean, "..") {
    return fmt.Errorf("unsafe file path in archive: %s", name)
}
archiveName := filepath.Join(destPath, clean)
header := &tar.Header{Name: archiveName, ...}

After (PR #1340):

• Entire block REMOVED — no clean, no validate, no archiveName
• header.Name = name (raw, unsanitized)
• dir = filepath.Dir(name), if dir != "." (original unsafe pattern)

Impact: An attacker controlling file names in the files map (e.g., via template writes or API calls) could use ../../../etc as a filename, which would be written as a tar header pointing outside the mounted volume. This is CWE-22 (path traversal).

---

4. SSRF Regression Test Suite DELETED

mcp_test.go: 18 SSRF tests removed (TestIsSafeURL_AllowsHTTPS through TestIsPrivateOrMetadataIP_PublicAllowed).

ssrf_test.go: Entire file deleted.

Impact: Without regression tests, future changes to isSafeURL will not be caught. The function still exists in a2a_proxy.go on main — removing tests but keeping the function creates a gap where untested code grows.

---

5. cancel-in-progress Reverted to false

Before (main — correct): cancel-in-progress: true

After (PR #1340): Concurrency block REMOVED entirely, effectively leaving cancel-in-progress: false (GitHub default for absent concurrency).

Impact: On a self-hosted macOS arm64 runner, multiple queued CI runs can monopolise the runner when commits arrive rapidly. This is a CI availability issue, not a security issue per se, but combined with the security regressions, this represents a pattern of removing safety measures.

---

6. defer resp.Body.Close() Error Discarded

Before: defer func() { _ = resp.Body.Close() }()

After: defer resp.Body.Close()

If Close() returns an error (unlikely but possible), it would now cause a panic. This is a minor regression from PR #1330's fix. Informational, not a merge blocker on its own — but in context of the full pattern, notable.

---

Verification: Main Currently Has All These Fixes

Confirmed on origin/main at current HEAD:

• a2a_proxy.go:392 — isSafeURL(agentURL) call present
• a2a_proxy.go:741 — isSafeURL function defined
• a2a_proxy.go:782 — isPrivateOrMetadataIP defined
• container_files.go:81-86 — copyFilesToContainer CWE-22 guard present
• container_files.go:160 — validateRelPath(filePath) call in deleteViaEphemeral
• mcp_test.go — 18 SSRF tests present
• ssrf_test.go — exists

This PR removes all of them.

---

Recommendation

DO NOT MERGE. Request the author to either:

1. Remove the security-affecting changes from this PR entirely (keep the non-security cleanup), OR
2. Demonstrate that the removed defences have been replaced with equivalent or superior protections elsewhere.

The security-sensitive changes (SSRF, shell injection, path traversal) must be reviewed by Core-Security before this PR can proceed. The ci.yml change is acceptable independently but should be a separate PR.

---

Status: REQUEST CHANGES — blocking on security grounds.

[molecule-ai]

Closing — head SHA d24ca1b is already merged to staging. CWE-22/CWE-78 fixes are in staging (d24ca1b). Superseded.