Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #230: Make REPORT_PERCENTAGE a float #242

Merged
merged 2 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions csp/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,4 @@ class CspConfig(AppConfig):
name = "csp"

def ready(self) -> None:
# Ignore known issue typeddjango/django-stubs #2232
# The overload of CheckRegistry.register as a function is incomplete
checks.register(check_django_csp_lt_4_0, checks.Tags.security) # type: ignore
checks.register(check_django_csp_lt_4_0, checks.Tags.security)
2 changes: 1 addition & 1 deletion csp/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def migrate_settings() -> tuple[dict[str, Any], bool]:

_REPORT_PERCENTAGE = getattr(settings, "CSP_REPORT_PERCENTAGE", None)
if _REPORT_PERCENTAGE is not None:
config["REPORT_PERCENTAGE"] = round(_REPORT_PERCENTAGE * 100)
config["REPORT_PERCENTAGE"] = _REPORT_PERCENTAGE * 100

include_nonce_in = getattr(settings, "CSP_INCLUDE_NONCE_IN", [])

Expand Down
3 changes: 2 additions & 1 deletion csp/contrib/rate_limiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ def get_policy_parts(self, request: HttpRequest, response: HttpResponseBase, rep
if policy is None:
return policy_parts

remove_report = random.randint(0, 99) >= policy.get("REPORT_PERCENTAGE", 100)
# `random.random` returns a value in the range [0.0, 1.0) so all values will be < 100.0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# `random.random` returns a value in the range [0.0, 1.0) so all values will be < 100.0.
# `random.random` returns a value in the range [0.0, 1.0] so all values will be < 100.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was actually intentional to denote that 1.0 isn't included in the range that random.random() can return.

remove_report = random.random() * 100 >= policy.get("REPORT_PERCENTAGE", 100)
if remove_report:
if policy_parts.replace is None:
policy_parts.replace = {
Expand Down
2 changes: 1 addition & 1 deletion csp/tests/test_checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
def test_migrate_settings() -> None:
config, report_only = migrate_settings()
assert config == {
"REPORT_PERCENTAGE": 25,
"REPORT_PERCENTAGE": 25.0,
"EXCLUDE_URL_PREFIXES": ["/admin/"],
"DIRECTIVES": {"default-src": ["'self'", "example.com"]},
}
Expand Down
15 changes: 15 additions & 0 deletions csp/tests/test_contrib.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,21 @@ def test_report_percentage() -> None:
assert 400 <= times_seen <= 600


@override_settings(CONTENT_SECURITY_POLICY={"REPORT_PERCENTAGE": 9.9, "DIRECTIVES": {"report-uri": "x"}})
def test_report_percentage_float() -> None:
times_seen = 0
for _ in range(5000):
request = rf.get("/")
response = HttpResponse()
mw.process_response(request, response)
if "report-uri" in response[HEADER]:
times_seen += 1
if "report-to" in response[HEADER]:
times_seen += 1
# Roughly 10%
assert 400 <= times_seen <= 600


@override_settings(CONTENT_SECURITY_POLICY={"REPORT_PERCENTAGE": 100, "DIRECTIVES": {"report-uri": "x"}})
def test_report_percentage_100() -> None:
times_seen = 0
Expand Down
7 changes: 3 additions & 4 deletions docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,9 @@ policy.
on the same origin.

``REPORT_PERCENTAGE``
Percentage of requests that should see the ``report-uri`` directive.
Use this to throttle the number of CSP violation reports made to your
``report-uri``. An **integer** between 0 and 100 (0 = no reports at all).
Ignored if ``report-uri`` isn't set.
Percentage of requests that should see the ``report-uri`` directive. Use this to throttle the
number of CSP violation reports made to your ``report-uri``. A **float** between 0.0 and 100.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A float between 0.0 and 100.0

Won't a float of 100.0 give us a REPORT_PERCENTAGE of 10000.0? Would be good to warn about invalid values maybe?

        config["REPORT_PERCENTAGE"] = _REPORT_PERCENTAGE * 100

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That * 100 is only when converting from the old settings config when using CSP_REPORT_PERCENTAGE which was a float between 0 and 1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worthwhile to add a Django check in checks.py to validate some of the settings? Like this value being 0 <= x <= 100 and possibly some others that makes sense and emit a warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice, to help avoid surprises, but can be a follow-on thing or a ticket for someone in the community to pick up?

(0.0 = no reports at all, 100.0 = always report). Ignored if ``report-uri`` isn't set.

``DIRECTIVES``
A dictionary of policy directives. Each key in the dictionary is a directive and the value is a
Expand Down
8 changes: 4 additions & 4 deletions docs/migration-guide.rst
Original file line number Diff line number Diff line change
Expand Up @@ -130,14 +130,14 @@ The new settings would be:

.. note::

If you were using the ``CSP_REPORT_PERCENTAGE`` setting, this should be updated to be an integer
percentage and not a decimal value in the new settings format. For example, if you had
``CSP_REPORT_PERCENTAGE = 0.1``, this should be updated to:
If you were using the ``CSP_REPORT_PERCENTAGE`` setting, this should be updated to be a float
percentage between 0.0 and 100.0. For example, if you had ``CSP_REPORT_PERCENTAGE = 0.1``, this
should be updated to ``10.0`` to represent 10% of CSP errors will be reported:

.. code-block:: python

CONTENT_SECURITY_POLICY = {
"REPORT_PERCENTAGE": 10,
"REPORT_PERCENTAGE": 10.0,
"DIRECTIVES": {
"report-uri": "/csp-report/",
# ...
Expand Down
4 changes: 2 additions & 2 deletions docs/reports.rst
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ and set the ``REPORT_PERCENTAGE`` option:

``REPORT_PERCENTAGE``
Percentage of requests that should see the ``report-uri`` directive. Use this to throttle the
number of CSP violation reports made to your ``report-uri``. An **integer** between 0 and 100 (0
= no reports at all). Ignored if ``report-uri`` isn't set.
number of CSP violation reports made to your ``report-uri``. A **float** between 0.0 and 100.0
(0.0 = no reports at all, 100.0 = always report). Ignored if ``report-uri`` isn't set.

.. _report: http://www.w3.org/TR/CSP/#sample-violation-report