I can't seem to get the tests running here, but I think this might not be correct. It seems like force_encoding will set the strings to UTF-8, but you can encode any arbitrary bytes with URI encoding. force_encoding means that we'll end up with strings that are tagged as UTF-8 but aren't actually UTF-8 data.

This problem is actually demonstrated by the test in this PR. \x80 is not valid unicode:

irb(main):005:0> x = '/foo%80/bar%e2%80%8cbaz'
=> "/foo%80/bar%e2%80%8cbaz"
irb(main):006:0> y = URI.unescape x
=> "/foo\x80/bar‌baz"
irb(main):007:0> y.force_encoding "UTF-8"
=> "/foo\x80/bar‌baz"
irb(main):008:0> y.valid_encoding?
=> false

Hm good point. I was trying to take into account how things were done in v1.4.8:

https://github.com/sinatra/sinatra/blob/v1.4.8/lib/sinatra/base.rb#L1006

Wow! Good job tracking that down!!

I think just not calling force_encoding is the right thing to do (that's what we're doing in a monkey patch), but it looks like calling force_encoding is what Sinatra used to do, so maybe everyone has written their apps based on that behavior.

I'm not sure what is the best path forward (pun intended but also a true statement).

It turns out URI::Parser#unescape already calls force_encoding, so that was redundant anyway. You're right though, the results are tagged UTF-8 but valid_encoding? returns false.

Yes, it will call force_encoding, but it uses the encoding of the string that was passed in. When we read the path from the socket, the socket will return a string that is encoded in binary, not utf-8.

For example, a path encoded with UTF-8:

irb(main):002:0> path = "/foo%80/bar%e2%80%8cbaz"
=> "/foo%80/bar%e2%80%8cbaz"
irb(main):003:0> path.encoding
=> #<Encoding:UTF-8>
irb(main):004:0> x = URI.unescape path
=> "/foo\x80/bar‌baz"
irb(main):005:0> x.encoding
=> #<Encoding:UTF-8>
irb(main):006:0> x.valid_encoding?
=> false

Now if the path is binary:

irb(main):007:0> path = "/foo%80/bar%e2%80%8cbaz".b
=> "/foo%80/bar%e2%80%8cbaz"
irb(main):008:0> path.encoding
=> #<Encoding:ASCII-8BIT>
irb(main):009:0> x = URI.unescape path
=> "/foo\x80/bar\xE2\x80\x8Cbaz"
irb(main):010:0> x.encoding
=> #<Encoding:ASCII-8BIT>
irb(main):011:0> x.valid_encoding?
=> true

The test differs from production here because the default encoding of strings inside a file is UTF-8 (so the string literal will be tagged with UTF-8), but the strings we read from the socket will be tagged as binary.

Doing this might be more closely represent production:

get '/foo%80/bar%e2%80%8cbaz'.b

Ah, gotcha!

Here we should also do:

assert_predicate params['foo'], :valid_encoding?

I suspect it will fail.

You were right on, this assertion fails.

This fails because params are force encoded with the default encoding here:

https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L1026

Just droping the permalink version of that link here as the line it points to might change in the future: