fix(security): close remaining high-severity CodeQL alerts

- svg-sanitize.ts: strip each dangerous element repeatedly until stable with
  whitespace-tolerant end tags, defeating nested/overlapping tags (closes 5
  incomplete-multi-character-sanitization + 1 bad-tag-filter; the prior
  single-pass regex could leave a residual <script>/<iframe>).
- file-preview.ts: add a resolve()+containment barrier (the path-traversal
  guard CodeQL recognizes) on top of the id charset check (closes 9
  path-injection).
- metadata.ts: bound the XMP namespace:name key segments so parseXmp cannot
  backtrack polynomially (closes js/polynomial-redos).
- analytics-disabled.spec.ts: match analytics by URL host, not substring
  (closes 4 incomplete-url-substring-sanitization).

typecheck + lint green; svg (119), preview (22), metadata (164) tests pass.
This commit is contained in:
SnapOtter
2026-06-21 13:47:22 +08:00
parent 4fdd10f488
commit bdadb843d8
4 changed files with 63 additions and 30 deletions
+29 -19
View File
@@ -11,6 +11,21 @@ function decodeNumericEntities(input: string): string {
.replace(/&#(\d+);/g, (_m, dec) => String.fromCharCode(Number.parseInt(dec, 10)));
}
/**
* Apply removal regexes repeatedly until the string stops changing, so that
* nested or overlapping dangerous tags (e.g. `<scr<script>ipt>`) cannot survive
* a single pass.
*/
function stripUntilStable(input: string, ...patterns: RegExp[]): string {
let prev: string;
let out = input;
do {
prev = out;
for (const pattern of patterns) out = out.replace(pattern, "");
} while (out !== prev);
return out;
}
/**
* Sanitize an SVG buffer to prevent XXE, SSRF, and script injection.
* Throws if the SVG exceeds the maximum allowed size.
@@ -49,28 +64,23 @@ export function sanitizeSvg(buffer: Buffer): Buffer {
// Remove XML processing instructions except <?xml version...?>
svg = svg.replace(/<\?(?!xml\s)[^?]*\?>/gi, "");
// Remove XInclude elements and namespace declarations
svg = svg.replace(/<[^>]*xi:include[^>]*\/?>/gi, "");
svg = stripUntilStable(svg, /<[^>]*\bxi:include\b[^>]*\/?>/gi);
svg = svg.replace(/xmlns:xi\s*=\s*["'][^"']*["']/gi, "");
// ── Strip dangerous elements ──
// Remove script tags (including nested inside <svg>)
svg = svg.replace(/<script[\s\S]*?<\/script>/gi, "");
svg = svg.replace(/<script[^>]*\/>/gi, "");
// Remove foreignObject elements (can embed arbitrary HTML)
svg = svg.replace(/<foreignObject[\s\S]*?<\/foreignObject>/gi, "");
svg = svg.replace(/<foreignObject[^>]*\/>/gi, "");
// Remove iframe elements (non-SVG, can load external content)
svg = svg.replace(/<iframe[\s\S]*?<\/iframe>/gi, "");
svg = svg.replace(/<iframe[^>]*\/>/gi, "");
// Remove embed elements (non-SVG, can load external content)
svg = svg.replace(/<embed[\s\S]*?<\/embed>/gi, "");
svg = svg.replace(/<embed[^>]*\/>/gi, "");
// Remove <set> elements (can inject attributes/URIs at runtime)
svg = svg.replace(/<set[\s\S]*?<\/set>/gi, "");
svg = svg.replace(/<set\b[^>]*\/>/gi, "");
// Remove <animate> elements (can inject attributes/URIs at runtime)
svg = svg.replace(/<animate[\s\S]*?<\/animate>/gi, "");
svg = svg.replace(/<animate\b[^>]*\/>/gi, "");
// For each dangerous element remove the paired form (with a whitespace-tolerant
// end tag, e.g. `</script >`), the self-closing form, and any residual open or
// close tag -- repeating until stable so nested or overlapping tags cannot
// survive a single pass (foreignObject/iframe/embed can embed HTML; set/animate
// can inject attributes/URIs at runtime).
for (const tag of ["script", "foreignObject", "iframe", "embed", "set", "animate"]) {
svg = stripUntilStable(
svg,
new RegExp(`<${tag}\\b[\\s\\S]*?<\\/${tag}\\s*>`, "gi"),
new RegExp(`<${tag}\\b[^>]*>`, "gi"),
new RegExp(`<\\/${tag}\\s*>`, "gi"),
);
}
// Remove event handlers (onload, onclick, onerror, etc.)
// Replace both the attribute name and its value to prevent residual payloads.
+18 -4
View File
@@ -9,7 +9,7 @@ import { randomUUID } from "node:crypto";
import { createReadStream } from "node:fs";
import { access, copyFile, mkdir, readFile, rename, rm, writeFile } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { join, resolve, sep } from "node:path";
import { convertDocument, sofficeAvailable } from "@snapotter/doc-engine";
import { runFfmpeg } from "@snapotter/media-engine";
import { eq } from "drizzle-orm";
@@ -33,8 +33,22 @@ async function ensurePreviewDir(): Promise<void> {
previewDirReady = true;
}
/**
* Resolve a name inside the preview directory and verify the result cannot
* escape it. The id is already charset-validated at the route; this containment
* check is the authoritative path-traversal barrier for every preview path.
*/
function resolveWithinPreviewDir(name: string): string {
const base = resolve(previewDirPath());
const resolved = resolve(base, name);
if (resolved !== base && !resolved.startsWith(base + sep)) {
throw new Error("Preview path escapes the preview directory");
}
return resolved;
}
function previewPath(fileId: string, ext: string): string {
return join(previewDirPath(), `${fileId}${ext}`);
return resolveWithinPreviewDir(`${fileId}${ext}`);
}
async function fileExists(path: string): Promise<boolean> {
@@ -127,7 +141,7 @@ export async function filePreviewRoutes(app: FastifyInstance): Promise<void> {
// Restrict to an alphanumeric extension (no path separators) -- the
// original filename is user-controlled and feeds a filesystem path.
const origExt = file.originalName.match(/\.[a-zA-Z0-9]+$/)?.[0] ?? "";
const tempInput = join(previewDirPath(), `${id}-input${origExt}`);
const tempInput = resolveWithinPreviewDir(`${id}-input${origExt}`);
await copyFile(inputPath, tempInput);
try {
@@ -136,7 +150,7 @@ export async function filePreviewRoutes(app: FastifyInstance): Promise<void> {
});
// convertDocument outputs next to the temp file; rename to cached path
const producedPath = join(previewDirPath(), `${id}-input.pdf`);
const producedPath = resolveWithinPreviewDir(`${id}-input.pdf`);
await rename(producedPath, cachedPath);
} catch (err) {
request.log.error({ err, fileId: id }, "Document preview generation failed");
+3 -1
View File
@@ -137,7 +137,9 @@ export function parseXmp(xmpBuffer: Buffer): Record<string, string> {
const xml = xmpBuffer.toString("utf-8");
const result: Record<string, string> = {};
for (const match of xml.matchAll(/(\w+:\w+)="([^"]+)"/g)) {
// Bound the namespace:name key segments (real XMP keys are short) so the
// pattern cannot backtrack polynomially on hostile input (CodeQL js/polynomial-redos).
for (const match of xml.matchAll(/(\w{1,80}:\w{1,80})="([^"]+)"/g)) {
const key = match[1];
if (key.startsWith("xmlns:") || key.startsWith("rdf:")) continue;
result[key] = match[2];
+13 -6
View File
@@ -57,13 +57,20 @@ test.describe("Analytics disabled by server", () => {
// Intercept ALL network requests and log any that hit analytics domains
await page.route("**/*", (route) => {
const url = route.request().url();
// Match on the URL host, not a substring, so an unrelated host that merely
// contains "posthog"/"sentry" can't false-trigger (CodeQL
// js/incomplete-url-substring-sanitization).
let host = "";
try {
host = new URL(url).hostname.toLowerCase();
} catch {
// non-URL scheme (data:/blob:) -- not an analytics host
}
if (
url.includes("posthog.com") ||
url.includes("posthog") ||
url.includes("sentry.io") ||
url.includes("sentry") ||
url.includes("us.i.posthog.com") ||
url.includes("ingest.sentry.io")
host === "posthog.com" ||
host.endsWith(".posthog.com") ||
host === "sentry.io" ||
host.endsWith(".sentry.io")
) {
analyticsRequests.push(url);
}