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

Jump to bottom

#19778 -- make utils/http.same_origin check if ports are set before comparing #705

Closed
wants to merge 6 commits into from
Closed

#19778 -- make utils/http.same_origin check if ports are set before comparing #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d3c1eda
Adds csrf (failing) tests that has a port number in the Host Req Header
jtinfors Feb 6, 2013
9d0e0a2
Adds fix for same_origin check where \'good_refer\' has port set to N…
jtinfors Feb 7, 2013
1d43c47
Adds csrf (failing) tests that has a port number in the Host Req Header
jtinfors Feb 6, 2013
ee1b511
Adds fix for same_origin check where \'good_refer\' has port set to N…
jtinfors Feb 7, 2013
cdf9de0
Merge branch 'master' of github.com:jtinfors/django
jtinfors Feb 7, 2013
21bea68
Adds a test for csrf with the port part of HTTP_HOST eader set to 80
jtinfors Feb 9, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion django/utils/http.py
View file
Open in desktop
Expand Up @@ -226,7 +226,9 @@ def same_origin(url1, url2):
Checks if two URLs are 'same-origin'
"""
p1, p2 = urllib_parse.urlparse(url1), urllib_parse.urlparse(url2)
return (p1.scheme, p1.hostname, p1.port) == (p2.scheme, p2.hostname, p2.port)
p1_port = p1.port or { 'http': 80, 'https': 443 }[p1.scheme]
p2_port = p2.port or { 'http': 80, 'https': 443 }[p2.scheme]
Copy link
Contributor

Choose a reason for hiding this comment

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

See the coding guidelines for correct space usage. Also, duplicating the port dict isn't DRY, so define it in a single spot. For example:

default_ports = {'http': 80, 'https': 443}
p1_port = p1.port or default_ports.get(p1.scheme, None)
p2_port = p2.port or default_ports.get(p2.scheme, None)

return (p1.scheme, p1.hostname, p1_port) == (p2.scheme, p2.hostname, p2_port)

def is_safe_url(url, host=None):
"""
Expand Down
16 changes: 16 additions & 0 deletions tests/regressiontests/csrf_tests/tests.py
View file
Open in desktop
Expand Up @@ -305,6 +305,22 @@ def test_https_good_referer_2(self):
req2 = CsrfViewMiddleware().process_view(req, post_form_view, (), {})
self.assertEqual(None, req2)

def test_https_good_referer_with_port_443(self):
req = self._get_POST_request_with_token()
req._is_secure_override = True
req.META['HTTP_HOST'] = 'www.example.com:443'
req.META['HTTP_REFERER'] = 'https://www.example.com/somepage'
req2 = CsrfViewMiddleware().process_view(req, post_form_view, (), {})
self.assertEqual(None, req2)

Copy link
Member

Choose a reason for hiding this comment

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

Could you also test with :80?

Copy link
Author

Choose a reason for hiding this comment

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

Sure Thing!
Naturally it fails due to the fact the the scheme of the host is hardcoded to 'https' (django/middleware/csrf.py:147):
good_referer = 'https://%s/' % request.get_host()

But I'm beginning to think my suggested patch might be a bad idea.. Reason being that the port of HTTP_HOST of the request in my particular case is not set by the browser but by a less competent gateway, a man-in-the-middle if you will.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this patch is still valid, to be honest. Having a reverse proxy or load balancer is a valid use case and it is not unusual or against the HTTP spec for those to include a port number in the HOST header.

def test_https_good_referer_with_port_80(self):
req = self._get_POST_request_with_token()
req._is_secure_override = True
req.META['HTTP_HOST'] = 'www.example.com:80'
req.META['HTTP_REFERER'] = 'https://www.example.com/somepage'
req2 = CsrfViewMiddleware().process_view(req, post_form_view, (), {})
self.assertEqual(None, req2)

def test_ensures_csrf_cookie_no_middleware(self):
"""
Tests that ensures_csrf_cookie decorator fulfils its promise
Expand Down