Opened 14 years ago
Closed 14 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 |
Description
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)
Change History (6)
comment:1 by , 14 years ago
comment:2 by , 14 years ago
comment:3 by , 14 years ago
Patch needs improvement: | set |
---|
by , 14 years ago
Attachment: | cookie_domain_validation.diff added |
---|
Patch to settings class, tests, and documentation
comment:5 by , 14 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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".
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., example.com. 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.