Adds tests for the image route's signature check and its error answers, plus /robots.txt and the health check, per the plan on #76. New tests and the TODO.md entry only; no code or existing test changes.
TestHandleImage_ErrorAnswers: one table through the image route, checking the status, Content-Type and JSON error body (error, status, timestamp) for: no sig or exp, exp without sig, a signature made with another key, a valid signature without its = padding, the same in upper case, an expired signature, and a valid signature sent for the signed host's parent domain, a sibling host, a subdomain, or the host with another domain appended (each 401 on a host not on the allowlist); an unparseable path (400); localhost as the upstream host (403); an upstream error (502). The image exists on every host those rows ask for, so a request wrongly let through would be served, not refused.
TestHandleImage_AllowlistOrSignature: an allowlisted host is served without a signature; the same request for another host is refused, and served with a valid signature.
TestHandleRobotsTxt, TestHandleHealthCheck: status and body, with every key README.md documents for the health check, while maintenance mode is on.
Disclosures:
Judgement call: the 403 row uses the real fetcher, which refuses localhost before any lookup or connection; the mock fetcher cannot return that error. Every other row uses httpfetcher.MockFetcher.
Difference: the issue body expected 410 for an expired signature; the handler answers 401, as the Routes section of README.md says. The test pins 401.
Model: opus-5-5
Adds tests for the image route's signature check and its error answers, plus `/robots.txt` and the health check, per the plan on https://git.eeqj.de/sneak/pixa/issues/76. New tests and the `TODO.md` entry only; no code or existing test changes.
- `TestHandleImage_ErrorAnswers`: one table through the image route, checking the status, `Content-Type` and JSON error body (`error`, `status`, `timestamp`) for: no `sig` or `exp`, `exp` without `sig`, a signature made with another key, a valid signature without its `=` padding, the same in upper case, an expired signature, and a valid signature sent for the signed host's parent domain, a sibling host, a subdomain, or the host with another domain appended (each 401 on a host not on the allowlist); an unparseable path (400); `localhost` as the upstream host (403); an upstream error (502). The image exists on every host those rows ask for, so a request wrongly let through would be served, not refused.
- `TestHandleImage_AllowlistOrSignature`: an allowlisted host is served without a signature; the same request for another host is refused, and served with a valid signature.
- `TestHandleRobotsTxt`, `TestHandleHealthCheck`: status and body, with every key `README.md` documents for the health check, while maintenance mode is on.
Disclosures:
- Judgement call: the 403 row uses the real fetcher, which refuses `localhost` before any lookup or connection; the mock fetcher cannot return that error. Every other row uses `httpfetcher.MockFetcher`.
- Difference: the issue body expected 410 for an expired signature; the handler answers 401, as the Routes section of `README.md` says. The test pins 401.
Model: opus-5-5
internal/handlers/image_errors_internal_test.go: the exact-match guarantee of #40, which item 2 of the definition of done in #76 asks to exercise through the route, is not tested. That guarantee is that a signature verifies only for the host (and path, size and format) it was made for: never for its parent domain, a sibling or deeper subdomain, or the host with another domain appended. Every signed URL in the new tests is sent for the host it was signed for; the padding and upper-case rows test how the sig text is compared, which is a different check. Acceptable: rows that send a valid signature for signedHost with a related host, at least its parent domain and signedHost with another domain appended, with the mock fetcher serving the image on each of those hosts so a wrongly accepted signature would be served, each answered 401.
internal/handlers/robots_healthcheck_internal_test.go, TestHandleHealthCheck: checks only status, appname, version and maintenance_mode, while README.md documents the body with now, uptime_seconds and uptime_human as well, and the plan asks for the documented body. Dropping or renaming any of those three keys still passes. Acceptable: also check that those three keys are present.
internal/handlers/image_errors_internal_test.go, sendGet: logs the whole response body, so every request answered 200 writes the JPEG's bytes into the test log. script/test reruns the suite verbosely whenever any test fails, and the build log then carries binary data. Acceptable: log the body only for a response that is not an image, or log only the status, as getImage does.
PR body, second disclosure: it says README.md names no status for an expired signature. On current next, the Routes section of README.md lists 401 for "an exp in the past", so it agrees with the code and with the test; the disclosure is no longer true. Acceptable: remove it, or say that README.md agrees with the 401 the test pins.
Disclosure: TODO.md conflicts with current next; I resolved it locally by keeping both entries (this PR's on top) for the review. The rework needs the same rebase.
Model: opus-5-5
**FAIL** (needs-rework)
Reviewed `390f8bd` rebased onto `next` at `363774c`.
1. `internal/handlers/image_errors_internal_test.go`: the exact-match guarantee of https://git.eeqj.de/sneak/pixa/pulls/40, which item 2 of the definition of done in https://git.eeqj.de/sneak/pixa/issues/76 asks to exercise through the route, is not tested. That guarantee is that a signature verifies only for the host (and path, size and format) it was made for: never for its parent domain, a sibling or deeper subdomain, or the host with another domain appended. Every signed URL in the new tests is sent for the host it was signed for; the padding and upper-case rows test how the `sig` text is compared, which is a different check. Acceptable: rows that send a valid signature for `signedHost` with a related host, at least its parent domain and `signedHost` with another domain appended, with the mock fetcher serving the image on each of those hosts so a wrongly accepted signature would be served, each answered 401.
2. `internal/handlers/robots_healthcheck_internal_test.go`, `TestHandleHealthCheck`: checks only `status`, `appname`, `version` and `maintenance_mode`, while `README.md` documents the body with `now`, `uptime_seconds` and `uptime_human` as well, and the plan asks for the documented body. Dropping or renaming any of those three keys still passes. Acceptable: also check that those three keys are present.
3. `internal/handlers/image_errors_internal_test.go`, `sendGet`: logs the whole response body, so every request answered 200 writes the JPEG's bytes into the test log. `script/test` reruns the suite verbosely whenever any test fails, and the build log then carries binary data. Acceptable: log the body only for a response that is not an image, or log only the status, as `getImage` does.
4. PR body, second disclosure: it says `README.md` names no status for an expired signature. On current `next`, the Routes section of `README.md` lists 401 for "an `exp` in the past", so it agrees with the code and with the test; the disclosure is no longer true. Acceptable: remove it, or say that `README.md` agrees with the 401 the test pins.
Disclosure: `TODO.md` conflicts with current `next`; I resolved it locally by keeping both entries (this PR's on top) for the review. The rework needs the same rebase.
Model: opus-5-5
Reworked for #177 (comment), rebased onto next with both TODO.md entries kept, this one on top.
TestHandleImage_ErrorAnswers has four new rows: a valid signature for signedHost sent for its parent domain, a sibling host, a subdomain, and the host with another domain appended, each answered 401; the mock fetcher serves the image on all four hosts.
TestHandleHealthCheck also checks that now, uptime_seconds and uptime_human are present.
sendGet logs only the status, as getImage does.
The PR body's second disclosure now says README.md agrees with the 401 the test pins.
Model: opus-5-5
Reworked for https://git.eeqj.de/sneak/pixa/pulls/177#issuecomment-122180, rebased onto `next` with both `TODO.md` entries kept, this one on top.
1. `TestHandleImage_ErrorAnswers` has four new rows: a valid signature for `signedHost` sent for its parent domain, a sibling host, a subdomain, and the host with another domain appended, each answered 401; the mock fetcher serves the image on all four hosts.
2. `TestHandleHealthCheck` also checks that `now`, `uptime_seconds` and `uptime_human` are present.
3. `sendGet` logs only the status, as `getImage` does.
4. The PR body's second disclosure now says `README.md` agrees with the 401 the test pins.
Model: opus-5-5
On current next, with #179 merged, the internal/handlers tests no longer compile: signedPhotoURL is declared twice. This PR's image_errors_internal_test.go has signedPhotoURL(host, sig string, expires time.Time) string, and request_id_internal_test.go from #179 has signedPhotoURL(t *testing.T, h *Handlers) string, used as a map value in TestImageLogLinesCarryRequestID. No other name in the two new files collides.
Acceptable: the two helpers no longer share a name and the new tests compile and pass on current next. Rebasing also conflicts in TODO.md, only because both PRs add an entry at the top of Completed Steps; keep both, this PR's on top.
Model: opus-5-5
On current `next`, with https://git.eeqj.de/sneak/pixa/pulls/179 merged, the `internal/handlers` tests no longer compile: `signedPhotoURL` is declared twice. This PR's `image_errors_internal_test.go` has `signedPhotoURL(host, sig string, expires time.Time) string`, and `request_id_internal_test.go` from https://git.eeqj.de/sneak/pixa/pulls/179 has `signedPhotoURL(t *testing.T, h *Handlers) string`, used as a map value in `TestImageLogLinesCarryRequestID`. No other name in the two new files collides.
Acceptable: the two helpers no longer share a name and the new tests compile and pass on current `next`. Rebasing also conflicts in `TODO.md`, only because both PRs add an entry at the top of Completed Steps; keep both, this PR's on top.
Model: opus-5-5
New tests in internal/handlers, with no network: the status and JSON
error body the image route answers for a missing, wrong, unpadded,
upper-case or expired signature on a host not on the allowlist, or a
valid one sent for its parent domain, a sibling host, a subdomain or
the host with another domain appended; an unparseable path, localhost
as the upstream host, and an upstream error; that an allowlisted host
is served without a signature and another host only with a valid one;
and the answers of /robots.txt and the health check. An expired
signature is answered 401, as the code and README.md say, where the
issue body expected 410. No code changes.
Model: opus-5-5
Renamed this PR's test helper signedPhotoURL in internal/handlers/image_errors_internal_test.go to photoURLWithSig, so it no longer collides with the helper of the same name added to next by #179; rebased onto next.
Model: opus-5-5
Renamed this PR's test helper `signedPhotoURL` in `internal/handlers/image_errors_internal_test.go` to `photoURLWithSig`, so it no longer collides with the helper of the same name added to `next` by https://git.eeqj.de/sneak/pixa/pulls/179; rebased onto `next`.
Model: opus-5-5
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.
Adds tests for the image route's signature check and its error answers, plus
/robots.txtand the health check, per the plan on #76. New tests and theTODO.mdentry only; no code or existing test changes.TestHandleImage_ErrorAnswers: one table through the image route, checking the status,Content-Typeand JSON error body (error,status,timestamp) for: nosigorexp,expwithoutsig, a signature made with another key, a valid signature without its=padding, the same in upper case, an expired signature, and a valid signature sent for the signed host's parent domain, a sibling host, a subdomain, or the host with another domain appended (each 401 on a host not on the allowlist); an unparseable path (400);localhostas the upstream host (403); an upstream error (502). The image exists on every host those rows ask for, so a request wrongly let through would be served, not refused.TestHandleImage_AllowlistOrSignature: an allowlisted host is served without a signature; the same request for another host is refused, and served with a valid signature.TestHandleRobotsTxt,TestHandleHealthCheck: status and body, with every keyREADME.mddocuments for the health check, while maintenance mode is on.Disclosures:
localhostbefore any lookup or connection; the mock fetcher cannot return that error. Every other row useshttpfetcher.MockFetcher.README.mdsays. The test pins 401.Model: opus-5-5
FAIL (needs-rework)
Reviewed
390f8bdrebased ontonextat363774c.internal/handlers/image_errors_internal_test.go: the exact-match guarantee of #40, which item 2 of the definition of done in #76 asks to exercise through the route, is not tested. That guarantee is that a signature verifies only for the host (and path, size and format) it was made for: never for its parent domain, a sibling or deeper subdomain, or the host with another domain appended. Every signed URL in the new tests is sent for the host it was signed for; the padding and upper-case rows test how thesigtext is compared, which is a different check. Acceptable: rows that send a valid signature forsignedHostwith a related host, at least its parent domain andsignedHostwith another domain appended, with the mock fetcher serving the image on each of those hosts so a wrongly accepted signature would be served, each answered 401.internal/handlers/robots_healthcheck_internal_test.go,TestHandleHealthCheck: checks onlystatus,appname,versionandmaintenance_mode, whileREADME.mddocuments the body withnow,uptime_secondsanduptime_humanas well, and the plan asks for the documented body. Dropping or renaming any of those three keys still passes. Acceptable: also check that those three keys are present.internal/handlers/image_errors_internal_test.go,sendGet: logs the whole response body, so every request answered 200 writes the JPEG's bytes into the test log.script/testreruns the suite verbosely whenever any test fails, and the build log then carries binary data. Acceptable: log the body only for a response that is not an image, or log only the status, asgetImagedoes.README.mdnames no status for an expired signature. On currentnext, the Routes section ofREADME.mdlists 401 for "anexpin the past", so it agrees with the code and with the test; the disclosure is no longer true. Acceptable: remove it, or say thatREADME.mdagrees with the 401 the test pins.Disclosure:
TODO.mdconflicts with currentnext; I resolved it locally by keeping both entries (this PR's on top) for the review. The rework needs the same rebase.Model: opus-5-5
390f8bdd87to41558c8d53Reworked for #177 (comment), rebased onto
nextwith bothTODO.mdentries kept, this one on top.TestHandleImage_ErrorAnswershas four new rows: a valid signature forsignedHostsent for its parent domain, a sibling host, a subdomain, and the host with another domain appended, each answered 401; the mock fetcher serves the image on all four hosts.TestHandleHealthCheckalso checks thatnow,uptime_secondsanduptime_humanare present.sendGetlogs only the status, asgetImagedoes.README.mdagrees with the 401 the test pins.Model: opus-5-5
PASS
41558c8rebased ontonextat363774c.Model: opus-5-5
On current
next, with #179 merged, theinternal/handlerstests no longer compile:signedPhotoURLis declared twice. This PR'simage_errors_internal_test.gohassignedPhotoURL(host, sig string, expires time.Time) string, andrequest_id_internal_test.gofrom #179 hassignedPhotoURL(t *testing.T, h *Handlers) string, used as a map value inTestImageLogLinesCarryRequestID. No other name in the two new files collides.Acceptable: the two helpers no longer share a name and the new tests compile and pass on current
next. Rebasing also conflicts inTODO.md, only because both PRs add an entry at the top of Completed Steps; keep both, this PR's on top.Model: opus-5-5
41558c8d53to6a52426278Renamed this PR's test helper
signedPhotoURLininternal/handlers/image_errors_internal_test.gotophotoURLWithSig, so it no longer collides with the helper of the same name added tonextby #179; rebased ontonext.Model: opus-5-5
PASS
6a52426onnextatc7173c4.Model: opus-5-5