diff --git a/CHANGELOG.md b/CHANGELOG.md index 98093eb..1f830f6 100644 --- a/CHANGELOG.md +++ b/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 y` 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 diff --git a/MarkupString.Ansi/Emitters/AnsiEmitterSupport.cs b/MarkupString.Ansi/Emitters/AnsiEmitterSupport.cs index 56423fa..10eb028 100644 --- a/MarkupString.Ansi/Emitters/AnsiEmitterSupport.cs +++ b/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 /// rejects — is written as plain text. /// + /// + /// 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. + /// + 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; + } + + /// An OSC 8 target, which is not HTML: only the control characters that would end the sequence go. + 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 body, IBufferWriter output) { if (style.LinkKind != LinkKind.Url @@ -113,7 +133,7 @@ internal static void WriteHyperlinked(in AnsiStyle style, ReadOnlySpan 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( : (""); 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(""); output.Write(body); output.Write(""); diff --git a/MarkupString.Ansi/Emitters/AnsiHtmlEmitter.cs b/MarkupString.Ansi/Emitters/AnsiHtmlEmitter.cs index 87d6fef..3deb990 100644 --- a/MarkupString.Ansi/Emitters/AnsiHtmlEmitter.cs +++ b/MarkupString.Ansi/Emitters/AnsiHtmlEmitter.cs @@ -123,13 +123,13 @@ private static void WriteAnchored(in AnsiStyle style, ReadOnlySpan body, I if (style.LinkKind == LinkKind.Command) { output.Write(" body, I if (style.LinkText is { Length: > 0 } hint) { output.Write(" title=\""); - output.Write(WebUtility.HtmlEncode(hint)); + output.Write(AnsiEmitterSupport.EncodeAttribute(hint)); output.Write("\""); } diff --git a/MarkupString.Html/HtmlAttribute.cs b/MarkupString.Html/HtmlAttribute.cs index e7c9e53..8a48aba 100644 --- a/MarkupString.Html/HtmlAttribute.cs +++ b/MarkupString.Html/HtmlAttribute.cs @@ -1,24 +1,75 @@ +using System.Text; namespace MarkupString.Html; /// /// One attribute of an tag: a name and its unencoded value. It is written -/// as name="value" with the value encoded, so a quote or a > in it cannot end the -/// attribute or the tag. +/// as name="value" with the value encoded, so nothing in it can end the attribute, the tag +/// or the line. /// -/// The attribute name, checked by wherever it is written. +/// The attribute name, which must pass . /// The value as it should read after decoding; the empty string for a bare attribute. +/// is not an attribute name. public readonly record struct HtmlAttribute(string Name, string Value) { + /// The four characters that are markup inside a double-quoted attribute value. + private const string Significant = "&\"<>"; + + /// + /// Whether holds anything this has to rewrite: one of + /// , 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. drops controls from body text; a value + /// is no different, and a value read out of somewhere untrusted is where they arrive. + /// + private static bool NeedsEncoding(ReadOnlySpan value) => + value.IndexOfAny(Significant) >= 0 + || value.ContainsAnyInRange('\u0000', '\u001f') + || value.Contains('\u007f'); + + /// The attribute name, which must pass . + /// + /// Checked here rather than only in : is public, and + /// a name holding a space or a quote would write a second attribute out of one. + /// + public string Name { get; init => field = Validated(value); } = Validated(Name); + + /// is not an attribute name. + private static string Validated(string? name) => + name is null || HtmlMarkup.IsValidAttributeName(name) + ? name! + : throw new ArgumentException($"'{name}' is not an attribute name.", nameof(name)); + /// The attribute as written in a tag: name="value", the value encoded. public override string ToString() => $"{Name}=\"{Encode(Value)}\""; /// - /// Encodes the four characters that matter inside a double-quoted attribute value: &, - /// ", < and >. 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 — + /// &, ", < and > — 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. /// - internal static string Encode(string value) => - value.AsSpan().IndexOfAny("&\"<>") < 0 - ? value - : value.Replace("&", "&").Replace("\"", """).Replace("<", "<").Replace(">", ">"); + 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("&"); break; + case '"': written.Append("""); break; + case '<': written.Append("<"); break; + case '>': written.Append(">"); break; + default: + if (!char.IsControl(c)) written.Append(c); + break; + } + } + + return written.ToString(); + } } diff --git a/MarkupString.Tests/Ansi/AnsiRenderTests.cs b/MarkupString.Tests/Ansi/AnsiRenderTests.cs index e7a9d29..b81637c 100644 --- a/MarkupString.Tests/Ansi/AnsiRenderTests.cs +++ b/MarkupString.Tests/Ansi/AnsiRenderTests.cs @@ -375,6 +375,30 @@ public async Task Plain_StripsEveryStyle() await Assert.That(Render(text, MarkupFormat.Plain)).IsEqualTo("ab"); } + /// + /// 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. + /// + [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("click"); + await Assert.That(Render(link, MarkupFormat.Pueblo)).IsEqualTo("click"); + 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] diff --git a/MarkupString.Tests/Html/HtmlTagPolicyTests.cs b/MarkupString.Tests/Html/HtmlTagPolicyTests.cs index bc49b60..2570cf6 100644 --- a/MarkupString.Tests/Html/HtmlTagPolicyTests.cs +++ b/MarkupString.Tests/Html/HtmlTagPolicyTests.cs @@ -24,6 +24,30 @@ public async Task Tag_EncodesEveryValue() await Assert.That(Html(markup)).IsEqualTo("x"); } + /// + /// A value cannot end the line the tag is on. Entities decode to what a client reads, and a + /// &#10; or &#27; 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. + /// + [Test] + public async Task AValueCannotCarryAControlCharacter() + { + await Assert.That(HtmlTagPolicy.WellFormed.TryCreate("a", "title=\"x y\"", 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() + .Because("ToString() is public, and that name would write a second attribute out of one"); + } + [Test] public async Task Tag_LeavesAccentedTextAlone() {