From 8770f075ea8cbf1bc7e6721a9b3e7f8d8a5f8d3c Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 16:49:09 -0600 Subject: [PATCH 1/5] test: pin the sanitizer allowlist and resource-policy boundaries Widening the sanitizer's element, attribute or scheme allowlist left every test green, and resource-policy tests rejected their inputs for unrelated reasons, so removing a single check went unnoticed. Add an independent exact pin for each allowlist, single-component variations for origin, file type and forbidden characters, and killing tests for theme colours and labels, metadata escaping, per-property settings repair, schema approval and exact size limits. The fuzz corpus now generates the media, form and metadata elements its oracle forbids. Reserved Windows device names and forbidden characters become one written-out rule, WindowsFileNames, used by the resource policy and the theme export name. It covers every device name in any letter case, with any extension and with spaces before the extension, plus COM0, LPT0, CONIN$ and CONOUT$, and a double quote is refused like the other forbidden characters. Closes #48 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- docs/security/threat-model.md | 2 +- .../Documents/DocumentResourcePolicy.cs | 10 +- .../Themes/ThemeJson.cs | 5 +- .../WindowsFileNames.cs | 37 +++ .../DocumentResourcePolicyBoundaryTests.cs | 195 +++++++++++ .../SanitizerAllowlistTests.cs | 146 +++++++++ .../SecurityFuzzTests.cs | 7 +- .../UntrustedInputValidationTests.cs | 303 ++++++++++++++++++ .../WindowsFileNamesTests.cs | 107 +++++++ 9 files changed, 799 insertions(+), 13 deletions(-) create mode 100644 src/MarkdownViewr.Windows.Core/WindowsFileNames.cs create mode 100644 tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs create mode 100644 tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs create mode 100644 tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs create mode 100644 tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 37506ff..551daa2 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -43,7 +43,7 @@ physical directory. It does not authorize native commands or remote resource acc | Threat | Mitigation and evidence | | --- | --- | | Document script or native-command execution | Sanitizer regressions pass; host disables scripts/bridge; actual browser malicious-script and reader acceptance passed on 77dd04d | -| Image traversal, device/ADS paths or encoded separators | Core path-policy tests pass; denied before filesystem access | +| Image traversal, device/ADS paths or encoded separators | Core path-policy tests pass; denied before filesystem access. Reserved device names and forbidden characters are one written-out rule (`WindowsFileNames`), independent of the running Windows version: every device name in any letter case, with any extension, and with spaces before the extension | | Junction/symlink escape or check/reopen race | Check normalized physical path on opened image handle; read same handle; real junction rejection test passes | | Remote image tracking / resource exfiltration | No-referrer header, no document networking; actual listening-server browser denial passed on 77dd04d | | Oversized input / image denial of service | Pre-parse character bound and image byte bound; native limit tests pass; performance acceptance remains | diff --git a/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs b/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs index 7c9a983..5e2f51c 100644 --- a/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs +++ b/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs @@ -19,12 +19,6 @@ public sealed class DocumentResourcePolicy ".md", ".markdown", ".mdown", ".mkd", ".txt", }; - private static readonly HashSet s_deviceNames = new(StringComparer.OrdinalIgnoreCase) - { - "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", - "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", "COM¹", "COM²", "COM³", "LPT¹", "LPT²", "LPT³", - }; - private readonly string _prefix; @@ -93,7 +87,7 @@ public sealed class DocumentResourcePolicy private static bool IsSafeSegment(string segment) { return segment.Length > 0 && segment is not ("." or "..") && !segment.EndsWith(' ') && !segment.EndsWith('.') && - !segment.Any(character => char.IsControl(character) || "<>:\\|?*".Contains(character, StringComparison.Ordinal)) && - !s_deviceNames.Contains(segment.Split('.')[0]); + !segment.Any(character => char.IsControl(character) || WindowsFileNames.InvalidCharacters.Contains(character, StringComparison.Ordinal)) && + !WindowsFileNames.IsReservedDeviceName(segment); } } diff --git a/src/MarkdownViewr.Windows.Core/Themes/ThemeJson.cs b/src/MarkdownViewr.Windows.Core/Themes/ThemeJson.cs index 4430db3..2e3ad0e 100644 --- a/src/MarkdownViewr.Windows.Core/Themes/ThemeJson.cs +++ b/src/MarkdownViewr.Windows.Core/Themes/ThemeJson.cs @@ -33,14 +33,13 @@ public static class ThemeJson public static string SuggestedFileName(ReaderTheme theme) { ArgumentNullException.ThrowIfNull(theme); - string stem = string.Concat(theme.Name.Select(character => "<>:\"/\\|?*".Contains(character, StringComparison.Ordinal) ? '_' : character)) + string stem = string.Concat(theme.Name.Select(character => WindowsFileNames.InvalidCharacters.Contains(character, StringComparison.Ordinal) ? '_' : character)) .Replace(' ', '-').ToLowerInvariant().Trim('.', '-'); if(stem.Length == 0) { stem = "theme"; } - string device = stem.Split('.')[0]; - if(device is "con" or "prn" or "aux" or "nul" || device.Length == 4 && (device.StartsWith("com", StringComparison.Ordinal) || device.StartsWith("lpt", StringComparison.Ordinal)) && device[3] is >= '1' and <= '9' or '¹' or '²' or '³') + if(WindowsFileNames.IsReservedDeviceName(stem)) { stem = "theme-" + stem; } diff --git a/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs b/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs new file mode 100644 index 0000000..cc657c4 --- /dev/null +++ b/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs @@ -0,0 +1,37 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + + +namespace MarkdownViewr.Windows.Core; + +/// +/// The Windows file-name rules this application relies on, written out instead of asked of the +/// running platform: the characters a name cannot contain and the names that address a device. +/// +public static class WindowsFileNames +{ + /// Characters that cannot appear in a Windows file or folder name. + public const string InvalidCharacters = "<>:\"/\\|?*"; + + private static readonly HashSet s_devices = new(StringComparer.OrdinalIgnoreCase) + { + "CON", "PRN", "AUX", "NUL", "CONIN$", "CONOUT$", + "COM0", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "COM¹", "COM²", "COM³", + "LPT0", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", "LPT¹", "LPT²", "LPT³", + }; + + + + /// + /// Whether a single name addresses a device in any letter case. Windows ignores everything after + /// the first dot and any spaces before it, so nul.txt and COM1 .log are devices too. + /// + public static bool IsReservedDeviceName(string name) + { + ArgumentNullException.ThrowIfNull(name); + int dot = name.IndexOf('.', StringComparison.Ordinal); + return s_devices.Contains((dot < 0 ? name : name[..dot]).TrimEnd(' ')); + } +} diff --git a/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs new file mode 100644 index 0000000..8f81fd9 --- /dev/null +++ b/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs @@ -0,0 +1,195 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using MarkdownViewr.Windows.Core.Documents; + + +namespace MarkdownViewr.Windows.Core.Tests; + +/// +/// Each resource-policy check is exercised alone: a request that differs from a valid one in a +/// single component must be denied by that component's check, not by an unrelated one. +/// +public sealed class DocumentResourcePolicyBoundaryTests +{ + /// The valid baseline every single-component variation below is derived from. + [Fact] + public void Baseline_ValidRequests_AreAccepted() + { + var policy = new DocumentResourcePolicy(); + Assert.Equal("image.png", policy.GetImageRelativePath(Vary(policy, "image.png", _ => { }))); + Assert.Equal("child.md", policy.GetDocumentRelativePath(Vary(policy, "child.md", _ => { }))); + } + + + + /// Plain HTTP on the same host, port and path is not the document's origin. + [Fact] + public void OtherScheme_SameHostPortAndPath_IsDenied() + { + var policy = new DocumentResourcePolicy(); + Uri image = Vary(policy, "image.png", builder => + { + builder.Scheme = "http"; + builder.Port = 443; + }); + Assert.Equal(443, image.Port); + Assert.Null(policy.GetImageRelativePath(image)); + Assert.Null(policy.GetDocumentRelativePath(Vary(policy, "child.md", builder => + { + builder.Scheme = "http"; + builder.Port = 443; + }))); + } + + + + /// Another port on the same host is another origin. + [Fact] + public void OtherPort_SameSchemeHostAndPath_IsDenied() + { + var policy = new DocumentResourcePolicy(); + Assert.Null(policy.GetImageRelativePath(Vary(policy, "image.png", builder => builder.Port = 444))); + Assert.Null(policy.GetDocumentRelativePath(Vary(policy, "child.md", builder => builder.Port = 8443))); + } + + + + /// Credentials in the request never reach a resource. + [Fact] + public void UserInformation_SameOriginAndPath_IsDenied() + { + var policy = new DocumentResourcePolicy(); + Assert.Null(policy.GetImageRelativePath(Vary(policy, "image.png", builder => builder.UserName = "user"))); + Assert.Null(policy.GetDocumentRelativePath(Vary(policy, "child.md", builder => + { + builder.UserName = "user"; + builder.Password = "secret"; + }))); + } + + + + /// Another host, or the same host in another letter case position such as a subdomain, is denied. + [Theory] + [InlineData("example.com")] + [InlineData("sub.viewer.markdownviewr.invalid")] + [InlineData("viewer.markdownviewr.invalid.example.com")] + public void OtherHost_SameSchemePortAndPath_IsDenied(string host) + { + var policy = new DocumentResourcePolicy(); + Assert.Null(policy.GetImageRelativePath(Vary(policy, "image.png", builder => builder.Host = host))); + Assert.Null(policy.GetDocumentRelativePath(Vary(policy, "child.md", builder => builder.Host = host))); + } + + + + /// Exactly these image types are served; documents, pages, scripts and executables are not images. + [Theory] + [InlineData(".png", true)] + [InlineData(".jpg", true)] + [InlineData(".jpeg", true)] + [InlineData(".gif", true)] + [InlineData(".bmp", true)] + [InlineData(".webp", true)] + [InlineData(".svg", true)] + [InlineData(".avif", true)] + [InlineData(".ico", true)] + [InlineData(".PNG", true)] + [InlineData(".md", false)] + [InlineData(".markdown", false)] + [InlineData(".txt", false)] + [InlineData(".html", false)] + [InlineData(".htm", false)] + [InlineData(".js", false)] + [InlineData(".css", false)] + [InlineData(".json", false)] + [InlineData(".pdf", false)] + [InlineData(".tiff", false)] + [InlineData(".exe", false)] + [InlineData(".lnk", false)] + [InlineData(".url", false)] + [InlineData("", false)] + public void GetImageRelativePath_Extension_ServesOnlyImageTypes(string extension, bool served) + { + var policy = new DocumentResourcePolicy(); + Assert.Equal(served ? "file" + extension : null, policy.GetImageRelativePath(new Uri(policy.DocumentUri, "file" + extension))); + } + + + + /// Exactly these document types open in a new reader; pages, images, scripts and executables do not. + [Theory] + [InlineData(".md", true)] + [InlineData(".markdown", true)] + [InlineData(".mdown", true)] + [InlineData(".mkd", true)] + [InlineData(".txt", true)] + [InlineData(".MD", true)] + [InlineData(".html", false)] + [InlineData(".htm", false)] + [InlineData(".png", false)] + [InlineData(".svg", false)] + [InlineData(".json", false)] + [InlineData(".pdf", false)] + [InlineData(".exe", false)] + [InlineData(".bat", false)] + [InlineData(".cmd", false)] + [InlineData(".ps1", false)] + [InlineData(".lnk", false)] + [InlineData(".url", false)] + [InlineData(".mdx", false)] + [InlineData("", false)] + public void GetDocumentRelativePath_Extension_OpensOnlyDocumentTypes(string extension, bool opened) + { + var policy = new DocumentResourcePolicy(); + Assert.Equal(opened ? "file" + extension : null, policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, "file" + extension))); + } + + + + /// Each character Windows forbids in a name is refused on its own, for images and documents. + [Theory] + [InlineData("%3C")] + [InlineData("%3E")] + [InlineData("%3A")] + [InlineData("%22")] + [InlineData("%5C")] + [InlineData("%7C")] + [InlineData("%3F")] + [InlineData("%2A")] + public void ForbiddenCharacter_InAnySegment_IsDenied(string encoded) + { + var policy = new DocumentResourcePolicy(); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, "images/a" + encoded + "b.png"))); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, "im" + encoded + "ages/ab.png"))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, "docs/a" + encoded + "b.md"))); + } + + + + /// The generated heading fragment is matched exactly; another letter case is not a heading target. + [Theory] + [InlineData("#MV-HEADING-0")] + [InlineData("#Mv-Heading-0")] + [InlineData("#mv-heading-0x")] + [InlineData("#mv-heading-00a")] + [InlineData("#mv-heading- 0")] + public void GetHeadingIndex_OtherSpelling_IsNotAHeadingTarget(string fragment) + { + var policy = new DocumentResourcePolicy(); + Assert.Null(policy.GetHeadingIndex(new Uri(policy.DocumentUri.AbsoluteUri + fragment), 3)); + } + + + + /// Builds this document's own resource request and changes exactly one component of it. + private static Uri Vary(DocumentResourcePolicy policy, string file, Action change) + { + var builder = new UriBuilder(new Uri(policy.DocumentUri, file)); + change(builder); + return builder.Uri; + } +} diff --git a/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs new file mode 100644 index 0000000..eff6203 --- /dev/null +++ b/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs @@ -0,0 +1,146 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using AngleSharp.Dom; +using AngleSharp.Html.Dom; +using AngleSharp.Html.Parser; +using MarkdownViewr.Windows.Core.Documents; + + +namespace MarkdownViewr.Windows.Core.Tests; + +/// +/// Pins the exact elements, attributes and link schemes that authored HTML may keep. Widening +/// the renderer's allowlist fails here until this independent list is changed deliberately. +/// +public sealed class SanitizerAllowlistTests +{ + private static readonly string[] s_allowedTags = + [ + "a", "abbr", "b", "blockquote", "br", "caption", "code", "dd", "del", "details", "div", "dl", "dt", "em", + "h1", "h2", "h3", "h4", "h5", "h6", "hr", "i", "img", "input", "ins", "kbd", "li", "mark", "ol", "p", "pre", + "s", "small", "span", "strong", "sub", "summary", "sup", "table", "tbody", "td", "th", "thead", "tr", "ul", + ]; + + private static readonly string[] s_otherTags = + [ + "address", "applet", "area", "article", "aside", "audio", "base", "basefont", "bdi", "bdo", "bgsound", "big", "blink", + "button", "canvas", "center", "cite", "col", "colgroup", "data", "datalist", "dfn", "dialog", "dir", "embed", "fencedframe", + "fieldset", "figcaption", "figure", "font", "footer", "form", "frame", "frameset", "header", "hgroup", "iframe", + "keygen", "label", "legend", "link", "listing", "main", "map", "marquee", "math", "menu", "menuitem", "meta", "meter", + "nav", "nobr", "noembed", "noframes", "noscript", "object", "optgroup", "option", "output", "param", "picture", + "plaintext", "portal", "progress", "q", "rb", "rp", "rt", "rtc", "ruby", "samp", "script", "search", "section", "select", + "selectedcontent", "slot", "source", "strike", "style", "svg", "template", "textarea", "tfoot", "time", "title", "track", + "tt", "u", "var", "video", "wbr", "xmp", "custom-element", + ]; + + private static readonly string[] s_allowedAttributes = + [ + "align", "alt", "checked", "class", "colspan", "disabled", "height", "href", "rowspan", "src", "title", "type", "width", + ]; + + private static readonly string[] s_otherAttributes = + [ + "accesskey", "action", "allow", "as", "autocapitalize", "autofocus", "autoplay", "background", "bgcolor", "border", + "cellpadding", "cellspacing", "charset", "cite", "color", "content", "contenteditable", "controls", "coords", + "crossorigin", "data", "data-mv-body", "data-mv-source-fixed", "data-x", "datetime", "dir", "download", "draggable", + "enctype", "enterkeyhint", "face", "for", "form", "formaction", "formmethod", "headers", "hidden", "http-equiv", "id", + "inert", "inputmode", "integrity", "is", "ismap", "itemprop", "label", "lang", "list", "loading", "longdesc", "loop", + "lowsrc", "manifest", "max", "maxlength", "media", "method", "min", "multiple", "muted", "name", "nonce", "novalidate", + "nowrap", "onclick", "onerror", "onfocus", "onload", "onmouseover", "onpointerdown", "ontoggle", "open", "part", + "pattern", "ping", "placeholder", "popover", "popovertarget", "poster", "preload", "readonly", "referrerpolicy", "rel", + "required", "reversed", "role", "sandbox", "scope", "selected", "shape", "size", "sizes", "slot", "span", "spellcheck", + "srcdoc", "srclang", "srcset", "start", "step", "style", "tabindex", "target", "translate", "usemap", "valign", "value", + "wrap", "xlink:href", "xml:lang", "xmlns", "aria-label", + ]; + + + + /// Every allowed element survives in a document that nests each one where HTML permits it. + [Fact] + public void Render_EveryAllowedElement_SurvivesSanitation() + { + const string Markup = """ +

