httpfetcher: redirect-to-private-IP SSRF and semaphore release are untested #78

New Issue

clawbot · 2026-08-09T03:48:28+02:00

clawbot commented

2026-08-09 03:48:28 +02:00

Verified against main at 61f42e6.

internal/httpfetcher has 15 tests, but they cover helpers in isolation (DefaultConfig, isAllowedContentType, extractHost, isLocalhost, isPrivateIP, validateURL variants, limitedReader, the mock). The paths that actually carry risk have no coverage at all:

The CheckRedirect validator (httpfetcher.go:133-143) — i.e. redirect-based SSRF. An attacker who controls an allowlisted or signable host can return 302 Location: http://169.254.169.254/... and, if this validator is wrong, pixa follows it. The initial-URL check is well tested; the redirect check, which is the easier one to get wrong, is not tested at all. This is the highest-value gap in the package.
Per-host semaphore release on the failure path (:186-192) and semaphoreReleasingReadCloser (:466-477) — a leaked slot permanently reduces capacity for that host until restart. Silent, cumulative, and invisible until the host stops being fetchable.
MaxResponseSize enforcement end-to-end through Fetch — limitedReader is unit-tested, but not that Fetch actually applies it.
Non-2xx and disallowed-content-type rejection through Fetch.
ssrfSafeDialer — the authoritative check that closes the DNS-rebinding window. Untested.

These need an httptest.Server, which is why they were presumably skipped.

Definition of done

Redirect test: an httptest.Server returning a 302 to a private/link-local address; assert the fetch is refused with ErrSSRFBlocked and that no connection to the private address is attempted. Include a redirect chain and a redirect to a public address (which must still succeed).
Semaphore tests: the slot is released when Fetch returns an error, when the response body is closed, and when the body is abandoned without a full read. Assert by saturating the semaphore and confirming a subsequent fetch still proceeds.
MaxResponseSize test: a server returning more than the limit results in a bounded read and a clear error, not a truncated success.
Tests for non-2xx and disallowed content type through the full Fetch path.
ssrfSafeDialer test proving the dial-time re-resolution actually blocks a host whose DNS answer changes between validation and dial (rebinding), if that can be simulated with a custom resolver; if not, document why and cover what can be.
New tests only. make check green (suite runs with -race as of #55).

The blocked-networks issue extends isPrivateIP's coverage; these tests should be written so new ranges are easy to add to the table.

Verified against `main` at `61f42e6`. `internal/httpfetcher` has 15 tests, but they cover helpers in isolation (`DefaultConfig`, `isAllowedContentType`, `extractHost`, `isLocalhost`, `isPrivateIP`, `validateURL` variants, `limitedReader`, the mock). The paths that actually carry risk have no coverage at all: 1. **The `CheckRedirect` validator (`httpfetcher.go:133-143`)** — i.e. redirect-based SSRF. An attacker who controls an allowlisted or signable host can return `302 Location: http://169.254.169.254/...` and, if this validator is wrong, pixa follows it. The initial-URL check is well tested; the redirect check, which is the easier one to get wrong, is not tested at all. This is the highest-value gap in the package. 2. **Per-host semaphore release on the failure path (`:186-192`)** and **`semaphoreReleasingReadCloser` (`:466-477`)** — a leaked slot permanently reduces capacity for that host until restart. Silent, cumulative, and invisible until the host stops being fetchable. 3. **`MaxResponseSize` enforcement end-to-end through `Fetch`** — `limitedReader` is unit-tested, but not that `Fetch` actually applies it. 4. **Non-2xx and disallowed-content-type rejection through `Fetch`.** 5. **`ssrfSafeDialer`** — the authoritative check that closes the DNS-rebinding window. Untested. These need an `httptest.Server`, which is why they were presumably skipped. ## Definition of done 1. Redirect test: an `httptest.Server` returning a 302 to a private/link-local address; assert the fetch is refused with `ErrSSRFBlocked` and that no connection to the private address is attempted. Include a redirect chain and a redirect to a *public* address (which must still succeed). 2. Semaphore tests: the slot is released when `Fetch` returns an error, when the response body is closed, and when the body is abandoned without a full read. Assert by saturating the semaphore and confirming a subsequent fetch still proceeds. 3. `MaxResponseSize` test: a server returning more than the limit results in a bounded read and a clear error, not a truncated success. 4. Tests for non-2xx and disallowed content type through the full `Fetch` path. 5. `ssrfSafeDialer` test proving the dial-time re-resolution actually blocks a host whose DNS answer changes between validation and dial (rebinding), if that can be simulated with a custom resolver; if not, document why and cover what can be. 6. New tests only. `make check` green (suite runs with `-race` as of #55). ## Related The blocked-networks issue extends `isPrivateIP`'s coverage; these tests should be written so new ranges are easy to add to the table.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:48:28 +02:00

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#78