Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#24834 closed Bug (fixed)

Optional port in Host request header breaks get_current_site()

Reported by: Nick Pope Owned by: Nick Pope
Component: contrib.sites Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When calling django.contrib.sites.shortcuts.get_current_site() and relying on HttpRequest.get_host() to determine the current site, the query can fail if the Host header contains a port. This bug has been present since 1.5 where RequestSite was introduced and also turns up in 1.8 with support for using the request in SiteManager.get_current().

Change History (14)

comment:1 by Nick Pope, 9 years ago

Has patch: set
Owner: changed from nobody to Nick Pope
Status: newassigned

comment:2 by Nick Pope, 9 years ago

comment:3 by Claude Paroz, 9 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I think we should account for the scenario where someone runs several sites under different ports (example.org:8081, example.org:8082, etc.) and also includes the port in Site.domain.
Possible approaches:

  • try first with domain:port and fallback to the version without the port
  • strip the port only if it is the default port (80/443)

comment:4 by Loic Bistuer, 9 years ago

@claudep my thought exactly.

comment:5 by Nick Pope, 9 years ago

Fair enough. I can see that sites hosted on different ports might be desirable to some, although it would be a minor use case. For more background, my situation was a customer explicitly specifying a standard port (80/443) for a web service request and an HTTP 500 error was thrown out from the CurrentSiteMiddleware due to Site.DoesNotExist even though the domain part of the host header would have correctly matched a record in the database. Note that I do not configure SITE_ID as I want the site record to be chosen automatically based on the request and SITE_ID takes precedence.

Making these changes would mean that Site.domain is a bit of a misnomer, but I don't think we would be able to change that to Site.host without backwards incompatibility issues. C'est la vie.

Some questions:

  • Would it make sense to always strip if the port is 80/443 or would you definitely prefer to only do that as a last resort?
  • Would it make sense to always strip any port so that you could have example.com where the host is example.com:8080 and the only port that can be connected to is 8080?
  • I expect that we would need to make the documentation much clearer regarding what values can be stored in Site.domain?

I feel that we want to ensure that people do not needlessly create multiple records, e.g. example.com, example.com:80 and example.com:443. This may lead to confusion where other models have a foreign key to Site and may lead to "missing" content.

(Edit: I've just realised that the current implementation would have always supported domain:port so as my current version of the PR would break backwards compatibility, although I'm not sure anyone necessarily expected domain:port to be work.)

Last edited 9 years ago by Nick Pope (previous) (diff)

comment:6 by Claude Paroz, 9 years ago

I'd suggest going that route:

  • try first the "full" domain like now
  • catch DoesNotExist and try with the port stripped if any

+1 to improve documentation.

comment:7 by Markus Holtermann, 9 years ago

Since it's not a regression in 1.8, I think it should be included in 1.9.

comment:8 by Nick Pope, 9 years ago

Hi,

Sorry for the delay in getting back to this. I have already finished the code changes and am just finishing up the documentation now after which I will issue a new pull request.

I will be putting this forward for 1.9 but would like it to be considered for a backport to 1.8 as, while not a regression, the current behaviour is unexpected. It results in a situation where, even though the ALLOWED_HOSTS settings is restricted to a specific domain and that domain is present in a Site record, a user can submit a Host header with that domain combined with a port and trigger Site.DoesNotExist, particularly when using CurrentSiteMiddleware.

comment:9 by Nick Pope, 9 years ago

Updated PR

comment:10 by Tim Graham, 9 years ago

Patch needs improvement: unset

comment:11 by Markus Holtermann, 9 years ago

Patch needs improvement: set

comment:12 by Markus Holtermann, 9 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In b3d5dc69:

Fixed #24834 -- Fixed get_current_site() when Host header contains port.

When the Host header contains a port, looking up the Site record fails
as the host will never match the domain.

comment:14 by Tim Graham <timograham@…>, 9 years ago

In 0d97e73d:

Fixed reverse sites_tests failures introduced in refs #24834.

Note: See TracTickets for help on using tickets.
Back to Top