Skip to content

Commit

Permalink
Refactor csp_nonce
Browse files Browse the repository at this point in the history
  • Loading branch information
spatel033 committed Dec 13, 2024
1 parent 7d36385 commit 1480ec3
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 7 deletions.
11 changes: 10 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import pathlib
import secrets
from collections.abc import Callable
from time import monotonic

Expand Down Expand Up @@ -219,6 +220,7 @@ def init_app(application):
application.after_request(useful_headers_after_request)

# Load user first (as we want user_id to be available for all calls to API, which service+organisation might make.
application.before_request(make_nonce_before_request)
application.before_request(load_user_id_before_request)
application.before_request(load_service_before_request)
application.before_request(load_organisation_before_request)
Expand Down Expand Up @@ -336,6 +338,12 @@ def load_user_id_before_request():
g.user_id = get_user_id_from_flask_login_session()


def make_nonce_before_request():
# `govuk_frontend_jinja/template.html` can be extended and inline `<script>` can be added without CSP complaining
if not getattr(request, "csp_nonce", None):
request.csp_nonce = secrets.token_urlsafe(16)


# https://www.owasp.org/index.php/List_of_useful_HTTP_headers
def useful_headers_after_request(response):
response.headers.add("X-Content-Type-Options", "nosniff")
Expand All @@ -344,7 +352,7 @@ def useful_headers_after_request(response):
"Content-Security-Policy",
(
"default-src 'self' {asset_domain} 'unsafe-inline';"
"script-src 'self' {asset_domain} 'unsafe-inline' 'unsafe-eval' data:;"
"script-src 'self' {asset_domain} 'nonce-{csp_nonce}';"
"connect-src 'self';"
"object-src 'self';"
"font-src 'self' {asset_domain} data:;"
Expand All @@ -355,6 +363,7 @@ def useful_headers_after_request(response):
"frame-src 'self';".format(
asset_domain=current_app.config["ASSET_DOMAIN"],
logo_domain=current_app.config["LOGO_CDN_DOMAIN"],
csp_nonce=getattr(request, "csp_nonce", ""),
)
),
)
Expand Down
4 changes: 3 additions & 1 deletion app/templates/admin_template.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{% extends "govuk_frontend_jinja/template.html"%}

{% set cspNonce = request.csp_nonce %}

{% block headIcons %}
<link rel="icon" sizes="48x48" href="{{ asset_url('images/favicon.ico') }}">
<link rel="icon" sizes="any" href="{{ asset_url('images/favicon.svg') }}" type="image/svg+xml">
Expand Down Expand Up @@ -134,5 +136,5 @@
{% endblock %}

<script type="module" src="{{ asset_url('javascripts/all-esm.mjs') }}"></script>
<script type="text/javascript" src="{{ asset_url('javascripts/all.js') }}"></script>
<script nonce="{{ request.csp_nonce }}" type="text/javascript" src="{{ asset_url('javascripts/all.js') }}"></script>
{% endblock %}
2 changes: 1 addition & 1 deletion app/templates/components/webauthn-api-check.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{% macro webauthn_api_check() %}
<script>
<script nonce="{{ request.csp_nonce }}">
if ('credentials' in window.navigator) {
document.body.className = ((document.body.className) ? document.body.className + ' webauthn-api-enabled' : 'webauthn-api-enabled');
}
Expand Down
2 changes: 2 additions & 0 deletions app/templates/unbranded_template.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{% extends "govuk_frontend_jinja/template.html"%}

{% set cspNonce = request.csp_nonce %}

{% block headIcons %}
{% endblock %}

Expand Down
2 changes: 1 addition & 1 deletion app/templates/views/email-link-interstitial.html
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

</div>

<script type="text/javascript">
<script nonce="{{ request.csp_nonce }}" type="text/javascript">
document.getElementById("use-email-auth").submit();
</script>

