sneak/pixa
1
0
Fork 0
Code Issues 11 Pull Requests 4 Actions 16 Packages Projects Releases Wiki Activity

internal/handlers has 4 tests: the 401 signature boundary and every error path are untested #76

New Issue
Closed
opened 2026-08-09 03:48:01 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:48:01 +02:00
Collaborator
Copy Link
Copy Source

Verified against main at 61f42e6.

internal/handlers has exactly four test functions, all covering ETag/HEAD behavior on HandleImage: TestHandleImage_HEAD_ReturnsHeadersOnly, ..._IfNoneMatch_Returns304, ..._IfNoneMatch_DifferentETag, ..._ETagHeader.

This is the thinnest-tested package in the repo and the one with the most user-visible surface. Untested:

  • 401 on a missing or invalid signature for a non-allowlisted host. This is the product's core security boundary — the thing standing between pixa and being an open proxy — and no Go test exercises it.
  • 400 on an unparseable path or bad fit mode
  • 403 on ErrSSRFBlocked
  • 502 on ErrUpstreamError
  • 410 on an expired signature
  • HandleRobotsTxt, HandleHealthCheck

Status-code mapping is exactly the kind of logic that silently regresses during refactors, and #39 has more package extraction planned.

Definition of done

  1. Table-driven tests over the error paths above, asserting status code and the JSON error body shape, using the mock fetcher (internal/httpfetcher.MockFetcher) so no network is required.
  2. Explicit positive and negative signature cases: valid signature succeeds; tampered signature, absent signature, and expired signature each produce their documented status. Exercise the exact-match guarantee from #40 at the handler layer, not just in internal/signature.
  3. Allowlisted host without a signature succeeds (the documented bypass) — pinning that the bypass is scoped to allowlisted hosts only.
  4. New tests only; no existing test is modified (no owner approval needed for new tests).
  5. t.Parallel() on each test, matching the convention the canonical lint config enforces.
  6. make check green.

Related

Filed separately: coverage for /v1/e/ and the auth flows, which are also entirely untested in Go.

Verified against `main` at `61f42e6`. `internal/handlers` has exactly four test functions, all covering ETag/HEAD behavior on `HandleImage`: `TestHandleImage_HEAD_ReturnsHeadersOnly`, `..._IfNoneMatch_Returns304`, `..._IfNoneMatch_DifferentETag`, `..._ETagHeader`. This is the thinnest-tested package in the repo and the one with the most user-visible surface. Untested: - **401 on a missing or invalid signature for a non-allowlisted host.** This is the product's core security boundary — the thing standing between pixa and being an open proxy — and no Go test exercises it. - 400 on an unparseable path or bad fit mode - 403 on `ErrSSRFBlocked` - 502 on `ErrUpstreamError` - 410 on an expired signature - `HandleRobotsTxt`, `HandleHealthCheck` Status-code mapping is exactly the kind of logic that silently regresses during refactors, and #39 has more package extraction planned. ## Definition of done 1. Table-driven tests over the error paths above, asserting status code **and** the JSON error body shape, using the mock fetcher (`internal/httpfetcher.MockFetcher`) so no network is required. 2. Explicit positive and negative signature cases: valid signature succeeds; tampered signature, absent signature, and expired signature each produce their documented status. Exercise the exact-match guarantee from #40 at the handler layer, not just in `internal/signature`. 3. Allowlisted host without a signature succeeds (the documented bypass) — pinning that the bypass is scoped to allowlisted hosts only. 4. New tests only; no existing test is modified (no owner approval needed for new tests). 5. `t.Parallel()` on each test, matching the convention the canonical lint config enforces. 6. `make check` green. ## Related Filed separately: coverage for `/v1/e/` and the auth flows, which are also entirely untested in Go.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:48:01 +02:00
clawbot commented 2026-10-04 08:45:50 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan, checked against next at 6830bdc. Since this issue was filed, internal/handlers gained tests for the 400 cases (q, fit, exp, query string), the CSRF and login-limit paths, max-age and the busy 503. Still untested at the handler layer, and what this unit adds (new tests only, t.Parallel(), the mock fetcher, no network):

  • One table-driven test of /v1/image/ error answers, checking the status and the JSON error body: missing, tampered and expired signature on a host that is not allowlisted (401, 401, and whatever the handler answers for an expired one, as README.md documents it), an unparseable path (400), a blocked upstream address (403), an upstream error (502).
  • A valid signature succeeds; an allowlisted host without a signature succeeds, and the same request for a host that is not allowlisted is refused, pinning that the bypass is scoped to allowlisted hosts. The exact-match check of #40 is exercised through the handler: a signature that differs only in padding or case is refused.
  • HandleRobotsTxt and HandleHealthCheck answer 200 with their documented body.
  • Where a status the handler gives differs from what README.md says, the test pins the code's answer and the PR names the difference; fixing either belongs to its own issue.

Model: opus-5-5

Plan, checked against `next` at `6830bdc`. Since this issue was filed, `internal/handlers` gained tests for the 400 cases (`q`, `fit`, `exp`, query string), the CSRF and login-limit paths, `max-age` and the busy 503. Still untested at the handler layer, and what this unit adds (new tests only, `t.Parallel()`, the mock fetcher, no network): - One table-driven test of `/v1/image/` error answers, checking the status and the JSON error body: missing, tampered and expired signature on a host that is not allowlisted (401, 401, and whatever the handler answers for an expired one, as `README.md` documents it), an unparseable path (400), a blocked upstream address (403), an upstream error (502). - A valid signature succeeds; an allowlisted host without a signature succeeds, and the same request for a host that is not allowlisted is refused, pinning that the bypass is scoped to allowlisted hosts. The exact-match check of https://git.eeqj.de/sneak/pixa/issues/40 is exercised through the handler: a signature that differs only in padding or case is refused. - `HandleRobotsTxt` and `HandleHealthCheck` answer 200 with their documented body. - Where a status the handler gives differs from what `README.md` says, the test pins the code's answer and the PR names the difference; fixing either belongs to its own issue. Model: opus-5-5
clawbot commented 2026-10-04 09:53:10 +02:00
Author
Collaborator
Copy Link
Copy Source

Tests are in #177, per the plan above. One difference from the issue body: an expired signature is answered 401, not 410, and README.md names no status for it; the test pins 401.

Model: opus-5-5

Tests are in https://git.eeqj.de/sneak/pixa/pulls/177, per the plan above. One difference from the issue body: an expired signature is answered 401, not 410, and `README.md` names no status for it; the test pins 401. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#76