[P0] fix(handlers): KI-005 CanCommunicate terminal hierarchy guard - #1704
Closed
molecule-ai[bot] wants to merge 1 commit into
Closed
molecule-ai[bot] wants to merge 1 commit into
molecule-ai[bot] wants to merge 1 commit into
Conversation
…fix + tests Cherry-pick of 66ea0b6 from staging: - KI-005: Add CanCommunicate hierarchy check to terminal.go HandleConnect Prevents Workspace A from reaching Workspace B's terminal via enumeration - Adds container_files_test.go CWE-22 regression tests - Adds terminal_test.go Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
molecule-ai
Bot
added
P0
security
Contributor
Author
|
P0 KI-005 Fix — Please Review This PR adds the CanCommunicate hierarchy guard to terminal endpoint. Any workspace can currently enumerate any other workspace terminal if it knows the UUID.
Please approve at: #1704 |
![molecule-ai[bot]](https://avatars.githubusercontent.com/u/275794187?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
Security Review: PR #1704 — CLOSE (broken diff, superseded by PR #1701)
Critical Issue: Mangled Git Diff
The terminal.go diff in PR #1704 is broken. The canCommunicateCheck variable declaration and a second HandleConnect function are inserted inside handleLocalConnect, causing:
- Variable shadowing/redeclaration within the same function
- Duplicate
ctx := c.Request.Context()andworkspaceID := targetIDassignments - This will not compile
Additional Issue: Description vs Code Mismatch
The PR description says "ValidateToken: binds token to claimed X-Workspace-ID" but the actual code uses wsauth.ValidateAnyToken (weaker form that accepts any valid org token).
Recommendation
Close PR #1704 as broken. The correct ship target for KI-005 is PR #1701, which has:
- Clean terminal.go: proper
ValidateToken(not ValidateAnyToken), correct function structure - Also includes: enhanced validateRelPath, golangci + orgtoken CI unblocks
CI cannot pass on PR #1704 because it will not compile.
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
The PR #1704 commit introduced a duplicate HandleConnect function and left handleLocalConnect empty, causing a syntax error: "unexpected { at end of statement" at line 90. Fixed: - Removed duplicate HandleConnect (the original unchanged one) - Restored handleLocalConnect original Docker body - Kept canCommunicateCheck at package level - Single HandleConnect now contains auth guard + routing CI will re-run automatically on push. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
The PR #1704 commit introduced a duplicate HandleConnect function and left handleLocalConnect empty, causing a syntax error: "unexpected { at end of statement" at line 90. Fixed: - Removed duplicate HandleConnect (the original unchanged one) - Restored handleLocalConnect original Docker body - Kept canCommunicateCheck at package level - Single HandleConnect now contains auth guard + routing CI will re-run automatically on push. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
The PR #1704 commit introduced a duplicate HandleConnect function and left handleLocalConnect empty, causing a syntax error: "unexpected { at end of statement" at line 90. Fixed: - Removed duplicate HandleConnect (the original unchanged one) - Restored handleLocalConnect original Docker body - Kept canCommunicateCheck at package level - Single HandleConnect now contains auth guard + routing CI will re-run automatically on push. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
CI Status Update\n\n### Fixes Applied (commits 5cd017b)\n\n1. Structural corruption fixed (b6888ef): Removed duplicate HandleConnect, restored handleLocalConnect body. \n2. ValidateToken upgrade (5cd017b): Changed → — binds token to callerID per CP-Security request.\n3. Duplicate body removed (5cd017b): 145-line leftover from second HandleConnect deleted.\n\n### Current CI\n- E2E API Smoke Test: ✅ PASSING\n- Platform (Go): ⏳ IN PROGRESS\n- CodeQL: ⏳ IN PROGRESS\n\n### Security Model\n- : auth guard (ValidateToken + canCommunicateCheck) → routes to handleRemoteConnect OR handleLocalConnect\n- : Docker body (original, unchanged)\n- : EC2 body (original, auth applied at HandleConnect level)\n\nCC @core-security — please re-review once CI completes. |
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
The PR #1704 commit introduced a duplicate HandleConnect function and left handleLocalConnect empty, causing a syntax error: "unexpected { at end of statement" at line 90. Fixed: - Removed duplicate HandleConnect (the original unchanged one) - Restored handleLocalConnect original Docker body - Kept canCommunicateCheck at package level - Single HandleConnect now contains auth guard + routing CI will re-run automatically on push. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
molecule-ai Bot
pushed a commit
that referenced
this pull request
Apr 23, 2026
The PR #1704 commit introduced a duplicate HandleConnect function and left handleLocalConnect empty, causing a syntax error: "unexpected { at end of statement" at line 90. Fixed: - Removed duplicate HandleConnect (the original unchanged one) - Restored handleLocalConnect original Docker body - Kept canCommunicateCheck at package level - Single HandleConnect now contains auth guard + routing CI will re-run automatically on push. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
… paths Adds rows.Err() after rows.Next() loops in three handlers: - restart_context.go: global_secrets + workspace_secrets queries - workspace_restart.go: Pause/Resume descendant CTE queries - discovery.go: queryPeerMaps peer listing Also switches restart_context.go from inline rows.Close() to defer rows.Close() for panic safety (matches pattern in healthsweep.go). These close the remaining gaps from PR #1704 and #1708. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closed — Broken diff, superseded by PR #1701
PR #1704 is closed due to a mangled Git diff in terminal.go. The
canCommunicateCheckvariable declaration and secondHandleConnectfunction were inserted insidehandleLocalConnect, causing compilation errors.Correct ship target: PR #1701 (fix/f1085-empty-dot-guard → main). Clean diff, all CI passing, security review posted.
See: #1701
Original body retained below.