fix(core): classify one id per request, whatever the answer - #16
Merged
Conversation
Guard::assess fetched all ids in one batch and, if that batch failed, retried each id on its own. Bugzilla reports a nonexistent id by failing the whole request but a bug the key may not see by quietly omitting it from a successful one, so the retry fired for "no such bug" and not for "hidden bug": same bytes back, ~1 upstream request versus ~1+N. A client with a clock could ask about one id alongside twenty known-good ones and read the answer off the latency. The single-id path was equalised for exactly this reason; the batch path reopened it. Fetch each distinct id in its own request, always, and drop the retry. Classification work is now a function of the requested ids and nothing else. It also makes batch poisoning impossible rather than repairing it afterwards, which is what the retry was for, and a response counts for an id only when the server labels it with that id. An id now costs a whole request, so bound how many one call classifies. The bound lives in bugwarden-core next to the loop it bounds, because assess() is public API and must not hand an unbounded run of requests to an out-of-tree caller: past MAX_ASSESS_IDS (25) nothing is fetched and the excess is denied. Tools refuse an over-long list outright instead of answering partially. Both checks read only the request, never a verdict, so refusing discloses nothing. Note what this does NOT equalise: bug_info still issues a second, batched fetch for the bodies of Read-granted ids, so its size follows how many ids were granted. That discloses nothing further — those bodies are being returned anyway — and hidden, summary-only and nonexistent ids all cost exactly one request.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The hole
Guard::assessfetched every id in one batch and retried per id when thatbatch failed. Bugzilla signals the two "you don't get this bug" cases
differently: a nonexistent id fails the whole request, a bug the key
may not see is silently omitted from a successful one. So the retry fired
for the first and not the second — identical bytes returned, ~1+N upstream
requests versus ~1.
A client could name one id alongside twenty known-good ones and read the
answer off the clock, which is exactly what invariant I2 (a hidden bug is
indistinguishable from one that does not exist) forbids. The single-id path
had already been equalised for this reason; the batch path reopened it.
The fix
Every distinct id gets exactly one request, always, and the retry is gone.
Classification work is now a function of the requested ids and nothing else.
Two things come free: batch poisoning becomes impossible instead of being
repaired afterwards (which is all the retry did), and the "only trust a body
the server labels with this id" check — previously only on the retry path —
now covers every id.
Bounding what that costs
An id now costs a whole request, so the id count is a multiplier a client
controls.
MAX_ASSESS_IDS = 25lives inbugwarden-core, next to the loop itbounds, not in the binary:
assessis public API of a published crate andmust not hand an out-of-tree caller an unbounded run of requests. Past the
bound nothing is fetched and the excess is denied (fail closed); tools refuse
an over-long list outright rather than answering partially, and the limit is
stated in the
bug_idsschema so a client sees it before being refused.Sequential, not concurrent, because some deployments drop parallel
connections — the same reason
server_infofetches sequentially.Verification
133 tests (+4). The one that matters puts all three cases side by side —
served, nonexistent, withheld — and pins one request each by mock
construction, so the property holds by construction rather than coincidence.
Others pin the bounded fan-out, that repeats are fetched once, that one bad id
cannot poison the others, and that a body is never credited to the wrong id.
Scope
Two things this deliberately does not fix, both getting their own PR:
bug_infostill classifies one fetch and returns a second, and per-idclassification widens that window. The next PR re-classifies the body it
serves.
verbatim, which can carry Bugzilla's own "Bug #N does not exist." wording —
an I2 leak that predates this change and belongs with the
bug_infowork.