Skip to content

Commit

Permalink
fix: clear credentials when redirecting to a different port
Browse files Browse the repository at this point in the history
Note that in this case we treat cookies differently from credentials
per RFC 6265 section 8.5:

https://datatracker.ietf.org/doc/html/rfc6265#section-8.5

> Cookies do not provide isolation by port.  If a cookie is readable
> by a service running on one port, the cookie is also readable by a
> service running on another port of the same server.  If a cookie is
> writable by a service on one port, the cookie is also writable by a
> service running on another port of the same server.  For this
> reason, servers SHOULD NOT both run mutually distrusting services on
> different ports of the same host and use cookies to store security-
> sensitive information.
  • Loading branch information
flavorjones committed Jun 6, 2022
1 parent 70ebc34 commit 907c778
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 6 deletions.
13 changes: 9 additions & 4 deletions lib/mechanize/http/agent.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@

class Mechanize::HTTP::Agent

CREDENTIAL_HEADERS = ['Authorization', 'Cookie']
CREDENTIAL_HEADERS = ['Authorization']
COOKIE_HEADERS = ['Cookie']
POST_HEADERS = ['Content-Length', 'Content-MD5', 'Content-Type']

# :section: Headers
Expand Down Expand Up @@ -998,10 +999,14 @@ def response_redirect(response, method, page, redirects, headers,
end

# Make sure we clear credential headers if being redirected to another site
if new_uri.host != page.uri.host
CREDENTIAL_HEADERS.each do |ch|
headers.delete_if { |h| h.casecmp?(ch) }
if new_uri.host == page.uri.host
if new_uri.port != page.uri.port
# https://datatracker.ietf.org/doc/html/rfc6265#section-8.5
# cookies are OK to be shared across ports on the same host
CREDENTIAL_HEADERS.each { |ch| headers.delete_if { |h| h.casecmp?(ch) } }
end
else
(COOKIE_HEADERS + CREDENTIAL_HEADERS).each { |ch| headers.delete_if { |h| h.casecmp?(ch) } }
end

fetch new_uri, redirect_method, headers, [], referer, redirects + 1
Expand Down
25 changes: 23 additions & 2 deletions test/test_mechanize_http_agent.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1569,7 +1569,7 @@ def test_response_redirect_to_cross_site_with_credential
refute_includes(headers.keys, "AUTHORIZATION")
refute_includes(headers.keys, "cookie")

assert_match 'range|bytes=0-9999', page.body
assert_match("range|bytes=0-9999", page.body)
refute_match("authorization|Basic xxx", page.body)
refute_match("cookie|name=value", page.body)
end
Expand All @@ -1590,11 +1590,32 @@ def test_response_redirect_to_same_site_with_credential
assert_includes(headers.keys, "AUTHORIZATION")
assert_includes(headers.keys, "cookie")

assert_match 'range|bytes=0-9999', page.body
assert_match("range|bytes=0-9999", page.body)
assert_match("authorization|Basic xxx", page.body)
assert_match("cookie|name=value", page.body)
end

def test_response_redirect_to_same_site_diff_port_with_credential
@agent.redirect_ok = true

headers = {
'Range' => 'bytes=0-9999',
'AUTHORIZATION' => 'Basic xxx',
'cookie' => 'name=value',
}

page = html_page ''
page = @agent.response_redirect({ 'Location' => 'http://example:81/http_headers' }, :get,
page, 0, headers)

refute_includes(headers.keys, "AUTHORIZATION")
assert_includes(headers.keys, "cookie")

assert_match("range|bytes=0-9999", page.body)
refute_match("authorization|Basic xxx", page.body)
assert_match("cookie|name=value", page.body)
end

def test_response_redirect_not_ok
@agent.redirect_ok = false

Expand Down

0 comments on commit 907c778

Please sign in to comment.