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

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

Merged
merged 12 commits into from
Aug 3, 2023

Conversation

rwasef1830
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

rwasef1830 commented Jan 28, 2023

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
Copy link
Contributor Author

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

@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Jan 30, 2023

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
Copy link
Member

@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.

@NickCraver NickCraver self-assigned this Feb 9, 2023
@rwasef1830
Copy link
Contributor Author

Miniprofiler list page also needs work, uses inline stuff.

@rwasef1830
Copy link
Contributor Author

rwasef1830 commented Jun 15, 2023

@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.

Copy link
Member

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Made some additional changes to support all scenarios, thanks for all the work here! Apologies I haven't had time until vacation this week to get some OSS backlog done.

@NickCraver NickCraver merged commit 921601f into MiniProfiler:main Aug 3, 2023
1 check passed
@PhenX
Copy link

PhenX commented Nov 8, 2023

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

ferpaz pushed a commit to ferpaz/dotnet that referenced this pull request Jun 25, 2024
…CSP for style. (MiniProfiler#634)

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.

Co-authored-by: Nick Craver <nrcraver@gmail.com>
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.

3 participants