Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
korbin opened this issue Aug 13, 2024 · 2 comments · Fixed by #172
Closed

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

korbin opened this issue Aug 13, 2024 · 2 comments · Fixed by #172
Milestone

Comments

@korbin
Copy link

korbin commented Aug 13, 2024

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
Copy link
Member

Ah yes, that comment is incorrect.

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

I'll fix the documentation.

@korbin
Copy link
Author

korbin commented Aug 13, 2024

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!

@korbin korbin closed this as completed Aug 13, 2024
@ioquatix ioquatix added this to the v0.70.0 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants