Keep the SSRF-safe transport in clientForConfig (closes #69) #74
Reference in New Issue
Block a user
Delete Branch "issue-69-ssrf-client-timeout"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
clientForConfig()ininternal/delivery/engine.gobuilt a freshhttp.Clientwithout a Transport when a per-target timeout was configured, dropping the request-time private-IP guard for that path.It now reuses the shared client's SSRF-safe transport (
e.client.Transport, the sameNewSSRFSafeTransportinstance), overriding only theTimeout. Behaviour is unchanged when no per-target timeout is set (the shared client is returned as before), so no engine code path makes an outbound target request with a client lacking the SSRF-safe transport.Adds a delivery-package test proving a client from
clientForConfig()with a per-target timeout still refuses private/reserved/link-local destinations, that the timeout is applied, that the SSRF-safe transport is reused (not duplicated), and that the no-timeout path returns the shared client unchanged.Confined to
internal/delivery/only; handlers and server code untouched.Closes #69
What changed
internal/delivery/engine.go—clientForConfig(): when a per-target timeout is set, the returnedhttp.Clientnow setsTransport: e.client.Transport(the sharedNewSSRFSafeTransportinstance), overriding onlyTimeout. Previously it returned a transport-less client, silently dropping the SSRF dialer for that path. No-timeout path is unchanged (still returns the sharede.client).internal/delivery/export_test.go— addedExportClientForConfigandExportClienthelpers for the delivery-package test.internal/delivery/client_ssrf_test.go(new) — asserts a client fromclientForConfig()with a per-target timeout still refuses127.0.0.1,169.254.169.254, and[fe80::1]; that the timeout is applied; that the SSRF-safe transport is reused (same pointer), not duplicated; and that the no-timeout path returns the shared client unchanged.Diffstat: 3 files changed, 129 insertions(+).
Validation
make fmtclean. Docker build gate (runs fmt-check, lint, test, build):Independent review — PASS (merge-ready)
Adversarial review against the issue spec and repo policies. This one is security-sensitive, so I scrutinized it closely.
internal/delivery/engine.goclientForConfig(): the per-target-timeout branch now setsTransport: e.client.Transport, reusing the shared SSRF-safe transport and overriding onlyTimeout. The no-timeout branch still returns the sharede.client. So no outbound-request path drops the request-time private-IP guard, and the transport is shared rather than duplicated. Correct.internal/delivery/client_ssrf_test.gois a genuine functional test, not just pointer assertions: for127.0.0.1,169.254.169.254, and[fe80::1]it builds a per-target-timeout client and actually callsclient.Do, asserting the request is blocked by the guard (error contains "blocked"), the per-target timeout is applied, and the transport pointer matches the shared client's. A second test asserts the no-timeout path returns the shared client unchanged. The cases are offline and deterministic — private/reserved addresses are rejected before any dial.internal/delivery/;make fmtclean;docker build .green; no AI/tooling references; commit subject closes the issue.Verdict: meets the bar, with strong test coverage. Marking merge-ready and handing to @sneak for final review.