Skip to content

Fix #525, fix #524 - Add form-action and base-uri requirement to CSP#809

Merged
mxsasha merged 3 commits intomainfrom
fix-525
Mar 8, 2023
Merged

Fix #525, fix #524 - Add form-action and base-uri requirement to CSP#809
mxsasha merged 3 commits intomainfrom
fix-525

Conversation

@mxsasha
Copy link
Copy Markdown
Collaborator

@mxsasha mxsasha commented Nov 17, 2022

As we are making the CSP test more complicated, and it was already confusing users at times, we should make #577 a priority along with this change.

@mxsasha
Copy link
Copy Markdown
Collaborator Author

mxsasha commented Jan 9, 2023

@baknu I think this is ready as a fix for #524 and #525, here are examples of what we do and do not allow:

def test_missing_base_uri(self):
headers = "form-action 'none'; default-src 'self'; frame-ancestors 'self'"
self._is_bad(headers)
def test_self_base_uri(self):
headers = "form-action 'none'; base-uri 'self'; default-src 'self'; frame-ancestors 'self'"
self._is_good(headers)
def test_uri_base_uri(self):
headers = "form-action 'none'; base-uri www.internet.nl; default-src 'self'; frame-ancestors 'self'"
self._is_good(headers)
def test_missing_form_action(self):
headers = "base-uri 'none'; default-src 'self'; frame-ancestors 'self'"
self._is_bad(headers)
def test_self_form_action(self):
headers = "form-action 'self'; base-uri 'none'; default-src 'self'; frame-ancestors 'self'"
self._is_good(headers)
def test_uri_form_action(self):
headers = "form-action www.internet.nl; base-uri 'none'; default-src 'self'; frame-ancestors 'self'"
self._is_good(headers)

In addition to that code, 'none' is valid for both, it's in the default test policy.

If you agree this is correct, I will merge and we'll need to update content.

@baknu baknu added enhancement Issues that suggest slight improvements to existing code, tests, etc. content Change (needed) to the content repository alongside with this issue/PR labels Jan 17, 2023
@baknu baknu self-assigned this Jan 17, 2023
@mxsasha mxsasha requested a review from bwbroersma February 14, 2023 09:10
@mxsasha
Copy link
Copy Markdown
Collaborator Author

mxsasha commented Feb 14, 2023

@thestinger as reporter of #524 and #525, could you also have a look my the previous comment to see if that seems right?

@thestinger
Copy link
Copy Markdown

Yeah, that looks right.

Copy link
Copy Markdown
Collaborator

@bwbroersma bwbroersma left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍.

Values like 'unsafe-inline', 'strict-dynamic' (see MDN), 'unsafe-eval' and some other keywords are allowed for 'base-uri'. While not unsafe, maybe we should warn for insane values. But that would be a very low priority CSP improvement ticket.

@mxsasha mxsasha merged commit f5fcc99 into main Mar 8, 2023
@mxsasha mxsasha deleted the fix-525 branch March 8, 2023 13:54
@mxsasha
Copy link
Copy Markdown
Collaborator Author

mxsasha commented Mar 8, 2023

@baknu merged this, but explanation of our CSP requirement still needs updating in content repo. I have already added templates for the new error messages for these fields to https://github.com/internetstandards/Internet.nl_content/pull/30

@baknu
Copy link
Copy Markdown
Contributor

baknu commented Mar 13, 2023

@mxsasha Two questions:

  1. How do we evaluate 'base-uri' and 'form-action' when they exist but have a different value than 'none', 'self' or a URI? Maybe I missed it, but I did not see a unit test for that scenario.
  2. In the CSP3 specification there is a paragraph on Strict CSP which states "base-uri: Specify a value of either "['self']". See: https://www.w3.org/TR/CSP3/#strict-csp Should we be more strict regarding 'base-uri' and only allow it to have sources that we allow for 'default-src'? For the latter see test explanation sub 1 (https://en.internet.nl/site/example.nl/1978698/#control-panel-29).

@mxsasha
Copy link
Copy Markdown
Collaborator Author

mxsasha commented Mar 14, 2023

2. In the CSP3 specification there is a paragraph on Strict CSP which states "base-uri: Specify a value of either "['self']". See: https://www.w3.org/TR/CSP3/#strict-csp Should we be more strict regarding 'base-uri' and only allow it to have sources that we allow for 'default-src'? For the latter see test explanation sub 1 (https://en.internet.nl/site/example.nl/1978698/#control-panel-29).

@baknu: this is done as discussed in #895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

content Change (needed) to the content repository alongside with this issue/PR enhancement Issues that suggest slight improvements to existing code, tests, etc.

4 participants