Drop control characters from attribute values, and check attribute names

CHANGELOG.md

@@ -8,6 +8,19 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## Unreleased
 
+### Fixed
+
+- **An attribute value can no longer carry a control character.** `HtmlAttribute` encoded `&`, `"`,
+  `<` and `>` and passed everything else through, while `HtmlMarkup.TryParseAttributes` decodes
+  entities — so a value written as `x&#10;&#27;[31my` became a raw newline and a live ESC inside the
+  tag. Under MXP's secure-line framing the newline split the line mid-tag and the rest reached the
+  player as text; on a terminal the ESC was obeyed. Controls are now dropped, as
+  `MarkupTextRenderer` already dropped them from body text. The same applies to a link's own
+  attributes, and to an OSC 8 target, where a BEL ended the sequence early.
+- **`HtmlAttribute` checks its name.** `ToString()` is public, so a name holding a space or a quote
+  wrote a second attribute out of one. It now throws `ArgumentException`, as `HtmlMarkup.Tag`
+  already did.
+
 ### Added
 
 - **Checked `HtmlMarkup` construction.** `HtmlMarkup.Tag(name, params attributes)` validates the

MarkupString.Ansi/Emitters/AnsiEmitterSupport.cs

@@ -102,6 +102,26 @@ internal static void WriteWrapped(
 	/// only navigate, so a command link — and any URL with a scheme
 	/// <see cref="UrlSafety.IsSafeNavigableUrl"/> rejects — is written as plain text.
 	/// </summary>
+	/// <summary>
+	/// A link's URL, hint or text as it is written into an attribute or an escape sequence: HTML-encoded,
+	/// with control characters dropped. A newline would end the line the tag is on — under MXP's
+	/// secure-line framing, the rest of the tag then reaches the player as text — and a BEL or ESC would
+	/// end an OSC 8 sequence early and put the remainder on the terminal as escapes.
+	/// </summary>
+	internal static string EncodeAttribute(string value)
+	{
+		var encoded = WebUtility.HtmlEncode(value);
+		return encoded.AsSpan().ContainsAnyInRange('\u0000', '\u001f') || encoded.Contains('\u007f')
+			? new string([.. encoded.Where(c => !char.IsControl(c))])
+			: encoded;
+	}
+
+	/// <summary>An OSC 8 target, which is not HTML: only the control characters that would end the sequence go.</summary>
+	internal static string EncodeOsc8(string url) =>
+		url.AsSpan().ContainsAnyInRange('\u0000', '\u001f') || url.Contains('\u007f')
+			? new string([.. url.Where(c => !char.IsControl(c))])
+			: url;
+
 	internal static void WriteHyperlinked(in AnsiStyle style, ReadOnlySpan<char> body, IBufferWriter<char> output)
 	{
 		if (style.LinkKind != LinkKind.Url
@@ -113,7 +133,7 @@ internal static void WriteHyperlinked(in AnsiStyle style, ReadOnlySpan<char> bod
 		}
 
 		output.Write(Osc8);
-		output.Write(url);
+		output.Write(EncodeOsc8(url));
 		output.Write(Bel);
 		output.Write(body);
 		output.Write(Osc8);
@@ -171,12 +191,12 @@ private static void WriteTaggedLink(
 				: ("<A XCH_CMD=\"", " XCH_HINT=\"", "</A>");
 
 			output.Write(open);
-			output.Write(WebUtility.HtmlEncode(url));
+			output.Write(EncodeAttribute(url));
 			output.Write("\"");
 			if (style.LinkText is { Length: > 0 } text)
 			{
 				output.Write(hint);
-				output.Write(WebUtility.HtmlEncode(text));
+				output.Write(EncodeAttribute(text));
 				output.Write("\"");
 			}
 			output.Write(">");
@@ -192,7 +212,7 @@ private static void WriteTaggedLink(
 		}
 
 		output.Write("<A HREF=\"");
-		output.Write(WebUtility.HtmlEncode(url));
+		output.Write(EncodeAttribute(url));
 		output.Write("\">");
 		output.Write(body);
 		output.Write("</A>");

MarkupString.Ansi/Emitters/AnsiHtmlEmitter.cs

@@ -123,13 +123,13 @@ private static void WriteAnchored(in AnsiStyle style, ReadOnlySpan<char> body, I
 		if (style.LinkKind == LinkKind.Command)
 		{
 			output.Write("<a class=\"ms-cmd-link\" role=\"button\" tabindex=\"0\" xch_cmd=\"");
-			output.Write(WebUtility.HtmlEncode(url));
+			output.Write(AnsiEmitterSupport.EncodeAttribute(url));
 			output.Write("\"");
 		}
 		else if (UrlSafety.IsSafeNavigableUrl(url))
 		{
 			output.Write("<a href=\"");
-			output.Write(WebUtility.HtmlEncode(url));
+			output.Write(AnsiEmitterSupport.EncodeAttribute(url));
 			output.Write("\" target=\"_blank\" rel=\"noopener noreferrer\"");
 		}
 		else
@@ -141,7 +141,7 @@ private static void WriteAnchored(in AnsiStyle style, ReadOnlySpan<char> body, I
 		if (style.LinkText is { Length: > 0 } hint)
 		{
 			output.Write(" title=\"");
-			output.Write(WebUtility.HtmlEncode(hint));
+			output.Write(AnsiEmitterSupport.EncodeAttribute(hint));
 			output.Write("\"");
 		}
 

MarkupString.Html/HtmlAttribute.cs

@@ -1,24 +1,75 @@
+using System.Text;
 namespace MarkupString.Html;
 
 /// <summary>
 /// One attribute of an <see cref="HtmlMarkup"/> tag: a name and its unencoded value. It is written
-/// as <c>name="value"</c> with the value encoded, so a quote or a <c>&gt;</c> in it cannot end the
-/// attribute or the tag.
+/// as <c>name="value"</c> with the value encoded, so nothing in it can end the attribute, the tag
+/// or the line.
 /// </summary>
-/// <param name="Name">The attribute name, checked by <see cref="HtmlMarkup.IsValidAttributeName"/> wherever it is written.</param>
+/// <param name="Name">The attribute name, which must pass <see cref="HtmlMarkup.IsValidAttributeName"/>.</param>
 /// <param name="Value">The value as it should read after decoding; the empty string for a bare attribute.</param>
+/// <exception cref="ArgumentException"><paramref name="Name"/> is not an attribute name.</exception>
 public readonly record struct HtmlAttribute(string Name, string Value)
 {
+	/// <summary>The four characters that are markup inside a double-quoted attribute value.</summary>
+	private const string Significant = "&\"<>";
+
+	/// <summary>
+	/// Whether <paramref name="value"/> holds anything this has to rewrite: one of
+	/// <see cref="Significant"/>, or a control character. A newline in a value would end the line the tag
+	/// sits on — in MXP, a line in secure mode whose successor is not — and an ESC would open an escape
+	/// sequence inside the tag. <see cref="MarkupTextRenderer"/> drops controls from body text; a value
+	/// is no different, and a value read out of somewhere untrusted is where they arrive.
+	/// </summary>
+	private static bool NeedsEncoding(ReadOnlySpan<char> value) =>
+		value.IndexOfAny(Significant) >= 0
+		|| value.ContainsAnyInRange('\u0000', '\u001f')
+		|| value.Contains('\u007f');
+
+	/// <summary>The attribute name, which must pass <see cref="HtmlMarkup.IsValidAttributeName"/>.</summary>
+	/// <remarks>
+	/// Checked here rather than only in <see cref="HtmlMarkup.Tag"/>: <see cref="ToString"/> is public, and
+	/// a name holding a space or a quote would write a second attribute out of one.
+	/// </remarks>
+	public string Name { get; init => field = Validated(value); } = Validated(Name);
+
+	/// <exception cref="ArgumentException"><paramref name="name"/> is not an attribute name.</exception>
+	private static string Validated(string? name) =>
+		name is null || HtmlMarkup.IsValidAttributeName(name)
+			? name!
+			: throw new ArgumentException($"'{name}' is not an attribute name.", nameof(name));
+
 	/// <summary>The attribute as written in a tag: <c>name="value"</c>, the value encoded.</summary>
 	public override string ToString() => $"{Name}=\"{Encode(Value)}\"";
 
 	/// <summary>
-	/// Encodes the four characters that matter inside a double-quoted attribute value: <c>&amp;</c>,
-	/// <c>"</c>, <c>&lt;</c> and <c>&gt;</c>. Nothing else is touched — a numeric entity for an
-	/// accented letter is one more thing an MXP or Pueblo client may not decode.
+	/// Encodes the four characters that are markup inside a double-quoted attribute value —
+	/// <c>&amp;</c>, <c>"</c>, <c>&lt;</c> and <c>&gt;</c> — and drops control characters. Nothing else
+	/// is touched: a numeric entity for an accented letter is one more thing an MXP or Pueblo client may
+	/// not decode.
 	/// </summary>
-	internal static string Encode(string value) =>
-		value.AsSpan().IndexOfAny("&\"<>") < 0
-			? value
-			: value.Replace("&", "&amp;").Replace("\"", "&quot;").Replace("<", "&lt;").Replace(">", "&gt;");
+	internal static string Encode(string? value)
+	{
+		if (string.IsNullOrEmpty(value)) return string.Empty;
+
+		var text = value.AsSpan();
+		if (!NeedsEncoding(text)) return value;
+
+		var written = new StringBuilder(value.Length + 16);
+		foreach (var c in text)
+		{
+			switch (c)
+			{
+				case '&': written.Append("&amp;"); break;
+				case '"': written.Append("&quot;"); break;
+				case '<': written.Append("&lt;"); break;
+				case '>': written.Append("&gt;"); break;
+				default:
+					if (!char.IsControl(c)) written.Append(c);
+					break;
+			}
+		}
+
+		return written.ToString();
+	}
 }

MarkupString.Tests/Ansi/AnsiRenderTests.cs

@@ -375,6 +375,30 @@ public async Task Plain_StripsEveryStyle()
 		await Assert.That(Render(text, MarkupFormat.Plain)).IsEqualTo("ab");
 	}
 
+	/// <summary>
+	/// A link's own attributes cannot end the line the tag is on. Under MXP's secure-line framing a
+	/// newline in a hint would leave the rest of the tag on a line that is no longer in secure mode, and
+	/// the player reads it as text; a BEL or ESC in a URL would end an OSC 8 sequence early.
+	/// </summary>
+	[Test]
+	public async Task ALinksAttributesCannotCarryAControlCharacter()
+	{
+		var link = MarkupText.Wrap(
+			AnsiMarkup.Create(linkUrl: "look", linkKind: LinkKind.Command, linkText: "a\nb"), "click");
+
+		await Assert.That(Render(link, MarkupFormat.Mxp)).IsEqualTo("<SEND HREF=\"look\" HINT=\"ab\">click</SEND>");
+		await Assert.That(Render(link, MarkupFormat.Pueblo)).IsEqualTo("<A XCH_CMD=\"look\" XCH_HINT=\"ab\">click</A>");
+		await Assert.That(Render(link, MarkupFormat.Html)).Contains("title=\"ab\"");
+
+		var url = MarkupText.Wrap(AnsiMarkup.Create(linkUrl: "https://example.com/\a\u001b[2J"), "click");
+		var rendered = Render(url, MarkupFormat.Ansi);
+
+		await Assert.That(rendered).DoesNotContain("\a\u001b")
+			.Because("a BEL in the target would end the OSC 8 sequence and leave an ESC on the terminal");
+		await Assert.That(rendered).Contains($"{Esc}]8;;https://example.com/[2J\a")
+			.Because("the control characters are dropped; what was between them is inert text");
+	}
+
 	// ── Safety ───────────────────────────────────────────────────────────────────
 
 	[Test]

MarkupString.Tests/Html/HtmlTagPolicyTests.cs

@@ -24,6 +24,30 @@ public async Task Tag_EncodesEveryValue()
 		await Assert.That(Html(markup)).IsEqualTo("<font color=\"red&quot;&gt;&lt;script&gt;&amp;\">x</font>");
 	}
 
+	/// <summary>
+	/// A value cannot end the line the tag is on. Entities decode to what a client reads, and a
+	/// <c>&amp;#10;</c> or <c>&amp;#27;</c> in an attribute would otherwise put a raw newline or ESC inside
+	/// the tag — splitting the line under MXP's secure-line framing, and writing an escape sequence a
+	/// terminal obeys.
+	/// </summary>
+	[Test]
+	public async Task AValueCannotCarryAControlCharacter()
+	{
+		await Assert.That(HtmlTagPolicy.WellFormed.TryCreate("a", "title=\"x&#10;&#13;&#27;[31my\"", out var parsed)).IsTrue();
+		await Assert.That(parsed!.Attributes).IsEqualTo("title=\"x[31my\"");
+
+		var built = HtmlMarkup.Tag("a", new HtmlAttribute("title", "x\n\u001b[31my"));
+		await Assert.That(built.Attributes).IsEqualTo("title=\"x[31my\"");
+		await Assert.That(Html(built, "body")).DoesNotContain("\n");
+	}
+
+	[Test]
+	public async Task AnAttributeNameIsCheckedWhereverOneIsBuilt()
+	{
+		await Assert.That(() => new HtmlAttribute("href=x onclick", "1")).Throws<ArgumentException>()
+			.Because("ToString() is public, and that name would write a second attribute out of one");
+	}
+
 	[Test]
 	public async Task Tag_LeavesAccentedTextAlone()
 	{