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

Ability to pass JavaScript nonce via RenderIncludes() #393

Closed
i4rilu opened this issue Jun 12, 2019 · 5 comments · Fixed by #465
Closed

Ability to pass JavaScript nonce via RenderIncludes() #393

i4rilu opened this issue Jun 12, 2019 · 5 comments · Fixed by #465

Comments

@i4rilu
Copy link

i4rilu commented Jun 12, 2019

Please implement an ability to pass nonce (random string - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script) via MiniProfiler.Current.RenderIncludes() method.

This would allow to apply Content-Security-Policy rules on the website and white-list MiniProfiler includes.min.js JavaScript as a safe one.

@NickCraver
Copy link
Member

Yeah we could add this - just so I'm clear - is this asking for a MiniProfiler.Current?.RenderIncludes(Nonce: "mynonce") setup? If not, could you clarify with an example.

I'm also what @vcsjones thinks on adoption here :)

@i4rilu
Copy link
Author

i4rilu commented Jun 12, 2019

Yes, that's correct.
Additional parameter for MiniProfilerWebExtensions.RenderIncludes() extension method.

@vcsjones
Copy link
Contributor

That seems useful. Would need to make sure that there is no caching of the script tags with the nonce. Perhaps another option to think about would be a factory somewhere, maybe on MiniProfilerBaseOptions that looks something like this:

public Func<HttpContext, string> CspNonceFactory { get; set; }

So that you only need to configure it once. It takes an HttpContext since there needs to be some coordination of the nonce between the script tag and whatever renders the CSP header. Just thinking out-loud here though.

I'm also what @vcsjones thinks on adoption here :)

Nonces are mostly well supported by everything except IE. Edge (the old Edge, not Edgium) does not support nonces on scripts that use src, only on inline scripts, but I think that's OK here.

@NickCraver
Copy link
Member

I'm taking a peek at this now - it'd have to be on the per-provider options which makes things a bit more complicated. That's because HttpContext differs between ASP.NET and ASP.NET Core and Render lives in the shared lib. But, adding a parameter is also a breaking change...so we'll need some overloads here to handle it. I'm debating if we're getting too many parameters in there, but allocating an object every time isn't great either. Going to ponder options here for a hot second before adding more overloads (but that's likely most practical at the moment).

NickCraver pushed a commit that referenced this issue Apr 4, 2020
Fix for #393, allows passing a nonce through the new `RenderOptions` API added in #451.
@NickCraver NickCraver linked a pull request Apr 4, 2020 that will close this issue
@NickCraver
Copy link
Member

Hey all, sorry this lingered - how to version the RenderIncludes API was the vexing piece and I needed to consider a few more additions to it and what things collectively looked like.

After talking with @mgravell, we decided that the small class allocation on call is okay enough in the scheme of things - it's 1 class allocation and not a big one. I'll likely optimize the writing path in ASP.NET Core specifically as a follow-up (to eliminate the string allocation).

Can y'all please see if #465 does what you want here? Now that the blocker is gone, I'd like to merge this and dogfood on Stack Overflow this week before doing a NuGet release.

NickCraver added a commit that referenced this issue Apr 4, 2020
Fix for #393, allows passing a nonce through the new `RenderOptions` API added in #451.

Also a minor optimization for async...we don't need that attribute value for any browser that'll support us today.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants