-
Notifications
You must be signed in to change notification settings - Fork 604
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ public static string Includes( | |
var sb = StringBuilderCache.Get(); | ||
var options = profiler.Options; | ||
|
||
sb.Append("<script async=\"async\" id=\"mini-profiler\" src=\""); | ||
sb.Append("<script async id=\"mini-profiler\" src=\""); | ||
sb.Append(path); | ||
sb.Append("includes.min.js?v="); | ||
sb.Append(options.VersionHash); | ||
|
@@ -82,6 +82,10 @@ public static string Includes( | |
{ | ||
sb.Append(" data-start-hidden=\"true\""); | ||
} | ||
if (renderOptions?.Nonce.HasValue() ?? false) | ||
{ | ||
sb.Append(" nonce=\"").Append(renderOptions.Nonce).Append("\""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I see that 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), Is it assumed that the caller will do the escaping for miniprofiler? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
} | ||
|
||
sb.Append(" data-max-traces=\""); | ||
sb.Append((renderOptions?.MaxTracesToShow ?? options.PopupMaxTracesToShow).ToString(CultureInfo.InvariantCulture)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using StackExchange.Profiling.Internal; | ||
using Xunit; | ||
using Xunit.Abstractions; | ||
|
||
namespace StackExchange.Profiling.Tests | ||
{ | ||
public class RenderTests : BaseTest | ||
{ | ||
public RenderTests(ITestOutputHelper output) : base(output) { } | ||
|
||
[Fact] | ||
public void DefaultRender() | ||
{ | ||
var profiler = GetBasicProfiler(); | ||
var renderOptions = new RenderOptions(); | ||
var result = Render.Includes(profiler, "/", true, renderOptions, new List<Guid>() { profiler.Id }); | ||
Output.WriteLine("Result: " + result); | ||
|
||
Assert.NotNull(result); | ||
Assert.Contains("id=\"mini-profiler\"", result); | ||
|
||
var expected = $@"<script async id=""mini-profiler"" src=""/includes.min.js?v={Options.VersionHash}"" data-version=""{Options.VersionHash}"" data-path=""/"" data-current-id=""{profiler.Id}"" data-ids=""{profiler.Id}"" data-position=""Left"""" data-scheme=""Light"" data-authorized=""true"" data-max-traces=""15"" data-toggle-shortcut=""Alt+P"" data-trivial-milliseconds=""2.0"" data-ignored-duplicate-execute-types=""Open,OpenAsync,Close,CloseAsync""></script>"; | ||
Assert.Equal(expected, result); | ||
} | ||
|
||
[Fact] | ||
public void OptionsSet() | ||
{ | ||
var profiler = GetBasicProfiler(); | ||
var renderOptions = new RenderOptions() | ||
{ | ||
ColorScheme = ColorScheme.Auto, | ||
MaxTracesToShow = 12, | ||
Nonce = "myNonce", | ||
PopupToggleKeyboardShortcut = "Alt+Q", | ||
Position = RenderPosition.Right, | ||
ShowControls = true, | ||
ShowTimeWithChildren = true, | ||
ShowTrivial = true, | ||
StartHidden = true, | ||
TrivialDurationThresholdMilliseconds = 23 | ||
}; | ||
var result = Render.Includes(profiler, "/", true, renderOptions, new List<Guid>() { profiler.Id }); | ||
Output.WriteLine("Result: " + result); | ||
|
||
Assert.NotNull(result); | ||
Assert.Contains("id=\"mini-profiler\"", result); | ||
|
||
var expected = $@"<script async id=""mini-profiler"" src=""/includes.min.js?v={Options.VersionHash}"" data-version=""{Options.VersionHash}"" data-path=""/"" data-current-id=""{profiler.Id}"" data-ids=""{profiler.Id}"" data-position=""Right"""" data-scheme=""Auto"" data-authorized=""true"" data-trivial=""true"" data-children=""true"" data-controls=""true"" data-start-hidden=""true"" nonce=""myNonce"" data-max-traces=""12"" data-toggle-shortcut=""Alt+Q"" data-trivial-milliseconds=""23"" data-ignored-duplicate-execute-types=""Open,OpenAsync,Close,CloseAsync""></script>"; | ||
Assert.Equal(expected, result); | ||
} | ||
|
||
[Theory] | ||
[InlineData(null, @"data-scheme=""Light""")] | ||
[InlineData(ColorScheme.Auto, @"data-scheme=""Auto""")] | ||
[InlineData(ColorScheme.Dark, @"data-scheme=""Dark""")] | ||
[InlineData(ColorScheme.Light, @"data-scheme=""Light""")] | ||
public void ColorSchemes(ColorScheme? scheme, string expected) | ||
{ | ||
var profiler = GetBasicProfiler(); | ||
var renderOptions = new RenderOptions() { ColorScheme = scheme }; | ||
|
||
var result = Render.Includes(profiler, " / ", true, renderOptions); | ||
Output.WriteLine("Result: " + result); | ||
|
||
Assert.NotNull(result); | ||
Assert.Contains(expected, result); | ||
} | ||
|
||
[Theory] | ||
[InlineData(null, @"data-position=""Left""")] | ||
[InlineData(RenderPosition.BottomLeft, @"data-position=""BottomLeft""")] | ||
[InlineData(RenderPosition.BottomRight, @"data-position=""BottomRight""")] | ||
[InlineData(RenderPosition.Left, @"data-position=""Left""")] | ||
[InlineData(RenderPosition.Right, @"data-position=""Right""")] | ||
public void Positions(RenderPosition? position, string expected) | ||
{ | ||
var profiler = GetBasicProfiler(); | ||
var renderOptions = new RenderOptions() { Position = position }; | ||
|
||
var result = Render.Includes(profiler, " / ", true, renderOptions); | ||
Output.WriteLine("Result: " + result); | ||
|
||
Assert.NotNull(result); | ||
Assert.Contains(expected, result); | ||
} | ||
|
||
[Fact] | ||
public void Nonce() | ||
{ | ||
var profiler = GetBasicProfiler(); | ||
var renderOptions = new RenderOptions(); | ||
|
||
// Default | ||
var result = Render.Includes(profiler, "/", true, renderOptions); | ||
Output.WriteLine("Result: " + result); | ||
Assert.DoesNotContain("nonce", result); | ||
|
||
// With nonce | ||
var nonce = Guid.NewGuid().ToString(); | ||
renderOptions.Nonce = nonce; | ||
result = Render.Includes(profiler, "/", true, renderOptions); | ||
Assert.Contains($@"nonce=""{nonce}""", result); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.