Opened 12 years ago
Closed 11 years ago
#19778 closed Bug (wontfix)
csrf middleware report BAD_REFERER when HTTP_HOST contains port
Reported by: | Owned by: | ||
---|---|---|---|
Component: | CSRF | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
For a given request with the HTTP_HOST header set to 'www.example.com:443' and the HTTP_REFERER set to 'https://www.example.com/somepage' the same_origin check will report REASON_BAD_REFERER due to fact that HOST has port and REFERER does not. Perhaps, as in my case, a less competent firewall has tampered with the request on its way to the server.
Here's a failing test (that I added to tests/regressiontests/csrf_tests/tests.py):
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)
I know the port is a valid part of the same_origin check but I'm thinking that if it's missing from REFERER, the scheme is https and HTTP_HOST uses the default port, maybe we can skip the port from the same_origin check.
What do you think?
Incidentally, if I set a port number on the HTTP_REFERER the test still fails:
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:443' req2 = CsrfViewMiddleware().process_view(req, post_form_view, (), {}) self.assertEqual(None, req2)
kind regards /jens
Change History (6)
comment:1 by , 12 years ago
Version: | 1.4 → master |
---|
comment:2 by , 12 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 12 years ago
Keywords: | sprint2013 added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:4 by , 12 years ago
Keywords: | sprint2013 removed |
---|---|
Owner: | removed |
Status: | assigned → new |
comment:5 by , 11 years ago
Type: | Cleanup/optimization → Bug |
---|
From the HTTP/1.1 RFC 2616:
A "host" without any trailing port information implies the default port for the service requested (e.g., "80" for an HTTP URL). For example, a request on the origin server for <http://www.w3.org/pub/WWW/> would properly include:
The tests you've added also fail in the current master, and this should be categorized as a bug.
charettes, your patch is quite out-dated and cannot be applied on master. Also there's some styling issues with the code (correct use of spaces in dicts). If you could fix those issues and squash the commits into a single commit, this patch could be moved forwards.
comment:6 by , 11 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
From the submitter (comment on GitHub)
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!
PR at https://github.com/django/django/pull/705