Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) #6483

dsotirho-ucsc · 2024-08-07T16:17:26Z

Connected issues: DataBiosphere/azul-private#6

Checklist

Author

PR is a draft
Target branch is develop
Name of PR branch matches issues/<GitHub handle of author>/<issue#>-<slug>
On ZenHub, PR is connected to all issues it (partially) resolves
PR description links to connected issues
PR title matches¹ that of a connected issue _{or comment in PR explains why they're different}
PR title references all connected issues
For each connected issue, there is at least one commit whose title references that issue

¹ when the issue title describes a problem, the corresponding PR
title is Fix: followed by the issue title

Author (partiality)

Added p tag to titles of partial commits
This PR is labeled partial _{or completely resolves all connected issues}
This PR partially resolves each of the connected issues _{or does not have the partial label}

Author (chains)

This PR is blocked by previous PR in the chain _{or is not chained to another PR}
The blocking PR is labeled base _{or this PR is not chained to another PR}
This PR is labeled chained _{or is not chained to another PR}

Author (reindex, API changes)

Added r tag to commit title _{or the changes introduced by this PR will not require reindexing of any deployment}
This PR is labeled reindex:dev _{or the changes introduced by it will not require reindexing of dev}
This PR is labeled reindex:anvildev _{or the changes introduced by it will not require reindexing of anvildev}
This PR is labeled reindex:anvilprod _{or the changes introduced by it will not require reindexing of anvilprod}
This PR is labeled reindex:prod _{or the changes introduced by it will not require reindexing of prod}
This PR is labeled reindex:partial and its description documents the specific reindexing procedure for dev, anvildev, anvilprod and prod _{or requires a full reindex or carries none of the labels reindex:dev, reindex:anvildev, reindex:anvilprod and reindex:prod}
This PR and its connected issues are labeled API _{or this PR does not modify a REST API}
Added a (A) tag to commit title for backwards (in)compatible changes _{or this PR does not modify a REST API}
Updated REST API version number in app.py _{or this PR does not modify a REST API}

Author (upgrading deployments)

Ran make image_manifests.json and committed the resulting changes _{or this PR does not modify azul_docker_images, or any other variables referenced in the definition of that variable}
Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading deployments}
Added u tag to commit title _{or this PR does not require upgrading deployments}
This PR is labeled upgrade _{or does not require upgrading deployments}
This PR is labeled deploy:shared _{or does not modify image_manifests.json, and does not require deploying the shared component for any other reason}
This PR is labeled deploy:gitlab _{or does not require deploying the gitlab component}
This PR is labeled deploy:runner _{or does not require deploying the runner image}

Author (hotfixes)

Added F tag to main commit title _{or this PR does not include permanent fix for a temporary hotfix}
Reverted the temporary hotfixes for any connected issues _{or the none of the stable branches (anvilprod and prod) have temporary hotfixes for any of the issues connected to this PR}

Author (before every review)

Rebased PR branch on develop, squashed old fixups
Ran make requirements_update _{or this PR does not modify requirements*.txt, common.mk, Makefile and Dockerfile}
Added R tag to commit title _{or this PR does not modify requirements*.txt}
This PR is labeled reqs _{or does not modify requirements*.txt}
make integration_test passes in personal deployment _{or this PR does not modify functionality that could affect the IT outcome}

Peer reviewer (after approval)

PR is not a draft
Ticket is in Review requested column
PR is awaiting requested review from system administrator
PR is assigned to only the system administrator

System administrator (after approval)

Actually approved the PR
Labeled connected issues as demo or no demo
Commented on connected issues about demo expectations _{or all connected issues are labeled no demo}
Decided if PR can be labeled no sandbox
A comment to this PR details the completed security design review
PR title is appropriate as title of merge commit
N reviews label is accurate
Moved ticket to Approved column
PR is assigned to only the operator

Operator (before pushing merge the commit)

System administrator

Background migrations for dev.gitlab are complete _{or this PR is not labeled deploy:gitlab}
Background migrations for anvildev.gitlab are complete _{or this PR is not labeled deploy:gitlab}
PR is assigned to only the operator

Operator (before pushing merge the commit)

Operator (chain shortening)

