Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Correct HTML and JavaScript encoding of <link> and <script> attribute values #4272

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/Microsoft.AspNetCore.Mvc.TagHelpers/LinkTagHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,12 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Text;
using System.Text.Encodings.Web;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc.Razor.TagHelpers;
using Microsoft.AspNetCore.Mvc.Rendering;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Mvc.TagHelpers.Internal;
using Microsoft.AspNetCore.Mvc.ViewFeatures;
using Microsoft.AspNetCore.Razor.TagHelpers;
using Microsoft.Extensions.Caching.Memory;

Expand Down Expand Up @@ -362,15 +360,21 @@ private void AppendFallbackHrefs(TagHelperContent builder, IReadOnlyList<string>
firstAdded = true;
}

// fallbackHrefs come from bound attributes and globbing. Must always be non-null.
// fallbackHrefs come from bound attributes (a C# context) and globbing. Must always be non-null.
Debug.Assert(fallbackHrefs[i] != null);

var valueToWrite = fallbackHrefs[i];
if (AppendVersion == true)
{
valueToWrite = _fileVersionProvider.AddFileVersionToPath(fallbackHrefs[i]);
}

builder.AppendHtml(JavaScriptEncoder.Encode(valueToWrite));
// Must HTML-encode the href attribute value to ensure the written <link/> element is valid. Must also
// JavaScript-encode that value to ensure the doc.write() statement is valid.
valueToWrite = HtmlEncoder.Encode(valueToWrite);
valueToWrite = JavaScriptEncoder.Encode(valueToWrite);

builder.AppendHtml(valueToWrite);
builder.AppendHtml("\"");
}
builder.AppendHtml("]);");
Expand Down
55 changes: 54 additions & 1 deletion src/Microsoft.AspNetCore.Mvc.TagHelpers/ScriptTagHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@

using System;
using System.Diagnostics;
using System.IO;
using System.Text.Encodings.Web;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Html;
using Microsoft.AspNetCore.Mvc.Razor;
using Microsoft.AspNetCore.Mvc.Razor.TagHelpers;
using Microsoft.AspNetCore.Mvc.Rendering;
using Microsoft.AspNetCore.Mvc.Routing;
Expand Down Expand Up @@ -39,6 +42,7 @@ public class ScriptTagHelper : UrlResolutionTagHelper
private const string AppendVersionAttributeName = "asp-append-version";
private static readonly Func<Mode, Mode, int> Compare = (a, b) => a - b;
private FileVersionProvider _fileVersionProvider;
private StringWriter _stringWriter;

private static readonly ModeAttributes<Mode>[] ModeDetails = new[] {
// Regular src with file version alone
Expand Down Expand Up @@ -174,6 +178,20 @@ public override int Order
// Internal for ease of use when testing.
protected internal GlobbingUrlBuilder GlobbingUrlBuilder { get; set; }

// Shared writer for determining the string content of a TagHelperAttribute's Value.
private StringWriter StringWriter
{
get
{
if (_stringWriter == null)
{
_stringWriter = new StringWriter();
}

return _stringWriter;
}
}

/// <inheritdoc />
public override void Process(TagHelperContext context, TagHelperOutput output)
{
Expand Down Expand Up @@ -300,7 +318,7 @@ private void BuildFallbackBlock(TagHelperAttributeList attributes, DefaultTagHel
if (!attribute.Name.Equals(SrcAttributeName, StringComparison.OrdinalIgnoreCase))
{
var encodedKey = JavaScriptEncoder.Encode(attribute.Name);
var attributeValue = attribute.Value.ToString();
var attributeValue = GetAttributeValue(attribute.Value);
var encodedValue = JavaScriptEncoder.Encode(attributeValue);

AppendAttribute(builder, encodedKey, encodedValue, escapeQuotes: true);
Expand All @@ -324,6 +342,37 @@ private void BuildFallbackBlock(TagHelperAttributeList attributes, DefaultTagHel
}
}

private string GetAttributeValue(object value)
{
string stringValue;
var htmlEncodedString = value as HtmlEncodedString;
if (htmlEncodedString != null)
{
// Value likely came from an HTML context in the .cshtml file but may still contain double quotes
// since attribute could have been enclosed in single quotes.
stringValue = htmlEncodedString.Value;
if (stringValue.Contains("\""))
Copy link
Contributor

Choose a reason for hiding this comment

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

This check shouldn't be required. Calling stringValue.Replace would no-op if it doesn't have a quote.

Copy link
Member Author

Choose a reason for hiding this comment

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

👌 double-checked the native source and confirmed that.

{
stringValue = stringValue.Replace("\"", "&quot;");
}
}
else
{
var writer = StringWriter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the property directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈

RazorPage.WriteTo(writer, HtmlEncoder, value);
Copy link
Member Author

Choose a reason for hiding this comment

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

This static method isn't fitting so well in RazorPage but I haven't got a better place to throw it. Will leave as-is unless someone has a good suggestion.


// Value is now correctly HTML-encoded but may still contain double quotes since attribute could
// have been enclosed in single quotes and portions that were HtmlEncodedStrings are not re-encoded.
var builder = writer.GetStringBuilder();
builder.Replace("\"", "&quot;");

stringValue = builder.ToString();
builder.Clear();
}

return stringValue;
}

private void AppendEncodedVersionedSrc(
string srcName,
string srcValue,
Expand All @@ -337,6 +386,10 @@ private void AppendEncodedVersionedSrc(

if (generateForDocumentWrite)
{
// srcValue comes from a C# context and globbing. Must HTML-encode it to ensure the
// written <script/> element is valid. Must also JavaScript-encode that value to ensure
// the document.write() statement is valid.
srcValue = HtmlEncoder.Encode(srcValue);
srcValue = JavaScriptEncoder.Encode(srcValue);
}

Expand Down
Loading