Replies: 8 comments 30 replies
-
|
Hey buddy! This is an amazing feature idea! I really like it and I totally think this could be part of the module itself. I looked at the code and in general I have only one comment so far. I will be releasing quite a big new version in the next few days (1.0.0-rc.1) and there, few things have changed. Most importantly, I have introduced a new configuration property object called ssg. For now, it includes only the It would be a good idea to start your feature request from this branch or from main after wednesday as this will be a new develooment branch. |
Beta Was this translation helpful? Give feedback.
-
|
https://github.com/Baroshem/nuxt-security/blob/chore/1.0.0-rc.1/src/types/index.ts |
Beta Was this translation helpful? Give feedback.
-
|
Ok, I will definitely re-fork from rc.1 1 - On how I designed the SRI feature:
2 - On how I designed the CSP complementary feature:
As a result, I would think
What I'm trying to say, is that it should be the responsibility of the module to handle SSR vs SSG, and it would be preferable not to give the user the possibility to mess up with this via options. |
Beta Was this translation helpful? Give feedback.
-
|
Really interesting discovery. I would like to invite @trijpstra-fourlights into this discussion as he was developing nonce and he may have some interesting comments as well :) Great to see such traffic and cool ideas to improve this project 💚 |
Beta Was this translation helpful? Give feedback.
-
|
Yes, I agree with this. I thought I did state this in my original PR or even the docs, but I can't seem to find it so I must be mistaken. The added security when using nonce comes from injecting a non-deterministic value for each request in both the headers as in the actual served html, which the browser can then compare to determine if a resource should be loaded. So using it on SSG makes absolutely no sense from a security perspective. Thanks a lot for improving this! Please note that the nonce value is also valid in other csp attributes (i.e. not only Personally I would keep it explicit, as that feels more in line with how the rest of nuxt-security is configured. Or at least deprecate it for a few releases. On the other hand, the feature is pretty new still so maybe ripping of the bandaid now while it's still relatively fresh is preferable. |
Beta Was this translation helpful? Give feedback.
-
|
Ah, I only mentioned the nonce and SSG incompatibility in this thread: #178 (comment) |
Beta Was this translation helpful? Give feedback.
-
|
Just want to give a quick update here based on rc1. The security issue is partially resolved in rc1, because now the However please note that this is only a TypeScript check based on the type definition. In other words, we are relying on people to follow the instructions of their linter (assuming they did set it up properly). If they don't, the hole is still open. In order to separate concerns properly, I will make a first PR aiming at improving the TypeScript config in the repo, as we have several type mismatches and imports misconfigured, and I believe it will help spotting these issues. |
Beta Was this translation helpful? Give feedback.
-
|
Sounds awesome. Please create this PR. Depending on when you will do it I will either release it with rc.1 or rc.2 as I want to release Rc.1 on wednesday (during Nuxt Nation 😉) |
Beta Was this translation helpful? Give feedback.
-
|
I'm trying to get my head around pure-SSR/SSR+routes preredenring/pure-SSG/SSG+pre-rendering here. |
Beta Was this translation helpful? Give feedback.
-
|
@trijpstra-fourlights You will be the best person to answer :) |
Beta Was this translation helpful? Give feedback.
-
|
If you're serving assets from a nuxt api route, e.g. proxying from external service, you want nitro to not replace the current nonce, but still add it in the header. Because it's a new request from nitros perspective you in these advanced use cases need control over this. Mode:check does that, where mode: renew (the default) changes the nonce value. It's in the docs as well, albeit sparsely. As I said it's an advanced use case that most will not need, hence the default value being mode renew for all routes. |
Beta Was this translation helpful? Give feedback.
-
|
Ok I can see the use case now, thanks |
Beta Was this translation helpful? Give feedback.
-
|
No that's not correct. The aforementioned hypothetical external service has
nothing to do with the nonce or csp. It's essentially out of our control
for the sake of argument.
The nonce is set by the nuxt server, and is persisted using a cookie with
same site and secure. This allows the `useNonce` composable to access the
current nonce value in the frontend and allows nitro to reuse it (again,
only in these advanced use cases of mode:check) when applicable.
Within the same request, nitro/the nuxt server can access the h3 event
context which is a tiny bit quicker than using the cookie.
This is how SSR works with Nuxt. It's not applicable for SSG as nonce
values add no security benefit in a static context.
To better explain (one of the) potential advanced use cases:
Imagine an api backend which exposes an endpoint which returns a runtime
generated stylesheet which you want to include on your app, while keeping
strict csp in place. Using a Nuxt api route to proxy this runtime generated
endpoint allows you to do that, while also not exposing the underlying api
to the frontend.
I use this succesfully in production similarly to this example.
Op do 19 okt 2023 10:10 schreef vejja ***@***.***>:
… Ok I can see the use case now, thanks
Looks like you are extracting the external server's nonce from either
event.context or a nonce cookie.
How do you know these variables are set by the external server ?
—
Reply to this email directly, view it on GitHub
<#241 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A25JEPRVEP45ZPHUTR6P6B3YADOANAVCNFSM6AAAAAA6ALXWFSVHI2DSMVQWIX3LMV43SRDJONRXK43TNFXW4Q3PNVWWK3TUHM3TGMRTHAZDC>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Beta Was this translation helpful? Give feedback.
-
|
Sure, but let's do it the other way around. Can you make a stackblitz or similar with the exact set-up where you've verified it's working without Thanks in advance! |
Beta Was this translation helpful? Give feedback.
-
|
I've checked locally and I think I see where the disconnect is coming from. First of, you are correct in that using the test case in the playground will not cause CSP error. Furthermore the browser doesn't seem to care about the returned CSP headers for subsequent resources, and only matches it against the original headers. So you're correct in that as well. I was wrong in that and my explanation on the need for Still, the Let's see if I can explain what's happening: When disabling the
As you can see, the api request has a different CSP nonce and has updated the cookie accordingly. When using Now, this is where I was surprised that it's not throwing a CSP error because apparently the browser only cares about the original CSP header being matched to the nonce value. However, this For example, when you want to inject resources from javascript at runtime, you'll have to supply the nonce value (using the
|
Beta Was this translation helpful? Give feedback.
-
|
Well TBH I have higher-level security concerns about this approach. |
Beta Was this translation helpful? Give feedback.
-
|
Oh, a cliffhanger. Lovely! You know what, feel free to submit a PR removing whatever you feel fit. I don't care, it's not my module. I created the initial nonce implementation, if you feel it's not secure, then be my guest and improve upon it. Or remove it or whatever. If it breaks my code I'll make sure to create an issue for it. |
Beta Was this translation helpful? Give feedback.
-
|
Ok, I'll write a summary report below for people to reflect and give their feedback |
Beta Was this translation helpful? Give feedback.
-
CSP nonce implementationI wanted to reflect upon the discussion related to the implementation of CSP nonces. In a nutshell, it is currently possible to entirely bypass CSP.
Security Analysis
I do understand the use case where the user needs to generate scripts on the fly. However a strict CSP does not prevent this. CSP only ensures that you have identified the script and have authorized it explicitely via nonce. In summary, the current implementation does not enforce CSP, but allows to bypass CSP. Proposed modificationsAs a result, I would like to propose the following critical modifications:
This will allow the module to become CSP-compliant. |
Beta Was this translation helpful? Give feedback.
-
|
These are pretty strong points and should IMO indeed mandate action if they hold true. Can you show how an attacker can bypass the CSP in this way? As far as I can tell, Same applies to your claim that it's very easy to forge cookies, and thus to force the server to use a forged nonce. Can you show a POC for this? Similarly "Moreover, providing a useNonce composable in the frontend is equivalent to give the attacker a ready-to-use attack vector with an easy way to forge nonces server-side." -> can you show us how? |
Beta Was this translation helpful? Give feedback.
-
|
Additionally I'd like to point out that removing the Most notably, Furthermore, similar discussions in for example next.js show the real-world need for accessing the nonce value in the front-end. Various solutions to this problem are suggested, but they essentially all boil down to exposing the nonce value to the front-end in some way. Interestingly, I think this should not imply a security risk, as accessing this will require the attacker to be able to execute arbitrary code, which is not possible when using a strict CSP. From a real-world scenario, I have several third-party scripts that I'm required to inject due to business requirements (think analytics, usabila, etc.). These scripts are not static and often change. Without any of the current features in place, there is no way to use a strict CSP and have these scripts working. Thus the only way when removing the mentioned features will be to allow All in all, I think that Finally it should be noted that using a strict CSP is not a final solution, and only one of the many best-practices that should be used as a line of defense against attackers. That being said, I have a bad taste from this whole discussion and I"m refraining from adding to it anymore. |
Beta Was this translation helpful? Give feedback.
-
|
Guys, I really dont like where this discussion went. We started from sharing opinions about this topic to a point where we are trying to convince each other that the other person is wrong. And this is the last thing that I would like to see here as I really value how both of you have contributed to this discussion and the module in general. The changes @vejja is proposing are extremely destructive (especially considering the fact that yesterday I have released a version that is supposed to be stable and only include bug fixes not breaking changes). I appreciate your work here Vejja but even from the maintainer perspective I cannot agree to do these changes even if we could all agree that they are necessary. Users are expecting stability right now. I understand your concerns but as @trijpstra-fourlights suggested, it would be useful to see some examples why the current solution is wrong. Since the release of current implementation of nonce, there were several people who have tried it, asked questions and got responses usually from @trijpstra-fourlights (thanks for that!). And not to mention @trijpstra-fourlights himself who is using this solution as he mentioned. So I think that the current solution is not bad. @vejja is there maybe a way to address your concerns in a form of a documentation section rather than removing functionality that is already there? I think this solution would be the best right now considering the recent release and this whole discussion. And finally, please remember that both of you want to make this module better. I am really, really grateful for that. You just have different opinions and ideas and I will do my best to find the common path and let you both contribute in most appriopriate way possible. |
Beta Was this translation helpful? Give feedback.
-
|
While I don't believe all the arguments made by @vejja hold true, I do believe we should be showing and not telling, i.e. let's work from POCs and move from there. I'll try to find some time the coming days to show examples on when I believe you'll need the nonce value in the front-end. Unfortunately, it seems that stackblitz et al. are not suited to actively test this (due do them actively injecting the sandbox in the browser), so we'll have to work from full repo's on GitHub. I'll be sure to update this thread when I've got something to show. That being said, I do believe that @vejja 's concern about exposing the nonce to the frontend by default has some merit. As that seems to be the major concern by @vejja, I'd like to, in the interest of the upcoming v1.0 release, propose the following:
@Baroshem would you be willing to change the default behavior for nonce before the v1.0 release? I'll start work on this PR so that we can at least move forward. |
Beta Was this translation helpful? Give feedback.
-
|
The solution proposed by @trijpstra-fourlights is much more in line with what I would like to achieve as a maintainer. Modifying the existing interface instead of removing functionality is what I would like recommend. I do believe there is a way to achieve what @vejja is recommending without removing functionality and causinng breaking changes for our users and I think that the solution shown by @trijpstra-fourlights can do this thing. I am up for doing such changes for the upcoming release candidate versions and eventually the stable 1.0.0 version. |
Beta Was this translation helpful? Give feedback.
-
|
While working on the proposed PR, I've come to the conclusion that @vejja was completely right in his criticism and analysis and I completely wrong. My sincere apologies to you @vejja for not exploring your findings more thorough until now. I've created a PR which implements your proposed fixes. As to impact, I concur with @vejja 's observations that not many people will be using these so-called "advance features". And if they are, they should be actively resolving them. PR: #256 |
Beta Was this translation helpful? Give feedback.
-
|
Ok, roadmap here looks good to me and I'll contribute as set out. |
Beta Was this translation helpful? Give feedback.
-
|
@trijpstra-fourlights Please do not apologize, you did an absolutely amazing job and you deserve full credit for it. |
Beta Was this translation helpful? Give feedback.
-
|
I do have a POC now with a stackblitz here Update: You can also now test the POC live on Vercel here: https://nuxt-security-sraffray.vercel.app/ This is based on https://github.com/vejja/nuxt-security/tree/poc/strict-csp-nocookie The POC shows that you can load external scripts and stylesheets, and inject inline scripts and stylesheets with strict CSP |
Beta Was this translation helpful? Give feedback.
-
|
@Baroshem I think that based on this POC we can now close this discussion. |
Beta Was this translation helpful? Give feedback.
-
|
@vejja ok, I will close it then. Thanks for everything. |
Beta Was this translation helpful? Give feedback.
-
Hi @Baroshem
I am planning to propose a PR introducing SRI (sub resource integrity) injection.
This will also allow support of CSP 'strict-dynamic' policy for SSG.
I still have a few things to finalize but I already did the bulk of the work.
Can you please have a look at my wip code at : https://github.com/vejja/nuxt-security ?
It would be very helpful to have your feedback on my fork before issuing the final PR.
Many thanks
Beta Was this translation helpful? Give feedback.
All reactions