mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
fix(security): close the gaps a full 2.0 re-audit left open (#620)
Follow-up to a full re-audit of the 2.0 tree. Most prior findings were already fixed; this closes the ones that were not: - SAML assertion replay: validateInResponseTo ifPresent plus a Redis-backed CacheProvider, so a captured signed assertion cannot be replayed. ifPresent keeps IdP-initiated SSO working. - MFA login challenge burned after 5 wrong TOTP codes. - api_keys.key_prefix indexed; the per-request lookup was a full table scan. - MAX_AI_JOBS_PER_USER caps a user's in-flight single-file AI jobs (the AI pool runs at concurrency 1). Batch and pipeline AI stay uncapped. - MAX_WORKSPACE_SIZE_GB enforced instead of being dead config. - SUBPROCESS_MEMORY_LIMIT_MB (default off) for the native media and doc engines; not applied to the AI sidecar. - SVG sanitizer closes unquoted and whitespace-prefixed javascript: hrefs and the animateTransform/animateMotion/handler/mpath elements. - Windows-style paths stripped from error output to match the Sentry scrubber. - Postgres and Redis compose services get cap_drop plus pids_limit and cpus. - .env.example ships MAX_SVG_SIZE_MB=50 (0 disabled the cap). Adds security-focused unit and integration tests. typecheck, biome, and the full unit and integration suites pass.
This commit is contained in:
@@ -0,0 +1,17 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { isOverAiJobCap } from "../../../apps/api/src/lib/ai-quota.js";
|
||||
|
||||
describe("isOverAiJobCap", () => {
|
||||
it("is disabled (never over) when cap is 0", () => {
|
||||
expect(isOverAiJobCap(1000, 0)).toBe(false);
|
||||
});
|
||||
|
||||
it("is false while in-flight is below the cap", () => {
|
||||
expect(isOverAiJobCap(4, 5)).toBe(false);
|
||||
});
|
||||
|
||||
it("is true once in-flight reaches the cap", () => {
|
||||
expect(isOverAiJobCap(5, 5)).toBe(true);
|
||||
expect(isOverAiJobCap(6, 5)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -1,5 +1,9 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { friendlyError, stripControlChars } from "../../../apps/api/src/lib/errors.js";
|
||||
import {
|
||||
friendlyError,
|
||||
stripControlChars,
|
||||
stripInternalPaths,
|
||||
} from "../../../apps/api/src/lib/errors.js";
|
||||
|
||||
const GENERIC = "Processing failed. The file may be in an unsupported or corrupted format.";
|
||||
|
||||
@@ -14,6 +18,18 @@ describe("friendlyError", () => {
|
||||
expect(friendlyError("ffprobe exited 1: moov atom not found")).toBe(GENERIC);
|
||||
});
|
||||
|
||||
it("preserves short doc-engine errors that carry the actionable reason", () => {
|
||||
// qpdf/gs/pdfcpu stderr is already path-scrubbed and often IS the useful
|
||||
// message (e.g. a wrong PDF password), so it must not collapse to generic.
|
||||
// Only genuinely verbose dumps collapse, via the length/line-count guard.
|
||||
expect(friendlyError("qpdf exited 2: invalid password")).toBe(
|
||||
"qpdf exited 2: invalid password",
|
||||
);
|
||||
expect(friendlyError("pdfcpu exited 1: validation error at object 5")).toBe(
|
||||
"pdfcpu exited 1: validation error at object 5",
|
||||
);
|
||||
});
|
||||
|
||||
it("collapses python tracebacks", () => {
|
||||
expect(friendlyError("Traceback (most recent call last):\n File x\nValueError: boom")).toBe(
|
||||
GENERIC,
|
||||
@@ -83,3 +99,21 @@ describe("stripControlChars", () => {
|
||||
expect(stripControlChars("Café déjà vu")).toBe("Café déjà vu");
|
||||
});
|
||||
});
|
||||
|
||||
describe("stripInternalPaths", () => {
|
||||
it("strips POSIX internal roots", () => {
|
||||
expect(stripInternalPaths("wrote /tmp/workspace/out.mp4 ok")).toBe("wrote [internal] ok");
|
||||
expect(stripInternalPaths("model at /data/ai/models/whisper")).toBe("model at [internal]");
|
||||
});
|
||||
|
||||
it("strips Windows drive-letter paths (native Windows runs)", () => {
|
||||
// Matches sentry-scrub's PATH_RE so client responses and Sentry agree.
|
||||
expect(stripInternalPaths("failed reading C:\\Users\\snap\\secret.pdf")).toBe(
|
||||
"failed reading [internal]",
|
||||
);
|
||||
});
|
||||
|
||||
it("leaves messages with no path untouched", () => {
|
||||
expect(stripInternalPaths("Region exceeds image bounds")).toBe("Region exceeds image bounds");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,45 @@
|
||||
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import {
|
||||
computeWorkspaceUsedBytes,
|
||||
isOverWorkspaceCap,
|
||||
} from "../../../apps/api/src/lib/object-storage.js";
|
||||
|
||||
describe("isOverWorkspaceCap", () => {
|
||||
it("is disabled (never over) when maxGb is 0", () => {
|
||||
expect(isOverWorkspaceCap(999 * 1024 ** 3, 0)).toBe(false);
|
||||
});
|
||||
|
||||
it("is false when usage is under the cap", () => {
|
||||
expect(isOverWorkspaceCap(5 * 1024 ** 3, 10)).toBe(false);
|
||||
});
|
||||
|
||||
it("is true when usage exceeds the cap", () => {
|
||||
expect(isOverWorkspaceCap(11 * 1024 ** 3, 10)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("computeWorkspaceUsedBytes", () => {
|
||||
let root = "";
|
||||
afterEach(async () => {
|
||||
if (root) await rm(root, { recursive: true, force: true });
|
||||
root = "";
|
||||
});
|
||||
|
||||
it("sums file sizes across uploads/ and outputs/ job dirs", async () => {
|
||||
root = await mkdtemp(join(tmpdir(), "snapotter-wscap-"));
|
||||
await mkdir(join(root, "uploads", "job1"), { recursive: true });
|
||||
await mkdir(join(root, "outputs", "job2"), { recursive: true });
|
||||
await writeFile(join(root, "uploads", "job1", "a.bin"), Buffer.alloc(1000));
|
||||
await writeFile(join(root, "outputs", "job2", "b.bin"), Buffer.alloc(2000));
|
||||
expect(await computeWorkspaceUsedBytes(root)).toBe(3000);
|
||||
});
|
||||
|
||||
it("returns 0 for an empty or missing workspace", async () => {
|
||||
root = await mkdtemp(join(tmpdir(), "snapotter-wscap-"));
|
||||
expect(await computeWorkspaceUsedBytes(root)).toBe(0);
|
||||
expect(await computeWorkspaceUsedBytes(join(root, "nope"))).toBe(0);
|
||||
});
|
||||
});
|
||||
@@ -312,6 +312,59 @@ describe("SVG sanitizer -- url() scheme blocking", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── href scheme obfuscation: whitespace + unquoted (defense-in-depth) ────────
|
||||
|
||||
describe("SVG sanitizer -- href scheme whitespace/unquoted bypass", () => {
|
||||
it("blocks an unquoted javascript: URI in href", () => {
|
||||
const svg = wrapSvg("<a href=javascript:alert(1)><text>x</text></a>");
|
||||
const result = sanitize(svg);
|
||||
expect(result).not.toContain("javascript:");
|
||||
});
|
||||
|
||||
it("blocks a javascript: URI with leading whitespace inside quotes", () => {
|
||||
const svg = wrapSvg('<a href=" javascript:alert(1)"><text>x</text></a>');
|
||||
const result = sanitize(svg);
|
||||
expect(result).not.toContain("javascript:");
|
||||
});
|
||||
|
||||
it("blocks javascript: on xlink:href", () => {
|
||||
const svg = wrapSvg(
|
||||
'<a xlink:href="javascript:alert(1)"><text>x</text></a>',
|
||||
'xmlns:xlink="http://www.w3.org/1999/xlink"',
|
||||
);
|
||||
const result = sanitize(svg);
|
||||
expect(result).not.toContain("javascript:");
|
||||
});
|
||||
});
|
||||
|
||||
// ── Extended animation / event elements ──────────────────────────────────────
|
||||
|
||||
describe("SVG sanitizer -- extended animation elements", () => {
|
||||
it("strips <animateTransform> with a javascript: value", () => {
|
||||
const svg = wrapSvg('<animateTransform attributeName="transform" to="javascript:alert(1)"/>');
|
||||
const result = sanitize(svg);
|
||||
expect(result).not.toContain("<animateTransform");
|
||||
expect(result).not.toContain("javascript:");
|
||||
});
|
||||
|
||||
it("strips <animateMotion> and its <mpath>", () => {
|
||||
const svg = wrapSvg('<animateMotion><mpath href="#p"/></animateMotion>');
|
||||
const result = sanitize(svg);
|
||||
expect(result).not.toContain("<animateMotion");
|
||||
expect(result).not.toContain("<mpath");
|
||||
});
|
||||
|
||||
it("strips the <handler> SVG-Tiny event-handler element", () => {
|
||||
const svg = wrapSvg(
|
||||
'<handler ev:event="load">alert(1)</handler>',
|
||||
'xmlns:ev="http://www.w3.org/2001/xml-events"',
|
||||
);
|
||||
const result = sanitize(svg);
|
||||
expect(result).not.toContain("<handler");
|
||||
expect(result).not.toContain("alert(1)");
|
||||
});
|
||||
});
|
||||
|
||||
// ── Clean SVGs pass through ──────────────────────────────────────────────────
|
||||
|
||||
describe("SVG sanitizer -- clean SVGs pass through", () => {
|
||||
|
||||
@@ -0,0 +1,43 @@
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { wrapWithMemoryLimit } from "../../../packages/shared/src/subprocess-limit.js";
|
||||
|
||||
const KEY = "SUBPROCESS_MEMORY_LIMIT_MB";
|
||||
const orig = process.env[KEY];
|
||||
|
||||
describe("wrapWithMemoryLimit", () => {
|
||||
afterEach(() => {
|
||||
if (orig === undefined) delete process.env[KEY];
|
||||
else process.env[KEY] = orig;
|
||||
});
|
||||
|
||||
it("returns the command unchanged when the limit is unset (default)", () => {
|
||||
delete process.env[KEY];
|
||||
expect(wrapWithMemoryLimit("ffmpeg", ["-i", "a.mp4"])).toEqual(["ffmpeg", ["-i", "a.mp4"]]);
|
||||
});
|
||||
|
||||
it("returns the command unchanged when the limit is 0 or non-numeric", () => {
|
||||
process.env[KEY] = "0";
|
||||
expect(wrapWithMemoryLimit("gs", ["-dSAFER"])).toEqual(["gs", ["-dSAFER"]]);
|
||||
process.env[KEY] = "not-a-number";
|
||||
expect(wrapWithMemoryLimit("gs", ["-dSAFER"])).toEqual(["gs", ["-dSAFER"]]);
|
||||
});
|
||||
|
||||
it("wraps in an ulimit -v sh shim (limit in KB) when a positive MB limit is set", () => {
|
||||
process.env[KEY] = "512";
|
||||
const [bin, args] = wrapWithMemoryLimit("ffmpeg", ["-i", "in.mp4", "out.mp4"]);
|
||||
expect(bin).toBe("/bin/sh");
|
||||
expect(args[0]).toBe("-c");
|
||||
expect(args[1]).toContain("ulimit -v");
|
||||
expect(args[1]).toContain('exec "$@"');
|
||||
// sh -c <script> sh <kb> <realbin> <realargs...>
|
||||
expect(args.slice(2)).toEqual(["sh", String(512 * 1024), "ffmpeg", "-i", "in.mp4", "out.mp4"]);
|
||||
});
|
||||
|
||||
it("passes user args positionally so a crafted arg is never re-parsed by the shell", () => {
|
||||
process.env[KEY] = "256";
|
||||
const [, args] = wrapWithMemoryLimit("gs", ["-sOutputFile=/x/$(whoami).pdf"]);
|
||||
// The metacharacter-laden value is a positional param, not part of the script body.
|
||||
expect(args[1]).not.toContain("whoami");
|
||||
expect(args).toContain("-sOutputFile=/x/$(whoami).pdf");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user