-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix integer underflow in try_range_response for empty files
#3566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Range request DoS via integer underflow in try_range_response - axum.pdf
Outdated
Show resolved
Hide resolved
430699d to
d5b43a3
Compare
|
Just to add a note from spec:
So, with a file of 0 bytes, the first-pos of 0 is not less than the length, and thus is now rejected. |
|
From CI: |
d5b43a3 to
4d568ab
Compare
|
Fixed thanks. |
f86bcea to
24c5b86
Compare
darth-raijin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question, feel free to ignore it was meant as food for thought
| let total_size = metadata.len(); | ||
|
|
||
| if total_size == 0 { | ||
| return Ok((StatusCode::RANGE_NOT_SATISFIABLE, "Range Not Satisfiable").into_response()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
I noticed that the error body "Range Not Satisfiable" is hard-coded directly into the response. Would it make sense to extract this into a shared constant so tests can assert on the exact same value?
Having the literal inline makes tests fragile, since they need to duplicate the exact string. If the message ever changes, that could also unintentionally break consumers relying on the current wording or cause test drift.
In particular, there are two branches in this PR that both return StatusCode::RANGE_NOT_SATISFIABLE. It might be useful if the test also asserted on the error body, to ensure the StatusCode::RANGE_NOT_SATISFIABLE comes from the branch in file_stream.rs with the expected response body, and not the test branch.
Thanks to @mufeedvh for reporting this bug.