Changed the target branch of the blocked PR to develop _{or this PR is not labeled base}
Removed the chained label from the blocked PR _{or this PR is not labeled base}
Removed the blocking relationship from the blocked PR _{or this PR is not labeled base}
Removed the base label from this PR _{or this PR is not labeled base}

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Propagated the deploy:shared, deploy:gitlab, deploy:runner, API, reindex:partial, reindex:anvilprod and reindex:prod labels to the next promotion PRs _{or this PR carries none of these labels}
Propagated any specific instructions related to the deploy:shared, deploy:gitlab, deploy:runner, API, reindex:partial, reindex:anvilprod and reindex:prod labels, from the description of this PR to that of the next promotion PRs _{or this PR carries none of these labels}
PR is assigned to no one

Shorthand for review comments

L line is too long
W line wrapping is wrong
Q bad quotes
F other formatting problem

coveralls · 2024-08-07T16:39:37Z

coverage: 85.39% (-0.009%) from 85.399%
when pulling a45d3ad on issues/dsotirho-ucsc/6-azul-csp-header-2
into bbff541 on develop.

codecov · 2024-08-07T16:43:10Z

Codecov Report

Attention: Patch coverage is 72.91667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.37%. Comparing base (bbff541) to head (a45d3ad).

Files with missing lines	Patch %	Lines
src/azul/chalice.py	60.86%	9 Missing ⚠️
test/integration_test.py	0.00%	3 Missing ⚠️
test/service/test_app_logging.py	95.45%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6483      +/-   ##
===========================================
- Coverage    85.38%   85.37%   -0.02%     
===========================================
  Files          155      155              
  Lines        20788    20807      +19     
===========================================
+ Hits         17749    17763      +14     
- Misses        3039     3044       +5

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

dsotirho-ucsc · 2024-08-13T17:05:12Z

6483_IT_2024-08-13.txt

Swagger UI & successful authorization with nonce value in CSP header:

nadove-ucsc · 2024-08-14T01:00:31Z

src/azul/chalice.py

@@ -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:


For consistency

Suggested change

def content_security_policy(self, nonce: Optional[str] = None) -> str:

def content_security_policy(self, nonce: str | None = None) -> str:

nadove-ucsc · 2024-08-14T01:03:16Z

src/azul/chalice.py

+        self = q('self')
+        none = q('none')
+        if nonce is not None:
+            nonce = q(f'nonce-{nonce}')


Contributing guide says to prefer concatenation over f-strings when the expressions are of equal length, as they are here:

Suggested change

nonce = q(f'nonce-{nonce}')

nonce = q('nonce-' + nonce)

nadove-ucsc · 2024-08-14T01:06:54Z

src/azul/chalice.py

@@ -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:


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.

nadove-ucsc · 2024-08-14T01:08:50Z

src/azul/chalice.py

+        def s(*args: str | None) -> str:
+            return ' '.join(filter(None, args))
+
+        self = q('self')


To avoid overriding the reference to the self parameter.

Suggested change

self = q('self')

self_ = q('self')

dsotirho-ucsc · 2024-08-14T23:53:07Z

6483_IT_2024-08-14.txt

nadove-ucsc · 2024-08-15T00:50:34Z

src/azul/strings.py

+    return surround(jw(*words), "'")
+
+
+def qq(*words: str) -> str:


There is at least one other place this function could be used

There are other places. Search for f'" and f"' (that's a single quote and double quote in either order)

nadove-ucsc · 2024-08-15T00:53:34Z

src/azul/strings.py

+    return ' '.join(words)
+
+
+def surround(string: str, end: str) -> str:


azul.bigquery.backtick could make use of this function

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.

Alright, as long as the build's green on GitHub I think we're okay.

nadove-ucsc · 2024-08-15T01:11:02Z

src/azul/strings.py

@@ -232,11 +232,11 @@ def jl(*lines: str) -> str:
    return '\n'.join(lines)


-def jw(*words: str) -> str:
+def jw(*words: str | None) -> str:


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).

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 · 2024-08-19T17:05:36Z

6483_IT_2024-08-19.txt

scripts/convert_environment.py

nadove-ucsc · 2024-08-20T01:53:02Z

src/azul/strings.py

