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

Add a nonce option and Render testing #465

Merged
merged 3 commits into from Apr 4, 2020
Merged

Add a nonce option and Render testing #465

merged 3 commits into from Apr 4, 2020

Conversation

NickCraver
Copy link
Member

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

Fix for #393, allows passing a nonce through the new `RenderOptions` API added in #451.
@NickCraver NickCraver linked an issue Apr 4, 2020 that may be closed by this pull request
...and go ahead and update release notes in general.
@@ -46,7 +46,7 @@
@RenderSection("scripts", required: false)

@* Simple options are exposed...or make a full options class for customizing. *@
<mini-profiler position="@RenderPosition.Right" max-traces="5" color-scheme="ColorScheme.Auto" />
<mini-profiler position="@RenderPosition.Right" max-traces="5" color-scheme="ColorScheme.Auto" nonce="45" />
Copy link
Contributor

@vcsjones vcsjones Apr 4, 2020

Choose a reason for hiding this comment

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

Given that nonce is supposed to be random on every page load, I'm not sure it makes sense to have it as an attribute on mini-profiler? Would the intention instead be to use something like @NonceSource.GetNonce()?

As far as documentation / samples go, what is the story for "I want to inform miniprofiler about a nonce that comes from some piece of middleware?" The middleware shoves it in the context somewhere and it gets pulled in here?

Copy link
Member Author

@NickCraver NickCraver Apr 4, 2020

Choose a reason for hiding this comment

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

Correct - you'd pass in a unique value. Even putting say a <Func<HttpContext, string> is complicated due to the different primitives with .NET vs. .NET Core...maybe when we can drop .NET Full Framework support one day that gets easier since there's no common ancestor need.

Copy link
Member Author

@NickCraver NickCraver Apr 4, 2020

Choose a reason for hiding this comment

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

Oh sorry misread the second part - there's no built-in middleware story I can think of that makes sense here. The problem is generally you want MiniProfiler to profile the things you're doing and we have an order inversion problems with dependencies about the middleware doing it. Either direction we choose would be wrong for some subset of users. At least in v1, I'd want to leave it up to the users to handle their nonce generation.

@@ -82,6 +82,10 @@ public static class Render
{
sb.Append(" data-start-hidden=\"true\"");
}
if (renderOptions?.Nonce.HasValue() ?? false)
{
sb.Append(" nonce=\"").Append(renderOptions.Nonce).Append("\"");
Copy link
Contributor

@vcsjones vcsjones Apr 4, 2020

Choose a reason for hiding this comment

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

Since I see that sb contains other markup elements, (line 114) it is presumed that it's contents are HtmlSafe? Is there any concern that Nonce is an XSS point, or at least requires some escaping?

Alternatively, what about simply rendering the incorrect thing? If a nonce is supposed to be random and someone's nonce generator uses the printable ascii range (0x20-0x7F), &, ", etc. could feasibly end up breaking rendering if this isn't escaping like I am concerned about.

Is it assumed that the caller will do the escaping for miniprofiler?

Copy link
Member Author

@NickCraver NickCraver Apr 4, 2020

Choose a reason for hiding this comment

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

Doh, absolutely - this was the first text thing a user can pass and definitely needs encoding, total goof. Updated (and added a few tests for these cases).

This is a user-passed string, do the right thing here.
@NickCraver
Copy link
Member Author

@NickCraver NickCraver commented Apr 4, 2020

@vcsjones if time allows, mind taking one more peek please?

@NickCraver
Copy link
Member Author

@NickCraver NickCraver commented Apr 4, 2020

@vcsjones Thanks!

@NickCraver NickCraver merged commit b5313cc into master Apr 4, 2020
3 checks passed
@NickCraver NickCraver deleted the craver/js-nonce branch Apr 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants