clientForConfig() in internal/delivery/engine.go built a fresh http.Client without 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 same NewSSRFSafeTransport instance), overriding only the Timeout. 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.
`clientForConfig()` in `internal/delivery/engine.go` built a fresh `http.Client` without 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 same `NewSSRFSafeTransport` instance), overriding only the `Timeout`. 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
internal/delivery/engine.go — clientForConfig(): when a per-target timeout is set, the returned http.Client now sets Transport: e.client.Transport (the shared NewSSRFSafeTransport instance), overriding only Timeout. Previously it returned a transport-less client, silently dropping the SSRF dialer for that path. No-timeout path is unchanged (still returns the shared e.client).
internal/delivery/export_test.go — added ExportClientForConfig and ExportClient helpers for the delivery-package test.
internal/delivery/client_ssrf_test.go (new) — asserts a client from clientForConfig() with a per-target timeout still refuses 127.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.
### What changed
- `internal/delivery/engine.go` — `clientForConfig()`: when a per-target timeout is set, the returned `http.Client` now sets `Transport: e.client.Transport` (the shared `NewSSRFSafeTransport` instance), overriding only `Timeout`. Previously it returned a transport-less client, silently dropping the SSRF dialer for that path. No-timeout path is unchanged (still returns the shared `e.client`).
- `internal/delivery/export_test.go` — added `ExportClientForConfig` and `ExportClient` helpers for the delivery-package test.
- `internal/delivery/client_ssrf_test.go` (new) — asserts a client from `clientForConfig()` with a per-target timeout still refuses `127.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 fmt` clean. Docker build gate (runs fmt-check, lint, test, build):
```
docker build . exit code: 0
```
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 sets Transport: e.client.Transport, reusing the shared SSRF-safe transport and overriding only Timeout. The no-timeout branch still returns the shared e.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.go is a genuine functional test, not just pointer assertions: for 127.0.0.1, 169.254.169.254, and [fe80::1] it builds a per-target-timeout client and actually calls client.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.
Scope confined to internal/delivery/; make fmt clean; 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.
## 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.go` `clientForConfig()`: the per-target-timeout branch now sets `Transport: e.client.Transport`, reusing the shared SSRF-safe transport and overriding only `Timeout`. The no-timeout branch still returns the shared `e.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.go` is a genuine functional test, not just pointer assertions: for `127.0.0.1`, `169.254.169.254`, and `[fe80::1]` it builds a per-target-timeout client and actually calls `client.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.
- Scope confined to `internal/delivery/`; `make fmt` clean; `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.
sneak
was assigned by clawbot2026-08-07 14:02:23 +02:00
sneak
merged commit b1f43c9520 into main2026-08-07 14:03:38 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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.