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

Do not allow forwarding of authorization headers on redirect. #89

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
30 changes: 22 additions & 8 deletions lib/down/net_http.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def download(url, *args, **options)
headers = options.delete(:headers)
uri_normalizer = options.delete(:uri_normalizer)
extension = options.delete(:extension)
auth_on_redirect = options.delete(:auth_on_redirect)

# Use open-uri's :content_lenth_proc or :progress_proc to raise an
# exception early if the file is too large.
Expand Down Expand Up @@ -91,7 +92,7 @@ def download(url, *args, **options)
uri.password = nil
end

open_uri_file = open_uri(uri, open_uri_options, follows_remaining: max_redirects)
open_uri_file = open_uri(uri, open_uri_options, follows_remaining: max_redirects, auth_on_redirect: )

# Handle the fact that open-uri returns StringIOs for small files.
extname = extension ? ".#{extension}" : File.extname(open_uri_file.base_uri.path)
Expand All @@ -107,14 +108,15 @@ def download(url, *args, **options)
def open(url, *args, **options)
options = merge_options(@options, *args, **options)

max_redirects = options.delete(:max_redirects)
uri_normalizer = options.delete(:uri_normalizer)
max_redirects = options.delete(:max_redirects)
uri_normalizer = options.delete(:uri_normalizer)
auth_on_redirect = options.delete(:auth_on_redirect)

uri = ensure_uri(normalize_uri(url, uri_normalizer: uri_normalizer))

# Create a Fiber that halts when response headers are received.
request = Fiber.new do
net_http_request(uri, options, follows_remaining: max_redirects) do |response|
net_http_request(uri, options, follows_remaining: max_redirects, auth_on_redirect:) do |response|
Fiber.yield response
end
end
Expand All @@ -141,20 +143,25 @@ def open(url, *args, **options)
private

# Calls open-uri's URI::HTTP#open method. Additionally handles redirects.
def open_uri(uri, options, follows_remaining:)
def open_uri(uri, options, follows_remaining:, auth_on_redirect:)
uri.open(options)
rescue OpenURI::HTTPRedirect => exception
raise Down::TooManyRedirects, "too many redirects" if follows_remaining == 0

# fail if redirect URI scheme is not http or https
begin
same_host = uri.host.eql? exception.uri.host
uri = ensure_uri(exception.uri)
rescue Down::InvalidUrl
response = rebuild_response_from_open_uri_exception(exception)

raise ResponseError.new("Invalid Redirect URI: #{exception.uri}", response: response)
end

# do not leak credentials on redirect
options.delete("Authorization") unless auth_on_redirect
options.delete(:http_basic_authentication) unless auth_on_redirect
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question in a different place - should there be some logic to keep the creds if it's a relative redirect?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's not a good way to preserve on a relative redirect - i can check to see if the host is the same, but that doesn't help from a test perspective. for now, i'm keping the logic as-is.


# forward cookies on the redirect
if !exception.io.meta["set-cookie"].to_s.empty?
options["Cookie"] ||= ''
Expand Down Expand Up @@ -200,7 +207,7 @@ def ensure_tempfile(io, extension)
end

# Makes a Net::HTTP request and follows redirects.
def net_http_request(uri, options, follows_remaining:, &block)
def net_http_request(uri, options, follows_remaining:, auth_on_redirect:, &block)
http, request = create_net_http(uri, options)

begin
Expand Down Expand Up @@ -231,10 +238,17 @@ def net_http_request(uri, options, follows_remaining:, &block)
raise ResponseError.new("Invalid Redirect URI: #{response["Location"]}", response: response)
end

# do not leak credentials on redirect
options[:headers].delete("Authorization") unless auth_on_redirect

# handle relative redirects
location = uri + location if location.relative?
if location.relative?
location = uri + location
uri.user = nil unless auth_on_redirect
uri.password = nil unless auth_on_redirect
end

net_http_request(location, options, follows_remaining: follows_remaining - 1, &block)
net_http_request(location, options, follows_remaining: follows_remaining - 1, auth_on_redirect:, &block)
end
end

Expand Down
35 changes: 35 additions & 0 deletions test/net_http_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,21 @@
# "Set-Cookie" header.
end

it "removes Authorization header on redirects" do
tempfile = Down::NetHttp.download("#{$httpbin}/redirect/1", headers: {"Authorization" => "Basic dXNlcjpwYXNzd29yZA=="})
assert_nil JSON.parse(tempfile.read)["headers"]["Authorization"]
end

it "removes Basic Auth credentials header on redirects" do
tempfile = Down::NetHttp.download("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", )
assert_nil JSON.parse(tempfile.read)["headers"]["Authorization"]
end

it "preserves Authorization header on redirect, when asked" do
tempfile = Down::NetHttp.download("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", auth_on_redirect:true )
assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(tempfile.read)["headers"]["Authorization"]
end

# I don't know how to test that the proxy is actually used
it "accepts proxy" do
tempfile = Down::NetHttp.download("#{$httpbin}/bytes/100", proxy: $httpbin)
Expand Down Expand Up @@ -324,6 +339,26 @@
assert_equal "#{$httpbin}/get", JSON.parse(io.read)["url"]
end

it "removes Authorization header on redirects" do
io = Down::NetHttp.open("#{$httpbin}/redirect/1", headers: {"Authorization" => "Basic dXNlcjpwYXNzd29yZA=="})
assert_nil JSON.parse(io.read)["headers"]["Authorization"]
end

it "removes Basic Auth credentials header on absolute redirects" do
io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/absolute-redirect/1", )
assert_nil JSON.parse(io.read)["headers"]["Authorization"]
end

it "preserves Basic Auth credentials header on relative redirects" do
io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/relative-redirect/1", )
assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(io.read)["headers"]["Authorization"]
end

it "preserves Authorization header on redirect, when asked" do
io = Down::NetHttp.open("#{$httpbin.sub("http://", '\0user:password@')}/redirect/1", auth_on_redirect:true )
assert_equal "Basic dXNlcjpwYXNzd29yZA==", JSON.parse(io.read)["headers"]["Authorization"]
end

it "returns content in encoding specified by charset" do
io = Down::NetHttp.open("#{$httpbin}/stream/10")
assert_equal Encoding::BINARY, io.read.encoding
Expand Down