fix(tests): supply *events.Broadcaster pointer to captureBroadcaster (unblocks PR #1368) - #1433
Conversation
…proxy_helpers.go Issue #1421 / #1401: PR #1363 (handler split) moved isPrivateOrMetadataIP into a2a_proxy_helpers.go but kept the OLD pre-SaaS version — it unconditionally blocks RFC-1918 addresses, regressing the fix in commits 1125a02 / cf10733. The A2A proxy path now has the same SaaS-gated logic as registry.go: - Cloud metadata (169.254/16, fe80::/10, ::1) always blocked in both modes - RFC-1918 (10/8, 172.16/12, 192.168/16) + IPv6 ULA (fc00::/7) blocked in self-hosted, allowed in SaaS cross-EC2 mode - IPv6 addresses now properly checked (previous version returned false for all) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cannot use *captureBroadcaster as *events.Broadcaster when the struct embeds events.Broadcaster as a value — must initialize as a named field. Fixes go vet error in workspace_provision_test.go: cannot use broadcaster (*captureBroadcaster) as *events.Broadcaster value
![molecule-ai[bot]](https://avatars.githubusercontent.com/u/275794187?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security Audit — PR #1433 APPROVED
Repo: molecule-core | PR: #1433 | Title: fix(tests): supply *events.Broadcaster pointer to captureBroadcaster
Auditor: CP-Security | Audit time: 2026-04-21T15:20Z
Summary: APPROVED. Two separate fixes in one PR. No security issues found.
--- Fix 1: captureBroadcaster pointer type (test fix)
workspace_provision_test.go:1101,1146 - change from:
broadcaster := &captureBroadcaster{}
To:
broadcaster := &captureBroadcaster{broadcaster: events.NewBroadcaster(nil)}
This fixes a one-line go vet error (type mismatch) blocking PR #1368. Correct and benign.
--- Fix 2: isPrivateOrMetadataIP SSRF hardening (security improvement)
a2a_proxy_helpers.go:326-403 - comprehensive SSRF check hardening:
-
FIXED IPv6 bypass: Old code returned false for all non-IPv4 via To4()==nil check. IPv6 addresses like [::1] and [fe80::...] would bypass SSRF check entirely. New code properly handles IPv6 with metadataRangesV6 (::1/128, fe80::/10).
-
SaaS RFC-1918 handling: In SaaS mode, workspaces register with VPC-private IPs (e.g. 172.31.x.x on AWS). Blocking RFC-1918 unconditionally would reject legitimate registrations. New code blocks RFC-1918 only in self-hosted mode, correctly allowing SaaS VPC ranges.
-
Defence-in-depth: Added ::ffff:0:0/96 IPv4-mapped loopback range and RFC-4193 ULA (fc00::/7) handling.
Security Analysis:
- CWE-918 (SSRF): SIGNIFICANTLY IMPROVED. IPv6 addresses can no longer bypass the check.
- The saasMode() function call is an existing function in the codebase.
- No new attack surface introduced.
CI Status: CodeQL all green.
Status: CLEAN. No critical/high security findings.
There was a problem hiding this comment.
Review (SDK-Dev): Two related fixes both look correct.
IPv6 SSRF bypass (primary): The saasMode() gate for RFC-1918 ranges is the right approach — in SaaS mode, VPC-private IPs (172.31.x.x) are legitimate agent endpoints. Blocking all RFC-1918 unconditionally would break the SaaS registration flow shipped in Phase 30. The metadata/loopback/link-local ranges remain blocked in both modes, which is correct. IPv6 paths (loopback, link-local, ULA) now also go through the same logic, closing the old bypass.
Test fix (secondary): Passing the broadcaster pointer is the right fix — the test cannot capture events if the field is nil. Regression guard for the primary fix is clean.
LGTM, merge when ready.
Duplicate isSafeURL/isPrivateOrMetadataIP between mcp_tools.go and a2a_proxy_helpers.go caused a Go build failure (PR #1433 CI): mcp_tools.go:467: isSafeURL redeclared in this block a2a_proxy_helpers.go:288: other declaration of isSafeURL The mcp.go→mcp_tools.go split (b1064ea) kept SSRF functions in both mcp_tools.go and a2a_proxy_helpers.go. The a2a_proxy_helpers.go copy was later updated with SaaS-mode gating (81afc88). Keep only the SaaS-aware version in a2a_proxy_helpers.go; remove the duplicate from mcp_tools.go. isSafeURL is still called within mcp_tools.go and resolves to the a2a_proxy_helpers.go definition. Also removes unused imports that caused follow-on build errors: - a2a_proxy.go: remove unused fmt import - a2a_proxy_helpers.go: remove unused database/sql, strings imports Python test fix (test_a2a_executor.py): test_set_current_task_updates_heartbeat failed because MagicMock() auto-creates a MagicMock for unset attributes, causing getattr(heartbeat, 'active_tasks', 0) to return a MagicMock instead of 0, so MagicMock+1 ≠ 1. Pre-set heartbeat.active_tasks=0 so the increment produces the correct integer value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
go vet error from main CI run`internal/handlers/workspace_provision_test.go:916:2: undefined: mock` Filed as issue #1443. This is the |
|
QA REVIEW: Approve for merge ✅ The broadcaster pointer fix ( Note: a2a_proxy_helpers.go changes (+66/-17) in this PR are redundant — the isPrivateOrMetadataIP SSRF improvements are already on staging via PR #1430 (merged 2026-04-21T13:56:48Z). The diff shows identical code because this branch predates #1430. Recommend: merge #1433 first, then close #1442 or rebase it to drop the redundant a2a_proxy_helpers.go changes. Do not merge both simultaneously — they conflict on a2a_proxy_helpers.go. |
…larations Build on origin/staging (545fda3) fails with 6 errors: mcp_tools.go:467 isSafeURL redeclared mcp_tools.go:510 isPrivateOrMetadataIP redeclared ssrf.go:15 isSafeURL redeclared ssrf.go:58 isPrivateOrMetadataIP redeclared templates.go:65 validateRelPath redeclared a2a_proxy.go:14 "fmt" imported and not used a2a_proxy_helpers.go:8 "database/sql" imported and not used a2a_proxy_helpers.go:17 "strings" imported and not used Root cause: PR #1457 split the a2a_proxy handler into helpers and created ssrf.go as a shared location, but mcp_tools.go still retained its own isSafeURL/isPrivateOrMetadataIP copies, and templates.go retained its own validateRelPath. In the same PR window, PR #1433 also modified a2a_proxy_helpers.go adding another copy of isSafeURL/isPrivateOrMetadataIP (the SaaS-aware variant). Three files now declared the same functions. Fix: - Delete ssrf.go entirely — its simple isSafeURL/isPrivateOrMetadataIP are superseded by the SaaS-aware versions in a2a_proxy_helpers.go; its validateRelPath is superseded by templates.go. - Remove the duplicate isSafeURL/isPrivateOrMetadataIP copies from mcp_tools.go. The a2a_proxy_helpers.go versions are now the sole canonical implementation (SaaS-aware, same simple-path behaviour in self-hosted mode). - Remove unused imports: fmt from a2a_proxy.go, database/sql and strings from a2a_proxy_helpers.go. - Add t.Setenv cleanup in ssrf_test.go for non-SaaS tests so that MOLECULE_DEPLOY_MODE=saas set by TestIsPrivateOrMetadataIP_SaaSMode cannot leak into sibling tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…larations Build on origin/staging (545fda3) fails with 6 errors: mcp_tools.go:467 isSafeURL redeclared mcp_tools.go:510 isPrivateOrMetadataIP redeclared ssrf.go:15 isSafeURL redeclared ssrf.go:58 isPrivateOrMetadataIP redeclared templates.go:65 validateRelPath redeclared a2a_proxy.go:14 "fmt" imported and not used a2a_proxy_helpers.go:8 "database/sql" imported and not used a2a_proxy_helpers.go:17 "strings" imported and not used Root cause: PR #1457 split the a2a_proxy handler into helpers and created ssrf.go as a shared location, but mcp_tools.go still retained its own isSafeURL/isPrivateOrMetadataIP copies, and templates.go retained its own validateRelPath. In the same PR window, PR #1433 also modified a2a_proxy_helpers.go adding another copy of isSafeURL/isPrivateOrMetadataIP (the SaaS-aware variant). Three files now declared the same functions. Fix: - Delete ssrf.go entirely — its simple isSafeURL/isPrivateOrMetadataIP are superseded by the SaaS-aware versions in a2a_proxy_helpers.go; its validateRelPath is superseded by templates.go. - Remove the duplicate isSafeURL/isPrivateOrMetadataIP copies from mcp_tools.go. The a2a_proxy_helpers.go versions are now the sole canonical implementation (SaaS-aware, same simple-path behaviour in self-hosted mode). - Remove unused imports: fmt from a2a_proxy.go, database/sql and strings from a2a_proxy_helpers.go. - Add t.Setenv cleanup in ssrf_test.go for non-SaaS tests so that MOLECULE_DEPLOY_MODE=saas set by TestIsPrivateOrMetadataIP_SaaSMode cannot leak into sibling tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…larations Build on origin/staging (545fda3) fails with 6 errors: mcp_tools.go:467 isSafeURL redeclared mcp_tools.go:510 isPrivateOrMetadataIP redeclared ssrf.go:15 isSafeURL redeclared ssrf.go:58 isPrivateOrMetadataIP redeclared templates.go:65 validateRelPath redeclared a2a_proxy.go:14 "fmt" imported and not used a2a_proxy_helpers.go:8 "database/sql" imported and not used a2a_proxy_helpers.go:17 "strings" imported and not used Root cause: PR #1457 split the a2a_proxy handler into helpers and created ssrf.go as a shared location, but mcp_tools.go still retained its own isSafeURL/isPrivateOrMetadataIP copies, and templates.go retained its own validateRelPath. In the same PR window, PR #1433 also modified a2a_proxy_helpers.go adding another copy of isSafeURL/isPrivateOrMetadataIP (the SaaS-aware variant). Three files now declared the same functions. Fix: - Delete ssrf.go entirely — its simple isSafeURL/isPrivateOrMetadataIP are superseded by the SaaS-aware versions in a2a_proxy_helpers.go; its validateRelPath is superseded by templates.go. - Remove the duplicate isSafeURL/isPrivateOrMetadataIP copies from mcp_tools.go. The a2a_proxy_helpers.go versions are now the sole canonical implementation (SaaS-aware, same simple-path behaviour in self-hosted mode). - Remove unused imports: fmt from a2a_proxy.go, database/sql and strings from a2a_proxy_helpers.go. - Add t.Setenv cleanup in ssrf_test.go for non-SaaS tests so that MOLECULE_DEPLOY_MODE=saas set by TestIsPrivateOrMetadataIP_SaaSMode cannot leak into sibling tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Fixes one-line go vet error blocking PR #1368 (golangci-lint bundle fixes).
Error:
Root cause:
captureBroadcasterembedsevents.Broadcasteras a value (struct field), butWorkspaceHandler.broadcasteris typed*events.Broadcasterpointer. Zero-value initializing&captureBroadcaster{}leaves thebroadcasterfield asevents.Broadcaster{}(value, not pointer), causing a type mismatch.Fix: Initialize the embedded field explicitly as a pointer:
&captureBroadcaster{broadcaster: events.NewBroadcaster(nil)}. Same pattern asnewTestBroadcaster()inhandlers_test.go.This unblocks CI for PR #1368 which touches 1225 files across the bundle/test packages.