A test that failed two percent of the time

A test in our suite went red, then green on the next run with no code change in between. That pattern has exactly one meaning, and the rest of the story is about finding which test had learned to roll dice.

The guard

The homepage check form posts an endpoint URL and redirects to a result page whose URL carries a 43-character base64url token. Because the redirect target is built from request data, there is an open-redirect guard: a test that submits a set of hostile probe strings and asserts that none of them shows up in the Location header. Eleven probes, things like //evil.example.com, ../evil, a percent-encoded %2F%2Fevil.example.com, and a few locale-shaped strings such as "ZH" and "de".

The assertion was location.includes(probe) === false, for each probe.

The dice

Two of the probes were "ZH" and "de". A 43-character base64url token is drawn from 64 symbols, and a specific two-letter sequence appears somewhere in its 42 adjacent pairs with probability close to 42/4096 per probe. Two probes, about 2% per test run. A calculation and a sampling run agreed on the number.

So the guard was not detecting open redirects two percent of the time. It was detecting that a random token contained the letters “de”.

The first fix would have blinded the guard

The obvious repair: strip the token segment from the Location before the includes check. It was proposed and, at first, accepted. Then, while preparing the change, we traced it. The token segment was the only part of Location derived from anything but a fixed path. Strip it, and the remaining string is a constant that could never contain a probe. The test would be green forever, because it would no longer look at the one part of Location a probe could reach.

That is worth being precise about, because our own notes were wrong the first time. The original substring check was not useless. It looked at the whole Location, token included, and it caught two kinds of mutation the other checks missed. Its only real problem was the randomness. The proposed fix would have traded a guard that was occasionally wrong for one that could never be right.

That version was withdrawn before it merged. The TODO entry that had described it as the plan was rewritten, because a TODO that presents a disproven fix as the plan is a false statement in the repository, and we correct those before merging.

The fix that stayed

The redirect’s only variable content is the token, and the token is written to a ledger row at the moment it is issued. So the guard now does the precise thing: it reads the token back from the row the request just created and asserts that Location is exactly the expected path with exactly that token. Equality, not substring. The eleven probes are kept as inputs, since a probe that somehow made it into the token would also break equality. Nothing random is left in the assertion.

The rule we wrote down

Every guard comment in this repository now has to back each thing it claims to catch with a measurement: the command that mutates the code in that way and the assertion that goes red because of it. “This test protects against open redirects” is a claim. “Mutating the redirect to include //evil.example.com makes this assertion fail, and here is the command that shows it” is evidence. A comment that claims nothing needs no measurement. A comment that claims something without one is not accepted in review.

The second rule is smaller and more painful: a randomly failing test does not fail at a convenient time. Ours would have fired on the CI run a deployment was waiting for, on the one day we could least afford to argue with CI. We fixed it alongside a feature, in that feature’s pull request. A reviewer later ran into a sibling by chance, a second test with a random ingredient in it, and that one is still on the list.

Where the facts come from

The probe list and the assertion are in the test file’s history in our repository; the first version of the test already contained the locale-shaped probes. The 2% figure comes from the arithmetic above and from one sampling run recorded in the repository’s TODO entry for this bug, which also records the withdrawn fix and the reason it was withdrawn.