abc
defghjklmnopqr

s
t

1

2

3

4

5
6
u
v
wx
  • y
  1. z
c
h
d
+ """; + using IHtmlDocument document = Parse(new MarkdownRenderer().Render(Markup)); + Assert.All(s_allowedTags, tag => Assert.NotNull(document.Body!.QuerySelector(tag))); + Assert.Equal(s_allowedTags.Order(StringComparer.Ordinal), ElementNames(document), StringComparer.Ordinal); + } + + + + /// No other element survives, alone or between allowed elements; its text may remain but its capability does not. + [Fact] + public void Render_EveryOtherElement_IsRemoved() + { + var renderer = new MarkdownRenderer(); + Assert.All(s_otherTags, tag => + { + using IHtmlDocument document = Parse(renderer.Render($"
<{tag} title=\"t\">innerafter
")); + Assert.Empty(ElementNames(document).Except(["div", "span"], StringComparer.Ordinal)); + }); + Assert.Empty(s_allowedTags.Intersect(s_otherTags, StringComparer.Ordinal)); + } + + + + /// Exactly the allowed attributes survive on an allowed element; identity, behaviour, styling and data attributes do not. + [Fact] + public void Render_EveryAttribute_KeepsExactlyTheAllowedOnes() + { + string attributes = string.Join(' ', s_allowedAttributes.Concat(s_otherAttributes).Select(name => string.Equals(name, "class", StringComparison.Ordinal) ? "class=\"frontmatter\"" : name + "=\"1\"")); + using IHtmlDocument document = Parse(new MarkdownRenderer().Render($"
text
")); + IElement span = Assert.Single(document.Body!.QuerySelectorAll("span")); + Assert.Equal(s_allowedAttributes, span.Attributes.Select(attribute => attribute.Name).Order(StringComparer.Ordinal), StringComparer.Ordinal); + Assert.Empty(s_allowedAttributes.Intersect(s_otherAttributes, StringComparer.Ordinal)); + } + + + + /// Only web and mail links keep a target; every other scheme, including ones Windows would hand to another application, is removed. + [Theory] + [InlineData("http://example.com/a", true)] + [InlineData("https://example.com/a", true)] + [InlineData("mailto:someone@example.com", true)] + [InlineData("child.md", true)] + [InlineData("#section", true)] + [InlineData("ftp://example.com/a", false)] + [InlineData("file:///C:/private.txt", false)] + [InlineData("javascript:run()", false)] + [InlineData("vbscript:run()", false)] + [InlineData("data:text/html,

x

", false)] + [InlineData("blob:https://example.com/1", false)] + [InlineData("about:blank", false)] + [InlineData("ws://example.com", false)] + [InlineData("tel:5551234", false)] + [InlineData("sms:5551234", false)] + [InlineData("ms-settings:defaultapps", false)] + [InlineData("ms-msdt:/id", false)] + [InlineData("search-ms:query=x", false)] + [InlineData("shell:startup", false)] + [InlineData("res://ieframe.dll/x", false)] + [InlineData("view-source:https://example.com", false)] + [InlineData("onenote:x", false)] + [InlineData("HTTPS://example.com/a", true)] + [InlineData("FILE:///C:/private.txt", false)] + public void Render_LinkScheme_KeepsOnlyWebAndMailTargets(string target, bool kept) + { + using IHtmlDocument document = Parse(new MarkdownRenderer().Render($"link")); + IElement link = Assert.Single(document.Body!.QuerySelectorAll("a")); + Assert.Equal(kept, link.HasAttribute("href")); + } + + + + /// The distinct element names in the sanitized body, in ordinal order. + private static string[] ElementNames(IHtmlDocument document) + => document.Body!.QuerySelectorAll("*").Select(element => element.LocalName).Distinct(StringComparer.Ordinal).Order(StringComparer.Ordinal).ToArray(); + + + + /// Parses renderer output the way the browser surface does. + private static IHtmlDocument Parse(string html) => new HtmlParser().ParseDocument("" + html + ""); +} diff --git a/tests/MarkdownViewr.Windows.Core.Tests/SecurityFuzzTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/SecurityFuzzTests.cs index 9be1570..15b3d42 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/SecurityFuzzTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/SecurityFuzzTests.cs @@ -35,6 +35,10 @@ public sealed class SecurityFuzzTests "[section](#heading) ![image](images/hello%20world.png)\n", "| A | B |\n|---|---|\n| x | y |\n", "- [x] Task\n> Quote\n", "😀 café\0\t\r\n", "", "", "", "<script>literal</script>", "\"'<>:=[]()\\/!?%", + "", + "x", + "", + "x", ]; @@ -242,11 +246,12 @@ public sealed class SecurityFuzzTests /// Uses the parsed DOM's executable capabilities as the oracle, not the renderer's private allowlist. private static void AssertPassive(IHtmlDocument document) { - Assert.Empty(document.QuerySelectorAll("script,svg,math,iframe,object,embed,form,button,style,base,meta[http-equiv],audio,video")); + Assert.Empty(document.QuerySelectorAll("script,svg,math,iframe,object,embed,form,button,style,base,meta,link,audio,video,source,param,textarea,select,option")); foreach(IElement element in document.QuerySelectorAll("*")) { Assert.DoesNotContain(element.Attributes, attribute => attribute.Name.StartsWith("on", StringComparison.OrdinalIgnoreCase)); Assert.False(element.HasAttribute("srcdoc") || element.HasAttribute("srcset") || element.HasAttribute("style")); + Assert.False(element.HasAttribute("target") || element.HasAttribute("ping") || element.HasAttribute("name") || element.HasAttribute("autofocus")); foreach(string name in new[] { "href", "src" }) { string? value = element.GetAttribute(name); diff --git a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs new file mode 100644 index 0000000..61118d7 --- /dev/null +++ b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs @@ -0,0 +1,303 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using System.Text.Json; +using System.Text.Json.Nodes; +using AngleSharp.Dom; +using AngleSharp.Html.Dom; +using AngleSharp.Html.Parser; +using MarkdownViewr.Windows.Core.Documents; +using MarkdownViewr.Windows.Core.Editors; +using MarkdownViewr.Windows.Core.Printing; +using MarkdownViewr.Windows.Core.Themes; + + +namespace MarkdownViewr.Windows.Core.Tests; + +/// +/// Validation of text that comes from a document, a settings file, an imported theme or the +/// browser surface: each check is pinned by an input that only that check rejects. +/// +public sealed class UntrustedInputValidationTests +{ + /// Metadata keys and values are display text: markup in either is escaped, never parsed. + [Fact] + public void RenderMetadata_MarkupInKeyAndValue_IsEscapedText() + { + const string Key = "key"; + const string Value = " & \"quoted\""; + using IHtmlDocument parsed = new HtmlParser().ParseDocument(DocumentFrontmatter.RenderMetadata(Key + ": " + Value)); + Assert.Equal(Key, Assert.Single(parsed.QuerySelectorAll("th.fm-key")).TextContent); + Assert.Equal(Value, Assert.Single(parsed.QuerySelectorAll("td.fm-value")).TextContent); + Assert.Empty(parsed.QuerySelectorAll("b,img")); + } + + + + /// A line that starts with its colon still has a key cell, so the table keeps two columns. + [Fact] + public void RenderMetadata_LeadingColon_KeepsAnEmptyKeyCell() + { + using IHtmlDocument parsed = new HtmlParser().ParseDocument(DocumentFrontmatter.RenderMetadata(": value")); + Assert.Equal("", Assert.Single(parsed.QuerySelectorAll("th.fm-key")).TextContent); + IElement value = Assert.Single(parsed.QuerySelectorAll("td.fm-value")); + Assert.Equal("value", value.TextContent); + Assert.False(value.HasAttribute("colspan")); + } + + + + /// The private source marker is new for every render, so an authored attribute cannot predict it. + [Fact] + public void SourceMap_EveryRender_UsesAnUnpredictableAttributeName() + { + string first = new DocumentSourceMap("# Title").AttributeName; + string second = new DocumentSourceMap("# Title").AttributeName; + Assert.Matches("^data-mv-source-[0-9a-f]{32}$", first); + Assert.Matches("^data-mv-source-[0-9a-f]{32}$", second); + Assert.NotEqual(first, second, StringComparer.Ordinal); + } + + + + /// Raw HTML headings map to their source at every level, including the sixth. + [Theory] + [InlineData(1)] + [InlineData(5)] + [InlineData(6)] + public void Outline_RawHtmlHeadingLevel_MapsItsSource(int level) + { + string heading = $"Raw"; + string source = "Intro\n\n" + heading + "\n\nAfter\n"; + var outline = new DocumentOutline(source); + Assert.Equal(level, Assert.Single(outline.Headings).Level); + DocumentSourceHeading mapped = Assert.Single(outline.SourceHeadings); + Assert.Equal(heading, source[mapped.Start..mapped.End]); + } + + + + /// A level the HTML standard does not define is not a heading and gets no source mapping. + [Fact] + public void Outline_RawHtmlLevelSeven_IsNotAHeading() + { + var outline = new DocumentOutline("Not a heading\n"); + Assert.Empty(outline.Headings); + Assert.Empty(outline.SourceHeadings); + } + + + + /// Loading repairs every stored list and object, not only the ones the window happens to read first. + [Fact] + public void Sanitize_CorruptPrintEditorsAndThemeLists_RepairsEachProperty() + { + var valid = new EditorConfiguration(Guid.NewGuid(), "Editor", @"C:\Editor\editor.exe"); + var settings = new AppSettings + { + Print = new PrintOptions(Theme: "Bad\u0001Theme", PaddingPixels: 5000), + ConfiguredEditors = [valid, new EditorConfiguration(Guid.NewGuid(), "Script", @"C:\Editor\run.cmd"), new EditorConfiguration(Guid.Empty, "Empty", @"C:\Editor\other.exe")], + DisabledThemes = ["Nord", "Nord", "Bad\u0001Name", ""], + DeletedBuiltInThemes = ["Dracula", "Dracula", "Bad\u0002Name"], + ThemeOrder = ["Nord", "Nord", "Bad\u0003Name"], + DocumentTheme = "Bad\u0001Theme", + }; + settings.Sanitize(); + Assert.Equal("Clean Printing", settings.Print.Theme); + Assert.Equal(96, settings.Print.PaddingPixels); + Assert.Equal([valid], settings.ConfiguredEditors); + Assert.Equal(valid.Executable, settings.ExternalEditor); + Assert.Equal(["Nord"], settings.DisabledThemes); + Assert.Equal(["Dracula"], settings.DeletedBuiltInThemes); + Assert.Equal(["Nord"], settings.ThemeOrder); + Assert.Equal("Catppuccin Mocha", settings.DocumentTheme); + } + + + + /// A missing print object, as an older file or a hand edit leaves it, becomes the defaults. + [Fact] + public void Sanitize_MissingPrintObject_UsesDefaults() + { + var settings = new AppSettings { Print = null! }; + settings.Sanitize(); + Assert.Equal(new PrintOptions(), settings.Print); + } + + + + /// Editing a theme cannot silently raise it to a newer schema; that needs the import confirmation. + [Fact] + public void SaveEdited_SchemaVersionRaised_RequiresConfirmation() + { + ReaderTheme original = ThemeCatalog.Find(null); + JsonNode raised = JsonNode.Parse(original.Json)!; + raised["schemaVersion"] = 2; + var edited = ReaderTheme.ParseCompatible(raised.ToJsonString()); + Assert.Equal(1, original.SchemaVersion); + Assert.Equal(2, edited.SchemaVersion); + Assert.Throws(() => ThemeManagement.SaveEdited(new AppSettings(), original, edited)); + } + + + + /// An already confirmed newer-schema theme stays editable at its own version. + [Fact] + public void SaveEdited_SameNewerSchema_IsSaved() + { + JsonNode newer = JsonNode.Parse(ThemeCatalog.Find(null).Json)!; + newer["schemaVersion"] = 2; + newer["name"] = "Owned future"; + var theme = ReaderTheme.ParseCompatible(newer.ToJsonString()); + AppSettings imported = ThemeManagement.Import(new AppSettings(), theme, approveNewerSchema: true); + AppSettings saved = ThemeManagement.SaveEdited(imported, theme, theme); + Assert.Equal("Owned future", saved.DocumentTheme); + Assert.Equal(2, ThemeManagement.Active(saved).SchemaVersion); + } + + + + /// A copy of a theme whose name is already at the limit still has a name the theme parser accepts. + [Fact] + public void CopyTheme_LongestNameTakenTwice_StaysWithinTheNameLimit() + { + ReaderTheme longest = ThemeCatalog.Find(null).Rename(new string('N', 80)); + AppSettings settings = ThemeManagement.Import(new AppSettings(), longest); + ReaderTheme firstCopy = ThemeManagement.CopyTheme(settings, longest); + settings = ThemeManagement.Import(settings, firstCopy); + ReaderTheme secondCopy = ThemeManagement.CopyTheme(settings, longest); + Assert.Equal(80, firstCopy.Name.Length); + Assert.Equal(80, secondCopy.Name.Length); + Assert.EndsWith(" (2)", secondCopy.Name, StringComparison.Ordinal); + Assert.Equal(secondCopy.Name, ReaderTheme.ParseCompatible(secondCopy.Json).Name); + } + + + + /// Editor and document paths are accepted up to the Windows limit and refused one character past it. + [Fact] + public void EditorLaunch_PathAtAndPastTheWindowsLimit_IsBoundedExactly() + { + string atLimit = @"C:\" + new string('e', 32767 - 7) + ".exe"; + string pastLimit = @"C:\" + new string('e', 32768 - 7) + ".exe"; + Assert.Equal(32767, atLimit.Length); + Assert.True(EditorLaunch.IsExecutablePath(atLimit)); + Assert.False(EditorLaunch.IsExecutablePath(pastLimit)); + string document = @"C:\" + new string('d', 32767 - 6) + ".md"; + Assert.Equal(32767, document.Length); + Assert.Equal(document, Assert.Single(EditorLaunch.Create(@"C:\Editor\editor.exe", document).ArgumentList)); + Assert.Throws(() => EditorLaunch.Create(@"C:\Editor\editor.exe", document + "x")); + } + + + + /// Recent paths are kept up to the Windows limit and dropped one character past it. + [Fact] + public void RecentDocuments_PathAtAndPastTheWindowsLimit_IsBoundedExactly() + { + string atLimit = @"C:\" + new string('d', 32767 - 6) + ".md"; + Assert.Equal([atLimit], RecentDocuments.Sanitize([atLimit, atLimit + "x"])); + } + + + + /// A find term is accepted at its limit and refused one character past it. + [Fact] + public void DocumentFind_QueryAtAndPastTheLimit_IsBoundedExactly() + { + string atLimit = new('q', DocumentFind.MaximumQueryCharacters); + Assert.Equal(atLimit, new DocumentFind(atLimit + " tail", atLimit).Query); + Assert.Throws(() => new DocumentFind("source", atLimit + "q")); + } + + + + /// The viewport result is read up to its size cap and ignored past it, even when it would parse. + [Fact] + public void DocumentViewport_ResultAtAndPastTheSizeCap_IsBoundedExactly() + { + string atCap = "[0.25," + new string(' ', 128 - 11) + "0.75]"; + Assert.Equal(128, atCap.Length); + Assert.Equal(0.75, DocumentViewport.FromBrowserResult(atCap).VerticalFraction); + Assert.Equal(default, DocumentViewport.FromBrowserResult(atCap.Replace("[", "[ ", StringComparison.Ordinal))); + } + + + + /// The scroll-state result is read up to its size cap for the heading count and ignored past it. + [Theory] + [InlineData(0)] + [InlineData(3)] + public void DocumentScrollState_ResultAtAndPastTheSizeCap_IsBoundedExactly(int headings) + { + int cap = headings * 128 + 512; + string atCap = "[1,2,[],[]" + new string(' ', cap - 11) + "]"; + Assert.Equal(cap, atCap.Length); + Assert.NotNull(DocumentScrollState.FromBrowserResult(atCap, headings)); + Assert.Null(DocumentScrollState.FromBrowserResult(atCap.Replace("[1,", "[ 1,", StringComparison.Ordinal), headings)); + } + + + + /// The scroll-state result has exactly four parts; a longer array is not the application's own query result. + [Theory] + [InlineData("[1,2,[]]")] + [InlineData("[1,2,[],[],[]]")] + [InlineData("[1,2,[],[],0]")] + public void DocumentScrollState_WrongNumberOfParts_IsRejected(string json) + => Assert.Null(DocumentScrollState.FromBrowserResult(json, 2)); + + + + /// Print styles are built from repaired options, so a corrupt padding value cannot reach the stylesheet. + [Theory] + [InlineData(double.NaN, "padding:0px")] + [InlineData(double.PositiveInfinity, "padding:0px")] + [InlineData(5000, "padding:96px")] + [InlineData(-3, "padding:0px")] + public void PrintStyles_CorruptPadding_UsesTheRepairedValue(double padding, string expected) + { + ReaderTheme theme = ThemeCatalog.Find(null); + string css = PrintStyles.Build(new PrintOptions(PaddingPixels: padding), theme, [theme], 1); + Assert.Contains(expected, css, StringComparison.Ordinal); + Assert.DoesNotContain("NaN", css, StringComparison.Ordinal); + Assert.DoesNotContain("Infinity", css, StringComparison.Ordinal); + Assert.DoesNotContain("∞", css, StringComparison.Ordinal); + } + + + + /// Only the exact lower-case channel name selects the development identity. + [Theory] + [InlineData("DEV")] + [InlineData("Dev")] + [InlineData("dev ")] + [InlineData("development")] + public void ForChannel_OtherSpellingOfDev_KeepsTheStableIdentity(string channel) + => Assert.Equal("markdownviewr-windows", AppIdentity.ForChannel(channel).Id); + + + + /// A parse error anywhere in a theme is reported as invalid JSON content, the one failure callers handle. + [Theory] + [InlineData("colors.background", "\";ffffff\"")] + [InlineData("colors.background", "\"#fffffff\"")] + [InlineData("colors.background", "\"#ffffffff\"")] + [InlineData("colors.background", "\"#fffff\"")] + [InlineData("colors.background", "\"ffffff#\"")] + [InlineData("colors.text", "\"#12345g\"")] + [InlineData("name", "\"Theme\\u0001Name\"")] + [InlineData("name", "\"Theme\\u007fName\"")] + [InlineData("fonts.heading", "\"Font\\u0001Name\"")] + [InlineData("fonts.code", "\"Font\\u009bName\"")] + public void ThemeParse_MalformedColorOrLabel_IsRejected(string path, string json) + { + JsonNode root = JsonNode.Parse(ThemeCatalog.Find(null).Json)!; + string[] parts = path.Split('.'); + JsonNode parent = parts.Length == 1 ? root : root[parts[0]]!; + parent[parts[^1]] = JsonNode.Parse(json); + Assert.Throws(() => ReaderTheme.Parse(root.ToJsonString())); + } +} diff --git a/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs new file mode 100644 index 0000000..d6b90f6 --- /dev/null +++ b/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2026 James Maes + * SPDX-License-Identifier: MIT + */ + +using MarkdownViewr.Windows.Core.Documents; +using MarkdownViewr.Windows.Core.Themes; + + +namespace MarkdownViewr.Windows.Core.Tests; + +/// The written-out Windows name rules, and the two places that depend on them. +public sealed class WindowsFileNamesTests +{ + private static readonly string[] s_devices = + [ + "CON", "PRN", "AUX", "NUL", "CONIN$", "CONOUT$", + "COM0", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "COM¹", "COM²", "COM³", + "LPT0", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", "LPT¹", "LPT²", "LPT³", + ]; + + + + /// Every device name, in each letter case and with the suffixes Windows ignores. + public static TheoryData DeviceSpellings() + { + var data = new TheoryData(); + foreach(string device in s_devices) + { + string mixed = char.ToUpperInvariant(device[0]) + device[1..].ToLowerInvariant(); + foreach(string spelling in new[] { device, device.ToLowerInvariant(), mixed }) + { + data.Add(spelling); + data.Add(spelling + ".png"); + data.Add(spelling + ".tar.md"); + data.Add(spelling + " .png"); + data.Add(spelling + " "); + } + } + return data; + } + + + + /// A device is a device whatever its case, extension, or trailing spaces before the extension. + [Theory] + [MemberData(nameof(DeviceSpellings))] + public void IsReservedDeviceName_DeviceSpelling_IsReserved(string name) => Assert.True(WindowsFileNames.IsReservedDeviceName(name)); + + + + /// Names that only resemble a device stay ordinary file names. + [Theory] + [InlineData("")] + [InlineData("console.md")] + [InlineData("COM10.png")] + [InlineData("COM")] + [InlineData("LPT.txt")] + [InlineData("xCON.png")] + [InlineData("CON-notes.md")] + [InlineData("null.md")] + [InlineData(" NUL.png")] + [InlineData("readme.NUL")] + [InlineData("COM⁴.png")] + public void IsReservedDeviceName_OrdinaryName_IsNotReserved(string name) => Assert.False(WindowsFileNames.IsReservedDeviceName(name)); + + + + /// A missing name is a contract error, not an ordinary file. + [Fact] + public void IsReservedDeviceName_Null_RejectsInput() => Assert.Throws(() => WindowsFileNames.IsReservedDeviceName(null!)); + + + + /// The characters Windows forbids in a name are exactly these nine. + [Fact] + public void InvalidCharacters_ListsTheNineForbiddenCharacters() => Assert.Equal("\"*/:<>?\\|", new string(WindowsFileNames.InvalidCharacters.Order().ToArray())); + + + + /// No image or linked-document request can name a device in any spelling, at any depth. + [Theory] + [MemberData(nameof(DeviceSpellings))] + public void ResourcePolicy_DeviceSegment_DeniesImageAndDocument(string name) + { + var policy = new DocumentResourcePolicy(); + string segment = Uri.EscapeDataString(name.TrimEnd(' ')); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + (name.Contains('.', StringComparison.Ordinal) ? "" : ".png")))); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + "/picture.png"))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, segment + (name.Contains('.', StringComparison.Ordinal) ? "" : ".md")))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, "docs/" + segment + "/child.md"))); + } + + + + /// A theme named after a device, in any case, never becomes the suggested export file name. + [Fact] + public void SuggestedFileName_EveryDeviceName_IsPrefixed() + { + Assert.All(s_devices, device => + { + Assert.Equal("theme-" + device.ToLowerInvariant() + ".json", ThemeJson.SuggestedFileName(ThemeCatalog.Find(null).Rename(device))); + Assert.Equal("theme-" + device.ToLowerInvariant() + ".json", ThemeJson.SuggestedFileName(ThemeCatalog.Find(null).Rename(device.ToLowerInvariant()))); + Assert.Equal("theme-" + device.ToLowerInvariant() + ".backup.json", ThemeJson.SuggestedFileName(ThemeCatalog.Find(null).Rename(device + ".backup"))); + }); + } +} -- 2.54.0 From 6ca8284f1b871a6f4af1f5780701fae61e318616 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 17:18:59 -0600 Subject: [PATCH 2/5] test: compare the configured allowlists as sets and reject undecodable theme text The element pin missed elements it did not list and five it listed in a context where the parser never creates them. The sanitizer's configured elements, attributes and schemes, and the resource policy's file types, are now compared with independent lists as sets, so any change to them has to be made in the test as well. Table-only elements are tested inside a table. A theme string holding an unpaired surrogate escape made the JSON reader throw InvalidOperationException, which import and saved-theme repair do not handle. ReaderTheme now reports it as invalid theme JSON like every other malformed value. Also pin the styling-class filter, the exact path prefix and document comparison, the reserved-name rule's negatives and the export-name trimming; describe the reserved-name rule as the legacy Win32 rule applied on every Windows version; and run the device spellings as two tests instead of 960 theory rows. Refs #48 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- .../Documents/DocumentResourcePolicy.cs | 10 ++ .../Documents/MarkdownRenderer.cs | 5 + .../Themes/ReaderTheme.cs | 10 +- .../WindowsFileNames.cs | 8 +- .../DocumentResourcePolicyBoundaryTests.cs | 46 +++++++--- .../SanitizerAllowlistTests.cs | 92 ++++++++++++++----- .../UntrustedInputValidationTests.cs | 46 +++++++--- .../WindowsFileNamesTests.cs | 86 ++++++++++------- 8 files changed, 218 insertions(+), 85 deletions(-) diff --git a/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs b/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs index 5e2f51c..6ea1565 100644 --- a/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs +++ b/src/MarkdownViewr.Windows.Core/Documents/DocumentResourcePolicy.cs @@ -32,6 +32,16 @@ public sealed class DocumentResourcePolicy + /// The image file types served to the document surface. + internal static IReadOnlyCollection ImageExtensions => s_imageExtensions; + + + + /// The document file types a link may open in a new reader. + internal static IReadOnlyCollection DocumentExtensions => s_documentExtensions; + + + /// The only top-level document URI the browser host may serve. public Uri DocumentUri { get; } diff --git a/src/MarkdownViewr.Windows.Core/Documents/MarkdownRenderer.cs b/src/MarkdownViewr.Windows.Core/Documents/MarkdownRenderer.cs index a911826..a5b1b62 100644 --- a/src/MarkdownViewr.Windows.Core/Documents/MarkdownRenderer.cs +++ b/src/MarkdownViewr.Windows.Core/Documents/MarkdownRenderer.cs @@ -59,6 +59,11 @@ public sealed class MarkdownRenderer + /// The configured sanitizer, for comparing its allowlists with an independent list. + internal HtmlSanitizer Policy => _sanitizer; + + + /// Preserves useful markup while sanitizing every parser result, including authored raw HTML. public string Render(string markdown, FrontmatterMode frontmatter = FrontmatterMode.Hide) => Render(markdown, frontmatter, null); diff --git a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs index 3dc0eec..bfa1623 100644 --- a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs +++ b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs @@ -260,7 +260,15 @@ public sealed class ReaderTheme { throw new JsonException("The theme is missing a required string: " + name); } - return value.GetString()!; + try + { + return value.GetString()!; + } + catch(InvalidOperationException exception) + { + // An unpaired surrogate escape is valid JSON syntax that has no string value. + throw new JsonException("The theme contains text that cannot be decoded: " + name, exception); + } } diff --git a/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs b/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs index cc657c4..d053552 100644 --- a/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs +++ b/src/MarkdownViewr.Windows.Core/WindowsFileNames.cs @@ -12,7 +12,7 @@ namespace MarkdownViewr.Windows.Core; /// public static class WindowsFileNames { - /// Characters that cannot appear in a Windows file or folder name. + /// Printable characters that cannot appear in a Windows file or folder name. Control characters are also forbidden; callers refuse those themselves. public const string InvalidCharacters = "<>:\"/\\|?*"; private static readonly HashSet s_devices = new(StringComparer.OrdinalIgnoreCase) @@ -25,8 +25,10 @@ public static class WindowsFileNames /// - /// Whether a single name addresses a device in any letter case. Windows ignores everything after - /// the first dot and any spaces before it, so nul.txt and COM1 .log are devices too. + /// Whether a single name addresses a device under the legacy Win32 rule: any letter case, and + /// ignoring everything after the first dot and any spaces before it, so nul.txt and + /// COM1 .log count. Current Windows versions resolve fewer of these spellings to a + /// device; the rule is applied in full on every version so the answer never depends on it. /// public static bool IsReservedDeviceName(string name) { diff --git a/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs index 8f81fd9..f3bbed2 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/DocumentResourcePolicyBoundaryTests.cs @@ -16,7 +16,7 @@ public sealed class DocumentResourcePolicyBoundaryTests { /// The valid baseline every single-component variation below is derived from. [Fact] - public void Baseline_ValidRequests_AreAccepted() + public void GetRelativePath_OwnDocumentRequest_IsAccepted() { var policy = new DocumentResourcePolicy(); Assert.Equal("image.png", policy.GetImageRelativePath(Vary(policy, "image.png", _ => { }))); @@ -27,7 +27,7 @@ public sealed class DocumentResourcePolicyBoundaryTests /// Plain HTTP on the same host, port and path is not the document's origin. [Fact] - public void OtherScheme_SameHostPortAndPath_IsDenied() + public void GetRelativePath_OtherScheme_IsDenied() { var policy = new DocumentResourcePolicy(); Uri image = Vary(policy, "image.png", builder => @@ -48,7 +48,7 @@ public sealed class DocumentResourcePolicyBoundaryTests /// Another port on the same host is another origin. [Fact] - public void OtherPort_SameSchemeHostAndPath_IsDenied() + public void GetRelativePath_OtherPort_IsDenied() { var policy = new DocumentResourcePolicy(); Assert.Null(policy.GetImageRelativePath(Vary(policy, "image.png", builder => builder.Port = 444))); @@ -59,7 +59,7 @@ public sealed class DocumentResourcePolicyBoundaryTests /// Credentials in the request never reach a resource. [Fact] - public void UserInformation_SameOriginAndPath_IsDenied() + public void GetRelativePath_UserInformation_IsDenied() { var policy = new DocumentResourcePolicy(); Assert.Null(policy.GetImageRelativePath(Vary(policy, "image.png", builder => builder.UserName = "user"))); @@ -72,12 +72,12 @@ public sealed class DocumentResourcePolicyBoundaryTests - /// Another host, or the same host in another letter case position such as a subdomain, is denied. + /// Another host is denied, including a subdomain of the viewer's host and a host that merely starts with it. [Theory] [InlineData("example.com")] [InlineData("sub.viewer.markdownviewr.invalid")] [InlineData("viewer.markdownviewr.invalid.example.com")] - public void OtherHost_SameSchemePortAndPath_IsDenied(string host) + public void GetRelativePath_OtherHost_IsDenied(string host) { var policy = new DocumentResourcePolicy(); Assert.Null(policy.GetImageRelativePath(Vary(policy, "image.png", builder => builder.Host = host))); @@ -86,7 +86,32 @@ public sealed class DocumentResourcePolicyBoundaryTests - /// Exactly these image types are served; documents, pages, scripts and executables are not images. + /// The served image types and the openable document types are exactly these sets. + [Fact] + public void Extensions_ConfiguredSets_AreExactlyTheListedTypes() + { + Assert.Equal([".avif", ".bmp", ".gif", ".ico", ".jpeg", ".jpg", ".png", ".svg", ".webp"], DocumentResourcePolicy.ImageExtensions.Order(StringComparer.Ordinal), StringComparer.Ordinal); + Assert.Equal([".markdown", ".md", ".mdown", ".mkd", ".txt"], DocumentResourcePolicy.DocumentExtensions.Order(StringComparer.Ordinal), StringComparer.Ordinal); + } + + + + /// The document's own path prefix is matched exactly; another letter case is another document. + [Fact] + public void GetRelativePath_PrefixInAnotherLetterCase_IsDenied() + { + var policy = new DocumentResourcePolicy(); + string shouted = policy.DocumentUri.AbsoluteUri.Replace("/documents/", "/DOCUMENTS/", StringComparison.Ordinal); + Assert.NotEqual(policy.DocumentUri.AbsoluteUri, shouted, StringComparer.Ordinal); + Assert.Null(policy.GetImageRelativePath(new Uri(new Uri(shouted), "image.png"))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(new Uri(shouted), "child.md"))); + Assert.Null(policy.GetHeadingIndex(new Uri(shouted + "#mv-heading-0"), 3)); + Assert.Equal(0, policy.GetHeadingIndex(new Uri(policy.DocumentUri.AbsoluteUri + "#mv-heading-0"), 3)); + } + + + + /// Image types are served in any letter case; documents, pages, scripts and executables are not images. [Theory] [InlineData(".png", true)] [InlineData(".jpg", true)] @@ -120,7 +145,7 @@ public sealed class DocumentResourcePolicyBoundaryTests - /// Exactly these document types open in a new reader; pages, images, scripts and executables do not. + /// Document types open in a new reader in any letter case; pages, images, scripts and executables do not. [Theory] [InlineData(".md", true)] [InlineData(".markdown", true)] @@ -160,7 +185,7 @@ public sealed class DocumentResourcePolicyBoundaryTests [InlineData("%7C")] [InlineData("%3F")] [InlineData("%2A")] - public void ForbiddenCharacter_InAnySegment_IsDenied(string encoded) + public void GetRelativePath_ForbiddenCharacter_IsDenied(string encoded) { var policy = new DocumentResourcePolicy(); Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, "images/a" + encoded + "b.png"))); @@ -174,9 +199,6 @@ public sealed class DocumentResourcePolicyBoundaryTests [Theory] [InlineData("#MV-HEADING-0")] [InlineData("#Mv-Heading-0")] - [InlineData("#mv-heading-0x")] - [InlineData("#mv-heading-00a")] - [InlineData("#mv-heading- 0")] public void GetHeadingIndex_OtherSpelling_IsNotAHeadingTarget(string fragment) { var policy = new DocumentResourcePolicy(); diff --git a/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs index eff6203..03bf37a 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs @@ -6,14 +6,16 @@ using AngleSharp.Dom; using AngleSharp.Html.Dom; using AngleSharp.Html.Parser; +using Ganss.Xss; using MarkdownViewr.Windows.Core.Documents; namespace MarkdownViewr.Windows.Core.Tests; /// -/// Pins the exact elements, attributes and link schemes that authored HTML may keep. Widening -/// the renderer's allowlist fails here until this independent list is changed deliberately. +/// The elements, attributes and link schemes authored HTML may keep. The configured policy is +/// compared with the lists below as sets, so any change to it has to be made here as well, and +/// the rendering tests show the policy doing what the lists say. /// public sealed class SanitizerAllowlistTests { @@ -26,14 +28,14 @@ public sealed class SanitizerAllowlistTests private static readonly string[] s_otherTags = [ - "address", "applet", "area", "article", "aside", "audio", "base", "basefont", "bdi", "bdo", "bgsound", "big", "blink", - "button", "canvas", "center", "cite", "col", "colgroup", "data", "datalist", "dfn", "dialog", "dir", "embed", "fencedframe", - "fieldset", "figcaption", "figure", "font", "footer", "form", "frame", "frameset", "header", "hgroup", "iframe", - "keygen", "label", "legend", "link", "listing", "main", "map", "marquee", "math", "menu", "menuitem", "meta", "meter", - "nav", "nobr", "noembed", "noframes", "noscript", "object", "optgroup", "option", "output", "param", "picture", - "plaintext", "portal", "progress", "q", "rb", "rp", "rt", "rtc", "ruby", "samp", "script", "search", "section", "select", - "selectedcontent", "slot", "source", "strike", "style", "svg", "template", "textarea", "tfoot", "time", "title", "track", - "tt", "u", "var", "video", "wbr", "xmp", "custom-element", + "acronym", "address", "applet", "area", "article", "aside", "audio", "base", "basefont", "bdi", "bdo", "bgsound", "big", + "blink", "button", "canvas", "center", "cite", "data", "datalist", "dfn", "dialog", "dir", "embed", "fencedframe", + "fieldset", "figcaption", "figure", "font", "footer", "form", "header", "hgroup", "iframe", "keygen", "label", "legend", + "link", "listing", "main", "map", "marquee", "math", "menu", "menuitem", "meta", "meter", "nav", "nobr", "noembed", + "noframes", "noscript", "object", "optgroup", "option", "output", "param", "picture", "plaintext", "portal", "progress", + "q", "rb", "rp", "rt", "rtc", "ruby", "samp", "script", "search", "section", "select", "selectedcontent", "slot", "source", + "strike", "style", "svg", "template", "textarea", "time", "title", "track", "tt", "u", "var", "video", "wbr", "xmp", + "custom-element", ]; private static readonly string[] s_allowedAttributes = @@ -46,18 +48,34 @@ public sealed class SanitizerAllowlistTests "accesskey", "action", "allow", "as", "autocapitalize", "autofocus", "autoplay", "background", "bgcolor", "border", "cellpadding", "cellspacing", "charset", "cite", "color", "content", "contenteditable", "controls", "coords", "crossorigin", "data", "data-mv-body", "data-mv-source-fixed", "data-x", "datetime", "dir", "download", "draggable", - "enctype", "enterkeyhint", "face", "for", "form", "formaction", "formmethod", "headers", "hidden", "http-equiv", "id", - "inert", "inputmode", "integrity", "is", "ismap", "itemprop", "label", "lang", "list", "loading", "longdesc", "loop", - "lowsrc", "manifest", "max", "maxlength", "media", "method", "min", "multiple", "muted", "name", "nonce", "novalidate", - "nowrap", "onclick", "onerror", "onfocus", "onload", "onmouseover", "onpointerdown", "ontoggle", "open", "part", - "pattern", "ping", "placeholder", "popover", "popovertarget", "poster", "preload", "readonly", "referrerpolicy", "rel", - "required", "reversed", "role", "sandbox", "scope", "selected", "shape", "size", "sizes", "slot", "span", "spellcheck", - "srcdoc", "srclang", "srcset", "start", "step", "style", "tabindex", "target", "translate", "usemap", "valign", "value", - "wrap", "xlink:href", "xml:lang", "xmlns", "aria-label", + "enctype", "enterkeyhint", "face", "for", "form", "formaction", "formmethod", "formtarget", "headers", "hidden", + "http-equiv", "id", "inert", "inputmode", "integrity", "is", "ismap", "itemprop", "label", "lang", "list", "loading", + "longdesc", "loop", "lowsrc", "manifest", "max", "maxlength", "media", "method", "min", "multiple", "muted", "name", + "nonce", "novalidate", "nowrap", "onclick", "onerror", "onfocus", "onload", "onmouseover", "onpointerdown", "ontoggle", + "onwheel", "open", "part", "pattern", "ping", "placeholder", "popover", "popovertarget", "poster", "preload", "readonly", + "referrerpolicy", "rel", "required", "reversed", "role", "sandbox", "scope", "selected", "shape", "size", "sizes", "slot", + "span", "spellcheck", "srcdoc", "srclang", "srcset", "start", "step", "tabindex", "target", "translate", "usemap", + "valign", "value", "wrap", "xlink:href", "xml:lang", "xmlns", "aria-label", ]; + /// The policy the renderer configures is exactly these elements, attributes and schemes, with no styling or data attributes. + [Fact] + public void Constructor_ConfiguredPolicy_AllowsExactlyTheListedMarkup() + { + HtmlSanitizer policy = new MarkdownRenderer().Policy; + Assert.Equal(s_allowedTags.Order(StringComparer.Ordinal), policy.AllowedTags.Order(StringComparer.Ordinal), StringComparer.Ordinal); + Assert.Equal(s_allowedAttributes.Order(StringComparer.Ordinal), policy.AllowedAttributes.Order(StringComparer.Ordinal), StringComparer.Ordinal); + Assert.Equal(["http", "https", "mailto"], policy.AllowedSchemes.Order(StringComparer.Ordinal), StringComparer.Ordinal); + Assert.Empty(policy.AllowedCssProperties); + Assert.Empty(policy.AllowedAtRules); + Assert.False(policy.AllowDataAttributes); + Assert.False(policy.AllowCssCustomProperties); + } + + + /// Every allowed element survives in a document that nests each one where HTML permits it. [Fact] public void Render_EveryAllowedElement_SurvivesSanitation() @@ -66,13 +84,12 @@ public sealed class SanitizerAllowlistTests

abc
defghjklmnopqr

s
t

1

2

3

4

5
6
u
v
wx
  • y
  1. z
c
h
d
"""; using IHtmlDocument document = Parse(new MarkdownRenderer().Render(Markup)); - Assert.All(s_allowedTags, tag => Assert.NotNull(document.Body!.QuerySelector(tag))); Assert.Equal(s_allowedTags.Order(StringComparer.Ordinal), ElementNames(document), StringComparer.Ordinal); } - /// No other element survives, alone or between allowed elements; its text may remain but its capability does not. + /// No other element survives between allowed elements; its text may remain but the element does not. [Fact] public void Render_EveryOtherElement_IsRemoved() { @@ -87,11 +104,24 @@ public sealed class SanitizerAllowlistTests + /// Table parts that only exist inside a table, and are not allowed, are removed there while the allowed parts stay. + [Fact] + public void Render_TablePartsOutsideTheAllowlist_AreRemovedInsideATable() + { + const string Markup = "
h
d
f
"; + using IHtmlDocument document = Parse(new MarkdownRenderer().Render(Markup)); + Assert.Empty(document.Body!.QuerySelectorAll("colgroup,col,tfoot")); + Assert.NotNull(document.Body.QuerySelector("table > thead > tr > th")); + Assert.NotNull(document.Body.QuerySelector("table > tbody > tr > td")); + } + + + /// Exactly the allowed attributes survive on an allowed element; identity, behaviour, styling and data attributes do not. [Fact] public void Render_EveryAttribute_KeepsExactlyTheAllowedOnes() { - string attributes = string.Join(' ', s_allowedAttributes.Concat(s_otherAttributes).Select(name => string.Equals(name, "class", StringComparison.Ordinal) ? "class=\"frontmatter\"" : name + "=\"1\"")); + string attributes = string.Join(' ', s_allowedAttributes.Concat(s_otherAttributes).Select(name => string.Equals(name, "class", StringComparison.Ordinal) ? "class=\"frontmatter\"" : name + "=\"1\"")) + " style=\"color:red\""; using IHtmlDocument document = Parse(new MarkdownRenderer().Render($"
text
")); IElement span = Assert.Single(document.Body!.QuerySelectorAll("span")); Assert.Equal(s_allowedAttributes, span.Attributes.Select(attribute => attribute.Name).Order(StringComparer.Ordinal), StringComparer.Ordinal); @@ -100,6 +130,21 @@ public sealed class SanitizerAllowlistTests + /// Only the fixed styling classes and bounded code-language classes survive, spelled exactly. + [Theory] + [InlineData("frontmatter fm-key fm-value task-list task-list-item", "frontmatter fm-key fm-value task-list task-list-item")] + [InlineData("language-csharp hljs keyword", "language-csharp")] + [InlineData("LANGUAGE-csharp Language-go language-rust", "language-rust")] + [InlineData("Frontmatter FM-KEY", null)] + [InlineData("language- language-a;b language-", null)] + public void Render_ClassNames_KeepsOnlyTheStylingClasses(string authored, string? kept) + { + using IHtmlDocument document = Parse(new MarkdownRenderer().Render($"text")); + Assert.Equal(kept, Assert.Single(document.Body!.QuerySelectorAll("span")).GetAttribute("class")); + } + + + /// Only web and mail links keep a target; every other scheme, including ones Windows would hand to another application, is removed. [Theory] [InlineData("http://example.com/a", true)] @@ -107,8 +152,10 @@ public sealed class SanitizerAllowlistTests [InlineData("mailto:someone@example.com", true)] [InlineData("child.md", true)] [InlineData("#section", true)] + [InlineData("HTTPS://example.com/a", true)] [InlineData("ftp://example.com/a", false)] [InlineData("file:///C:/private.txt", false)] + [InlineData("FILE:///C:/private.txt", false)] [InlineData("javascript:run()", false)] [InlineData("vbscript:run()", false)] [InlineData("data:text/html,

x

", false)] @@ -119,13 +166,12 @@ public sealed class SanitizerAllowlistTests [InlineData("sms:5551234", false)] [InlineData("ms-settings:defaultapps", false)] [InlineData("ms-msdt:/id", false)] + [InlineData("ms-word:ofe|u|https://example.com/a.docx", false)] [InlineData("search-ms:query=x", false)] [InlineData("shell:startup", false)] [InlineData("res://ieframe.dll/x", false)] [InlineData("view-source:https://example.com", false)] [InlineData("onenote:x", false)] - [InlineData("HTTPS://example.com/a", true)] - [InlineData("FILE:///C:/private.txt", false)] public void Render_LinkScheme_KeepsOnlyWebAndMailTargets(string target, bool kept) { using IHtmlDocument document = Parse(new MarkdownRenderer().Render($"link")); diff --git a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs index 61118d7..886b141 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs @@ -51,7 +51,7 @@ public sealed class UntrustedInputValidationTests /// The private source marker is new for every render, so an authored attribute cannot predict it. [Fact] - public void SourceMap_EveryRender_UsesAnUnpredictableAttributeName() + public void AttributeName_EachSourceMap_IsUnpredictable() { string first = new DocumentSourceMap("# Title").AttributeName; string second = new DocumentSourceMap("# Title").AttributeName; @@ -67,7 +67,7 @@ public sealed class UntrustedInputValidationTests [InlineData(1)] [InlineData(5)] [InlineData(6)] - public void Outline_RawHtmlHeadingLevel_MapsItsSource(int level) + public void SourceHeadings_RawHtmlHeadingLevel_MapsItsSource(int level) { string heading = $"Raw"; string source = "Intro\n\n" + heading + "\n\nAfter\n"; @@ -81,7 +81,7 @@ public sealed class UntrustedInputValidationTests /// A level the HTML standard does not define is not a heading and gets no source mapping. [Fact] - public void Outline_RawHtmlLevelSeven_IsNotAHeading() + public void SourceHeadings_RawHtmlLevelSeven_IsNotAHeading() { var outline = new DocumentOutline("Not a heading\n"); Assert.Empty(outline.Headings); @@ -178,7 +178,7 @@ public sealed class UntrustedInputValidationTests /// Editor and document paths are accepted up to the Windows limit and refused one character past it. [Fact] - public void EditorLaunch_PathAtAndPastTheWindowsLimit_IsBoundedExactly() + public void IsExecutablePath_PathAtAndPastTheWindowsLimit_IsBoundedExactly() { string atLimit = @"C:\" + new string('e', 32767 - 7) + ".exe"; string pastLimit = @"C:\" + new string('e', 32768 - 7) + ".exe"; @@ -195,7 +195,7 @@ public sealed class UntrustedInputValidationTests /// Recent paths are kept up to the Windows limit and dropped one character past it. [Fact] - public void RecentDocuments_PathAtAndPastTheWindowsLimit_IsBoundedExactly() + public void Sanitize_RecentPathAtAndPastTheWindowsLimit_IsBoundedExactly() { string atLimit = @"C:\" + new string('d', 32767 - 6) + ".md"; Assert.Equal([atLimit], RecentDocuments.Sanitize([atLimit, atLimit + "x"])); @@ -205,7 +205,7 @@ public sealed class UntrustedInputValidationTests /// A find term is accepted at its limit and refused one character past it. [Fact] - public void DocumentFind_QueryAtAndPastTheLimit_IsBoundedExactly() + public void Constructor_FindTermAtAndPastTheLimit_IsBoundedExactly() { string atLimit = new('q', DocumentFind.MaximumQueryCharacters); Assert.Equal(atLimit, new DocumentFind(atLimit + " tail", atLimit).Query); @@ -216,7 +216,7 @@ public sealed class UntrustedInputValidationTests /// The viewport result is read up to its size cap and ignored past it, even when it would parse. [Fact] - public void DocumentViewport_ResultAtAndPastTheSizeCap_IsBoundedExactly() + public void FromBrowserResult_ViewportAtAndPastTheSizeCap_IsBoundedExactly() { string atCap = "[0.25," + new string(' ', 128 - 11) + "0.75]"; Assert.Equal(128, atCap.Length); @@ -230,7 +230,7 @@ public sealed class UntrustedInputValidationTests [Theory] [InlineData(0)] [InlineData(3)] - public void DocumentScrollState_ResultAtAndPastTheSizeCap_IsBoundedExactly(int headings) + public void FromBrowserResult_ScrollStateAtAndPastTheSizeCap_IsBoundedExactly(int headings) { int cap = headings * 128 + 512; string atCap = "[1,2,[],[]" + new string(' ', cap - 11) + "]"; @@ -246,7 +246,7 @@ public sealed class UntrustedInputValidationTests [InlineData("[1,2,[]]")] [InlineData("[1,2,[],[],[]]")] [InlineData("[1,2,[],[],0]")] - public void DocumentScrollState_WrongNumberOfParts_IsRejected(string json) + public void FromBrowserResult_ScrollStateWithWrongNumberOfParts_IsRejected(string json) => Assert.Null(DocumentScrollState.FromBrowserResult(json, 2)); @@ -257,7 +257,7 @@ public sealed class UntrustedInputValidationTests [InlineData(double.PositiveInfinity, "padding:0px")] [InlineData(5000, "padding:96px")] [InlineData(-3, "padding:0px")] - public void PrintStyles_CorruptPadding_UsesTheRepairedValue(double padding, string expected) + public void Build_CorruptPadding_UsesTheRepairedValue(double padding, string expected) { ReaderTheme theme = ThemeCatalog.Find(null); string css = PrintStyles.Build(new PrintOptions(PaddingPixels: padding), theme, [theme], 1); @@ -269,6 +269,28 @@ public sealed class UntrustedInputValidationTests + /// + /// Text the JSON reader cannot decode, an unpaired surrogate escape, is an invalid theme like any + /// other: parsing, import and saved-theme repair all handle it as one. + /// + [Theory] + [InlineData("\"name\": \"Nord\"", "\"name\": \"\\uD83D\"")] + [InlineData("\"background\": \"#2e3440\"", "\"background\": \"\\uD800\"")] + [InlineData("\"body\": \"System\"", "\"body\": \"A\\uDC00B\"")] + public void Parse_UndecodableText_IsRejectedAsAnInvalidTheme(string valid, string undecodable) + { + string original = ThemeCatalog.Find("Nord").Json; + Assert.Contains(valid, original, StringComparison.Ordinal); + string json = original.Replace(valid, undecodable, StringComparison.Ordinal); + Assert.Throws(() => ReaderTheme.Parse(json)); + Assert.Throws(() => ReaderTheme.ParseCompatible(json)); + var settings = new AppSettings { UserThemes = [json, original] }; + settings.Sanitize(); + Assert.Equal([original], settings.UserThemes); + } + + + /// Only the exact lower-case channel name selects the development identity. [Theory] [InlineData("DEV")] @@ -280,7 +302,7 @@ public sealed class UntrustedInputValidationTests - /// A parse error anywhere in a theme is reported as invalid JSON content, the one failure callers handle. + /// A colour that is not six hexadecimal digits after a hash, or a label containing a control character, is refused. [Theory] [InlineData("colors.background", "\";ffffff\"")] [InlineData("colors.background", "\"#fffffff\"")] @@ -292,7 +314,7 @@ public sealed class UntrustedInputValidationTests [InlineData("name", "\"Theme\\u007fName\"")] [InlineData("fonts.heading", "\"Font\\u0001Name\"")] [InlineData("fonts.code", "\"Font\\u009bName\"")] - public void ThemeParse_MalformedColorOrLabel_IsRejected(string path, string json) + public void Parse_MalformedColorOrLabel_IsRejected(string path, string json) { JsonNode root = JsonNode.Parse(ThemeCatalog.Find(null).Json)!; string[] parts = path.Split('.'); diff --git a/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs index d6b90f6..09bc140 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs @@ -21,35 +21,24 @@ public sealed class WindowsFileNamesTests - /// Every device name, in each letter case and with the suffixes Windows ignores. - public static TheoryData DeviceSpellings() + /// A device is a device in any letter case, with any extension, and with spaces before the extension or at the end. + [Fact] + public void IsReservedDeviceName_EveryDeviceSpelling_IsReserved() { - var data = new TheoryData(); - foreach(string device in s_devices) + Assert.All(Spellings(), spelling => { - string mixed = char.ToUpperInvariant(device[0]) + device[1..].ToLowerInvariant(); - foreach(string spelling in new[] { device, device.ToLowerInvariant(), mixed }) - { - data.Add(spelling); - data.Add(spelling + ".png"); - data.Add(spelling + ".tar.md"); - data.Add(spelling + " .png"); - data.Add(spelling + " "); - } - } - return data; + Assert.True(WindowsFileNames.IsReservedDeviceName(spelling), spelling); + Assert.True(WindowsFileNames.IsReservedDeviceName(spelling + ".png"), spelling); + Assert.True(WindowsFileNames.IsReservedDeviceName(spelling + ".tar.md"), spelling); + Assert.True(WindowsFileNames.IsReservedDeviceName(spelling + " .png"), spelling); + Assert.True(WindowsFileNames.IsReservedDeviceName(spelling + " "), spelling); + }); + Assert.Equal(96, Spellings().Count()); } - /// A device is a device whatever its case, extension, or trailing spaces before the extension. - [Theory] - [MemberData(nameof(DeviceSpellings))] - public void IsReservedDeviceName_DeviceSpelling_IsReserved(string name) => Assert.True(WindowsFileNames.IsReservedDeviceName(name)); - - - - /// Names that only resemble a device stay ordinary file names. + /// Names that only resemble a device stay ordinary file names; only spaces are ignored before the extension. [Theory] [InlineData("")] [InlineData("console.md")] @@ -58,10 +47,15 @@ public sealed class WindowsFileNamesTests [InlineData("LPT.txt")] [InlineData("xCON.png")] [InlineData("CON-notes.md")] + [InlineData("CON-.md")] [InlineData("null.md")] [InlineData(" NUL.png")] + [InlineData("NUL .png")] + [InlineData("NUL\t.png")] [InlineData("readme.NUL")] [InlineData("COM⁴.png")] + [InlineData("CLOCK$")] + [InlineData("CLOCK$.txt")] public void IsReservedDeviceName_OrdinaryName_IsNotReserved(string name) => Assert.False(WindowsFileNames.IsReservedDeviceName(name)); @@ -72,23 +66,29 @@ public sealed class WindowsFileNamesTests - /// The characters Windows forbids in a name are exactly these nine. + /// The printable characters Windows forbids in a name; control characters are refused separately by each caller. [Fact] - public void InvalidCharacters_ListsTheNineForbiddenCharacters() => Assert.Equal("\"*/:<>?\\|", new string(WindowsFileNames.InvalidCharacters.Order().ToArray())); + public void InvalidCharacters_Constant_ListsThePrintableForbiddenCharacters() => Assert.Equal("\"*/:<>?\\|", new string(WindowsFileNames.InvalidCharacters.Order().ToArray())); - /// No image or linked-document request can name a device in any spelling, at any depth. - [Theory] - [MemberData(nameof(DeviceSpellings))] - public void ResourcePolicy_DeviceSegment_DeniesImageAndDocument(string name) + /// No image or linked-document request can name a device in any spelling, as the file or as a folder on the way to it. + [Fact] + public void GetRelativePath_DeviceSegment_IsDenied() { var policy = new DocumentResourcePolicy(); - string segment = Uri.EscapeDataString(name.TrimEnd(' ')); - Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + (name.Contains('.', StringComparison.Ordinal) ? "" : ".png")))); - Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + "/picture.png"))); - Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, segment + (name.Contains('.', StringComparison.Ordinal) ? "" : ".md")))); - Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, "docs/" + segment + "/child.md"))); + Assert.All(Spellings(), spelling => + { + string segment = Uri.EscapeDataString(spelling); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + ".png"))); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + "%20.png"))); + Assert.Null(policy.GetImageRelativePath(new Uri(policy.DocumentUri, segment + "/picture.png"))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, segment + ".md"))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, segment + ".tar.md"))); + Assert.Null(policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, "docs/" + segment + "/child.md"))); + }); + Assert.Equal("console.png", policy.GetImageRelativePath(new Uri(policy.DocumentUri, "console.png"))); + Assert.Equal("docs/com10/child.md", policy.GetDocumentRelativePath(new Uri(policy.DocumentUri, "docs/com10/child.md"))); } @@ -104,4 +104,22 @@ public sealed class WindowsFileNamesTests Assert.Equal("theme-" + device.ToLowerInvariant() + ".backup.json", ThemeJson.SuggestedFileName(ThemeCatalog.Find(null).Rename(device + ".backup"))); }); } + + + + /// Leading and trailing dots and hyphens are dropped from a suggested name, and a name made only of them falls back to a plain one. + [Theory] + [InlineData("-Edge-", "edge.json")] + [InlineData(".Dotted.", "dotted.json")] + [InlineData("--", "theme.json")] + [InlineData(" spaced ", "spaced.json")] + [InlineData("-con-", "theme-con.json")] + public void SuggestedFileName_LeadingAndTrailingSeparators_AreTrimmed(string name, string expected) + => Assert.Equal(expected, ThemeJson.SuggestedFileName(ThemeCatalog.Find(null).Rename(name))); + + + + /// Each device name in upper, lower and mixed case. + private static IEnumerable Spellings() + => s_devices.SelectMany(device => new[] { device, device.ToLowerInvariant(), char.ToUpperInvariant(device[0]) + device[1..].ToLowerInvariant() }); } -- 2.54.0 From 419de2c263534baa205a7b697a45a43b7818d14a Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 17:44:14 -0600 Subject: [PATCH 3/5] fix(themes): reject a theme with undecodable text or a repeated property anywhere A theme is validated for the strings this version reads, but it is kept, renamed, edited and re-serialized as a whole document. An unpaired surrogate escape in a property name or in a value this version ignores, or a property name that appears twice, was accepted and then threw InvalidOperationException or ArgumentException from import, rename, copy, edit and saved-theme repair, none of which handle those. ReaderTheme now walks the document once and reports either case as invalid theme JSON, the failure every caller already handles. Also pin the code-language class length limit and that the source attribute is allowed only during a source-aware render. Refs #48 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- .../Themes/ReaderTheme.cs | 58 ++++++++++++++++--- .../SanitizerAllowlistTests.cs | 28 +++++++++ .../UntrustedInputValidationTests.cs | 36 ++++++++++-- .../WindowsFileNamesTests.cs | 2 +- 4 files changed, 110 insertions(+), 14 deletions(-) diff --git a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs index bfa1623..6d10048 100644 --- a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs +++ b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs @@ -157,6 +157,7 @@ public sealed class ReaderTheme using var document = JsonDocument.Parse(json, new JsonDocumentOptions { MaxDepth = 16 }); JsonElement root = document.RootElement; RequireObject(root); + RequireReadableText(root); int schemaVersion = 1; if(root.TryGetProperty("schemaVersion", out JsonElement version) && (version.ValueKind != JsonValueKind.Number || !version.TryGetInt32(out schemaVersion) || schemaVersion < 1 || !allowNewer && schemaVersion != 1)) { @@ -260,15 +261,7 @@ public sealed class ReaderTheme { throw new JsonException("The theme is missing a required string: " + name); } - try - { - return value.GetString()!; - } - catch(InvalidOperationException exception) - { - // An unpaired surrogate escape is valid JSON syntax that has no string value. - throw new JsonException("The theme contains text that cannot be decoded: " + name, exception); - } + return value.GetString()!; } @@ -286,6 +279,53 @@ public sealed class ReaderTheme + /// + /// Checks every property name and string in the theme once, including ones this version does + /// not read, because a kept theme is later renamed, edited and re-serialized as a whole. An + /// unpaired surrogate escape is valid JSON syntax with no string value, and a repeated + /// property name has no single value; both are invalid themes. + /// + private static void RequireReadableText(JsonElement root) + { + try + { + var pending = new Stack(); + pending.Push(root); + while(pending.TryPop(out JsonElement element)) + { + if(element.ValueKind == JsonValueKind.Object) + { + var names = new HashSet(StringComparer.Ordinal); + foreach(JsonProperty property in element.EnumerateObject()) + { + if(!names.Add(property.Name)) + { + throw new JsonException("The theme repeats a property: " + property.Name); + } + pending.Push(property.Value); + } + } + else if(element.ValueKind == JsonValueKind.Array) + { + foreach(JsonElement item in element.EnumerateArray()) + { + pending.Push(item); + } + } + else if(element.ValueKind == JsonValueKind.String) + { + _ = element.GetString(); + } + } + } + catch(InvalidOperationException exception) + { + throw new JsonException("The theme contains text that cannot be decoded.", exception); + } + } + + + /// Prevents wrong-shaped JSON from reaching property accessors. private static void RequireObject(JsonElement element) { diff --git a/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs index 03bf37a..5b1e454 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/SanitizerAllowlistTests.cs @@ -145,6 +145,34 @@ public sealed class SanitizerAllowlistTests + /// A code-language class is kept up to 80 characters and dropped one character past that. + [Fact] + public void Render_CodeLanguageClassAtAndPastTheLengthLimit_IsBoundedExactly() + { + string atLimit = "language-" + new string('x', 71); + Assert.Equal(80, atLimit.Length); + using IHtmlDocument kept = Parse(new MarkdownRenderer().Render($"text")); + Assert.Equal(atLimit, Assert.Single(kept.Body!.QuerySelectorAll("span")).GetAttribute("class")); + using IHtmlDocument dropped = Parse(new MarkdownRenderer().Render($"text")); + Assert.Null(Assert.Single(dropped.Body!.QuerySelectorAll("span")).GetAttribute("class")); + } + + + + /// The attribute that carries source positions is allowed only during a source-aware render, whether that render finishes or fails. + [Fact] + public void RenderWithSources_AfterTheRender_NoLongerAllowsTheSourceAttribute() + { + var renderer = new MarkdownRenderer(); + var sources = new DocumentSourceMap("# Title"); + Assert.Contains("Title", renderer.RenderWithSources("# Title", FrontmatterMode.Hide, sources), StringComparison.Ordinal); + Assert.DoesNotContain(sources.AttributeName, renderer.Policy.AllowedAttributes); + Assert.Throws(() => renderer.RenderWithSources("# Title", (FrontmatterMode)99, sources)); + Assert.DoesNotContain(sources.AttributeName, renderer.Policy.AllowedAttributes); + } + + + /// Only web and mail links keep a target; every other scheme, including ones Windows would hand to another application, is removed. [Theory] [InlineData("http://example.com/a", true)] diff --git a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs index 886b141..937d7d1 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs @@ -270,18 +270,26 @@ public sealed class UntrustedInputValidationTests /// - /// Text the JSON reader cannot decode, an unpaired surrogate escape, is an invalid theme like any - /// other: parsing, import and saved-theme repair all handle it as one. + /// Text the JSON reader cannot decode, an unpaired surrogate escape, makes the theme invalid + /// wherever it appears: in a value the theme reads, in a property name, or in a value this + /// version ignores but would later re-serialize. So does a repeated property name. /// [Theory] [InlineData("\"name\": \"Nord\"", "\"name\": \"\\uD83D\"")] [InlineData("\"background\": \"#2e3440\"", "\"background\": \"\\uD800\"")] [InlineData("\"body\": \"System\"", "\"body\": \"A\\uDC00B\"")] - public void Parse_UndecodableText_IsRejectedAsAnInvalidTheme(string valid, string undecodable) + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"\\uD83D\": 1")] + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"note\": \"\\uD83D\"")] + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"notes\": [1, [\"ok\", {\"deep\": \"\\uDFFF\"}]]")] + [InlineData("\"body\": \"System\"", "\"body\": \"System\", \"\\uD800x\": \"y\"")] + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"name\": \"Nord\"")] + [InlineData("\"body\": \"System\"", "\"body\": \"System\", \"body\": \"Other\"")] + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"extra\": {\"a\": 1, \"a\": 2}")] + public void Parse_UndecodableTextOrRepeatedProperty_IsRejectedAsAnInvalidTheme(string valid, string invalid) { string original = ThemeCatalog.Find("Nord").Json; Assert.Contains(valid, original, StringComparison.Ordinal); - string json = original.Replace(valid, undecodable, StringComparison.Ordinal); + string json = original.Replace(valid, invalid, StringComparison.Ordinal); Assert.Throws(() => ReaderTheme.Parse(json)); Assert.Throws(() => ReaderTheme.ParseCompatible(json)); var settings = new AppSettings { UserThemes = [json, original] }; @@ -291,6 +299,26 @@ public sealed class UntrustedInputValidationTests + /// + /// A theme that parses can be renamed, copied, edited and imported under a new name, whatever + /// extra properties it carries: those operations re-serialize the whole document. + /// + [Fact] + public void Parse_AcceptedThemeWithExtraProperties_SurvivesRenameCopyEditAndImport() + { + string json = ThemeCatalog.Find("Nord").Json.Replace("\"name\": \"Nord\"", "\"name\": \"Nord\", \"note\": \"caf\\u00e9 \\ud83d\\ude00\", \"future\": {\"list\": [1, \"two\", null]}", StringComparison.Ordinal); + var theme = ReaderTheme.Parse(json); + Assert.Equal("Renamed", theme.Rename("Renamed").Name); + AppSettings imported = ThemeManagement.Import(new AppSettings(), theme); + Assert.Contains(ThemeManagement.All(imported), item => item.Name.StartsWith("Nord (", StringComparison.Ordinal)); + Assert.EndsWith(" Copy", ThemeManagement.CopyTheme(imported, theme).Name, StringComparison.Ordinal); + ReaderTheme edited = theme.Edit("Edited", new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), ""); + Assert.Equal("Edited", edited.Name); + Assert.Contains("caf", edited.Json, StringComparison.Ordinal); + } + + + /// Only the exact lower-case channel name selects the development identity. [Theory] [InlineData("DEV")] diff --git a/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs index 09bc140..f6637da 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/WindowsFileNamesTests.cs @@ -50,7 +50,7 @@ public sealed class WindowsFileNamesTests [InlineData("CON-.md")] [InlineData("null.md")] [InlineData(" NUL.png")] - [InlineData("NUL .png")] + [InlineData("NUL\u00A0.png")] [InlineData("NUL\t.png")] [InlineData("readme.NUL")] [InlineData("COM⁴.png")] -- 2.54.0 From 2fff66cf76b1c4fabfdc74b2d4f0319a9d75a760 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 18:14:55 -0600 Subject: [PATCH 4/5] fix(themes): keep renamed themes within the size limit Renaming, copying and editing a theme wrote every quote mark, angle bracket and non-ASCII character as a six-character escape, so a theme well inside the size limit could grow past it and be refused. Those operations now write characters as themselves. Theme text that is not well-formed UTF-16 is refused as an invalid theme. Tests pin that property names differing only in case, the same name in two objects, an escaped spelling of a repeated name and an undecodable string directly inside an array are each handled as intended. Refs #48 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- .../Themes/ReaderTheme.cs | 30 +++++++++-- .../UntrustedInputValidationTests.cs | 52 +++++++++++++++++-- 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs index 6d10048..fd0f5f9 100644 --- a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs +++ b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs @@ -6,6 +6,7 @@ using System.Globalization; using System.Text; +using System.Text.Encodings.Web; using System.Text.Json; using System.Text.Json.Nodes; @@ -17,6 +18,14 @@ public sealed class ReaderTheme { /// Bounds schema input before a native file read allocates the complete JSON text. public const int MaximumCharacters = 128 * 1024; + + /// + /// How a renamed or edited theme is written back. Characters are written as themselves where + /// JSON allows it, so a theme within the size limit stays within it: the default encoder turns + /// every quote mark, angle bracket and non-ASCII character into a six-character escape. Theme + /// JSON is stored and exported, never placed in a page. + /// + private static readonly JsonSerializerOptions s_compact = new() { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; private static readonly string[] s_colors = ["background", "text", "heading1", "heading2", "heading3", "heading4", "heading5", "heading6", "link", "codeBackground", "codeText", "blockquoteBorder", "blockquoteBackground", "highlightBackground", "highlightText"]; private static readonly string[] s_sizes = ["baseFontSize", "h1Size", "h2Size", "h3Size", "h4Size", "h5Size", "h6Size", "codeFontSize", "lineHeight"]; private static readonly string s_baseStyles = LoadBaseStyles(); @@ -59,7 +68,7 @@ public sealed class ReaderTheme { JsonNode source = JsonNode.Parse(Json)!; source["name"] = name; - return ParseCompatible(source.ToJsonString()); + return ParseCompatible(source.ToJsonString(s_compact)); } @@ -103,7 +112,7 @@ public sealed class ReaderTheme } source["colors"]![key] = value?.Trim(); } - return ParseCompatible(source.ToJsonString()); + return ParseCompatible(source.ToJsonString(s_compact)); } @@ -154,7 +163,7 @@ public sealed class ReaderTheme { throw new JsonException("The theme exceeds the 128 Ki-character limit."); } - using var document = JsonDocument.Parse(json, new JsonDocumentOptions { MaxDepth = 16 }); + using JsonDocument document = ParseDocument(json); JsonElement root = document.RootElement; RequireObject(root); RequireReadableText(root); @@ -279,6 +288,21 @@ public sealed class ReaderTheme + /// Reads the theme text as JSON; text that is not well-formed UTF-16 is an invalid theme like any other malformed input. + private static JsonDocument ParseDocument(string json) + { + try + { + return JsonDocument.Parse(json, new JsonDocumentOptions { MaxDepth = 16 }); + } + catch(ArgumentException exception) + { + throw new JsonException("The theme contains text that cannot be decoded.", exception); + } + } + + + /// /// Checks every property name and string in the theme once, including ones this version does /// not read, because a kept theme is later renamed, edited and re-serialized as a whole. An diff --git a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs index 937d7d1..de6c2d6 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs @@ -285,6 +285,8 @@ public sealed class UntrustedInputValidationTests [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"name\": \"Nord\"")] [InlineData("\"body\": \"System\"", "\"body\": \"System\", \"body\": \"Other\"")] [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"extra\": {\"a\": 1, \"a\": 2}")] + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"na\\u006de\": \"Nord\"")] + [InlineData("\"name\": \"Nord\"", "\"name\": \"Nord\", \"notes\": [\"\\uD83D\"]")] public void Parse_UndecodableTextOrRepeatedProperty_IsRejectedAsAnInvalidTheme(string valid, string invalid) { string original = ThemeCatalog.Find("Nord").Json; @@ -300,13 +302,14 @@ public sealed class UntrustedInputValidationTests /// - /// A theme that parses can be renamed, copied, edited and imported under a new name, whatever - /// extra properties it carries: those operations re-serialize the whole document. + /// A theme that parses can be renamed, copied, edited and imported under a new name with its + /// extra properties intact: those operations re-serialize the whole document. Names that differ + /// only in case, and the same name in two different objects, are not repeats. /// [Fact] public void Parse_AcceptedThemeWithExtraProperties_SurvivesRenameCopyEditAndImport() { - string json = ThemeCatalog.Find("Nord").Json.Replace("\"name\": \"Nord\"", "\"name\": \"Nord\", \"note\": \"caf\\u00e9 \\ud83d\\ude00\", \"future\": {\"list\": [1, \"two\", null]}", StringComparison.Ordinal); + string json = ThemeCatalog.Find("Nord").Json.Replace("\"name\": \"Nord\"", "\"name\": \"Nord\", \"Name\": \"other case\", \"note\": \"caf\\u00e9 \\ud83d\\ude00\", \"future\": {\"name\": \"inner\", \"list\": [1, \"two\", null]}", StringComparison.Ordinal); var theme = ReaderTheme.Parse(json); Assert.Equal("Renamed", theme.Rename("Renamed").Name); AppSettings imported = ThemeManagement.Import(new AppSettings(), theme); @@ -315,6 +318,49 @@ public sealed class UntrustedInputValidationTests ReaderTheme edited = theme.Edit("Edited", new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), ""); Assert.Equal("Edited", edited.Name); Assert.Contains("caf", edited.Json, StringComparison.Ordinal); + Assert.Contains("\"Name\":\"other case\"", edited.Json, StringComparison.Ordinal); + Assert.Contains("\"name\":\"inner\"", edited.Json, StringComparison.Ordinal); + } + + + + /// + /// Renaming, copying and editing write characters as themselves, so a theme near the size + /// limit whose text is full of quote marks, angle brackets or non-ASCII characters stays + /// within the limit instead of growing sixfold and being refused. + /// + [Theory] + [InlineData("a>b+c{font:'x'}")] + [InlineData("/* \u6f22\u5b57 caf\u00e9 & */")] + public void Rename_ThemeNearTheSizeLimit_DoesNotGrowPastIt(string fragment) + { + string css = string.Concat(Enumerable.Repeat(fragment, 60_000 / fragment.Length)); + string original = ThemeCatalog.Find("Nord").Json; + Assert.Contains("\"name\": \"Nord\"", original, StringComparison.Ordinal); + string json = original.Replace("\"name\": \"Nord\"", "\"name\": \"Nord\", \"note\": \"" + css + "\"", StringComparison.Ordinal); + var theme = ReaderTheme.Parse(json); + ReaderTheme renamed = theme.Rename("Renamed"); + Assert.True(renamed.Json.Length <= json.Length); + Assert.Contains(fragment, renamed.Json, StringComparison.Ordinal); + ReaderTheme edited = renamed.Edit("Edited", new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), ""); + Assert.True(edited.Json.Length <= json.Length); + Assert.EndsWith(" Copy", ThemeManagement.CopyTheme(new AppSettings(), theme).Name, StringComparison.Ordinal); + } + + + + /// A string that is not well-formed UTF-16 is refused as an invalid theme, and a stored one is dropped, rather than failing as a programming error. + [Fact] + public void Parse_TextWithAnUnpairedSurrogate_IsRejectedAsAnInvalidTheme() + { + string original = ThemeCatalog.Find("Nord").Json; + string json = original.Replace("\"name\": \"Nord\"", "\"name\": \"No\uD83Drd\"", StringComparison.Ordinal); + Assert.DoesNotContain("\"name\": \"Nord\"", json, StringComparison.Ordinal); + Assert.Throws(() => ReaderTheme.Parse(json)); + Assert.Throws(() => ReaderTheme.ParseCompatible(json)); + var settings = new AppSettings { UserThemes = [json, original] }; + settings.Sanitize(); + Assert.Equal([original], settings.UserThemes); } -- 2.54.0 From 7d3cf921ce2fecfc999b3839a0cb97ea85e38b0d Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 10 Nov 2026 18:27:41 -0600 Subject: [PATCH 5/5] fix(themes): leave room for a kept theme to grow when rewritten JSON writes emoji, no-break spaces and private-use glyphs as six-character escapes, so renaming a theme made of them could push it past the import limit and be refused. The import limit still bounds the file read; a kept theme now has its own, larger limit, so a theme that could be imported can always be renamed, copied and edited. Refs #48 Co-Authored-By: Claude Fable 5.2 Signed-off-by: James Maes --- .../Themes/ReaderTheme.cs | 22 +++++++++---- .../ReaderThemeTests.cs | 7 ++-- .../UntrustedInputValidationTests.cs | 32 +++++++++++++++++++ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs index fd0f5f9..9c52487 100644 --- a/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs +++ b/src/MarkdownViewr.Windows.Core/Themes/ReaderTheme.cs @@ -16,14 +16,22 @@ namespace MarkdownViewr.Windows.Core.Themes; /// Validates Darin's version-one theme format before generating passive document CSS. public sealed class ReaderTheme { - /// Bounds schema input before a native file read allocates the complete JSON text. + /// The largest theme file accepted for import; bounds the read before the complete JSON text is allocated. public const int MaximumCharacters = 128 * 1024; /// - /// How a renamed or edited theme is written back. Characters are written as themselves where - /// JSON allows it, so a theme within the size limit stays within it: the default encoder turns - /// every quote mark, angle bracket and non-ASCII character into a six-character escape. Theme - /// JSON is stored and exported, never placed in a page. + /// The largest theme the app keeps. Renaming or editing rewrites the whole theme, and JSON + /// writes some characters (emoji, no-break spaces, private-use glyphs) as six-character + /// escapes, so a kept theme can be several times the size of the file it was imported from. + /// The bound leaves room for that, so a theme that could be imported can always be renamed. + /// + public const int MaximumStoredCharacters = 8 * MaximumCharacters; + + /// + /// How a renamed or edited theme is written back. Quote marks, angle brackets and most + /// non-ASCII letters are written as themselves instead of as six-character escapes, which + /// keeps ordinary CSS and text close to its imported size. Theme JSON is stored and exported, + /// never placed in a page. /// private static readonly JsonSerializerOptions s_compact = new() { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; private static readonly string[] s_colors = ["background", "text", "heading1", "heading2", "heading3", "heading4", "heading5", "heading6", "link", "codeBackground", "codeText", "blockquoteBorder", "blockquoteBackground", "highlightBackground", "highlightText"]; @@ -159,9 +167,9 @@ public sealed class ReaderTheme private static ReaderTheme ParseSchema(string json, bool allowNewer) { ArgumentNullException.ThrowIfNull(json); - if(json.Length > MaximumCharacters) + if(json.Length > MaximumStoredCharacters) { - throw new JsonException("The theme exceeds the 128 Ki-character limit."); + throw new JsonException("The theme is too large."); } using JsonDocument document = ParseDocument(json); JsonElement root = document.RootElement; diff --git a/tests/MarkdownViewr.Windows.Core.Tests/ReaderThemeTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/ReaderThemeTests.cs index 7b31b34..3d3f688 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/ReaderThemeTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/ReaderThemeTests.cs @@ -124,12 +124,15 @@ public sealed class ReaderThemeTests - /// Null and oversized input are rejected before parser work. + /// Null input is refused, and a theme is kept up to the stored size limit exactly and refused one character past it. [Fact] public void Parse_OutsideSizeContract_RejectsInput() { Assert.Throws(() => ReaderTheme.Parse(null!)); - Assert.Throws(() => ReaderTheme.Parse(new string(' ', 128 * 1024 + 1))); + string valid = ThemeCatalog.Find("Nord").Json; + Assert.Equal("Nord", ReaderTheme.Parse(valid.PadRight(ReaderTheme.MaximumStoredCharacters)).Name); + Assert.Throws(() => ReaderTheme.Parse(valid.PadRight(ReaderTheme.MaximumStoredCharacters + 1))); + Assert.Equal(8 * 128 * 1024, ReaderTheme.MaximumStoredCharacters); } diff --git a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs index de6c2d6..1840f31 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/UntrustedInputValidationTests.cs @@ -349,6 +349,38 @@ public sealed class UntrustedInputValidationTests + /// + /// JSON writes emoji, no-break and ideographic spaces and private-use glyphs as six-character + /// escapes whatever the encoder. A theme as large as an import may be, made of nothing else, + /// can still be renamed, copied and edited: the stored limit leaves room for the growth. + /// + [Theory] + [InlineData("😀")] + [InlineData(" ")] + [InlineData("　")] + [InlineData("")] + public void Rename_ThemeAtTheImportLimitMadeOfEscapedCharacters_IsKept(string character) + { + string original = ThemeCatalog.Find("Nord").Json; + string opening = "\"name\": \"Nord\", \"note\": \""; + int room = ReaderTheme.MaximumCharacters - original.Length - (opening.Length - "\"name\": \"Nord\"".Length) - 1; + string filler = string.Concat(Enumerable.Repeat(character, room / character.Length)).PadRight(room, 'x'); + string json = original.Replace("\"name\": \"Nord\"", opening + filler + "\"", StringComparison.Ordinal); + Assert.Equal(ReaderTheme.MaximumCharacters, json.Length); + var theme = ReaderTheme.Parse(json); + ReaderTheme renamed = theme.Rename("A considerably longer name than the one it had"); + Assert.InRange(renamed.Json.Length, json.Length + 1, ReaderTheme.MaximumStoredCharacters); + Assert.Equal(renamed.Json.Length, renamed.Rename(renamed.Name).Json.Length); + Assert.EndsWith(" Copy", ThemeManagement.CopyTheme(new AppSettings(), renamed).Name, StringComparison.Ordinal); + ReaderTheme edited = renamed.Edit("Edited", new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), new Dictionary(StringComparer.Ordinal), "body { color: red }"); + Assert.Equal("Edited", edited.Name); + var settings = new AppSettings { UserThemes = [edited.Json] }; + settings.Sanitize(); + Assert.Equal([edited.Json], settings.UserThemes); + } + + + /// A string that is not well-formed UTF-16 is refused as an invalid theme, and a stored one is dropped, rather than failing as a programming error. [Fact] public void Parse_TextWithAnUnpairedSurrogate_IsRejectedAsAnInvalidTheme() -- 2.54.0