URLs can't contain utf-8 characters #16108

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@karlentwistle
  • URI::Parser:: unescape is monkey patched in active support to force the encoding to utf-8 if it isn't by default. Lets set unescape to use ASCII-8BIT

Possible fix for #16104

Karl Entwistle URLs can't contain utf-8 characters
* URI::Parser::unescpae is monkey patched in active support to force the encoding to utf-8 if it isn't by default. Lets set unescape to use ASCII-8BIT
be3ccb1
@seuros
Ruby on Rails member
@pixeltrix pixeltrix commented on the diff Jul 9, 2014
actionpack/test/journey/router/utils_test.rb
@@ -27,6 +28,10 @@ def test_normalize_path_not_greedy
def test_normalize_path_uppercase
assert_equal "/foo%AAbar%AAbaz", Utils.normalize_path("/foo%aabar%aabaz")
end
+
+ def test_us_ascii_to_utf_8_ruby_1_9_3
+ assert_equal "/person/Šašinková", Utils.normalize_path("/person/Šašinková")
@pixeltrix
pixeltrix Jul 9, 2014

This isn't really testing the problem - it needs to be checking that unescaping a path which contains non-ASCII characters as percent-encoded values doesn't raise a error.

@pixeltrix pixeltrix commented on the diff Jul 9, 2014
actionpack/lib/action_dispatch/journey/router/utils.rb
@@ -53,7 +53,7 @@ def escape_segment(segment)
end
def unescape_uri(uri)
- uri.gsub(ESCAPED) { [$&[1, 2].hex].pack('C') }.force_encoding(uri.encoding)
+ uri.gsub(ESCAPED) { [$&[1, 2].hex].pack('C') }.force_encoding(ENCODING)
@pixeltrix
pixeltrix Jul 9, 2014

This is force encoding it to ASCII - we want to force encode it to UTF-8 like the monkey patch in Active Support

@pixeltrix
Ruby on Rails member

@karlentwistle you need to add an entry to CHANGELOG.md (newest is at the top) and also the commit message needs to describe the problem better. Also the commit message needs to say 'Fixes #16104' at the end so that it will automatically close the issue when we merge the PR.

@seuros
Ruby on Rails member

@karlentwistle are you going to do it ? or i can do it instead ?

@karlentwistle

@seuros Have closed this PR because it was on my 'master' branch. New PR here #16123

@seuros
Ruby on Rails member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment