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

Move CSP-Headers to middleware #415

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Move CSP-Headers to middleware #415

wants to merge 4 commits into from

Conversation

fraxachun
Copy link
Contributor

Description

Until this PR we defined the Content-Security-Policy header in next.config.js, which is then created during build. The problem is that (at least in our pipelines) environment variables (like process.env.ADMIN_URL) are not available during the build resulting in empty header values.

This has not turned out as a problem until now, because when using the site embedded in an I-Frame it was protected by the OAuth2-Proxy which strips this header. But since vivid-planet/comet#2554 this will produce an error.

The solution is to send this header in the middleware where we have access to the environment variables. However, this (probably) comes with a speed penalty since the code is executed for every request.

thomasdax98
thomasdax98 previously approved these changes Nov 11, 2024
@nsams
Copy link
Member

nsams commented Nov 12, 2024

Not ideal, I would like to have the middleware as small as possible. But there seems to be no other way to do this.

Maybe, you could extract that to an function (and file)?

Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

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

What about the block preview, which isn't handled by the middleware? Shouldn't we add the headers for it as well? And redirects/rewrites (although I believe we don't need them there)

@fraxachun
Copy link
Contributor Author

Maybe, you could extract that to an function (and file)?

e7406b1

What about the block preview, which isn't handled by the middleware?

With this commit the block-preview also sends the headers. However, I'm not sure if the block-preview needs to send them, /block-preview could be excluded by the matcher in the first place. However, we still need the frame-ancestor directive for the site preview, so moving to the middleware still is necessary.

@nsams
Copy link
Member

nsams commented Nov 13, 2024

I will come up with a proposal for more elegantly combining multiple middlewares (in a future PR)

@nsams
Copy link
Member

nsams commented Nov 13, 2024

I will come up with a proposal for more elegantly combining multiple middlewares (in a future PR)

vivid-planet/comet#2728

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.

5 participants