socketry / async-http

MIT License
320 stars 46 forks source link

`Async::HTTP::RelativeLocation` doesn't seem to work for absolute URLs #171

Closed korbin closed 3 months ago

korbin commented 3 months ago

In RelativeLocation, we immediately return if a URI is absolute:

while hops <= @maximum_hops
    response = super(request)

    if response.redirection?
        hops += 1

        # Get the redirect location:
        unless location = response.headers['location']
            return response
        end

        response.finish

        uri = URI.parse(location)

        if uri.absolute?
            return response
        else
...

Perhaps I am using it wrong, but it seems like this doesn't follow the absolute redirect as it should?

If I change this block, the while loop correctly iterates and the redirect is transparently followed as I would expect:

if uri.absolute?
    endpoint = Endpoint[uri]
    request.scheme = endpoint.scheme
    request.authority = endpoint.authority
    request.path = endpoint.path
else
...

Maybe this isn't the intended behavior (please close this issue and disregard) or I am using the library incorrectly - I didn't see a test for absolute redirects, but the comments seem to indicate that this is a "Client wrapper which transparently handles both relative and absolute redirects to a given maximum number of hops."

ioquatix commented 3 months ago

Ah yes, that comment is incorrect.

It's a security issue to redirect to absolute URLs IMHO.

I'll fix the documentation.

korbin commented 3 months ago

I agree that following redirects shouldn't be implicit and could potentially present security issues.

We've found there are plenty of places where absolute redirects are necessary and safe: following redirects to S3 pre-signed URLs being an easy, obvious case.

URI::HTTP.open does seem to follow all same-scheme redirects by default.

172 should make it simple and clean to build additional behavior/filtered redirects/etc. on top of RelativeLocation though.

Thanks for the quick follow-up!