Opened 8 years ago

Closed 8 years ago

#15797 closed Bug (invalid)

*_COOKIE_DOMAIN settings should reject values that won't work in modern browsers

Reported by: Steven Cummings Owned by: nobody
Component: Core (Other) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


When either of the cookie domain (CSRF_COOKIE_DOMAIN, SESSION_COOKIE_DOMAIN) settings are given values that browsers won't send to the server, they should be rejected to keep the programmer from thinking that such a value is okay.

So, one day I discovered that using ".localhost" as a *_COOKIE_DOMAIN setting doesn't work well as browsers avoid it. Specifically any domain with less than two dots. So ".com", etc. would be avoided too.

After searching I discovered ticket #10560, wherein the reporter thought it a bug with Django. It was closed as wontfix because, of course, it's not. However, I thought that the settings should at least reject such values so save programmers the time spent debugging this gotcha. So, I created a patch to do just that and am opening this ticket to see if others agree.

Some notes about the patch:

  • Instead of attempting to capture the full range cookie requirements in the check, I decided to keep it simply ensuring that there are 2+ dots. Of course other mistakes could be made with the cookie domain, but I believe they would be more obvious.
  • I decided to raise ImproperConfig instead of just warning. This could be a point of debate.

Attachments (1)

cookie_domain_validation.diff (5.0 KB) - added by Steven Cummings 8 years ago.
Patch to settings class, tests, and documentation

Download all attachments as: .zip

Change History (6)

comment:1 Changed 8 years ago by anonymous

comment:2 Changed 8 years ago by Steven Cummings

I need to fix this patch (probably do it tonight). I failed to consider the case where you really only want to support a naked domain, e.g., This cookie domain would be valid and simply wouldn't match any sub-domains at all. So the new condition will be: starts with a dot and has 2+, or doesn't and has at least one.

comment:3 Changed 8 years ago by Steven Cummings

Patch needs improvement: set

Changed 8 years ago by Steven Cummings

Patch to settings class, tests, and documentation

comment:4 Changed 8 years ago by Steven Cummings

Patch needs improvement: unset

Okay, updated the patch per comment 2.

comment:5 Changed 8 years ago by Luke Plant

Resolution: invalid
Status: newclosed

I'm going to close as invalid, essentially for the same reasons as Malcolm on the other bug i.e. this is entirely to do with the way that browsers work. Yes, it could be useful if Django warned you about this, but that's not a good enough reason - there are many other mistakes that Django could potentially warn you about, but it's not Django's job to do so.

In addition, there is the fact that we are trying to replicate browser behaviour that is very complicated - much more complicated than counting the number of dots. The validation you suggest is conservative - I don't think it rejects useful values, but it doesn't reject some values that browsers will reject i.e. false negatives, which means that as soon as we add it, we have added more bugs, whereas at present there are no bugs.

All I think we could do here is add a simple note in the SESSION_COOKIE_DOMAIN/CSRF_COOKIE_DOMAIN documentation, pointing out that browsers have rules that mean they will ignore some values of the domain for security reasons e.g. ".com" and ".localhost".

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