+    ...
+    azul.RequirementError: foo's
+    """
+    require(end not in string, string)


Suggested change

require(end not in string, string)

reject(end in string, string)

nadove-ucsc · 2024-08-20T02:00:10Z

src/azul/strings.py

+    return ' '.join(words)
+
+
+def surround(string: str, end: str) -> str:


Alright, as long as the build's green on GitHub I think we're okay.

nadove-ucsc · 2024-08-20T02:02:00Z

src/azul/strings.py

+    Prefix and postfix a string with another. The pre/postfix cannot be a
+    substring of the source string.


Nitpick, but I think the English can be streamlined a bit

Suggested change

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.

(feel free to disregard if you feel "affix" is too obscure a word to be helpful here)

dsotirho-ucsc · 2024-10-04T17:17:22Z

6483_IT_2024-10-04.txt

achave11-ucsc

LGTM ✅

🔎👁️ Consider moving the commit 'Move import statement above docstring' closer to the beginning, perhaps after the quotes change, preceding the refactor. Keeps the semantically neutral changes closer together.

test/service/test_app_logging.py

nadove-ucsc · 2024-10-04T22:38:50Z

test/service/test_app_logging.py

                    response_headers = {
                        'Access-Control-Allow-Origin': '*',
                        'Access-Control-Allow-Headers': 'Authorization,'
                                                        'Content-Type,'
                                                        'X-Amz-Date,'
                                                        'X-Amz-Security-Token,'
                                                        'X-Api-Key',
-                        'Content-Security-Policy': f"default-src {sq('self')}",
+                        'Content-Security-Policy': f"default-src {self_};"


With the inner quotes extracted, the double quotes in these paragraphs should be converted to single quotes in this commit.

nadove-ucsc · 2024-10-04T22:43:39Z

src/azul/chalice.py

-    # 5XX responses from API Gateway. Use of these headers addresses known
-    # security vulnerabilities.
-    #
-    security_headers = {


Nitpick: I would refactor this from a variable to a method in a separate commit. This makes it easier to read the changes by isolating the noisy insertion of () away from the semantic changes.

nadove-ucsc · 2024-10-04T22:48:34Z

test/test_app_logging.py

+                        none = sq('none')
+                        headers = {
+                            'Content-Type': 'text/plain',
+                            'Content-Security-Policy': f"default-src {self_};"


Quotes again

…l-private#6)

dsotirho-ucsc · 2024-10-05T01:00:56Z

6483_IT_2024-10-04.txt

nadove-ucsc

Comments were thumbs-upped but requested changes were not implemented:

a45d3ad#r1788392095
a45d3ad#r1788396246

github-actions bot added the orange [process] Done by the Azul team label Aug 7, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch 4 times, most recently from 00b8e08 to 13a04fb Compare August 13, 2024 16:26

dsotirho-ucsc assigned nadove-ucsc Aug 13, 2024

dsotirho-ucsc requested a review from nadove-ucsc August 13, 2024 17:07

nadove-ucsc requested changes Aug 14, 2024

View reviewed changes

nadove-ucsc removed their assignment Aug 14, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch 2 times, most recently from a406e1b to a36f7c5 Compare August 14, 2024 21:59

dsotirho-ucsc requested a review from nadove-ucsc August 14, 2024 23:55

dsotirho-ucsc assigned nadove-ucsc Aug 14, 2024

nadove-ucsc requested changes Aug 15, 2024

View reviewed changes

nadove-ucsc removed their assignment Aug 15, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch 5 times, most recently from 4311908 to e2dab47 Compare August 19, 2024 16:48

dsotirho-ucsc requested a review from nadove-ucsc August 19, 2024 17:16

dsotirho-ucsc assigned nadove-ucsc Aug 19, 2024

nadove-ucsc requested changes Aug 20, 2024

View reviewed changes

nadove-ucsc removed their assignment Aug 20, 2024

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch from e2dab47 to 7756e47 Compare August 20, 2024 16:43

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch 6 times, most recently from ccb1884 to 6e4f20d Compare October 3, 2024 22:55

Replace double quotes with single quotes

59d1e61

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch from 6e4f20d to 59d1e61 Compare October 4, 2024 01:25

dsotirho-ucsc requested a review from achave11-ucsc October 4, 2024 17:19

dsotirho-ucsc assigned achave11-ucsc Oct 4, 2024

achave11-ucsc previously approved these changes Oct 4, 2024

View reviewed changes

achave11-ucsc removed their assignment Oct 4, 2024

achave11-ucsc requested a review from nadove-ucsc October 4, 2024 22:14

achave11-ucsc assigned nadove-ucsc Oct 4, 2024

nadove-ucsc requested changes Oct 4, 2024

View reviewed changes

nadove-ucsc removed their assignment Oct 4, 2024

dsotirho-ucsc added 5 commits October 4, 2024 17:04

Move import statement above docstring

1c6521a

Rename file parameter in swagger_resource()

983c3ab

Refactor assert of request and response headers in test case

f7dbb4c

Refactor security_headers property into a method

7f75bd3

Fix: Content Security Policy (CSP) Not Implemented (DataBiosphere/azu…

a45d3ad

…l-private#6)

dsotirho-ucsc dismissed achave11-ucsc’s stale review via 3ff035a October 5, 2024 00:07

dsotirho-ucsc force-pushed the issues/dsotirho-ucsc/6-azul-csp-header-2 branch 2 times, most recently from 3ff035a to 59d1e61 Compare October 5, 2024 00:12

dsotirho-ucsc requested a review from nadove-ucsc October 5, 2024 01:01

dsotirho-ucsc assigned nadove-ucsc Oct 5, 2024

nadove-ucsc requested changes Oct 5, 2024

View reviewed changes

nadove-ucsc removed their assignment Oct 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) #6483

Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) #6483

dsotirho-ucsc commented Aug 7, 2024 •

edited by achave11-ucsc

Loading

coveralls commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

dsotirho-ucsc commented Aug 13, 2024

nadove-ucsc Aug 14, 2024

nadove-ucsc Aug 14, 2024

nadove-ucsc Aug 14, 2024

nadove-ucsc Aug 14, 2024

dsotirho-ucsc commented Aug 14, 2024

nadove-ucsc Aug 15, 2024

nadove-ucsc Aug 15, 2024

nadove-ucsc Aug 15, 2024

nadove-ucsc Aug 15, 2024

dsotirho-ucsc Aug 19, 2024 •

edited

Loading

nadove-ucsc Aug 20, 2024

nadove-ucsc Aug 15, 2024

nadove-ucsc Aug 15, 2024

dsotirho-ucsc commented Aug 19, 2024

nadove-ucsc Aug 20, 2024

nadove-ucsc Aug 20, 2024

nadove-ucsc Aug 20, 2024

nadove-ucsc Aug 20, 2024

dsotirho-ucsc commented Oct 4, 2024

achave11-ucsc left a comment

nadove-ucsc Oct 4, 2024

nadove-ucsc Oct 4, 2024

nadove-ucsc Oct 4, 2024

dsotirho-ucsc commented Oct 5, 2024

nadove-ucsc left a comment

	def content_security_policy(self, nonce: Optional[str] = None) -> str:
	def content_security_policy(self, nonce: str \| None = None) -> str:

		return ' '.join(words)


		def surround(string: str, end: str) -> str:

	require(end not in string, string)
	reject(end in string, string)

		Prefix and postfix a string with another. The pre/postfix cannot be a
		substring of the source string.

Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) #6483

Are you sure you want to change the base?

Content Security Policy (CSP) Not Implemented (DataBiosphere/azul-private#6) #6483

Conversation

dsotirho-ucsc commented Aug 7, 2024 • edited by achave11-ucsc Loading

Checklist

Author

Author (partiality)

Author (chains)

Author (reindex, API changes)

Author (upgrading deployments)

Author (hotfixes)

Author (before every review)

Peer reviewer (after approval)

System administrator (after approval)

Operator (before pushing merge the commit)

System administrator

Operator (before pushing merge the commit)

Operator (chain shortening)

Operator (after pushing the merge commit)

Operator (reindex)

Operator

Shorthand for review comments

coveralls commented Aug 7, 2024 • edited Loading

codecov bot commented Aug 7, 2024 • edited Loading

Codecov Report

dsotirho-ucsc commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsotirho-ucsc commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsotirho-ucsc Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsotirho-ucsc commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsotirho-ucsc commented Oct 4, 2024

achave11-ucsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsotirho-ucsc commented Oct 5, 2024

nadove-ucsc left a comment

Choose a reason for hiding this comment

dsotirho-ucsc commented Aug 7, 2024 •

edited by achave11-ucsc

Loading

coveralls commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

dsotirho-ucsc Aug 19, 2024 •

edited

Loading