[Django] #36206: Issues in the Existing SecurityMiddleware Code 1. Incorrect use of response.setdefault() instead of response.headers.setdefault() 2. In the process_request() method, HTTPS redirection is done While this works, %-formatting is less readable and slightly less performant than modern alternatives like f-strings 3. Preventing Overwriting of Existing Headers

[Django]

#36206: Issues in the Existing SecurityMiddleware Code 1. Incorrect use of
response.setdefault() instead of response.headers.setdefault() 2. In the
process_request() method, HTTPS redirection is done While this works,
%-formatting is less readable and slightly less performant than modern
alternatives like f-strings 3. Preventing Overwriting of Existing Headers
--------------------------------+-----------------------------------------
Reporter: Abhijeet Kumar | Type: Bug
Status: new | Component: Uncategorized
Version: 5.1 | Severity: Normal
Keywords: security.py | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
1. Incorrect use of response.setdefault() instead of
response.headers.setdefault()
Issue:
In the original code, the Cross-Origin-Opener-Policy (COOP) header is set
using:

response.setdefault("Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy)

This is incorrect because:

a. response.setdefault() does not exist in Django’s HttpResponse class.
b. Headers should be set using response.headers.setdefault() to ensure
they are only added if they don’t already exist.

Suggested Modification:
Replace:

response.setdefault("Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy)

With:

response.headers.setdefault("Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy)

2. Improving String Formatting for Readability & Performance

Issue:
In the process_request() method, HTTPS redirection is done using:

return HttpResponsePermanentRedirect(
"https://%s%s" % (host, request.get_full_path())
)

While this works, %-formatting is less readable and slightly less
performant than modern alternatives like f-strings.

Suggested Modification:
Change:

return HttpResponsePermanentRedirect(
"https://%s%s" % (host, request.get_full_path())
)

To:
return
HttpResponsePermanentRedirect(f"https://{host}{request.get_full_path()}")


3. Preventing Overwriting of Existing Headers

Issue:

The original code unconditionally sets security headers like:

response.headers["Strict-Transport-Security"] = sts_header
response.headers["X-Content-Type-Options"] = "nosniff"


This could Override existing security policies set by other middleware or
custom responses &
Prevent flexibility in modifying security headers dynamically.

Suggested Modification:

Use setdefault() instead of direct assignment:

response.headers.setdefault("Strict-Transport-Security", sts_header)
response.headers.setdefault("X-Content-Type-Options", "nosniff")


Suggested Code:

import re

from django.conf import settings
from django.http import HttpResponsePermanentRedirect
from django.utils.deprecation import MiddlewareMixin


class SecurityMiddleware(MiddlewareMixin):
def __init__(self, get_response):
super().__init__(get_response)
self.sts_seconds = settings.SECURE_HSTS_SECONDS
self.sts_include_subdomains =
settings.SECURE_HSTS_INCLUDE_SUBDOMAINS
self.sts_preload = settings.SECURE_HSTS_PRELOAD
self.content_type_nosniff = settings.SECURE_CONTENT_TYPE_NOSNIFF
self.redirect = settings.SECURE_SSL_REDIRECT
self.redirect_host = settings.SECURE_SSL_HOST
self.redirect_exempt = [re.compile(r) for r in
settings.SECURE_REDIRECT_EXEMPT]
self.referrer_policy = settings.SECURE_REFERRER_POLICY
self.cross_origin_opener_policy =
settings.SECURE_CROSS_ORIGIN_OPENER_POLICY

def process_request(self, request):
path = request.path.lstrip("/")
if (
self.redirect
and not request.is_secure()
and not any(pattern.search(path) for pattern in
self.redirect_exempt)
):
host = self.redirect_host or request.get_host()
return
HttpResponsePermanentRedirect(f"https://{host}{request.get_full_path()}")

def process_response(self, request, response):
if (
self.sts_seconds
and request.is_secure()
and "Strict-Transport-Security" not in response.headers
):
sts_header = f"max-age={self.sts_seconds}"
if self.sts_include_subdomains:
sts_header += "; includeSubDomains"
if self.sts_preload:
sts_header += "; preload"
response.headers.setdefault("Strict-Transport-Security",
sts_header)

if self.content_type_nosniff:
response.headers.setdefault("X-Content-Type-Options",
"nosniff")

if self.referrer_policy:
# Support a comma-separated string or iterable of values to
allow fallback.
response.headers.setdefault(
"Referrer-Policy",
",".join(
[v.strip() for v in self.referrer_policy.split(",")]
if isinstance(self.referrer_policy, str)
else self.referrer_policy
),
)

if self.cross_origin_opener_policy:
response.headers.setdefault(
"Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy,
)

return response
--
Ticket URL: <https://code.djangoproject.com/ticket/36206>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36206: Issues in the Existing SecurityMiddleware Code 1. Incorrect use of
response.setdefault() instead of response.headers.setdefault() 2. In the
process_request() method, HTTPS redirection is done While this works,
%-formatting is less readable and slightly less performant than modern
alternatives like f-strings 3. Preventing Overwriting of Existing Headers
--------------------------------+--------------------------------------

Reporter: Abhijeet Kumar | Owner: (none)

Type: Bug | Status: new
Component: Uncategorized | Version: 5.1

Severity: Normal | Resolution:

Keywords: security.py | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Comment (by Jake Howard):

The formatting for the description is a little broken - could you fix it?

You're also describing 3 separate issues in one ticket, which makes
reviewing difficult. Could you separate them?

They also seem to be in the realm of nitpicks and small changes (eg
converting an f-string), which doesn't have a noticable impact on, well
anything.
--
Ticket URL: <https://code.djangoproject.com/ticket/36206#comment:1>

[Django]

#36206: Issues in the Existing SecurityMiddleware Code 1. Incorrect use of
response.setdefault() instead of response.headers.setdefault() 2. In the
process_request() method, HTTPS redirection is done While this works,
%-formatting is less readable and slightly less performant than modern
alternatives like f-strings 3. Preventing Overwriting of Existing Headers

-------------------------------------+-------------------------------------

Reporter: Abhijeet Kumar | Owner: (none)

Type: | Status: closed
Cleanup/optimization |
Component: HTTP handling | Version: 5.1
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:

| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Tim Graham):

* component: Uncategorized => HTTP handling
* keywords: security.py =>
* resolution: => invalid
* status: new => closed
* type: Bug => Cleanup/optimization


Old description:

> 1. Incorrect use of response.setdefault() instead of
> response.headers.setdefault()

> Issue:
> In the original code, the Cross-Origin-Opener-Policy (COOP) header is set
> using:
>
> response.setdefault("Cross-Origin-Opener-Policy",
> self.cross_origin_opener_policy)
>
> This is incorrect because:
>
> a. response.setdefault() does not exist in Django’s HttpResponse class.
> b. Headers should be set using response.headers.setdefault() to ensure
> they are only added if they don’t already exist.
>
> Suggested Modification:
> Replace:
>
> response.setdefault("Cross-Origin-Opener-Policy",
> self.cross_origin_opener_policy)
>
> With:
>
> response.headers.setdefault("Cross-Origin-Opener-Policy",
> self.cross_origin_opener_policy)
>
> 2. Improving String Formatting for Readability & Performance
>
> Issue:
> In the process_request() method, HTTPS redirection is done using:
>
> return HttpResponsePermanentRedirect(
> "https://%s%s" % (host, request.get_full_path())
> )
>

> While this works, %-formatting is less readable and slightly less

> performant than modern alternatives like f-strings.
>
> Suggested Modification:
> Change:
>
> return HttpResponsePermanentRedirect(
> "https://%s%s" % (host, request.get_full_path())
> )
>
> To:
> return
> HttpResponsePermanentRedirect(f"https://{host}{request.get_full_path()}")
>

> 3. Preventing Overwriting of Existing Headers
>

New description:

1. Incorrect use of `response.setdefault()` instead of

`response.headers.setdefault()`

Issue:
In the original code, the Cross-Origin-Opener-Policy (COOP) header is set
using:

{{{#!python

response.setdefault("Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy)
}}}
This is incorrect because:

a. `response.setdefault()` does not exist in Django’s `HttpResponse`
class.
b. Headers should be set using `response.headers.setdefault()` to ensure
they are only added if they don’t already exist.

Suggested Modification:
Replace:

{{{#!python

response.setdefault("Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy)
}}}
With:

{{{#!python

response.headers.setdefault("Cross-Origin-Opener-Policy",
self.cross_origin_opener_policy)
}}}

2. Improving String Formatting for Readability & Performance

Issue:

In the process_request() method, HTTPS redirection is done using:
{{{#!python

return HttpResponsePermanentRedirect(
"https://%s%s" % (host, request.get_full_path())
)
}}}

While this works, %-formatting is less readable and slightly less

performant than modern alternatives like f-strings.

Suggested Modification:
Change:
{{{#!python

return HttpResponsePermanentRedirect(
"https://%s%s" % (host, request.get_full_path())
)
}}}
To:

{{{#!python

return
HttpResponsePermanentRedirect(f"https://{host}{request.get_full_path()}")
}}}

3. Preventing Overwriting of Existing Headers

Issue:

The original code unconditionally sets security headers like:

{{{#!python

response.headers["Strict-Transport-Security"] = sts_header
response.headers["X-Content-Type-Options"] = "nosniff"
}}}

is could Override existing security policies set by other middleware or
custom responses &
Prevent flexibility in modifying security headers dynamically.

Suggested Modification:

Use `setdefault()` instead of direct assignment:

{{{#!python

response.headers.setdefault("Strict-Transport-Security", sts_header)
response.headers.setdefault("X-Content-Type-Options", "nosniff")
}}}

Suggested Code:

{{{#!python

Comment:

1. `HttpResponse.set_default()`
[https://github.com/django/django/blob/87c5de3b7f2316aa17353d74f54e6ff19013d049/django/http/response.py#L278-L280
exists].

2. Per [https://docs.djangoproject.com/en/dev/internals/contributing
/writing-code/coding-style/#python-style Python style guide], we won't
accept PRs that merely change formatting style: "Don’t waste time doing
unrelated refactoring of existing code to adjust the formatting method."

3. It could be useful, but at this point it would be backward-
incompatible, so I think we should leave it alone unless we have a very
compelling use case. Changing anything security related can't be done
lightly.

Thanks for the ideas. As Jake said, in the future, please don't combine
separate issues in one ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/36206#comment:2>