Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fix ArgumentError being raised in case of invalid byte sequences #5337

Merged
merged 1 commit into from

4 participants

@mreinsch

this resolves an issue when a request with an invalid byte sequence comes in. In that case, the framework should not throw an ArgumentError as it does right now.

@josevalim
Owner

/cc @wycats

@tenderlove
Owner

Don't we want an exception if there is invalid characters in the URL? Maybe not an ArgumentError, but some sort of exception.

@mreinsch

@tenderlove well, I want to be able to handle this, and it might not even be a fatal failure. Currently I can't handle it, as it doesn't even get to my controller because some non-related code raises an exception.

The fix I propose resolves a newly (since 3.2.2) introduced ArgumentError when looking up static resources. I don't think this rack module should raise an exception no matter what parameters it gets.

For instance, we are using slugs for SEO purposes. For many of those, the characters after the ID are non-essential for looking up the article or whatever. The issue with encoded characters: if a byte gets cut off (happens way to easy), the user will get a 500 error. If that happens with a slug only consisting of ASCII characters, it isn't an issue, we can just redirect you to the correct URL.

@mreinsch

@tenderlove so yes, we want to have a exception, which currently will happen when you process the param. I just don't want any unrelated Rack modules to cut me off and not allow me to process this at all.

@tenderlove
Owner

@mreinsch makes sense. Do you know the commit that introduced the exception?

@pixeltrix
Owner

@mreinsch this is just in development right? Or are you using config.serve_static_assets in production?

@mreinsch

sorry, don't know. This failed in our test cases, so it never went on production.

@pixeltrix
Owner

Default behaviour is to rely on Apache, nginx, etc. to serve files in production so chances are that it wouldn't have affected your app for real.

@tenderlove this doesn't feel like the right fix to me - shouldn't invalid urls be caught in a single place? In fact shouldn't Rack be catching them? If we are fixing it in this module then it should be in the unescape_path method as the URI.parser.unescape is the cause.

@tenderlove
Owner

@pixeltrix I think this fix is fine, for now. The problem with rack catching errors like this is as @mreinsch says, urls can get messed up easily, especially if we have multibyte characters involved. Handling errors like this seems application specific. Some people may want to reject, other people may not.

@tenderlove tenderlove merged commit 8f5f92c into rails:master
@pixeltrix
Owner

So this fix comes back to bite us in #13518 - if your app name contains UTF-8 characters it blows up trying to serve a static file with UTF-8 in its name since it's forced to be ASCII-8BIT encoded. :cry:

@pixeltrix pixeltrix referenced this pull request from a commit
@pixeltrix pixeltrix Fix Encoding::CompatibilityError when public path is UTF-8
In #5337 we forced the path encoding to ASCII-8BIT to prevent static
file handling from blowing up before an application has had chance to
deal with possibly invalid urls. However this has a negative side
effect of making it an incompatible encoding if the application's
public path has UTF-8 characters in it.

To work around the problem we check to see if the path has a valid
encoding once it has been unescaped. If it is not valid then we can
return early since it will not match any file anyway.

Fixes #13518
436ed51
@pixeltrix pixeltrix referenced this pull request from a commit
@pixeltrix pixeltrix Fix Encoding::CompatibilityError when public path is UTF-8
In #5337 we forced the path encoding to ASCII-8BIT to prevent static
file handling from blowing up before an application has had chance to
deal with possibly invalid urls. However this has a negative side
effect of making it an incompatible encoding if the application's
public path has UTF-8 characters in it.

To work around the problem we check to see if the path has a valid
encoding once it has been unescaped. If it is not valid then we can
return early since it will not match any file anyway.

Fixes #13518

(cherry picked from commit 436ed51)
1e37945
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  actionpack/lib/action_dispatch/middleware/static.rb
@@ -39,6 +39,7 @@ def unescape_path(path)
end
def escape_glob_chars(path)
+ path.force_encoding('binary') if path.respond_to? :force_encoding
path.gsub(/[*?{}\[\]]/, "\\\\\\&")
end
end
View
4 actionpack/test/dispatch/static_test.rb
@@ -7,6 +7,10 @@ def test_serves_dynamic_content
assert_equal "Hello, World!", get("/nofile").body
end
+ def test_handles_urls_with_bad_encoding
+ assert_equal "Hello, World!", get("/doorkeeper%E3E4").body
+ end
+
def test_sets_cache_control
response = get("/index.html")
assert_html "/index.html", response
Something went wrong with that request. Please try again.