Expand Down
11 changes: 9 additions & 2 deletions tests/app/main/views/test_headers.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
def test_owasp_useful_headers_set(
client_request,
mocker,
mock_get_service_and_organisation_counts,
mock_get_letter_rates,
mock_get_sms_rate,
fake_nonce,
):
mocker.patch("secrets.token_urlsafe", return_value=fake_nonce)

client_request.logout()
response = client_request.get_response(".index")

assert response.headers["X-Content-Type-Options"] == "nosniff"
assert response.headers["X-XSS-Protection"] == "1; mode=block"
assert response.headers["Content-Security-Policy"] == (
"default-src 'self' static.example.com 'unsafe-inline';"
"script-src 'self' static.example.com 'unsafe-inline' 'unsafe-eval' data:;"
"script-src 'self' static.example.com 'nonce-TESTs5Vr8v3jgRYLoQuVwA';"
"connect-src 'self';"
"object-src 'self';"
"font-src 'self' static.example.com data:;"
Expand Down Expand Up @@ -43,7 +47,10 @@ def test_headers_non_ascii_characters_are_replaced(
mock_get_service_and_organisation_counts,
mock_get_letter_rates,
mock_get_sms_rate,
fake_nonce,
):
mocker.patch("secrets.token_urlsafe", return_value=fake_nonce)

client_request.logout()
mocker.patch.dict(
"app.current_app.config",
Expand All @@ -54,7 +61,7 @@ def test_headers_non_ascii_characters_are_replaced(

assert response.headers["Content-Security-Policy"] == (
"default-src 'self' static.example.com 'unsafe-inline';"
"script-src 'self' static.example.com 'unsafe-inline' 'unsafe-eval' data:;"
"script-src 'self' static.example.com 'nonce-TESTs5Vr8v3jgRYLoQuVwA';"
"connect-src 'self';"
"object-src 'self';"
"font-src 'self' static.example.com data:;"
Expand Down
21 changes: 20 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -2899,7 +2899,7 @@ def os_environ():


@pytest.fixture # noqa (C901 too complex)
def client_request(request, _logged_in_client, mocker, service_one): # noqa (C901 too complex)
def client_request(request, _logged_in_client, mocker, service_one, fake_nonce): # noqa (C901 too complex)
def block_method(object, method_name, preferred_method_name):
def blocked_method(*args, **kwargs):
raise AttributeError(
Expand Down Expand Up @@ -2962,6 +2962,8 @@ def get_url(
):
from flask.templating import _render

mocker.patch("secrets.token_urlsafe", return_value=fake_nonce)

with mock.patch("flask.templating._render", wraps=_render) as mock_render:
resp = _logged_in_client.get(
url,
Expand Down Expand Up @@ -3007,6 +3009,8 @@ def get_url(
if _test_for_non_smart_quotes:
ClientRequest.test_for_non_smart_quotes(page)

ClientRequest.test_for_script_csp_nonce(page)

return page

@staticmethod
Expand Down Expand Up @@ -3149,6 +3153,16 @@ def test_for_non_smart_quotes(page):
"'" in el.text or '"' in el.text
), f"Non-smart quote or apostrophe found in <{el.name}>: {normalize_spaces(el.text)}"

@staticmethod
def test_for_script_csp_nonce(page):
for script_tag in page.select("script"):
src = script_tag.get("src")
nonce = script_tag.get("nonce")
if src and "javascripts/all-esm.mjs" in src:
assert nonce is None
else:
assert nonce == fake_nonce

return ClientRequest


Expand Down Expand Up @@ -4374,3 +4388,8 @@ def mock_get_notifications_count_for_service(mocker):
"app.notification_api_client.get_notifications_count_for_service",
return_value=100,
)


@pytest.fixture(scope="function")
def fake_nonce():
return "TESTs5Vr8v3jgRYLoQuVwA"

0 comments on commit 1480ec3

Please sign in to comment.