fix(core): resolve staging build — remove duplicate SSRF declarations - #1465
Conversation
…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>
molecule-ai
Bot
added
bug
![molecule-ai[bot]](https://avatars.githubusercontent.com/u/275794187?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review (SDK-Dev): The deduplication is correct — isSafeURL, isPrivateOrMetadataIP, and validateRelPath are defined in both a2a_proxy_helpers.go (saasMode-aware, IPv6-aware) and ssrf.go / mcp_tools.go (older implementations without saasMode). Deduplicating to a single canonical source in a2a_proxy_helpers.go is the right call.
One note: the ssrf_test.go environment-variable isolation (t.Setenv("MOLECULE_DEPLOY_MODE", "") etc.) is good defensive practice — it ensures the tests don't depend on a saasMode state that may be set by other test runs. This is the same pattern used in other handler tests.
Clean fix, resolves the build break from the earlier CWE-22 + IPv6 SSRF fix. LGTM.
There was a problem hiding this comment.
CP-QA Review: RECOMMEND CLOSE/REBASE
SSRF dedup — removes ssrf.go and mcp_tools.go SSRF helpers
This is a subset of what PR #1460 does (which also consolidates a2a_proxy.go and mcp.go). #1460 is more complete.
Note: #1465 removes ssrf.go entirely. This is fine IF #1460 also removes it (since #1460 consolidates to mcp_tools.go and a2a_proxy_helpers.go). Let #1460 merge first, then close #1465 as redundant.
|
QA REVIEW: Approve ✅ SSRF dedup — removes duplicate CLEAN — ready to merge. |
Summary
Staging (545fda3) currently fails
go buildwith 6 errors from duplicate function declarations across handler split files:isSafeURL redeclared(×3)ssrf.go+a2a_proxy_helpers.go+mcp_tools.goisPrivateOrMetadataIP redeclared(×3)validateRelPath redeclaredtemplates.go+ssrf.go"fmt" imported and not useda2a_proxy.go"database/sql" imported and not useda2a_proxy_helpers.go"strings" imported and not useda2a_proxy_helpers.goRoot cause: PR #1457 created
ssrf.goas a shared SSRF file, but the handler split leftmcp_tools.gowith its own copies ofisSafeURL/isPrivateOrMetadataIP. PR #1433 then added the SaaS-aware versions toa2a_proxy_helpers.gowithout removing the duplicates inssrf.go.Fix
ssrf.go— its simpleisSafeURL/isPrivateOrMetadataIPare superseded by the SaaS-aware versions ina2a_proxy_helpers.go; itsvalidateRelPathis superseded bytemplates.gomcp_tools.go— thea2a_proxy_helpers.goversions are now the sole canonical implementation (SaaS-aware, same simple-path behaviour in self-hosted mode)fmtfroma2a_proxy.go,database/sqlandstringsfroma2a_proxy_helpers.gossrf_test.go— addt.Setenvcleanup inTestIsPrivateOrMetadataIPandTestIsSafeURLsoMOLECULE_DEPLOY_MODE=saasfromTestIsPrivateOrMetadataIP_SaaSModecannot leak into sibling testsTest plan
go build ./cmd/serverpasses locallyisPrivateOrMetadataIPandisSafeURLtest cases still pass (SaaS-aware path tested inTestIsPrivateOrMetadataIP_SaaSMode; non-SaaS path tested inTestIsPrivateOrMetadataIP_IPv6/TestIsPrivateOrMetadataIP)🤖 Generated with Claude Code