MiniProfiler / dotnet

A simple but effective mini-profiler for ASP.NET (and Core) websites
https://miniprofiler.com/dotnet/
MIT License
2.93k stars 603 forks source link

Eliminate the use of inline styles, making it compatible with strict CSP for style. #634

Closed rwasef1830 closed 1 year ago

rwasef1830 commented 1 year ago

Dear MiniProfiler team, Thank you for making this project easy to build and fork. With this PR MiniProfiler will run under a strict CSP that disallows inline styles and scripts (by using nonce) with zero errors.

It accomplish this by putting dynamically generated style tag values as data attributes, and then later after appending the miniprofiler html, it queries for them and manipulates the style object on Element directly, thus eliminating the need for inline style.

NickCraver commented 1 year ago

Hey there - and thanks for this! The approach though will cause a redraw which we wouldn't want especially for a heavy page, is there any other way around the policy you're setting? I think if doing this, we'd want to make it opt-in for the performance impact if possible - thoughts?

rwasef1830 commented 1 year ago

Unfortunately due to the inline styles being dynamically generated by concatenating pixel widths, it is impossible to workaround using even unsafe-hashes. The big issue I see actually is that the script relies on HTML string concatenation instead of manipulating the dom directly and adding elements node by node.

I don't know another way, but I was pretty much forced to do it like this.

The nature of applications I am working on prohibit the allow of inline script and style completely. A second possible workaround would be the use of the attr CSS function to set the value of a CSS variable from an attribute, and then using the CSS file set the padding and margin using this variable, but if I remember correctly, this is not well supported.

https://caniuse.com/?search=attr

While it maybe possible to somehow enable inline style only when MiniProfiler is running, this will create a situation where the developer is running in a different permission environment than the actual live system which will lead to slipping of inline styles and introduction of bugs, and thus is not a viable (productive) workaround.

The redraws might be possible to eliminate if the miniprofiler HTML is set on a non-rendered element first, then the styles are manipulated, and then finally inserted into the actual visible container, do you think that would solve the redraw problem ?

rwasef1830 commented 1 year ago

@NickCraver would this latest change address the redraw issue or mitigate it ?

rwasef1830 commented 1 year ago

I have tested this on many heavy pages, and with the mitigation I have applied (using document.createElement to created a detached element, manipulate that then insert the nodes directly (not as html) to their final parent) I don't have any redraw issues.

NickCraver commented 1 year ago

@rwasef1830 Sorry I've been swamped at work, going to try and spin this up the next few days (took some time off to catch up on things) - trying to get everything updated for builds to pass here first so we have better checks running in #637.

rwasef1830 commented 1 year ago

Miniprofiler list page also needs work, uses inline stuff.

rwasef1830 commented 1 year ago

@NickCraver This PR should be complete now (rebased on main). I introduced a new NonceGetter property on MiniProfilerOptions that allows the consumer to set the value per request to use for the nonce. This value is also used as a fallback for the tag helper if the tag helper property is not set.

The "share" full result page is now working with no CSP errors.

Breaking changes: On public API: none On "Internal" namespace API: some of the static methods.

Requesting review, feedback and hopefully a merge after everything is made whole.

PhenX commented 1 year ago

Hello, @NickCraver do you have any ETA for a new release on NuGet.org with this change ? Thanks