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

Set the form-action directive in the report-only CSP #15554

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robhudson
Copy link
Member

Summary

This PR introduces the form-action directive to the report-only Content Security Policy (CSP) header. The goal is to test and evaluate its compatibility before eventually applying it to the enforced CSP header. The form-action directive restricts where forms on the site can send their data upon submission, adding an additional layer of security to prevent potential vulnerabilities.

Why form-action is Important for Security

The form-action directive is a key security measure designed to mitigate attacks that exploit form submissions, such as:

  • Phishing and data exfiltration attacks: Prevents malicious actors from redirecting form submissions to unauthorized or external servers.
  • Clickjacking: Works in tandem with other CSP directives to ensure that embedded forms are protected.
  • Cross-site scripting (XSS) risks: Reduces the attack surface by limiting submission endpoints, even if an attacker manages to inject a form.

By specifying trusted domains or paths for form submissions, form-action ensures that forms behave only as intended and cannot be abused for unauthorized data capture.

Next Steps

  1. Deploy the change and monitor CSP violation reports for any issues related to form-action.
  2. Refine the directive based on findings from the report-only stage.
  3. Once confident, apply the directive to the enforced CSP header to fully benefit from its security enhancements.

References

Issue / Bugzilla link

#15553 (from #11943)

Testing

  • Verify the addition of the form-action directive in the report-only CSP header locally.
    • To do this we need to set a couple local env vars:
      • DEBUG=False - CSP headers aren't added while in DEBUG mode
      • CSP_RO_REPORT_URI=/csp-violation - the report-only header will only be added if this is set.

@robhudson robhudson requested a review from a team as a code owner November 22, 2024 23:26
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.96%. Comparing base (33115aa) to head (572113c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15554   +/-   ##
=======================================
  Coverage   78.95%   78.96%           
=======================================
  Files         158      158           
  Lines        8293     8294    +1     
=======================================
+ Hits         6548     6549    +1     
  Misses       1745     1745           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@robhudson robhudson changed the title Set the form-action direction in the report-only CSP Set the form-action directive in the report-only CSP Nov 22, 2024
@@ -127,6 +127,7 @@
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["object-src"] = [csp.constants.NONE]
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["frame-ancestors"] = [csp.constants.NONE]
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["style-src"].remove(csp.constants.UNSAFE_INLINE)
CONTENT_SECURITY_POLICY_REPORT_ONLY["DIRECTIVES"]["form-action"] = [csp.constants.SELF, BASKET_URL, FXA_ENDPOINT]
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: FXA_ENDPOINT has a trailing slash and BASKET_URL does not. Is that gonna cause any wrinkles or is CSP smart enough to strip trailing slashes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing that. The browser, from what I've read, doesn't really differentiate, so this should be fine. But I'm a fan of consistency so perhaps I'll add some trailing slash stripping.

Copy link
Contributor

Choose a reason for hiding this comment

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

NB: The slash has a function in CSP wrt to being a path component (i.e. an implicit wildcard after it) or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the CSP path-part-match rules more closely...

  1. With https://example.com/ in our CSP, a form submit to https://example.com/ matches.
  2. With https://example.com in our CSP (no path), a form submit to https://example.com/ matches, since when split on '/', the path is empty string.

And vice-versa for the form submit to the non-slash URL.

Focusing on the BASKET_URL, however, reading the path-part-match rules @janbrasna mentioned, we would want the trailing slash for the basket URL. Without the trailing slash it is in "exact match" mode and a form submit to an actual path would fail. In the comment above I was just looking at the FXA_ENDPOINT in our code where we don't use any path.

So the slash is important, just not necessarily if we're submitting to "" vs /.

@janbrasna - does that "match" your understanding of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup… and I was only looking at FXA_ENDPOINT and CJMS_AFFILIATE_ENDPOINT for another rule, which is either a wildcard, or a particular endpoint only.

OTOH for Basket the easy way might be setting the form action CSP rule as f"{BASKET_URL}/news/" to match the existing code? (And make it even more specific? Or are there even more endpoints that I'm not aware of?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm working on adding some more basket APIs under /api/. So I'd probably keep that one at the root level for now. Then again, the ones under /api/ are more for javascript, not form submits.

Copy link
Collaborator

@stevejalim stevejalim left a comment

Choose a reason for hiding this comment

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

r+wc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants