fix: the unidentifiable-upload test is not random - #879
Open
blaipr wants to merge 1 commit into
Open
Conversation
`testUnidentifiableContentFallsBackToTheDeclaredType` built its payload as a fixed prefix followed by `random_bytes(48)`, and handed it to libmagic. Most of the time libmagic gives up and answers application/octet-stream, which is the inconclusive answer the test is about — the upload then falls back to the declared type and succeeds. Sometimes it does not. Measured over 3,000 payloads on this image, 16 came back as image/x-tga: about one run in two hundred. A recognised type is not inconclusive, so the declared application/pdf is not used, image/x-tga is not on the allow-list, and the upload is refused — the run fails with 400 where it wanted 200, on whichever pull request happened to be open when it landed. This is the same shape as the faker `randomNumber()` fixture already recorded here: a value drawn at random that is almost always in range, and occasionally is not. The payload is fixed now, and confirmed inconclusive. Found because it failed CI on an unrelated change, and reproduced by measuring rather than by re-running until it happened again.
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
1 participant
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.
testUnidentifiableContentFallsBackToTheDeclaredTypebuilt its payload as a fixed prefixfollowed by
random_bytes(48), and handed it to libmagic. Most of the time libmagic givesup and answers application/octet-stream, which is the inconclusive answer the test is
about — the upload then falls back to the declared type and succeeds.
Sometimes it does not. Measured over 3,000 payloads on this image, 16 came back as
image/x-tga: about one run in two hundred. A recognised type is not inconclusive, so the
declared application/pdf is not used, image/x-tga is not on the allow-list, and the upload
is refused — the run fails with 400 where it wanted 200, on whichever pull request
happened to be open when it landed.
This is the same shape as the faker
randomNumber()fixture already recorded here: avalue drawn at random that is almost always in range, and occasionally is not. The payload
is fixed now, and confirmed inconclusive.
Found because it failed CI on an unrelated change, and reproduced by measuring rather than
by re-running until it happened again.