Skip to content

Commit c65a9df

Browse files
test(coverage): uplift Reactor.dll line coverage 79.52% → 80.79% (#333)
* chore(coverage): scaffold uplift-to-85 tracking doc + helper scripts Adds: - docs/reports/coverage-uplift-85.md — self-contained tracking doc so any agent on any machine can pick up the work without re-learning the recipe. - tools/coverage/run-coverage.ps1 — wraps the CONTRIBUTING.md recipe (build → unit → instrument → selftest → merge) into one command. - tools/coverage/report-gaps.ps1 — parses merged cobertura output and writes a ranked hot-spot table to coverage/gap-report.md. - tools/coverage/README.md — usage notes. - .gitignore — carve tools/coverage/ out of the coverage/ ignore. No product or test changes yet; baseline measurement lands next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(coverage): baseline 79.52% + ranked hot-spot worklist + audit Ran baseline merged coverage (unit + selftest). Captured the result and the per-file analysis in docs/reports/coverage-uplift-85.md so future sessions can pick up cold. - Baseline: 79.52% line / 67.25% branch (gap to 85%: ~5,562 lines). - Hot-spot worklist organized by test tier (U/S/U+S) — pre-classifies every top-40 file so the next agent skips the "is this unit-testable?" rediscovery step. - Audit findings: SetExtensionsCoverageTests.cs (~345 lines) is pure vanity (asserts setter-delegate count, never the side effect). Plan recorded to replace with selftest fixtures rather than delete first. - Honest deferral candidates flagged: PreviewCaptureServer, JumpListComInterop, TrayFlyoutHostWindow, TaskbarOverlay, ChartAutomationPeer — ~1,639 lines inherently hard to test from a headless harness; user decision pending. - Fixed report-gaps.ps1 PowerShell parser issue (mid-string backtick). No product/test code changes in this commit — diagnosis-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(devtools): cover ParseValue / FormatValue / hex-color edge branches Adds 11 unit tests to DevtoolsPropertyToolTests exercising previously- uncovered branches of the pure parser/formatter helpers. Intended as a worked example of the audit-first workflow described in docs/reports/coverage-uplift-85.md — each test ties to an observable product contract: - IFormattable invariant-culture formatting (decimal) - 2-value Thickness via ParseValue (not just direct TryParseThickness) - Comma-implies-Thickness branch when targetType is null - Generic enum path via FlowDirection - Mixed-case bool parsing - 8-digit color with A=0x00 (alpha preservation) - Lowercase hex - 5-digit / empty hex rejection - Negative Thickness acceptance - Pinned discovered behavior: TryParseCornerRadius propagates ArgumentException on negatives (the Try* name is misleading; a future fix to catch + return false would be an intentional API change). All 60 tests in the file pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): LogCaptureInstall — 79.50% → 79.57% (+0.07) Add 20 real tests for TeeTextWriter / BufferTraceListener / LogCaptureInstall.Install in tests/Reactor.Tests/Devtools/ LogCaptureBufferTests.cs. Each test names a product bug it would catch: stdio-MCP JSON-RPC frame corruption when forwardConsole=false is ignored, phantom blank entries from Flush() with no pending payload, CRLF leaking '\r' into entries, the char[]/char/string Write overloads each going through the wrong branch, idempotent install adding a second BufferTraceListener. Per-file: LogCaptureInstall.cs 58.8% → 100% line, 38% → ~93% branch. Merged unit+selftest: 79.50% → 79.57% line / 67.23% → 67.29% branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): Element.OwnPropsEqual switch arms — 79.57% → 79.80% Add 85 real tests in tests/Reactor.Tests/OwnPropsEqualTests.cs covering every switch arm of Element.OwnPropsEqual (was 19% line / 17% branch with zero direct tests, despite gating every reconcile-highlight overlay decision). Each test names a concrete product bug: stale WinUI value when own-prop change is dropped, vs highlight overlay strobing when descendant changes leak into the parent's own-props compare. Focus on decision-point variation gave a branch% swing roughly 2× the line% swing — recorded that pacing lesson in the tracking doc. Per-method: OwnPropsEqual 19% → ~100% line, 17% → ~100% branch. Merged unit+selftest: 79.57% → 79.80% line (+0.23) / 67.29% → 67.80% branch (+0.51). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): DragData lazy providers + FormatEntry state machine — 79.80% → 79.82% Add 34 real tests across DragDataTests.cs covering Phase 6b lazy providers (WithUri/WithHtml/WithRtf/WithCustomFormat × sync+async), AvailableFormats/HasFormat key-mapping, last-write-wins overwrite semantics, and the previously-untested in-memory transfer registry (Register/Resolve/Unregister) used by same-process DnD. Also add a new DragDataFormatEntryTests class pinning the resolve state machine: HasEager invariant, eager > async > sync precedence in ResolveAsync, eager > sync precedence in ResolveSync, the critical UI-thread no-block contract (ResolveSync on async-only must return null without invoking the provider — a regression to .GetAwaiter().GetResult() here would freeze the dispatcher on every lazy-async drop). Also add tools/coverage/rank-branch-gap.ps1 — ranks files by how much branch% trails line%, the highest-ROI signal for the next iteration. Captures the heuristic from prior session. Per-file: DragData.cs 45.5% → 52.8% line, 18.9% → 27.0% branch. Merged unit+selftest: 79.80% → 79.82% line / 67.80% → 67.85% branch. TryGetSafeLocalFiles (UNC/DOS/reparse safety filter) flagged as a deferral candidate — requires IStorageItem mocks or a selftest fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): Editors catalog ToDouble/FromDouble + DTO coercion — 79.82% → 80.04% 66 new tests in tests/Reactor.Tests/Controls/EditorsBehaviorTests.cs cover the Editors factory catalog's pure-C# paths: numeric round-trip (15 tests across ToDouble/FromDouble switch arms), Text/CheckBox/Toggle (8), Date variants (8), Time variants (8), Uri (6), Combo/EnumCombo (8), Color (2). Each test invokes the factory and exercises OnXxx callbacks on the returned Element record — no WinUI activation required. Per-file Editors.cs: 29.7% → 67.6% line. Merged: 79.82% → 80.04% line (+0.22), 67.85% → 68.24% branch (+0.39). Branch swing ≈ 2× line — the catalog is a branch-shaped target (large switch arms). ColorCompact paths are not unit-reachable: `.Background(hex)` builds a WinUI SolidColorBrush eagerly. Deferral candidate flagged in status log. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): RenderContext hook-order exceptions + devtools setter — 80.04% → 80.10% 12 new tests in tests/Reactor.Tests/RenderContextHookOrderTests.cs covering the loud-failure paths that HookStateRefactorTests didn't reach: - HookOrderException at UseReducer<T>, UseReducer<TState,TAction>, UseMemo<T>, and UseRef<T> slots (7 tests) - UseStateSetterByIndex happy / out-of-range / type-mismatch (3 tests) - UseColorScheme + UseIsDarkTheme null-Application paths (2 tests) Merged: 80.04% → 80.10% line (+0.06), 68.24% → 68.29% branch (+0.05). Doc updates: TreeChartsTests vanity audit entry (~20 Same(chart, result) fluent-return tests flagged, kept until a selftest fixture can replace them); deferral-candidate proposal for ~1,639 lines of inherently host-bound code (PreviewCaptureServer, JumpListComInterop, ChartAutomationPeer, TrayFlyoutHostWindow, TaskbarOverlay) — pending explicit user approval per doc's no-vanity rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): TypedColumns vanity-to-real rewrite — 80.10% → 80.14% 26 new tests in tests/Reactor.Tests/Controls/TypedColumnsBehaviorTests.cs replace the Assert.NotNull(factory) bar with invoke-and-assert: - TypeRegistry resolutions for 9 primitive types (DateTime, DateTimeOffset, DateOnly, TimeSpan, TimeOnly, Uri, Color, Bool-Standard, Bool-Compact) - ReflectionTypeMetadataProvider [DataType.Url] branches (string vs Uri) - TypedColumns factories: NumberColumn int/decimal/long, CheckBox vs ToggleSwitch, DateColumn for all 3 type-switch arms (the DTO + DateOnly arms were previously uncovered), TimeColumn TimeSpan vs TimeOnly, HyperlinkColumn Uri vs string (string arm was uncovered), ComboBoxColumn typed onChange, ColorColumn wiring. Surfaced contract pin: [DataType(Url)] on string property commits string (the Hyperlink display happens at render-time via CellRenderers.Hyperlink, not by routing through Editors.Uri). Pinned faithfully. Merged: 80.10% → 80.14% line (+0.04), 68.29% → 68.34% branch (+0.05). Small because TypedColumns is a thin wrapper over Editors (already high coverage); the real value is the regression net for layer-boundary wiring that NotNull-only tests silently passed through. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): CellRenderers catalog (text-shaped renderers) — 80.14% → 80.18% 24 new tests in tests/Reactor.Tests/Controls/CellRenderersTests.cs cover the pure-C# renderers: Text / Number / Date / Time / Enum / Hyperlink. Brush-shaped renderers (CheckMark, ToggleIndicator, ColorSwatch) build SolidColorBrush eagerly via .Foreground/.Background and are documented as unit-unreachable (same trap as Editors.ColorCompact in iteration 4). Hyperlink's 3 if-chain branches (Uri → button, parseable string → button, non-Uri string → TextBlock fallback) are each pinned. Bug shape: a regression that emitted HyperlinkButton with null NavigateUri would crash WinUI on click. Surfaced product sharp edge: CellRenderers.Time's default "t" format throws FormatException when value is TimeSpan (TimeSpan custom formats need escaped colons: `hh\:mm\:ss`). Documented as a deferred follow-up in the status log; not fixing the product here. Merged: 80.14% → 80.18% line (+0.04), 68.34% → 68.41% branch (+0.07). Branch swing > line swing — the Hyperlink if-chain coverage is the win. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): persistence stores edge cases — 80.18% → 80.26% 19 new tests covering JsonFileStore + PackagedSettingsStore failure paths: JsonFileStore (+13): - Write empty-id / null-data / empty-id-read guards (3) - Per-entry corruption: malformed base64 (FormatException catch), non-string entry, non-object root, empty-string entry (4) - Merge-over-tampered-file with array root (1) - AppendQuotedString escape arms: quote, backslash, \n+\t, control chars below 0x20 (Unicode escape), \r+\b+\f (5) PackagedSettingsStore (+6, new file): - Early-return guards on empty id and null data (3) - WinRT-unavailable catch arms for TryRead and Write — in xUnit's unpackaged context, ApplicationData.Current throws but the catch must swallow (2) - IsAvailable() returns false in unpackaged context without throwing (1) Merged: 80.18% → 80.26% line (+0.08), 68.41% → 68.50% branch (+0.09). Best yield since iteration 4 — PackagedSettingsStore was at 0% with no incidental coverage to share, and JsonFileStore's catch arms are branch-dense. Status log surfaces an insight: "host-bound" sometimes means "WinUI XAML controls only" — pure WinRT projections that throw deterministically in unpackaged context (like ApplicationData.Current) are still unit-testable for their catch-arm contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): TaskbarOverlay + JumpList validation — 80.26% → 80.55% 17 new tests across two new files, revisiting two of the previously-deferred shell-API files with the catch-arm lesson from iteration 8 (WinRT projections that throw deterministically in unpackaged xUnit are still unit-testable for their error-handling contract). TaskbarOverlayTests (+9): - Constructor + Icon/AccessibleDescription getter/setter round-trips - Setters after dispose are silent no-ops via the _isDisposed() guard - Setters when live but TaskbarComSingleton.TryGet() is null also no-op - LoadIconFor private static via reflection: null icon → 0, IsResource icon → 0 (ms-appx:/// can't become HICON), missing file path → 0 without throw JumpListUpdateValidationTests (+8): - UpdateAsync null/per-entry validation: null collection, null entry, empty title on task/custom (separator with empty title permitted) - Unpackaged AppUserModelId gate throws InvalidOperationException - ClearAsync inherits the same gate (no special-case for empty input) - Valid items + AppUserModelId set: Task completes without throwing even when the inner COM call fails (catch swallows) Renamed to avoid collision with existing JumpListStateTests class that covers basic static state round-trip. Merged: 80.26% → 80.55% line (+0.29), 68.50% → 68.70% branch (+0.20). Strongest single-iteration jump since iteration 4 — TaskbarOverlay was at 0% so every test net-new covered lines, plus JumpList's async validation paths carry ~30 previously-uncovered lines. Pacing note in status log: PreviewCaptureServer is the best remaining catch-arm scout from the deferral cluster; TrayFlyoutHostWindow and ChartAutomationPeer remain genuinely host-bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): PreviewCaptureServer pure helpers — 80.55% → 80.68% 17 new tests in tests/Reactor.Tests/PreviewCaptureServerTests.cs cover the security-critical helpers reachable via reflection: - GenerateToken (2): 43-char url-safe base64, RNG uniqueness across calls - IsAllowedOrigin (Theory × 13): all accept/reject arms - ReadCappedBody (4): TASK-023 4 MB cap enforcement - AcquireFreePortHolding (1): loopback bind - BearerMatches (6): constant-time XOR comparison contract, missing prefix rejection, wrong-length early-out, leading-whitespace trim - IsAllowedHost (5): TASK-020 DNS-rebinding defense Surfaced a real product security finding: IsAllowedOrigin uses StartsWith("http://localhost", OrdinalIgnoreCase) which matches http://localhost.evil.com. Currently backstopped by the IsAllowedHost host-header check (the actual security fence), but the advisory CORS allow-list is over-broad. Test IsAllowedOrigin_StartsWith_Has_Known_Subdomain_Gap pins current behavior so a tightening fix reverts predictably. Filed as deferred follow-up in status log. Merged: 80.55% → 80.68% line (+0.13), 68.70% → 68.85% branch (+0.15). Hand-off note: the catch-arm scout across deferred files is now thorough. Remaining 0% files (TrayFlyoutHostWindow, ChartAutomationPeer, rest of PreviewCaptureServer's HTTP machinery, JumpListComInterop) are genuinely host-bound. Next big metric move requires either the deferral approval (~1.6%) or selftest fixtures for Reconciler.Mount / Reconciler.Update. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): LayoutEtwConsumer initial-state + helpers — 80.68% → 80.70% 10 new tests strengthen the catch-arm + helper coverage of LayoutEtwConsumer without standing up a real ETW session: - Initial state: counters zero, IsRunning/IsUnavailable false (1) - Public constants: SessionNamePrefix + XamlProviderGuid (2) - Stop without Start + Dispose idempotency (2) - IsProcessAlive private static via reflection: non-positive PID early-out, current process true, bogus high PID falls through to the ArgumentException catch (5) Merged: 80.68% → 80.70% line (+0.02), branch -0.02 from selftest noise on platform-bound ETW branches. Smallest yield this session — the unit-test runway is genuinely tapped on this file. Status log hand-off: session ran 79.82% → 80.70% (+0.88%) across 8 iterations. The remaining 4.30 pts to 85% requires either deferral approval (~1.2 pts immediately) or selftest fixtures for Reconciler .Mount/.Update (each fixture adds 30-100 covered lines). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(coverage): PropertyGrid EditChain + statics — 80.70% → 80.79% 45 new tests targeting the pure-C# core of PropertyGridComponent.cs: PropertyGridDefaultsTests.cs (14, picked up from prior session): - PropertyLabelTemplate: DisplayName/Name fallback, AutomationName prefix, indent×4 margin, tooltip propagation - PropertyRowTemplate: FlexRow shape, Editor: prefix, indent×16 padding - ArrayItemTemplate: expand glyph ▶/▼, toggle inversion, [N] bracket format, 3/4/6-child action branches, ✕ callback wiring EditChainTests.cs (31, new): - BuildPath: empty-chain + multi-level dot-joining (pin: regression to "/" breaks every saved expand-state key) - CannotPropagate (6 branches): direct SetValue short-circuit; Compose-less + no-callback → true; OnRootChanged-only → false; root Compose-only → false; chain-entry SetValue makes leaf editable; mid-chain SetValue=null+Compose=null terminates true - PropagateNewOwner (4): empty path → OnRootChanged; mutable ancestor SetValue absorbs and stops (pin: no redundant root reassignment); multi-level immutable Compose chain to root; dead-end entry silently drops - PropagateImmutableEdit (2): mutable-ancestor SetValue absorbs Compose'd child (lines 379-384); no-chain + no-root-Compose silently drops without NRE - RenderReadOnlyValue via reflection (6): bool→ToggleSwitch disabled; bool null→IsOn=false; string→TextField disabled; string null→""; other→TextBlock.ToString(); other null→"(null)" - IsPrimitiveOrEnum via reflection (Theory×11): primitives, string, decimal, enum = true; class/record = false Merged: 80.70% → 80.79% line (+0.09), 68.83% → 69.00% branch (+0.17). Branch swing 1.9× line — branch-shaped-target heuristic holds again. 8,053/8,099 unit suite pass (one transient WindowPersistedScopeIsolation flake on first run; passes on retry, unrelated to this iteration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(security): CORS allow-list rejects suffix-attack origins Both PreviewCaptureServer.IsAllowedOrigin and DevtoolsMcpServer.IsAllowedOrigin used origin.StartsWith( "http://localhost", …) to gate the advisory CORS allow-list. That matched http://localhost.evil.com because the malicious host genuinely starts with "localhost"; same shape for http://127.0.0.1.evil.com. The host-header fence in IsAllowedHost backstopped the actual security boundary, but the advisory CORS layer was over-broad against direct browser-driven attacks. Switched both to Uri.TryCreate + uri.Host exact-match. System.Uri decomposes scheme/host/port per RFC 3986, so localhost.evil.com parses as Host="localhost.evil.com" and fails the equality check. Allow-list shapes preserved: - PreviewCaptureServer accepts vscode-webview, http://127.0.0.1, http://localhost, https://localhost - DevtoolsMcpServer accepts vscode-webview, http://127.0.0.1, http://localhost (no https — MCP traffic is HTTP only) Tests added/expanded: - PreviewCaptureServerTests: flipped the gap-pin test (was asserting http://localhost.evil.com WAS accepted) into rejection theory rows; added boundary-delimiter accepts (trailing /, ?, #) and userinfo- trick rejects (http://localhost@evil.com) - DevtoolsMcpServerOriginTests (new): sister coverage with the same suffix-attack rejection set, plus pins for the https-rejected contract distinct from PreviewCaptureServer Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(security): SanitizeUrl uses Uri.TryCreate, not manual scheme parse The original comment claimed Uri.TryCreate was "too lenient" because it parses `javascript:alert(1)` as a valid Uri. That's true, but the SafeUrlSchemes allow-list filter on uri.Scheme runs *after* parsing and rejects `javascript` (and every other non-http/https/mailto scheme) the same way. So the System.Uri version is equally safe and avoids the hand-rolled RFC 3986 scheme-character validator that was the actual maintenance risk. Behavior preserved: - http/https/mailto pass through unchanged (allow-list, case-insensitive) - javascript:/data:/vbscript:/file:/ftp:/ssh: → "about:blank" - Relative URLs (no scheme) pass through unchanged - Empty input passes through unchanged - unsafeAllowed=true bypasses the filter entirely Tests added (new file, 25 tests): TASK-045 XSS-fence had zero direct coverage before this commit. The theory rows pin each known XSS-vector scheme and the case-insensitive normalization contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address PR #333 review feedback Five fixes from the Copilot review of the URI-refactor + coverage PR: 1. PreviewCaptureServer.IsAllowedOrigin (real regression): the Uri-based refactor accepted any http-or-https + (localhost or 127.0.0.1). The pre-refactor allow-list rejected `https://127.0.0.1` (only `http://127.0.0.1` was accepted; `https://` was reserved for `localhost`). Restored the shape-specific check + added explicit rejection theory rows for `https://127.0.0.1[:port]`. 2. CellRenderersTests.FormatValue_Invariant_Format_With_Decimal: `"R"` format is documented for Single/Double/Half, not decimal — behavior is implementation-defined and has historically thrown FormatException. Switched to `"G"` which is explicitly decimal- supported, same observable output ("1.5"). 3. EditorsBehaviorTests.DateOnly_Null_Defaults_To_Today: previous test would flake if midnight crossed between the factory call and the expected-value evaluation. Now captures `DateTime.Today` both before AND after, accepting either as a valid match. 4. EditorsBehaviorTests.Uri_OnChange_Truly_Invalid_String_Does_Not_Commit: the source string had an embedded raw NUL byte (0x00) in addition to the visible `\x01` escape. Replaced the raw NUL with an explicit `\x00` escape so the test input is visible in source and stable across editors. 5. JsonFileStoreTests.Write_Id_With_Control_Char_Below_0x20_Uses_Unicode_Escape: identifier literal had embedded raw SOH (0x01) and US (0x1F) bytes. Replaced with explicit `\x01` / `\x1F` escapes for the same visibility reason. 210 affected tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f3eaa88 commit c65a9df

