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

fix: csp false in rc5 removes custom csp header #321

Closed
dargmuesli opened this issue Dec 8, 2023 · 8 comments · Fixed by #322
Closed

fix: csp false in rc5 removes custom csp header #321

dargmuesli opened this issue Dec 8, 2023 · 8 comments · Fixed by #322
Assignees
Labels
bug Something isn't working

Comments

@dargmuesli
Copy link
Contributor

Reproduction Link

Build: https://github.com/maevsi/maevsi/actions/runs/7138462986/job/19440077662?pr=1475
PR: maevsi/maevsi#1475

Steps to reproduce

Set content-security-policy header using appendHeader in a server middleware and set contentSecurityPolicy: false for this module.

What is Expected?

Up until rc4 the csp set in the server middleware was applied and this module's default csp was not used.

What is actually happening?

There is no csp header at all in rc5. Seems like the existing header is removed when set to false?

@dargmuesli dargmuesli added the bug Something isn't working label Dec 8, 2023
@dargmuesli dargmuesli changed the title fix: csp false in rc5 removes csp header fix: csp false in rc5 removes custom csp header Dec 8, 2023
@Baroshem
Copy link
Owner

Baroshem commented Dec 8, 2023

@vejja

What are your thoughts on that? I think we recently had a PR where we were changing the boolean values for headers

@vejja
Copy link
Collaborator

vejja commented Dec 8, 2023

@dargmuesli you're right.

Setting header.contentSecurityPolicy: false now removes the header. Semantically we interpret it as 'do not send a CSP header'.
This is also the case for all other security headers controlled by Nuxt Security (e.g. Strict-Transport-Security, etc.).

However we could change this. It could mean 'do not set a CSP header'.

@vejja vejja self-assigned this Dec 8, 2023
@vejja vejja mentioned this issue Dec 8, 2023
6 tasks
@vejja
Copy link
Collaborator

vejja commented Dec 8, 2023

I'm about to patch this.

Just for clarity, the behavior is specific to false, ie:

  • If option is false, we do not remove user-defined headers.
  • If option is any other value, we overwrite user-defined headers.

This would revert to rc-4 behaviour.
This sounds logical because if you use Nuxt Security to set some security header values, there should be no reason to redefine them elsewhere.

@dargmuesli @Baroshem : do you agree this is the intended behavior ?

@dargmuesli
Copy link
Contributor Author

Yes, I understand the module as a helper to set values. In the best case it should be possible to disable single features of modules, e.g. to be able to use a custom implementation.

@vejja vejja linked a pull request Dec 8, 2023 that will close this issue
6 tasks
@Baroshem
Copy link
Owner

Baroshem commented Dec 8, 2023

Guys I am not sure about this change.

CSP is set by default when NuxtSecurity is added. I would expect when setting a csp to false that this csp wont be included at all.

Like with middleware (i.e. rateLimiter). If I dont want it I just set it to false and this middleware wont be enabled.

I think that the behavior in rc.4 was actually a bug and with rc.5 it is now correct.

But honestly, I am not sure if I understand your use case @dargmuesli. Why setting header in server middleware if you can just do it in the nuxt security?

@dargmuesli
Copy link
Contributor Author

Like with middleware (i.e. rateLimiter). If I dont want it I just set it to false and this middleware wont be enabled.

Right, but should it just not be enabled or should any such header be removed even if it has another source?

But honestly, I am not sure if I understand your use case @dargmuesli. Why setting header in server middleware if you can just do it in the nuxt security?

Because I cannot! 😂 I'm waiting for runtime support (#233, #298).

@vejja
Copy link
Collaborator

vejja commented Dec 8, 2023

I do agree with @dargmuesli

What I am doing in rc.5 is aggressively removing the 'Content-Security-Policy' header from the response with removeResponseHeader just before the response is sent.

Maybe the user wants to use RateLimiter but not CSP and prefers to set its CSP headers manually for whatever reason.

@Baroshem
Copy link
Owner

Baroshem commented Dec 8, 2023

Ahhh I see. Now it makes sense for me. Thanks for clarification guys!
Then you have a green light from me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants