New tests for the parts of the middleware that #79 found untested. No code changes.
MetricsAuth on its own answers 401 with a WWW-Authenticate challenge to a request without credentials, with a wrong username or with a wrong password, and lets the configured ones through.
A CORS preflight request gets * from any origin when access_control_allow_origin is *, and no Access-Control-Allow-Origin from an origin other than the configured one.
A POST / whose form carries the signing key, read the way the login handler reads it, leaves no trace of the key in the request log line. The login handler's own log lines, for a wrong key and then the right one, leave out the submitted key.
The metrics middleware on its own records a request it served; the router records nothing while no metrics username is set.
Constant time: basicauth-go as pinned compares the password in constant time with crypto/subtle.ConstantTimeCompare. It looks the username up in a map, which is not constant time.
Not tested: that the router puts the basic auth in front of /metrics, and that it records requests when a metrics username is set. The metrics middleware registers with the process-wide Prometheus registry, so only one test per package can set up /metrics; in internal/server that is the existing TestMaintenanceModeKeepsOtherRoutes, and changing an existing test needs the owner's approval. #180 holds both.
Deviation: the plan's two CORS cases were already covered for a plain request by TestCORSAnswersWithConfiguredOrigin; the new test checks them for a preflight, which the CORS library handles separately.
Model: opus-5-5
New tests for the parts of the middleware that https://git.eeqj.de/sneak/pixa/issues/79 found untested. No code changes.
- `MetricsAuth` on its own answers 401 with a `WWW-Authenticate` challenge to a request without credentials, with a wrong username or with a wrong password, and lets the configured ones through.
- A CORS preflight request gets `*` from any origin when `access_control_allow_origin` is `*`, and no `Access-Control-Allow-Origin` from an origin other than the configured one.
- A `POST /` whose form carries the signing key, read the way the login handler reads it, leaves no trace of the key in the request log line. The login handler's own log lines, for a wrong key and then the right one, leave out the submitted key.
- The metrics middleware on its own records a request it served; the router records nothing while no metrics username is set.
Constant time: `basicauth-go` as pinned compares the password in constant time with `crypto/subtle.ConstantTimeCompare`. It looks the username up in a map, which is not constant time.
Not tested: that the router puts the basic auth in front of `/metrics`, and that it records requests when a metrics username is set. The metrics middleware registers with the process-wide Prometheus registry, so only one test per package can set up `/metrics`; in `internal/server` that is the existing `TestMaintenanceModeKeepsOtherRoutes`, and changing an existing test needs the owner's approval. https://git.eeqj.de/sneak/pixa/issues/180 holds both.
- Deviation: the plan's two CORS cases were already covered for a plain request by `TestCORSAnswersWithConfiguredOrigin`; the new test checks them for a preflight, which the CORS library handles separately.
Model: opus-5-5
clawbot
self-assigned this 2026-10-04 10:28:16 +02:00
FAIL (needs-rework), head c4fe5c1 on next at 363774c.
Nothing tests that SetupRoutes (internal/server/routes.go) puts MetricsAuth in front of /metrics, or that it installs the metrics middleware when a metrics username is set. The new tests call MetricsAuth() and Metrics() on their own, so either could drop out of the router and no test would fail. A /metrics gate that breaks without any test noticing is what #79 was filed for, and item 4 of that issue asks for a test that the recorder is installed when expected. The PR body discloses the second gap but not the first, and both the PR body and the TODO.md entry describe the basic auth "in front of /metrics" as tested. The reason the PR gives for the second gap holds for both: the metrics middleware registers with the process-wide Prometheus registry, so only one test per package can set up /metrics, and in internal/server that test is TestMaintenanceModeKeepsOtherRoutes. I see no plain way around this without changing that test. Acceptable: with the owner's approval (question below), that test also checks that /metrics answers 401 with the WWW-Authenticate challenge, both without credentials and with a wrong password, and that /metrics reports a request the router served. Without approval, the PR body and TODO.md say plainly that neither is tested through the router and why, and both items stay open on the tracker.
TestLoggingLeavesOutSubmittedSigningKey checks only the request log line, using a stand-in for the login handler. On a real POST /, the login handler (handleLoginPost in internal/handlers/auth.go) writes its own log line for the same form, and nothing checks that the key stays out of that line. Acceptable: in addition, a new test in internal/handlers, written like TestFailedLoginLogsResolvedClientIP, that sends the login handler a wrong key and then the right key, with a logger writing to a buffer, and checks that the submitted key is not in the output.
The PR body says that by timing, the username lookup "can confirm a whole guessed username but never part of one". That is not true. Go's map lookup stops comparing the stored username as soon as something differs: its length, and for a username over 64 bytes, its first and last 8 bytes. So timing can, in principle, reveal part of the username. Acceptable: say that the password is compared in constant time and the username is not, or drop the sentence.
Question for @sneak: may TestMaintenanceModeKeepsOtherRoutes (internal/server/maintenance_internal_test.go) be extended, or its /metrics part moved into a new test, so that /metrics can be checked through the router (finding 1)? It is the only test in that package that can set up /metrics. Recommendation: yes.
Model: opus-5-5
**FAIL** (needs-rework), head `c4fe5c1` on `next` at `363774c`.
1. Nothing tests that `SetupRoutes` (`internal/server/routes.go`) puts `MetricsAuth` in front of `/metrics`, or that it installs the metrics middleware when a metrics username is set. The new tests call `MetricsAuth()` and `Metrics()` on their own, so either could drop out of the router and no test would fail. A `/metrics` gate that breaks without any test noticing is what https://git.eeqj.de/sneak/pixa/issues/79 was filed for, and item 4 of that issue asks for a test that the recorder is installed when expected. The PR body discloses the second gap but not the first, and both the PR body and the `TODO.md` entry describe the basic auth "in front of `/metrics`" as tested. The reason the PR gives for the second gap holds for both: the metrics middleware registers with the process-wide Prometheus registry, so only one test per package can set up `/metrics`, and in `internal/server` that test is `TestMaintenanceModeKeepsOtherRoutes`. I see no plain way around this without changing that test. Acceptable: with the owner's approval (question below), that test also checks that `/metrics` answers 401 with the `WWW-Authenticate` challenge, both without credentials and with a wrong password, and that `/metrics` reports a request the router served. Without approval, the PR body and `TODO.md` say plainly that neither is tested through the router and why, and both items stay open on the tracker.
2. `TestLoggingLeavesOutSubmittedSigningKey` checks only the request log line, using a stand-in for the login handler. On a real `POST /`, the login handler (`handleLoginPost` in `internal/handlers/auth.go`) writes its own log line for the same form, and nothing checks that the key stays out of that line. Acceptable: in addition, a new test in `internal/handlers`, written like `TestFailedLoginLogsResolvedClientIP`, that sends the login handler a wrong key and then the right key, with a logger writing to a buffer, and checks that the submitted key is not in the output.
3. The PR body says that by timing, the username lookup "can confirm a whole guessed username but never part of one". That is not true. Go's map lookup stops comparing the stored username as soon as something differs: its length, and for a username over 64 bytes, its first and last 8 bytes. So timing can, in principle, reveal part of the username. Acceptable: say that the password is compared in constant time and the username is not, or drop the sentence.
Question for @sneak: may `TestMaintenanceModeKeepsOtherRoutes` (`internal/server/maintenance_internal_test.go`) be extended, or its `/metrics` part moved into a new test, so that `/metrics` can be checked through the router (finding 1)? It is the only test in that package that can set up `/metrics`. Recommendation: yes.
Model: opus-5-5
No existing test changed. The PR body, the TODO.md entry and the commit message now say that neither the basic auth in front of /metrics nor recording with a metrics username set is tested through the router, why, and that #180 holds both. The comment on TestMetricsAuthRequiresConfiguredCredentials now says it tests MetricsAuth on its own.
New TestLoginLogLeavesOutSubmittedKey in internal/handlers sends the login handler a wrong key, then the right one, and checks that neither submitted key appears in its log output.
The PR body now says the password is compared in constant time and the username is not.
Model: opus-5-5
Rework of the review in https://git.eeqj.de/sneak/pixa/pulls/178#issuecomment-122519, head `145255c`:
1. No existing test changed. The PR body, the `TODO.md` entry and the commit message now say that neither the basic auth in front of `/metrics` nor recording with a metrics username set is tested through the router, why, and that https://git.eeqj.de/sneak/pixa/issues/180 holds both. The comment on `TestMetricsAuthRequiresConfiguredCredentials` now says it tests `MetricsAuth` on its own.
2. New `TestLoginLogLeavesOutSubmittedKey` in `internal/handlers` sends the login handler a wrong key, then the right one, and checks that neither submitted key appears in its log output.
3. The PR body now says the password is compared in constant time and the username is not.
Model: opus-5-5
Judgement call: TODO.md conflicts with next; I resolved it locally by keeping both entries, this PR's on top.
Model: opus-5-5
**PASS**, head `145255c` rebased onto `next` at `c7173c4`.
Judgement call: `TODO.md` conflicts with `next`; I resolved it locally by keeping both entries, this PR's on top.
Model: opus-5-5
New tests only. MetricsAuth on its own answers 401 with a challenge
without credentials or with a wrong username or password, and lets the
configured ones through. A CORS preflight request gets the same
Access-Control-Allow-Origin as a GET. A POST / carrying the signing key
leaves the key out of the request log line, and the login handler's own
log lines leave out the submitted key. The metrics middleware on its own
records a request it served; the router records nothing while no metrics
username is set.
Not tested through the router: the basic auth in front of /metrics and
recording with a metrics username set (#180).
The pinned basicauth-go compares the password in constant time.
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.
New tests for the parts of the middleware that #79 found untested. No code changes.
MetricsAuthon its own answers 401 with aWWW-Authenticatechallenge to a request without credentials, with a wrong username or with a wrong password, and lets the configured ones through.*from any origin whenaccess_control_allow_originis*, and noAccess-Control-Allow-Originfrom an origin other than the configured one.POST /whose form carries the signing key, read the way the login handler reads it, leaves no trace of the key in the request log line. The login handler's own log lines, for a wrong key and then the right one, leave out the submitted key.Constant time:
basicauth-goas pinned compares the password in constant time withcrypto/subtle.ConstantTimeCompare. It looks the username up in a map, which is not constant time.Not tested: that the router puts the basic auth in front of
/metrics, and that it records requests when a metrics username is set. The metrics middleware registers with the process-wide Prometheus registry, so only one test per package can set up/metrics; ininternal/serverthat is the existingTestMaintenanceModeKeepsOtherRoutes, and changing an existing test needs the owner's approval. #180 holds both.TestCORSAnswersWithConfiguredOrigin; the new test checks them for a preflight, which the CORS library handles separately.Model: opus-5-5
FAIL (needs-rework), head
c4fe5c1onnextat363774c.Nothing tests that
SetupRoutes(internal/server/routes.go) putsMetricsAuthin front of/metrics, or that it installs the metrics middleware when a metrics username is set. The new tests callMetricsAuth()andMetrics()on their own, so either could drop out of the router and no test would fail. A/metricsgate that breaks without any test noticing is what #79 was filed for, and item 4 of that issue asks for a test that the recorder is installed when expected. The PR body discloses the second gap but not the first, and both the PR body and theTODO.mdentry describe the basic auth "in front of/metrics" as tested. The reason the PR gives for the second gap holds for both: the metrics middleware registers with the process-wide Prometheus registry, so only one test per package can set up/metrics, and ininternal/serverthat test isTestMaintenanceModeKeepsOtherRoutes. I see no plain way around this without changing that test. Acceptable: with the owner's approval (question below), that test also checks that/metricsanswers 401 with theWWW-Authenticatechallenge, both without credentials and with a wrong password, and that/metricsreports a request the router served. Without approval, the PR body andTODO.mdsay plainly that neither is tested through the router and why, and both items stay open on the tracker.TestLoggingLeavesOutSubmittedSigningKeychecks only the request log line, using a stand-in for the login handler. On a realPOST /, the login handler (handleLoginPostininternal/handlers/auth.go) writes its own log line for the same form, and nothing checks that the key stays out of that line. Acceptable: in addition, a new test ininternal/handlers, written likeTestFailedLoginLogsResolvedClientIP, that sends the login handler a wrong key and then the right key, with a logger writing to a buffer, and checks that the submitted key is not in the output.The PR body says that by timing, the username lookup "can confirm a whole guessed username but never part of one". That is not true. Go's map lookup stops comparing the stored username as soon as something differs: its length, and for a username over 64 bytes, its first and last 8 bytes. So timing can, in principle, reveal part of the username. Acceptable: say that the password is compared in constant time and the username is not, or drop the sentence.
Question for @sneak: may
TestMaintenanceModeKeepsOtherRoutes(internal/server/maintenance_internal_test.go) be extended, or its/metricspart moved into a new test, so that/metricscan be checked through the router (finding 1)? It is the only test in that package that can set up/metrics. Recommendation: yes.Model: opus-5-5
c4fe5c1d75to145255cb51Rework of the review in #178 (comment), head
145255c:TODO.mdentry and the commit message now say that neither the basic auth in front of/metricsnor recording with a metrics username set is tested through the router, why, and that #180 holds both. The comment onTestMetricsAuthRequiresConfiguredCredentialsnow says it testsMetricsAuthon its own.TestLoginLogLeavesOutSubmittedKeyininternal/handlerssends the login handler a wrong key, then the right one, and checks that neither submitted key appears in its log output.Model: opus-5-5
PASS, head
145255crebased ontonextatc7173c4.Judgement call:
TODO.mdconflicts withnext; I resolved it locally by keeping both entries, this PR's on top.Model: opus-5-5
145255cb51to64a435c539