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

feature: adds dev-time CSP headers #3456

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jan 23, 2025

Adds stricter CSP headers to our development vite server.

Whilst this is dev time only, it ensures that our GUI runs on a similarly configured server (such as kumahq/kuma#12553)


As mentioned in other places, it would be good to add work so we can remove the style-src 'unsafe-inline'. This will require a globally available v-style directive which adds/removes/modifies styles imperatively behind the scenes.

Testing:

Using make run, add the following or similar anchor with an inline script and click it.

Untitled-1

@johncowen johncowen requested a review from a team as a code owner January 23, 2025 12:37
@johncowen johncowen requested review from schogges and removed request for a team January 23, 2025 12:37
Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit 08697df
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/6797614fe07b4c0008bc2b5f
😎 Deploy Preview https://deploy-preview-3456--kuma-gui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@schogges schogges left a comment

Choose a reason for hiding this comment

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

Great to have this 🙌
Just two nits 🙂 I think for both it doesn't hurt to add/remove them, so approving this anyways.

packages/kuma-gui/vite.plugins.ts Outdated Show resolved Hide resolved
packages/kuma-gui/vite.plugins.ts Outdated Show resolved Hide resolved
@johncowen
Copy link
Contributor Author

Cool, thanks! I addressed the inlines also, remember this is only dev-time so we can always come back and tweak things here. If you spot anything that we would usually do in development that we now can't for whatever reason lemme know

Gonna go ahead and merge 👍

@johncowen
Copy link
Contributor Author

Oh wait I just thought of something else I should check 😅 brb!

@johncowen
Copy link
Contributor Author

😅 I added a straight-forward guard so we can turn this off in places we might need to. Later on I'll probably expand this so we can inject the config from The Outside

@johncowen
Copy link
Contributor Author

As mentioned in other places, it would be good to add work so we can remove the style-src 'unsafe-inline'. This will require a globally available v-style directive which adds/removes/modifies styles imperatively behind the scenes.

I made #3458 which hopefully will allow us to remove the style-src 'unsafe-inline' also, so I'm gonna hold off on merge here wait for that and rebase.

@johncowen johncowen force-pushed the feature/add-dev-csp-headers branch from 4f6988a to 2c7ce50 Compare January 27, 2025 10:27
@johncowen
Copy link
Contributor Author

Hey sorry, I think I want to have only one PR go in for this as a single reference. At the moment I'm trying to make improve things by removing the need for unsafe-inline caused by other issues, so there is a little bit of change here even after the initial approval.

🤔 I'm going to move this to draft even though it has approval, and look for approval again once I'm happy that I've definitely finished here, just so its clearer what I'm doing.

@johncowen johncowen marked this pull request as draft January 27, 2025 10:44
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.

2 participants