Repository navigation
fix(server): name anonymous landing in identity preflight failures - #403
scarabeusiv wants to merge 5 commits into
Conversation
plusky
left a comment
plusky
left a comment
There was a problem hiding this comment.
Thanks for this. The code and tests hold up. There is no key, URL or Bugzilla text in the new messages (I12), and preflight still fails closed (I4). Keying on envelope code 410 is the right choice: it is login_required in 5.0, 5.2 and BMO, and REST answers it as 401. Each of the three new tests fails when its piece is reverted, and both commits pass the gates on their own.
One blocking point, docs only. docs/DESIGN.md (Identity preflight / client contract): "stock Bugzilla's own header form (X-Bugzilla-API-Key) is not sent". Stock Bugzilla 5.0 and 5.2 have no header form at all. They accept the key only as the Bugzilla_api_key / api_key parameter (Bugzilla/Auth/Login/APIKey.pm, WebService/Util.pm fix_credentials). The X-Bugzilla-API-Key header is BMO's (bmo/Bugzilla/Auth/Login/APIKey.pm, X_BUGZILLA_API_KEY). Suggested wording: "stock Bugzilla 5.0/5.2 has no header form at all; BMO's own header form (X-Bugzilla-API-Key) is not sent."
Non-blocking, take them or leave them:
- README and DESIGN say the query parameter is what BMO accepts. BMO still maps
api_key, but its API docs call the query parameter deprecated, a stopgap for old callers. The supported forms there areX-Bugzilla-API-Keyor an OAuth2 bearer token, so--use-auth-headeragainst BMO also lands anonymous. "still accepts, but documents as deprecated" would be more accurate. preflight_declared_login_transport_error_names_the_endpointonly asserts"GET /rest/valid_login", which both arms print. So alogin_required()that answered true for every error would survive on the declared arm. Anassert!(!msg.contains("anonymously"))there would pin it, mirroring the whoami test.server.rs: the new rustdoc paragraph ending "…names the possibility too." runs straight into "[BugWarden::new] already refuses…" with no blank///line, so the two render as one paragraph. DESIGN has the same thing just before "Errors: …".
The red rust-deny check is not yours. main fails it too, on RUSTSEC-2026-0346 to 0349 against rustls 0.23.45, and this PR touches no dependencies.
A Bugzilla probe refused with login_required (envelope code 410) reached Bugzilla anonymously: the API key did not arrive in a form the deployment honours. Preflight diagnosed it as something else — the whoami arm blamed the endpoint's existence, the declared arm a wrong login or a generic endpoint failure — and a valid_login false, which an anonymous request also produces, named only the account. Both server-custody arms now key on code 410 and name the anonymous landing and the API key placement (the api_key query parameter by default, an Authorization: Bearer header under --use-auth-header), including the transit that can strip the chosen form. The valid_login-false bail names the same possibility alongside the login. DESIGN.md records the diagnosis in the client contract and the Identity preflight section; three wiremock tests pin it, each verified to fail when its piece of the wording is removed.
The API key setup now says which deployment honours which placement (the query parameter for stock Bugzilla and BMO, the Bearer header for Red Hat Bugzilla), what a stripped placement looks like — every request anonymous, writes refused with code 410 — and that the identity preflight names the cause at startup.
Stock Bugzilla 5.0/5.2 has no header form at all; the X-Bugzilla-API-Key header is BMO's. BMO still accepts the query parameter, but documents it as deprecated; its supported forms are that header and OAuth2 bearer tokens.
…nymous-landing diagnosis Both preflight arms print GET /rest/valid_login in their transport and login_required contexts, so the endpoint assertion alone cannot catch a login_required() that answers true for every error. Assert the transport error never claims an anonymous landing, mirroring the whoami arm's pin.
…ogether The server.rs paragraph ending 'names the possibility too.' ran straight into the BugWarden::new paragraph, and DESIGN ran the same text straight into 'Errors:'; add the missing blank lines so both render as the two paragraphs they are.
6a909b9 to
60c4add
Compare
|
All four points addressed; new head 60c4add. The branch is also rebased onto current main (921c742, the rustls 0.23.46 bump from #404).
rust-deny: as you called it, the red check was main's (RUSTSEC-2026-0346..0349 against rustls 0.23.45). On the rebased tree |
2 participants
What
The identity preflight now diagnoses a
login_requiredrefusal (envelope code 410) for what it is — the request reached Bugzilla anonymously, so the API key did not arrive in the form the deployment honours — instead of blaming thewhoamiendpoint's existence or a wrong login. Thevalid_login-false bail names the same possibility, since an anonymous request answersfalsethere too.Why
When the network path between bugwarden and Bugzilla strips the chosen credential form (an egress proxy dropping custom headers, a gateway rewriting query strings), every request lands anonymous: reads silently narrow to the anonymous view and writes fail with "You must log in before using this part of Bugzilla", which reads like a guard-policy or permissions denial and sends debugging in the wrong direction. The placement switch itself already exists (
--use-auth-header/BUGZILLA_USE_AUTH_HEADER; theapi_keyquery parameter is the default) — what was missing is any surface naming this failure for what it is, and any guidance on which deployment honours which form.How
preflightkeys on theBugzillaErrorenvelope code (410; Bugzilla answers it as HTTP 401). Both server-custody arms name the anonymous landing, the placement in use, and the remedy; the non-410 wording is unchanged.UPSTREAM_HINTSstill gives auth codes the bare line to MCP callers, and no key material reaches the new text (I12). No guard behaviour changes.Verified
cargo fmt --check, clippy (workspace and--features gen,-D warnings), rustdoc (-D warnings, both),typoson the changed files: clean.cargo test --workspace --all-targets --locked: 1,048 passed, 0 failed;--doctests green.false-bail sentence removed).cargo deny check: advisories fails on pristinemainas well (RUSTSEC-2026 advisories against the locked rustls); this change touches no dependencies.