Fix: Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) #6483
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6483 +/- ##
===========================================
- Coverage 85.59% 85.58% -0.01%
===========================================
Files 155 155
Lines 20903 20952 +49
===========================================
+ Hits 17892 17932 +40
- Misses 3011 3020 +9 ☔ View full report in Codecov by Sentry. |
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
4 times, most recently
from
00b8e08
to
13a04fb
Compare
|
Swagger UI & successful authorization with nonce value in CSP header: |
src/azul/chalice.py
Outdated
| @@ -468,12 +471,34 @@ def catalog(self) -> str: | |||
| def _controller(self, controller_cls: Type[C], **kwargs) -> C: | |||
| return controller_cls(app=self, **kwargs) | |||
|
|
|||
| def content_security_policy(self, nonce: Optional[str] = None) -> str: | |||
There was a problem hiding this comment.
For consistency
| def content_security_policy(self, nonce: Optional[str] = None) -> str: | |
| def content_security_policy(self, nonce: str | None = None) -> str: |
src/azul/chalice.py
Outdated
| self = q('self') | ||
| none = q('none') | ||
| if nonce is not None: | ||
| nonce = q(f'nonce-{nonce}') |
There was a problem hiding this comment.
Contributing guide says to prefer concatenation over f-strings when the expressions are of equal length, as they are here:
| nonce = q(f'nonce-{nonce}') | |
| nonce = q('nonce-' + nonce) |
src/azul/chalice.py
Outdated
| @@ -468,12 +471,34 @@ def catalog(self) -> str: | |||
| def _controller(self, controller_cls: Type[C], **kwargs) -> C: | |||
| return controller_cls(app=self, **kwargs) | |||
|
|
|||
| def content_security_policy(self, nonce: Optional[str] = None) -> str: | |||
| def q(s: str) -> str: | |||
There was a problem hiding this comment.
Is it possible for the string to contain quote characters that need to be escaped?
Consider creating a dedicated, non-local function for this in src/azul/strings.py. I'm pretty sure there are other places in the codebase where we use this logic.
src/azul/chalice.py
Outdated
| def s(*args: str | None) -> str: | ||
| return ' '.join(filter(None, args)) | ||
|
|
||
| self = q('self') |
There was a problem hiding this comment.
To avoid overriding the reference to the self parameter.
| self = q('self') | |
| self_ = q('self') |
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
2 times, most recently
from
a406e1b
to
a36f7c5
Compare
src/azul/strings.py
Outdated
| return surround(jw(*words), "'") | ||
|
|
||
|
|
||
| def qq(*words: str) -> str: |
There was a problem hiding this comment.
There is at least one other place this function could be used
There was a problem hiding this comment.
There are other places. Search for f'" and f"' (that's a single quote and double quote in either order)
src/azul/strings.py
Outdated
| return ' '.join(words) | ||
|
|
||
|
|
||
| def surround(string: str, end: str) -> str: |
There was a problem hiding this comment.
azul.bigquery.backtick could make use of this function
There was a problem hiding this comment.
I believe the issue we saw on Friday regarding a circular import error when importing from azul.strings into azul.bigquery was due to a problem with my PyCharm configuration. After a reboot & reset of my Python interpreter path I am no longer able to reproduce the error.
There was a problem hiding this comment.
Alright, as long as the build's green on GitHub I think we're okay.
src/azul/strings.py
Outdated
| @@ -232,11 +232,11 @@ def jl(*lines: str) -> str: | |||
| return '\n'.join(lines) | |||
|
|
|||
|
|
|||
| def jw(*words: str) -> str: | |||
| def jw(*words: str | None) -> str: | |||
There was a problem hiding this comment.
I don't think this is a good idea. Filtering and joining the strings in the same function was okay when it was a local function that could only be used in one place, but a public function like this shouldn't be so fine-tuned to a single call site. There may be other call sites someday where the filtering isn't necessary or appropriate (it could hide bugs, for example).
There was a problem hiding this comment.
If I understand correctly, the only word that actually needs to be filtered is the nonce variable. How about this approach instead to eliminate the call to filter altogether?
Subject: [PATCH] review
---
Index: src/azul/chalice.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/chalice.py b/src/azul/chalice.py
--- a/src/azul/chalice.py (revision a36f7c5062b20fb1b79c21140c5bdb2c8db92fdf)
+++ b/src/azul/chalice.py (date 1723684328983)
@@ -478,14 +478,13 @@
def content_security_policy(self, nonce: str | None = None) -> str:
self_ = sq('self')
none = sq('none')
- if nonce is not None:
- nonce = sq('nonce-' + nonce)
+ nonce = [] if nonce is None else [sq('nonce-' + nonce)]
return ';'.join([
jw('default-src', self_),
jw('img-src', self_, 'data:'),
- jw('script-src', self_, nonce),
- jw('style-src', self_, nonce),
+ jw('script-src', self_, *nonce),
+ jw('style-src', self_, *nonce),
jw('frame-ancestors', none),
])
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
5 times, most recently
from
4311908
to
e2dab47
Compare
src/azul/strings.py
Outdated
| ... | ||
| azul.RequirementError: foo's | ||
| """ | ||
| require(end not in string, string) |
There was a problem hiding this comment.
| require(end not in string, string) | |
| reject(end in string, string) |
src/azul/strings.py
Outdated
| return ' '.join(words) | ||
|
|
||
|
|
||
| def surround(string: str, end: str) -> str: |
There was a problem hiding this comment.
Alright, as long as the build's green on GitHub I think we're okay.
src/azul/strings.py
Outdated
| Prefix and postfix a string with another. The pre/postfix cannot be a | ||
| substring of the source string. |
There was a problem hiding this comment.
Nitpick, but I think the English can be streamlined a bit
| Prefix and postfix a string with another. The pre/postfix cannot be a | |
| substring of the source string. | |
| Prepend and append an affix to a string. The affix cannot be a | |
| substring of the base string. |
There was a problem hiding this comment.
(feel free to disregard if you feel "affix" is too obscure a word to be helpful here)
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
from
e2dab47
to
7756e47
Compare
The nonce violates the RFCThe nonce is shorter than expectedThe nonce is longer than expectedCSP contains more than one script-src or style-src directiveCSP contains more than one valid nonce of the expected length in the script-src or style-src directiveCSP contains a valid nonce of the expected length and one that violates the RFCCSP contains a valid nonce of the expected length and a valid one of an unexpected length |
There was a problem hiding this comment.
Regarding #6483 (comment)
If you just provide the tracebacks, it is a little difficult for me to see what the actual value was that failed the assertion. I'd have to infer which may be error-prone.
Please refactor the code that parses and validates the CSP in the IT into a method and write doctests for that method. We'll decide afterwards if we want to keep those doctests or not.
src/azul/chalice.py
Outdated
| 'X-XSS-Protection': '1; mode=block' | ||
| } | ||
| @classmethod | ||
| def nonce_value(cls) -> str: |
There was a problem hiding this comment.
"nonce" is too unspecific. "value" is usually redundant. If a method returns something, that something is a value. "Value" would only be useful if there also was, say, a "key" to distinguish from.
| def nonce_value(cls) -> str: | |
| def csp_nonce(cls) -> str: |
src/azul/chalice.py
Outdated
| ... # doctest: +NORMALIZE_WHITESPACE | ||
| ["default-src 'self'", \ | ||
| "img-src 'self' data:", \ | ||
| "script-src 'self'", \ | ||
| "style-src 'self'", \ | ||
| "frame-ancestors 'none'"] |
There was a problem hiding this comment.
We usually use assert_json for asserting JSON structures in doctests.
test/integration_test.py
Outdated
| expected_expressions = { | ||
| sq('self'), | ||
| sq('none'), | ||
| 'data:' | ||
| } | ||
| expected_directives = { | ||
| 'default-src', | ||
| 'img-src', | ||
| 'script-src', | ||
| 'style-src', | ||
| 'frame-ancestors' | ||
| } |
There was a problem hiding this comment.
This is a regression to the point where we duplicated the the expected headers in assertions. The ONLY unpredictable part is the nonce so the previous approach of updating the expected headers with that nonce value is much less brittle, as long as the parsing of the CSP and the validation of the nonce's syntax doesn't let invalid or duplicate nonces through.
There was a problem hiding this comment.
Note that with the refactoring of the parsing & validation of the CSP into a separate method, I had to remove the CSP from the expected headers due to no longer having the nonce value available to generate the expected CSP in the test.
There was a problem hiding this comment.
Note that with the refactoring of the parsing & validation of the CSP into a separate method, I had to remove the CSP from the expected headers due to no longer having the nonce value available to generate the expected CSP in the test.
I don't understand. Just have the parsing/validation method return the expected header entry and inject the return value into the expected header dictionary. Please raise in PL if this is not clear.
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
from
d629b5c
to
dc81455
Compare
src/azul/chalice.py
Outdated
| # Not all expected directives are specified | ||
| >>> cls.validate_csp("default-src 'self'", has_nonce=False) |
There was a problem hiding this comment.
| # Not all expected directives are specified | |
| >>> cls.validate_csp("default-src 'self'", has_nonce=False) | |
| Not all expected directives are specified | |
| >>> cls.validate_csp("default-src 'self'", has_nonce=False) |
and so on
test/integration_test.py
Outdated
| expected_expressions = { | ||
| sq('self'), | ||
| sq('none'), | ||
| 'data:' | ||
| } | ||
| expected_directives = { | ||
| 'default-src', | ||
| 'img-src', | ||
| 'script-src', | ||
| 'style-src', | ||
| 'frame-ancestors' | ||
| } |
There was a problem hiding this comment.
Note that with the refactoring of the parsing & validation of the CSP into a separate method, I had to remove the CSP from the expected headers due to no longer having the nonce value available to generate the expected CSP in the test.
I don't understand. Just have the parsing/validation method return the expected header entry and inject the return value into the expected header dictionary. Please raise in PL if this is not clear.
src/azul/chalice.py
Outdated
| ... "style-src 'self' 'nonce-1234567890123456789012345678901234567890123';" | ||
| ... "frame-ancestors 'none'", has_nonce=True) | ||
|
|
||
| # Not all expected directives are specified |
There was a problem hiding this comment.
Remove this case. It is not one of the test cases I specified. Remove all other cases that become redundant by asserting the expected headers (with the expected CSP injected) in the IT, as I requested in previous review.
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
from
dc81455
to
45fa2b0
Compare
test/integration_test.py
Outdated
| expected = AzulChaliceApp.security_headers() | expected_headers | ||
| nonce = AzulChaliceApp.validate_csp(response.headers['Content-Security-Policy'], | ||
| has_nonce=path in ['/', '/oauth2_redirect']) | ||
| expected_csp = AzulChaliceApp.content_security_policy(nonce) |
There was a problem hiding this comment.
As I said in my previous review, have validate_csp return the expected CSP. The post-condition of that method is that the CSP is valid and it is therefore safe to inject it into the expectation.
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
from
45fa2b0
to
0d8d886
Compare
dsotirho-ucsc
force-pushed
the
issues/dsotirho-ucsc/6-azul-csp-header-2
branch
from
0d8d886
to
b9deb4b
Compare
Connected issues: DataBiosphere/azul-private#6
Checklist
Author
developissues/<GitHub handle of author>/<issue#>-<slug>1 when the issue title describes a problem, the corresponding PR
title is
Fix:followed by the issue titleAuthor (partiality)
ptag to titles of partial commitspartialor completely resolves all connected issuespartiallabelAuthor (chains)
baseor this PR is not chained to another PRchainedor is not chained to another PRAuthor (reindex, API changes)
rtag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:devor the changes introduced by it will not require reindexing ofdevreindex:anvildevor the changes introduced by it will not require reindexing ofanvildevreindex:anvilprodor the changes introduced by it will not require reindexing ofanvilprodreindex:prodor the changes introduced by it will not require reindexing ofprodreindex:partialand its description documents the specific reindexing procedure fordev,anvildev,anvilprodandprodor requires a full reindex or carries none of the labelsreindex:dev,reindex:anvildev,reindex:anvilprodandreindex:prodAPIor this PR does not modify a REST APIa(A) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.pyor this PR does not modify a REST APIAuthor (upgrading deployments)
make image_manifests.jsonand committed the resulting changes or this PR does not modifyazul_docker_images, or any other variables referenced in the definition of that variableutag to commit title or this PR does not require upgrading deploymentsupgradeor does not require upgrading deploymentsdeploy:sharedor does not modifyimage_manifests.json, and does not require deploying thesharedcomponent for any other reasondeploy:gitlabor does not require deploying thegitlabcomponentdeploy:runneror does not require deploying therunnerimageAuthor (hotfixes)
Ftag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprodandprod) have temporary hotfixes for any of the issues connected to this PRAuthor (before every review)
develop, squashed old fixupsmake requirements_updateor this PR does not modifyrequirements*.txt,common.mk,MakefileandDockerfileRtag to commit title or this PR does not modifyrequirements*.txtreqsor does not modifyrequirements*.txtmake integration_testpasses in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
System administrator (after approval)
demoorno demono demono sandboxN reviewslabel is accurateOperator (before pushing merge the commit)
reindex:…labels andrcommit title tagno demodevelop_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlab_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlabdeploy:gitlabdeploy:gitlabSystem administrator
dev.gitlabare complete or this PR is not labeleddeploy:gitlabanvildev.gitlabare complete or this PR is not labeleddeploy:gitlabOperator (before pushing merge the commit)
_select dev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runner_select anvildev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runnersandboxlabel or PR is labeledno sandboxdevor PR is labeledno sandboxanvildevor PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxor this PR does not remove catalogs or otherwise causes unreferenced indices indevanvilboxor this PR does not remove catalogs or otherwise causes unreferenced indices inanvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevpif the PR is also labeledpartialOperator (chain shortening)
developor this PR is not labeledbasechainedlabel from the blocked PR or this PR is not labeledbasebasebaselabel from this PR or this PR is not labeledbaseOperator (after pushing the merge commit)
devanvildevdevdevanvildevanvildev_select dev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shared_select anvildev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shareddevanvildevOperator (reindex)
devor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevOperator
deploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels to the next promotion PRs or this PR carries none of these labelsdeploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
Lline is too longWline wrapping is wrongQbad quotesFother formatting problem