Skip to content

Fix an indexing panic in compression #578

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

Merged
merged 1 commit into from
Jun 3, 2025

Conversation

sulami
Copy link
Contributor

@sulami sulami commented Jun 3, 2025

This guard was backwards, not only breaking the functionality it was intended to provide, but also causing panics under certain conditions.

In this case, the haystack is the pre-existing Vary header on the response, and the needle is "Accept-Econding".

If we are compressing a response, and there is already a Vary header on the response, it is <= 15 bytes and does not match Accept-Encoding, this would cause a range index error as the while statement would not provide an effective guard.

I'm happy to contribute a test or two as well, if so desired.

This guard was backwards, not only breaking the functionality it was
intended to provide, but also causing panics under certain conditions.

In this case, the haystack is the pre-existing Vary header on the
response, and the needle is "Accept-Econding".

If we are compressing a response, and there is already a Vary header on
the response, it is <= 15 bytes and does not match Accept-Encoding, this
would cause a range index error as the while statement would not provide
an effective guard.
@sulami sulami force-pushed the fix-compression-panic branch from df86aed to 43d704f Compare June 3, 2025 01:01
Copy link
Collaborator

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

@seanmonstar seanmonstar merged commit 359d997 into tower-rs:main Jun 3, 2025
11 checks passed
@jplatte
Copy link
Member

jplatte commented Jun 3, 2025

@seanmonstar Could you yank the latest version and release a new one? (for some reason the crates.io web interface doesn't offer me the Yank button)

@GlenDC
Copy link
Contributor

GlenDC commented Jun 3, 2025

You might want to add some tests first? and perhaps have it as a policy to at least have some tests. Even better if you can reuse this from a utility module or w/e. As that would have prevented the original mistake to begin with.

@jplatte
Copy link
Member

jplatte commented Jul 9, 2025

I'm swamped with staying on top of things already, so I don't have any spare time to write tests. A PR with tests would be very welcome though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants