[Django] #28359: SecurityMiddleware's SECURE_SSL_HOST only affects unsecure requests
Django
-----------------------------------------------+------------------------
Reporter: Matthias Kestenholz | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------------+------------------------
For search engine optimization purposes it is preferable to have a single
domain serving all requests. We have a custom middleware in most projects
which handles this but dropped the middleware for sites using SSL:
{{{
from django.conf import settings
from django.http import HttpResponsePermanentRedirect
def force_domain(get_response):
if settings.DEBUG or not settings.FORCE_DOMAIN:
return get_response
def middleware(request):
if request.method != 'GET':
return get_response(request)
if request.get_host() != settings.FORCE_DOMAIN:
target = 'http%s://%s%s' % (
request.is_secure() and 's' or '',
settings.FORCE_DOMAIN,
request.get_full_path())
return HttpResponsePermanentRedirect(target)
return get_response(request)
return middleware
}}}
It seems to me that setting `SECURE_SSL_REDIRECT` and `SECURE_SSL_HOST`
should also handle the case where 1. a request already uses a secure
connection but 2. the host does not equal `SECURE_SSL_HOST`.
(Categorized as new feature because that's more fun.)
--
Ticket URL: <https://code.djangoproject.com/ticket/28359>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Description changed by Matthias Kestenholz:
Old description:
> For search engine optimization purposes it is preferable to have a single
> domain serving all requests. We have a custom middleware in most projects
> which handles this but dropped the middleware for sites using SSL:
>
> {{{
> from django.conf import settings
> from django.http import HttpResponsePermanentRedirect
>
> def force_domain(get_response):
> if settings.DEBUG or not settings.FORCE_DOMAIN:
> return get_response
>
> def middleware(request):
> if request.method != 'GET':
> return get_response(request)
>
> if request.get_host() != settings.FORCE_DOMAIN:
> target = 'http%s://%s%s' % (
> request.is_secure() and 's' or '',
> settings.FORCE_DOMAIN,
> request.get_full_path())
> return HttpResponsePermanentRedirect(target)
>
> return get_response(request)
>
> return middleware
> }}}
>
> It seems to me that setting `SECURE_SSL_REDIRECT` and `SECURE_SSL_HOST`
> should also handle the case where 1. a request already uses a secure
> connection but 2. the host does not equal `SECURE_SSL_HOST`.
>
> (Categorized as new feature because that's more fun.)
New description:
For search engine optimization purposes it is preferable to have a single
domain serving all requests. We have a custom middleware in most projects
which handles this but dropped the middleware for sites using SSL:
{{{
from django.conf import settings
from django.http import HttpResponsePermanentRedirect
def force_domain(get_response):
if settings.DEBUG or not settings.FORCE_DOMAIN:
return get_response
def middleware(request):
if request.method != 'GET':
return get_response(request)
if request.get_host() != settings.FORCE_DOMAIN:
target = 'http%s://%s%s' % (
request.is_secure() and 's' or '',
settings.FORCE_DOMAIN,
request.get_full_path())
return HttpResponsePermanentRedirect(target)
return get_response(request)
return middleware
}}}
We noticed today that setting `SECURE_SSL_HOST = 'example.com'` does not
redirect requests to `https://www.example.com`
It seems to me that setting `SECURE_SSL_REDIRECT` and `SECURE_SSL_HOST`
should also handle the case where 1. a request already uses a secure
connection but 2. the host does not equal `SECURE_SSL_HOST`.
(Categorized as new feature because that's more fun.)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:1>
Django
Reporter: Matthias Kestenholz | Owner: nobody
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Matthias Kestenholz):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/8698 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:2>
Django
Reporter: Matthias Kestenholz | Owner: Irindu
| Indeera
Type: New feature | Status: assigned
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Irindu Indeera ):
* status: new => assigned
* owner: nobody => Irindu Indeera
Comment:
HI I'd like to work on this issue, but I am a newbie to open source, I am
currently a computer engineering undergraduate, I am confident that I'll
be able to contribute but I need some mentoring, Mr Matthias Kestenholz
can you please help me
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:3>
Django
Reporter: Matthias Kestenholz | Owner: Irindu
| Indeera
Type: New feature | Status: assigned
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Matthias Kestenholz):
You could help review the proposed feature itself and also the pull
request: https://github.com/django/django/pull/8698
A good idea would also be to follow the ticket triaging steps as
documented here:
https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#unreviewed
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:4>
Django
Reporter: Matthias Kestenholz | Owner: Irindu
| Indeera
Type: New feature | Status: assigned
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Old description:
> For search engine optimization purposes it is preferable to have a single
> domain serving all requests. We have a custom middleware in most projects
> which handles this but dropped the middleware for sites using SSL:
>
> {{{
> from django.conf import settings
> from django.http import HttpResponsePermanentRedirect
>
> def force_domain(get_response):
> if settings.DEBUG or not settings.FORCE_DOMAIN:
> return get_response
>
> def middleware(request):
> if request.method != 'GET':
> return get_response(request)
>
> if request.get_host() != settings.FORCE_DOMAIN:
> target = 'http%s://%s%s' % (
> request.is_secure() and 's' or '',
> settings.FORCE_DOMAIN,
> request.get_full_path())
> return HttpResponsePermanentRedirect(target)
>
> return get_response(request)
>
> return middleware
> }}}
>
> We noticed today that setting `SECURE_SSL_HOST = 'example.com'` does not
> redirect requests to `https://www.example.com`
>
> It seems to me that setting `SECURE_SSL_REDIRECT` and `SECURE_SSL_HOST`
> should also handle the case where 1. a request already uses a secure
> connection but 2. the host does not equal `SECURE_SSL_HOST`.
>
> (Categorized as new feature because that's more fun.)
New description:
For search engine optimization purposes it is preferable to have a single
domain serving all requests. We have a custom middleware in most projects
which handles this but dropped the middleware for sites using SSL:
{{{
from django.conf import settings
from django.http import HttpResponsePermanentRedirect
def force_domain(get_response):
if settings.DEBUG or not settings.FORCE_DOMAIN:
return get_response
def middleware(request):
if request.method != 'GET':
return get_response(request)
if request.get_host() != settings.FORCE_DOMAIN:
target = 'http%s://%s%s' % (
request.is_secure() and 's' or '',
settings.FORCE_DOMAIN,
request.get_full_path())
return HttpResponsePermanentRedirect(target)
return get_response(request)
return middleware
}}}
We noticed today that setting `SECURE_SSL_HOST = 'example.com'` does not
redirect requests to `https://www.example.com`
It seems to me that setting `SECURE_SSL_REDIRECT` and `SECURE_SSL_HOST`
should also handle the case where 1. a request already uses a secure
connection but 2. the host does not equal `SECURE_SSL_HOST`.
--
Comment (by Tim Graham):
I'm not sure if using `SecurityMiddelware` to do canonical URL redirection
(not a security-related thing, really) is a proper separation of concerns.
Also, doesn't the patch have the possibility to be backwards incompatible?
Currently with `SECURE_SSL_HOST` set, I think it's possible to directly
access different secure subdomains. I believe this proposal prohibits that
as all secure requests will be redirected to `SECURE_SSL_HOST`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:5>
Django
Reporter: Matthias Kestenholz | Owner: Irindu
| Indeera
Type: New feature | Status: assigned
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Matthias Kestenholz):
Yes, the patch would probably be backwards incompatible for the use case
you describe. I also agree that this does not have much to do with
security, but maybe neither has the `SECURE_SSL_HOST` functionality at
all.
I'd think that, in a scenario where there are multiple valid SSL hosts,
you'd rather want to keep the domain (which means that `SECURE_SSL_HOST`
would not be set), OR that you'd want a `next` parameter for a post login
redirect (which means that the standard `SecurityMiddleware` would have to
be replaced or augmented anyway)
(Sorry for not replying earlier, I still am on vacation.)
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:6>
Django
Reporter: Matthias Kestenholz | Owner: (none)
Type: New feature | Status: new
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Irindu Indeera ):
* owner: Irindu Indeera => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:7>
Django
Reporter: Matthias Kestenholz | Owner: (none)
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* cc: Carl Meyer (added)
* easy: 1 => 0
Comment:
Carl, any input about the intended use case of `SECURE_SSL_HOST`? I didn't
find any details in the [https://github.com/carljm/django-
secure/commit/51d0ce06928eff99a92a630ff0255e59ba77c86d django-secure
commit].
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:8>
Django
Reporter: Matthias Kestenholz | Owner: (none)
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Carl Meyer):
The intention of the setting was to provide some control over the
redirect-to-SSL functionality provided by SecurityMiddleware, and it does
that. I don't think SecurityMiddleware should be in the business of
enforcing a canonical host for all SSL requests. It's easy enough to write
your own middleware for that (and it can even use the value of
SECURE_SSL_HOST if you want). The current behavior is pretty clearly
documented; changing it would not be a bugfix but a change in the intended
behavior.
I agree that if we were implementing this from scratch, a reasonable case
could be made for the alternate behavior, but I don't think it's a strong
enough case for a backwards-incompatible change.
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:9>
Django
Reporter: Matthias Kestenholz | Owner: (none)
Component: HTTP handling | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Thanks, Tim and Carl.
I'm closing this ticket as wontfix. Maybe canonical URL redirection would
be a better fit for `CommonMiddleware` (since we have `PREPEND_WWW` there)
but it's easier and certainly less controversial to just implement this
outside Django core.
I'll add another Django package to my long list of maintained packages
then, and post the link here for posteriority :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:10>
Django
Reporter: Matthias Kestenholz | Owner: (none)
Type: New feature | Status: closed
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Matthias Kestenholz):
The project is here, already deployed into production but still missing
some documentation & real world testing:
https://github.com/matthiask/django-canonical-domain
--
Ticket URL: <https://code.djangoproject.com/ticket/28359#comment:11>