Suggestion: conservative_normalize!

[jarthod]

After removing my patch for #459 now that it's released under 2.8.1 (thanks 🎉), I noticed I also wrote this little additional method in order to only normalize the URL if needed and otherwise keep it as-is (to avoid changing some reserved chars which can be either encoded or not):

module Addressable
  class URI
    # only normalize if we see obivously unallowed chars, to avoid changing
    # user preference (encoded or not) for some reserved chars
    def conservative_normalize!
      self.path = normalized_path if path&.match?(/[^#{Addressable::URI::CharacterClasses::PATH + "%"}]/)
      self.query = normalized_query if query&.match?(/[^#{Addressable::URI::CharacterClasses::QUERY + "%"}]/)
      self.host = normalized_host if host&.match?(/[^#{Addressable::URI::CharacterClasses::HOST}]/)
      self.userinfo = normalized_userinfo if userinfo&.match?(/[^#{Addressable::URI::CharacterClasses::UNRESERVED + Addressable::URI::CharacterClasses::SUB_DELIMS + ":"}]/)
      self
    end
  end
end

I just though I would share it in case you (or somebody else) are interested in it.

Here is an example URL taken from my service and redacted (note the presence of / and %2F in the param):

http://domain.net/?param=fil:for(webp)/http%3A%2F%2Fwin.net%2Fpics.jpg

The current normalize! method would change the %2F back into /:

http://domain.net/?param=fil:for(webp)/http://win.net/pics.jpg

Both are valid URL, yes, but it's not what the person who wrote this webservice intendend and as they are doing some parsing before unencoding, it actually breaks their server (they probably expected to have a slash unencoded to split and then the rest encoded).

So I had to write this conservative_normalize! to avoid messing with the existing encoding as long as it's legal, and only normalize if necessary.

[sporkmonger]

Interesting concept. I've generally recommended an approach where normalization is done component-by-component when people have had similar needs, and it's partly why escaping methods already take character classes as parameters. That said, I could see an argument for why this is a common-enough use-case to warrant something like this.

[sporkmonger]

#472 is probably related and might make this issue redundant once fixed?

[jarthod]

Ah yes indeed, I did not had the courage to check spec to verify if it was acceptable to normalize this way so I choose the less resistance path by considering the normalize method as correct and wrote my own less invasive version. But if it's actually acceptable to modify the normalize method to avoid doing this it might make my method useless.

[dentarg]

Close this one in favour of #366? Looks like the normalization should be fixed, counted at least 5 different issues reported about it (besides this one)

[jarthod]

It's possible that once #366 is fixed this won't be needed any more indeed, I would have to run some tests on my end to see if I detect other problematic cases. In any case this method is monkey-patched on my end so yes the issue can be closed, it was just posted a suggestion and/or for others.

[dentarg]

I'll keep it open until there is a PR for #366 as a reminder about you double checking the work :)

[jarthod]

Yep ok 👍