mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
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:
@@ -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.
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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];
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user