Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 6 commits into from

5 participants

@jtinfors

The utils/http.same_origin is used by the csrf middleware to check same origin.
But it is missing the fact that sometimes the HTTP_HOST header might have port set and when comparing the HTTP_REFERER (which most likely does not have HTTP_HOST set) that same_origin test will report a REASON_BAD_REFERER when probably it should not.

And so I think we should check if port is present before performing the same_origin check.
test and fix is provided.

@charettes
Collaborator

You should open a ticket on Trac and link it to this PR. We only use Github's PR feature for the review process.

@claudep claudep commented on the diff
tests/regressiontests/csrf_tests/tests.py
@@ -305,6 +305,14 @@ 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(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)
+
@claudep Collaborator
claudep added a note

Could you also test with :80?

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Bouke Bouke commented on the diff
django/utils/http.py
@@ -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]
@Bouke
Bouke added a note

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Bouke

Also see comment:5 in the ticket on how to move this patch forward.

@jtinfors

Well, It's been almost a year since I submitted this pull request.
I say in the comment above that you should probably disregard it.

A year old bug!? If a bug at all!? "The tests fail in master", I shall hope so, it's a year old.

Let's close this one and move on shall we!

@jtinfors jtinfors closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 6, 2013
  1. @jtinfors
Commits on Feb 7, 2013
  1. @jtinfors

    Adds fix for same_origin check where \'good_refer\' has port set to N…

    jtinfors authored
    …one due to the fact that the HTTP_REFERER usually do not have port set
Commits on Feb 9, 2013
  1. @jtinfors
  2. @jtinfors

    Adds fix for same_origin check where \'good_refer\' has port set to N…

    jtinfors authored
    …one due to the fact that the HTTP_REFERER usually do not have port set
  3. @jtinfors
  4. @jtinfors
This page is out of date. Refresh to see the latest.
Showing with 19 additions and 1 deletion.
  1. +3 −1 django/utils/http.py
  2. +16 −0 tests/regressiontests/csrf_tests/tests.py
View
4 django/utils/http.py
@@ -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]
@Bouke
Bouke added a note

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ return (p1.scheme, p1.hostname, p1_port) == (p2.scheme, p2.hostname, p2_port)
def is_safe_url(url, host=None):
"""
View
16 tests/regressiontests/csrf_tests/tests.py
@@ -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)
+
@claudep Collaborator
claudep added a note

Could you also test with :80?

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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
Something went wrong with that request. Please try again.