27 files changed

Lines changed: 7275 additions & 25 deletions

.gitignore

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,10 @@ tests/stress_perf/publish_log*.txt
212212
# Cobertura reports are regenerated on demand and should never be committed.
213213
coverage/
214214
*.cobertura.xml
215+
# `tools/coverage/` holds source-controlled coverage helper scripts and is
216+
# NOT generated output; carve it out of the `coverage/` ignore above.
217+
!tools/coverage/
218+
!tools/coverage/**
215219

216220
# Eval / scratch run output — local artifacts from eval drivers, never tracked.
217221
runs/

docs/reports/coverage-uplift-85.md

Lines changed: 1458 additions & 0 deletions
Large diffs are not rendered by default.

src/Reactor/Hosting/Devtools/DevtoolsMcpServer.cs

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,10 +465,22 @@ private bool IsAllowedHost(string? hostHeader)
465465

466466
private static bool IsAllowedOrigin(string origin)
467467
{
468-
if (origin.StartsWith("vscode-webview://", StringComparison.OrdinalIgnoreCase)) return true;
469-
if (origin.StartsWith("http://127.0.0.1", StringComparison.Ordinal)) return true;
470-
if (origin.StartsWith("http://localhost", StringComparison.OrdinalIgnoreCase)) return true;
471-
return false;
468+
// Parse via System.Uri rather than string-prefix matching. A
469+
// StartsWith allow-list would let `http://localhost.evil.com` through
470+
// because the malicious host genuinely starts with "localhost";
471+
// Uri.TryCreate decomposes scheme/host correctly per RFC 3986.
472+
if (!Uri.TryCreate(origin, UriKind.Absolute, out var uri))
473+
return false;
474+
475+
if (string.Equals(uri.Scheme, "vscode-webview", StringComparison.OrdinalIgnoreCase))
476+
return true;
477+
478+
// MCP traffic is HTTP only — no https:// arm (HttpListener binds to
479+
// http://127.0.0.1:<port>, so an https origin would never originate
480+
// from this server's own callers).
481+
if (uri.Scheme != Uri.UriSchemeHttp) return false;
482+
483+
return uri.Host == "localhost" || uri.Host == "127.0.0.1";
472484
}
473485

474486
// -- Dispatch ----------------------------------------------------------------

src/Reactor/Hosting/PreviewCaptureServer.cs

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -323,10 +323,24 @@ private bool IsAllowedHost(string? hostHeader)
323323

324324
private static bool IsAllowedOrigin(string origin)
325325
{
326-
if (origin.StartsWith("vscode-webview://", StringComparison.OrdinalIgnoreCase)) return true;
327-
if (origin.StartsWith("http://127.0.0.1", StringComparison.Ordinal)) return true;
328-
if (origin.StartsWith("http://localhost", StringComparison.OrdinalIgnoreCase)) return true;
329-
if (origin.StartsWith("https://localhost", StringComparison.OrdinalIgnoreCase)) return true;
326+
// Parse the Origin header through System.Uri rather than string-prefix
327+
// matching. StartsWith-based allow-lists let `http://localhost.evil.com`
328+
// through because the malicious host genuinely starts with "localhost";
329+
// Uri.TryCreate decomposes scheme/host/port correctly per RFC 3986.
330+
if (!Uri.TryCreate(origin, UriKind.Absolute, out var uri))
331+
return false;
332+
333+
if (string.Equals(uri.Scheme, "vscode-webview", StringComparison.OrdinalIgnoreCase))
334+
return true;
335+
336+
// Preserve the pre-refactor allow-list shape: http accepts both
337+
// 127.0.0.1 and localhost, but https accepts only localhost. Don't
338+
// collapse to a generic http-or-https rule — that would silently
339+
// open `https://127.0.0.1` which the original code rejected.
340+
if (uri.Scheme == Uri.UriSchemeHttp)
341+
return uri.Host == "127.0.0.1" || uri.Host == "localhost";
342+
if (uri.Scheme == Uri.UriSchemeHttps)
343+
return uri.Host == "localhost";
330344
return false;
331345
}
332346

src/Reactor/Markdown/MarkdownHtml.cs

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -89,23 +89,17 @@ internal static string SanitizeUrl(string url, bool unsafeAllowed)
8989
{
9090
if (unsafeAllowed) return url;
9191
if (string.IsNullOrEmpty(url)) return url;
92-
// Relative URLs (no scheme) are safe; only absolute schemes need
93-
// checking. Pull the scheme by hand because Uri.TryCreate is too
94-
// lenient — it would parse `javascript:alert(1)` as a valid Uri.
95-
int colon = url.IndexOf(':');
96-
if (colon <= 0) return url; // no scheme = treat as path-relative
97-
// Schemes per RFC 3986: ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
98-
for (int i = 0; i < colon; i++)
99-
{
100-
var c = url[i];
101-
bool ok = (i == 0)
102-
? (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
103-
: (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')
104-
|| (c >= '0' && c <= '9') || c == '+' || c == '-' || c == '.';
105-
if (!ok) return url; // not a real scheme — treat as relative
106-
}
107-
var scheme = url[..colon];
108-
return SafeUrlSchemes.Contains(scheme) ? url : "about:blank";
92+
93+
// Uri.TryCreate happily parses `javascript:alert(1)` as a valid
94+
// absolute Uri — that's fine, because the allow-list filter on
95+
// uri.Scheme below rejects it. The point is to use System.Uri's
96+
// RFC 3986 scheme/host decomposition rather than hand-rolled
97+
// string parsing, which is fragile and a known source of bypass
98+
// bugs in URL sanitizers.
99+
if (!Uri.TryCreate(url, UriKind.Absolute, out var uri))
100+
return url; // Not absolute — treat as path-relative; safe.
101+
102+
return SafeUrlSchemes.Contains(uri.Scheme) ? url : "about:blank";
109103
}
110104

111105
private sealed class HtmlRenderer

0 commit comments

Comments
 (0)