fix(test): restore the registry SSRF allow-policy after loopback fixtures

[loloDawit]

Description

Test-only change. No production code is modified and no runtime behaviour changes.

TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP — the MCP-1076 / CWE-918 regression test — can pass without exercising the guard it covers. Reproduction on main (3.9s, two tests):

$ go test ./internal/server/ -count=2 \
    -run 'TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP|TestHandleUpstreamServers_AddFromRegistry_HappyPath'
--- FAIL: TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP (0.00s)
    Error Trace: internal/server/add_registry_source_test.go:52
    Error:       An error is expected but got nil.
    Messages:    must reject SSRF target "https://169.254.169.254/v0.1/servers"

buildRegistrySourceEntry accepts the cloud-metadata endpoint. The same command at -count=1 passes.

Root cause

The allow-policy is process-global (internal/registries/ssrf.go:42):

var registryAllowPrivateFetch atomic.Bool

Its only writer is SetAllowPrivateRegistryFetch, reached through SetRegistriesFromConfig (registry_data.go:66). Two fixtures disable the policy so they can serve a loopback httptest registry, and neither restores it:

• internal/server/mcp_add_from_registry_test.go — startTestRegistry
• internal/server/consistency_official_test.go — startOfficialTestRegistry

Both register t.Cleanup(srv.Close) for the httptest server, but nothing for the global. Every later test in the binary then runs with the policy disabled, so all five assertions in add_registry_source_test.go:43 (metadata endpoint, loopback, two RFC1918 ranges, IPv6 loopback) succeed without reaching the guard.

startTestRegistry's doc comment records the assumption behind this — "tests run sequentially so the last writer wins." That holds only for tests that write the policy; tests that read it observe whatever the last fixture left. The comment is corrected here.

Why the suite passes today

At -count=1, Go runs tests in file order, and add_registry_source_test.go sorts ahead of both consistency_official_test.go and mcp_add_from_registry_test.go, so the assertion runs before either fixture. -shuffle is not used in .github/workflows/, so no lane exercises a different order.

The order is incidental, not a property anything enforces. On unfixed main, running the victim alongside the two fixtures under -shuffle fails on 2 of 5 seeds at -count=1:

seed=1 -> ok
seed=2 -> ok
seed=3 -> --- FAIL: TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP
seed=4 -> ok
seed=5 -> --- FAIL: TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP

-count>1 also fails it, as does adopting t.Parallel(), or adding another policy-disabling fixture in a file sorting ahead of add_registry_source_test.go. With this change, all five seeds pass.

Production behaviour

Unaffected. add_registry_source.go:139, remove_registry_source.go:63 and edit_registry_source.go:103 each do updatedConfig := *currentConfig, a struct copy that carries AllowPrivateRegistryFetch forward, so the CRUD paths propagate the operator's setting rather than resetting it. The defect is confined to the test binary.

The scope is therefore test reliability, on a CWE-918 guard: a future change that broke isBlockedIP would not be caught by the test written to catch it.

Fix

t.Cleanup(func() { registries.SetRegistriesFromConfig(nil) }) in both fixtures. A nil config restores the default catalog and re-enables the policy in one exported call, which also clears the second value leaked from the same line: the testreg / officialreg entry pointing at a closed httptest URL. TestSetRegistriesFromConfig_NilConfigUsesDefaults already pins those semantics.

This follows withGuardActive in internal/registries/ssrf_test.go:21-31, which does save-and-restore for the same flag within its own package.

TestRegistryFixturesRestoreSSRFPolicy is the regression test. It resets the policy itself before running each fixture in a nested subtest, so it detects the leak independently of test order and without -shuffle.

Verified failing before the fix (both fixtures, t.Cleanup lines removed):

--- FAIL: TestRegistryFixturesRestoreSSRFPolicy/startTestRegistry
    Messages: startTestRegistry did not restore the SSRF allow-policy: the cloud-metadata endpoint was accepted
--- FAIL: TestRegistryFixturesRestoreSSRFPolicy/startOfficialTestRegistry
    Messages: startOfficialTestRegistry did not restore the SSRF allow-policy: the cloud-metadata endpoint was accepted

Not included

-shuffle=on in a CI lane. It would prevent this class from recurring, but it may surface unrelated order dependencies elsewhere in the tree, which should not be resolved as part of this fix. Happy to open it separately.

Pre-existing, not addressed here

TestGetServerLogs_MissingFileReturnsEmptyNotError also fails under -race -count=2, and it is unrelated to this change: the same command on unmodified main at 4e304ba0 reproduces both failures.

# clean main
--- FAIL: TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP     (fixed by this PR)
--- FAIL: TestGetServerLogs_MissingFileReturnsEmptyNotError     (pre-existing)

newLogsTestServer registers a server with AddServerConfig only, while NewServer starts config-as-source-of-truth reconciliation in the background; the server is absent from the config, so the reconciler removes it and GetServerLogs can observe "server not found". It passes 30/30 in isolation under -race, so it needs full-package timing to surface. Same class as this PR, different subsystem — happy to file it separately.

Testing

• I have tested these changes locally
• I have added/updated tests that prove my fix is effective
• All existing tests pass

Check	Result
TestRegistryFixturesRestoreSSRFPolicy	fails on unfixed code (both subtests), passes with the fix
Minimal repro above, -count=2	FAIL → ok
Same three tests, -shuffle= seeds 1-5, -count=1	2/5 fail on main; 5/5 pass here
go test -race -count=2 ./internal/server/..., CI's skip set	TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP no longer fails; 0 data races. One unrelated pre-existing failure — see below
./scripts/test-api-e2e.sh	65/65 passed, 0 failed
go vet ./internal/server/	clean
gofmt	clean
golangci-lint v2.13.2, .github/.golangci.yml	5 issues in ./internal/server/..., byte-identical to clean main at 4e304ba0 (1 govet inline hint, 4 SA1019 deprecated-field uses in existing test files). None in the three files changed here.

[Dumbris]

Thank you for this, Dawit — merged (auto-merge armed, it lands as soon as the last CI job finishes). This was a real find: the two loopback registry fixtures left the process-global SSRF allow-policy disabled, so TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP only passed because its file happened to sort ahead of them in the run order. I reproduced the -count=2 failure on main, confirmed the LIFO t.Cleanup restore fixes it across the shuffle seeds, and checked that the regression test still fails when the guard itself is removed, so the fix removed the ordering coupling without weakening what the test covers. The writeup made it an easy review — much appreciated.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!