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.

Sign up for GitHub

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

Jump to bottom

🐛[#68] add CSP headers for DRF spectacular schema #69

Merged
merged 3 commits into from
Sep 25, 2024
Merged

🐛[#68] add CSP headers for DRF spectacular schema #69

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
157bc99
:bug:[#68] add CSP headers for DRF spectacular schema
Coperh Sep 13, 2024
91abb7a
:recycle:[#68] move unsafe-inline source
Coperh Sep 20, 2024
45bef6b
:sparkles:[#68] remove CDN links
Coperh Sep 23, 2024
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
31 changes: 15 additions & 16 deletions open_api_framework/conf/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@
"vng_api_common",
"notifications_api_common",
"drf_spectacular",
"drf_spectacular_sidecar",
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't used this package before, so I'm not sure how it works, but what does it do in this case? If I check the github it contains the static assets, but we're still fetching them via CDNs right now.

I think it would be best if we use these self hosted assets, by changing the SPECTACULAR_SETTINGS (https://github.com/tfranzel/drf-spectacular?tab=readme-ov-file#self-contained-ui-installation), and then we can probably remove the changes to the CSP directives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's supposed to be self contained but stil has CSP errors without these links added.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stevenbal I thought so, but it looks even using sidecar required some csp tweaking:
https://drf-spectacular.readthedocs.io/en/latest/faq.html#solution-for-redoc

"rest_framework",
"django_markup",
"solo",
Expand Down Expand Up @@ -955,9 +956,7 @@ def init_sentry(before_send: Callable | None = None):
# NOTE: make sure values are a tuple or list, and to quote special values like 'self'

# ideally we'd use BASE_URI but it'd have to be lazy or cause issues
CSP_DEFAULT_SRC = [
"'self'",
] + config(
CSP_DEFAULT_SRC = ["'self'", "'unsafe-inline'"] + config(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to add unsafe-inline to the default? I'd prefer to add it only to the specific directives that need it (probably script-src)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved

"CSP_EXTRA_DEFAULT_SRC",
default=[],
split=True,
Expand Down Expand Up @@ -998,12 +997,16 @@ def init_sentry(before_send: Callable | None = None):
+ CORS_ALLOWED_ORIGINS
)

CSP_IMG_SRC = CSP_DEFAULT_SRC + config(
"CSP_EXTRA_IMG_SRC",
default=[],
split=True,
group="Content Security Policy",
help_text="Extra ``img-src`` sources for CSP other than ``CSP_DEFAULT_SRC``.",
CSP_IMG_SRC = (
CSP_DEFAULT_SRC
+ ["data:", "cdn.redoc.ly", "cdn.jsdelivr.net"] # used by DRF spectacular
Copy link
Contributor

@annashamray annashamray Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm It looks like you use two solutions in the same time:

  • sidecar to load assets locally
  • whitelist redoc CDN for CSP

I think one solution should be enough. If you want to whitelist it, there is no need in sidecar if I understand it correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed CDN links, does cause issues for the redoc logo which they do not include for "quick rebranding"

OZ does use raw.githubusercontent.com

Copy link
Contributor

@annashamray annashamray Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've read their solution for REDOC specifically and yeah, you were right, you need this CDN for the images
Sorry for my first comment. Could you please bring the setting back

+ config(
"CSP_EXTRA_IMG_SRC",
default=[],
split=True,
group="Content Security Policy",
help_text="Extra ``img-src`` sources for CSP other than ``CSP_DEFAULT_SRC``.",
)
)

# affects <object> and <embed> tags, block everything by default but allow deploy-time
Expand All @@ -1018,8 +1021,10 @@ def init_sentry(before_send: Callable | None = None):

# we must include this explicitly, otherwise the style-src only includes the nonce because
# of CSP_INCLUDE_NONCE_IN
CSP_STYLE_SRC = CSP_DEFAULT_SRC
CSP_STYLE_SRC = CSP_DEFAULT_SRC + ["fonts.googleapis.com"] # used by DRF spectacular
CSP_SCRIPT_SRC = CSP_DEFAULT_SRC
CSP_FONT_SRC = ("'self'", "fonts.gstatic.com")
CSP_WORKER_SRC = ("'self'", "blob:")

# firefox does not get the nonce from default-src, see
# https://stackoverflow.com/a/63376012
Expand All @@ -1035,9 +1040,3 @@ def init_sentry(before_send: Callable | None = None):
# CSP_SANDBOX # too much

CSP_UPGRADE_INSECURE_REQUESTS = False # TODO enable on production?

CSP_EXCLUDE_URL_PREFIXES = (
# ReDoc/Swagger pull in external sources, so don't enforce CSP on API endpoints/documentation.
"/api/",
"/admin/",
)
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ dependencies = [
"djangorestframework>=3.15.2",
"djangorestframework-gis>=1.0",
"django-filter>=24.2",
"drf-spectacular>=0.27.2",
"drf-spectacular[sidecar]>=0.27.2",
"django-csp>=3.8",
"djangorestframework-inclusions>=1.2.0",
"commonground-api-common>=1.12.1",
Expand Down
Loading