Skip to content
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

Do more conservative URL normalization #758

Merged
merged 5 commits into from
Oct 6, 2023
Merged

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Aug 3, 2023

http.rb normalises URLs before making a request. This normalisation may reformat a perfectly valid URL. Even though the normalised URL in theory is equivalent to the original URL, some servers may not recognise this (see e.g. mastodon/mastodon#26219).

Some of these problems are due to a bug in Addressable (see sporkmonger/addressable#366). But even with that bug fixes, there is no reason to normalise the URL before fetching it, apart from fixing obviously invalid aspects such as spaces in the path. There is no benefit, only potential problems.

I suggest we take a more conservative approach and only escape characters in path and query outside the printable ASCII range, and never decode any existing percent-escape sequences. This is the approach taken by cURL, so that seems like a safe bet.

If somebody wants more “aggressive” normalisation for better caching in HTTP proxies or whatever, and they are aware of the risks connected with this, they can provide a custom normaliser or just call Addressable::URI#normalize before invoking htp.rb. But I don't think such potentially risky behaviour should be the default.

Fixes #654

@c960657 c960657 changed the title Conservative normalization Do more conservative URL normalization Aug 3, 2023
spec/lib/http/uri_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@ixti ixti left a comment

Choose a reason for hiding this comment

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

Thank you! This is great. Can you please move NORMALIZER proc spec in a separate file though, to follow RSpec naming conventions? And, please, add @private documentation tag to self.percent_encode

lib/http/uri.rb Outdated Show resolved Hide resolved
@c960657 c960657 requested a review from ixti August 28, 2023 17:00
@c960657
Copy link
Contributor Author

c960657 commented Oct 4, 2023

Done :-)

@tarcieri tarcieri merged commit 8b802bf into httprb:main Oct 6, 2023
8 checks passed
@c960657 c960657 deleted the uri-normalizer branch December 28, 2023 17:42
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.

Encoded plus (+) %2B in path segment somehow not working
3 participants