celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

reel crashes with "the scheme does not accept registry part" #169

Open katjaeinsfeld opened 9 years ago

katjaeinsfeld commented 9 years ago

Setup:

Crash: Reel crashes with URI::InvalidURIError: the scheme does not accept registry part: up_and_running when I access request.path.

Fix: /lib/reel/mixins.rb, line 47, replace def uri with the following:

def uri
    url_path = url
    url_path = url_path[1..-1] if url_path.index('//') == 0
    @uri ||= URI(url_path)
end

I am not sure if this is the right place to fix that.

Asmod4n commented 9 years ago

Should this throw a 400 Bad Request or 500 Internal Server Error :?

digitalextremist commented 9 years ago

@katjaeinsfeld can you post your entire repro as a small repository? I would like to test whether it's // at the start, or // anywhere in the URI. I hesitate to believe a) it's a good idea to test every URI for // at start and b) it's just // at start.

I'd rather handle this by rescuing the method, testing for the value and rechecking if needed, otherwise returning a certain HTTP status/error, rather than a certain exception... and probably not correcting the URI.

But like @Asmod4n said, then what? What does the HTTP spec say?

katjaeinsfeld commented 9 years ago

If I understand https://tools.ietf.org/html/rfc3986#section-3.3 correctly, // is not allowed at the start. (If a URI does not contain an authority component, then the path cannot begin with two slash characters ("//").)

That seems to be consistent with the behavior of URI:

URI('/path_to//me') is ok. URI('//path') is ok (assuming 'path' is the authority component).

URI('//path_to') fails with

URI::InvalidURIError: the scheme  does not accept registry part: path_to (or bad hostname?)

as 'path_to' is not valid as authority component.

I would prefer reel to handle that with 400 Bad Request.

digitalextremist commented 9 years ago

400 seems appropriate to me, but I believe you stumbled on a general issue for URI parsing. So I continue to believe there ought not be a test for every request; it'd become this:

def uri
  @uri ||= URI(url_path)
rescue URI::InvalidURIError
  # Return 400 HTTP Status Code
end
katjaeinsfeld commented 9 years ago

ok