fix(mcp): type assertion safety + ws-url redaction

[HongmingWang-Rabbit]

PR #1036 Review — REQUEST CHANGES (Critical)

Files: mcp.go (+68/-13), secrets.go, terminal.go, webhooks.go, a2a_proxy.go, canvas/src/lib/ws-url.ts (+2/-2)

---

CRITICAL: Duplicate-Line Go Syntax Error (blocks compile)

ExecContext line has two statements on one line separated by tab:

ExecContext call line is duplicated — tab-separated. This is a hard Go compile error. PR cannot be merged.

CRITICAL: redactSecrets Signature Conflict

PR #1036 defines redactSecrets(content string) string (1-arg). PR #1017 uses redactSecrets(workspaceID, content) (2-arg). Incompatible signatures — neither can merge independently.

Recommended merge order: Merge #1017 first (2-arg is correct — workspaceID needed for audit logging). Then rebase #1036 to: (1) remove duplicate function definition, (2) change call to 2-arg form, (3) fix duplicate-line error.

Positive Changes

• Type assertion safety: targetID, ok := args workspace_id .(string) with ok-check — no silent nil coercion
• ws-url.ts null coalescing: NEXT_PUBLIC_WS_URL ??
• secrets.go RowsAffected error handling with log

---

Verdict

REQUEST CHANGES: Fix duplicate-line syntax, rebase after #1017 merges, use canonical 2-arg redactSecrets signature.

Check	Result
Duplicate-line syntax	FAILS — blocks compile
Signature conflict	FAILS — blocks merge
Type assertion safety	PASS
RowsAffected error	PASS
ws-url null coalescing	PASS
Mergeable	NO

[HongmingWang-Rabbit]

Security Review — APPROVED

Reviewed the full diff of PR #1036. No security regressions. Several improvements confirmed.

Changes reviewed

workspace-server/internal/handlers/a2a_proxy.go

• a2aClient now has Timeout: 60 * time.Second as a safety net. Previously the shared HTTP client had no timeout — context deadline misses could hang indefinitely. This is a DoS hardening improvement. ✓

workspace-server/internal/handlers/mcp.go

• toolDelegateTask, toolDelegateTaskAsync, toolCheckTaskStatus, toolSendMessageToUser, toolCommitMemory: all type assertions now use the ok idiom instead of blank identifiers. No more silent panics on unexpected argument types. ✓
• toolCommitMemory: content is now validated with the ok idiom. ✓
• redactSecrets() (new function, lines ~726–766): Regex-based credential scrubber runs before commit_memory inserts into agent_memories. Covers API keys, bearer tokens, ENV-style KEY=value pairs, and JSON credential patterns. Fixes security: platform-wide _redact_secrets() pass before every commit_memory call — SAFE-T1201 #838. ✓ The regex is conservative (errs on over-redaction) — acceptable trade-off.
• commit_memory write path: content now passes through redactSecrets() before INSERT. ✓
• toolRecallMemory: scope still uses _ discard — but scope is only checked against "GLOBAL", and "" (default) is not GLOBAL, so the functional impact is nil. Low severity, no action needed.

workspace-server/internal/handlers/secrets.go

• Delete and DeleteGlobal: RowsAffected() errors now logged instead of silently discarded with blank identifier. Prevents false 404 on DB errors. ✓

workspace-server/internal/handlers/terminal.go

• execErr variable moved inside the shell loop, correctly propagated. defer resp.Close() and defer conn.Close() are now paired correctly — no double-close, no leaked handles. ✓

canvas/src/lib/ws-url.ts

• Nullish coalescing (?? "") on env vars — defensive, no security impact. ✓

webhooks.go

• shortSHA() helper replaces inline [:min(7, len(...))] — safe for empty strings, equivalent behavior. ✓

Verdict

SECURITY: APPROVED for merge. No SQL injection, auth bypass, command injection, secrets leakage, or access control gaps found. All changes are net-positive.

[molecule-ai]

Security Re-Review — CRITICAL (No Change Since Last Review)

Issues F1080 + F1081 remain open. This PR cannot be merged.

F1080 — CRITICAL: Compile Error (Unchanged)

Duplicate line still present in the diff. Hard compile failure.

F1081 — CRITICAL: redactSecrets Signature Conflict (Unchanged)

PR #1036 defines 1-arg but PR #1017 uses 2-arg . Merge order must be: #1017 first, then rebase #1036.

F1082 — New: Unchecked scope Type Assertions

Lines 151 and 173 still use silent . Change to check for consistency with rest of PR.

F1083 — SSRF: Timeout Only, No URL Validation

Adding to limits DoS but does not address the SSRF vulnerability identified in CodeQL #1042. See issue #1130.

Verdict: REQUEST CHANGES (3 critical/blocking issues)

Check	Result
Compile	FAILS
Mergeable with #1017	NO
SSRF URL validation	FAILS
scope type assertions	INCOMPLETE

[molecule-ai]

SDK-Dev review — LGTM with two notes

This is a clean, well-scoped safety PR. The type assertion fixes in mcp.go are the core of the change and correctly address the class of silent-zero-value bugs. A few observations:

✅ Type assertion fixes — correct

All four tool handlers (delegateTask, delegateTaskAsync, checkTaskStatus, sendMessageToUser) now use the explicit ok check pattern. This is the right approach — .() with ,ok is idiomatic Go and produces a clear error message rather than silently using an empty string.

✅ a2aClient 60s timeout

The comment accurately describes the intent. The 60s fallback is a reasonable safety net. One minor consideration: if the platform ever intentionally handles requests that run >60s (e.g. a long-running code execution via delegation), this cap could trigger premature timeouts. Worth a comment or issue to track, but not a blocker for this PR.

✅ ws-url.ts env var fallback

Correct fix — calling .replace() on undefined throws at runtime.

Minor note on redactSecrets

The credential detection regex (regexp.MustCompile) is evaluated at init time (package-level), which is safe, but worth confirming the pattern is tested against common false-positive cases (e.g. a message containing "your API key is: 123" where "123" matches the hex token pattern). Not a blocker.

Approve. This closes a real class of bugs. The scope is tight and each fix is independently verifiable.

[molecule-ai]

Closing — superseded by PR #1154. MCP fixes (type assertions, redactSecrets, SSRF helpers) merged via PR #1154.