Conversation
ghost
added
the
SteveSandersonMS
changed the title
| var originalEncoder = _htmlEncoder; | ||
| try | ||
| { | ||
| _htmlEncoder = _nullHtmlEncoder; |
There was a problem hiding this comment.
There is a JavaScript encoder that we must use in these cases.
| // Script tags are special. Unlike in XHTML, the contents of script elements in HTML | ||
| // are parsed as plain text, so HTML entities are not recognized and are treated as | ||
| // literal text. Code like console.log("Hello") would be a syntax error. | ||
| // HTML does not make use of <![CDATA[ ... ]]> either. So inside <script>...</script>, | ||
| // we must disable HTML encoding. This is hardly a XSS issue since if an application | ||
| // is rendering user content inside <script>, they already have an XSS problem unless | ||
| // they have carefully escaped the user content. | ||
| // https://stackoverflow.com/a/14781466 | ||
| // https://html.spec.whatwg.org/multipage/scripting.html#restrictions-for-contents-of-script-elements |
There was a problem hiding this comment.
I don't think security wise this is enough guarantee, as per the comment above, we should use the JS encoder for this. See
There was a problem hiding this comment.
JavaScriptEncoder produces broken output in this scenario, e.g,:
<script>alert(\u0027The apostrophes must not be encoded...
The \u0027 is not valid there and is a syntax error.
There was a problem hiding this comment.
So after more investigation, if our goal is to be consistent with .cshtml, then it's best to understand this as a Razor compiler limitation: dotnet/razor#9204. We would need a compiler change to be able to know when it's safe to emit some unencoded content into <script> and when it is not.
If that is the case, then I think that means we have to not support it in the short term. So my proposed plan is:
- Keep the parts of this PR that make it possible to add
<script>tags at all - Remove the parts that deal with HTML encoding, so it will only work for
<script src='someUrl'></script>and<script>@(new MarkupString("myscript"))</script> - In the future, maybe Unable to render script elements with content from .razor files razor#9204 will get fixed and then it would automatically just start working for
<script>code</script>as well (as long ascodeis fixed at compile time, and does not include C# expressions).
There was a problem hiding this comment.
OK, with a bit of extra insight from dotnet/razor#9204, I think we can do a bit better than the above, and can actually make everything work with the correct encodings [*]
@javiercn's original suggestion to use JavaScriptEncoder was correct. It doesn't break cases where the developer's compile-time constant code gets compiled as AddMarkupContent, since that's emitted verbatim. All it does is force any dynamic expressions to be JavaScript-string encoded, which is correct and equivalent to the HTML-encoding that we enforce for dynamic expressions outside <script> contexts.
So altogether I think this PR is ready now.
[*] Until dotnet/razor#9204 is fixed, single-line <script> blocks will have too much encoding (even compile-time constant code will get JavaScript encoded), which is safe but prevents the code from executing. Developers can work around this by adding at least one linebreak. The issue will go away when that Razor compiler issue is fixed.
SteveSandersonMS
force-pushed
the
stevesa/fix-scripts-in-dynamic-content
branch
from
957031d to
e973b81
Compare
3 participants
Fixes #50424. See that issue for details.
The solution here involves several elements:
slotchange. This is nice anyway as the updated implementation is considerably simpler and less reliant on hard-to-reason-about behaviors and timings.<script>tags properly fromStaticHtmlRenderer. Previously we were using HTML encoding wrongly, so JS code would have been broken anyway.