Skip to content

Extract redirect location handling and clarify behaviour. #172

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

Merged
merged 1 commit into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 36 additions & 11 deletions lib/async/http/relative_location.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ module HTTP
class TooManyRedirects < StandardError
end

# A client wrapper which transparently handles both relative and absolute redirects to a given maximum number of hops.
# A client wrapper which transparently handles redirects to a given maximum number of hops.
#
# The default implementation will only follow relative locations (i.e. those without a scheme) and will switch to GET if the original request was not a GET.
#
# The best reference for these semantics is defined by the [Fetch specification](https://fetch.spec.whatwg.org/#http-redirect-fetch).
#
Expand Down Expand Up @@ -58,14 +60,38 @@ def redirect_with_get?(request, response)
end
end

# Handle a redirect to a relative location.
#
# @parameter request [Protocol::HTTP::Request] The original request, which you can modify if you want to handle the redirect.
# @parameter location [String] The relative location to redirect to.
# @returns [Boolean] True if the redirect was handled, false if it was not.
def handle_redirect(request, location)
uri = URI.parse(location)

if uri.absolute?
return false
end

# Update the path of the request:
request.path = Reference[request.path] + location

# Follow the redirect:
return true
end

def call(request)
# We don't want to follow redirects for HEAD requests:
return super if request.head?

if body = request.body
# We need to cache the body as it might be submitted multiple times if we get a response status of 307 or 308:
body = ::Protocol::HTTP::Body::Rewindable.new(body)
request.body = body
if body.respond_to?(:rewind)
# The request body was already rewindable, so use it as is:
body = request.body
else
# The request body was not rewindable, and we might need to resubmit it if we get a response status of 307 or 308, so make it rewindable:
body = ::Protocol::HTTP::Body::Rewindable.new(body)
request.body = body
end
end

hops = 0
Expand All @@ -83,23 +109,22 @@ def call(request)

response.finish

uri = URI.parse(location)

if uri.absolute?
unless handle_redirect(request, location)
return response
else
request.path = Reference[request.path] + location
end

# Ensure the request (body) is finished and set to nil before we manipulate the request:
request.finish

if request.method == GET or response.preserve_method?
# We (might) need to rewind the body so that it can be submitted again:
body&.rewind
request.body = body
else
# We are changing the method to GET:
request.method = GET

# Clear the request body:
request.finish
# We will no longer be submitting the body:
body = nil

# Remove any headers which are not allowed in a GET request:
Expand Down
22 changes: 19 additions & 3 deletions test/async/http/relative_location.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@
end

it 'should redirect POST to GET' do
response = relative_location.post('/')
body = Protocol::HTTP::Body::Buffered.wrap(["Hello, World!"])
expect(body).to receive(:finish)

response = relative_location.post('/', {}, body)

expect(response).to be(:success?)
expect(response.read).to be == "GET"
Expand All @@ -44,9 +47,22 @@
end

it 'should fail with maximum redirects' do
expect{
expect do
response = relative_location.get('/home')
}.to raise_exception(Async::HTTP::TooManyRedirects, message: be =~ /maximum/)
end.to raise_exception(Async::HTTP::TooManyRedirects, message: be =~ /maximum/)
end
end

with "handle_redirect returning false" do
before do
expect(relative_location).to receive(:handle_redirect).and_return(false)
end

it "should not follow the redirect" do
response = relative_location.get('/')
response.finish

expect(response).to be(:redirection?)
end
end
end
Expand Down
Loading