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

Merged
merged 1 commit into from Mar 15, 2012

Conversation

Projects
None yet
4 participants
Contributor

mreinsch commented Mar 8, 2012

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.

Member

josevalim commented Mar 8, 2012

/cc @wycats

Owner

tenderlove commented Mar 12, 2012

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

Contributor

mreinsch commented Mar 13, 2012

@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.

Contributor

mreinsch commented Mar 13, 2012

@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.

Owner

tenderlove commented Mar 13, 2012

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

Contributor

mreinsch commented Mar 14, 2012

sure: 86d3bc3

Owner

pixeltrix commented Mar 15, 2012

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

Contributor

mreinsch commented Mar 15, 2012

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

Owner

pixeltrix commented Mar 15, 2012

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.

Owner

tenderlove commented Mar 15, 2012

@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 added a commit that referenced this pull request Mar 15, 2012

@tenderlove tenderlove Merge pull request #5337 from mreinsch/static_invalid_byte_sequence
fix ArgumentError being raised in case of invalid byte sequences
8f5f92c

@tenderlove tenderlove merged commit 8f5f92c into rails:master Mar 15, 2012

Owner

pixeltrix commented Dec 29, 2013

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. 😢

@pixeltrix pixeltrix added a commit that referenced this pull request Dec 29, 2013

@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 added a commit that referenced this pull request Dec 29, 2013

@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