From 04ef1141fb5abe9aa2c70b737dbce77731fa4c58 Mon Sep 17 00:00:00 2001 From: SnapOtter Date: Mon, 3 Aug 2026 13:13:38 +0800 Subject: [PATCH] feat(telemetry): readable Sentry errors, Python tracebacks, and diagnostic mode Keeps a real, redacted error message instead of "Error: Error", surfaces Python tracebacks in Sentry as a vetted context, and adds an opt-in SNAPOTTER_SENTRY_DIAGNOSTIC verbose mode plus SNAPOTTER_SENTRY_DSN_OVERRIDE. The default fleet path ships nothing on the never-collect list; raw detail is reachable only via the opt-in flag. Also classifies Redis OOM/READONLY replies as operational and removes a ReDoS in stack-frame extraction. --- .env.example | 10 ++ apps/api/src/instrument.ts | 18 +++- apps/api/src/jobs/worker.ts | 4 + apps/api/src/lib/analytics-gate.ts | 6 ++ apps/api/src/lib/error-report.ts | 17 +++- apps/api/src/lib/sentry-scrub.ts | 47 ++++++--- apps/docs/guide/telemetry.md | 2 +- apps/web/src/lib/sentry-scrub.ts | 86 ++--------------- packages/ai/python/dispatcher.py | 12 ++- packages/ai/python/sidecar_errors.py | 78 +++++++++++++++ packages/ai/python/test_sidecar_errors.py | 31 ++++++ packages/ai/src/bridge.ts | 61 ++++++++++-- .../shared/src/analytics/error-sanitize.ts | 44 ++++++++- .../shared/src/analytics/redact-message.ts | 56 +++++++++++ packages/shared/src/index.ts | 1 + tests/unit/ai/bridge-error.test.ts | 24 +++++ tests/unit/api/capture-path.test.ts | 1 + tests/unit/api/error-report.test.ts | 11 +++ tests/unit/api/sentry-scrub.test.ts | 35 ++++++- tests/unit/shared/error-sanitize.test.ts | 62 +++++++++++- tests/unit/shared/redact-message.test.ts | 96 +++++++++++++++++++ tests/unit/web/sentry-scrub.test.ts | 69 +++---------- 22 files changed, 593 insertions(+), 178 deletions(-) create mode 100644 packages/ai/python/sidecar_errors.py create mode 100644 packages/ai/python/test_sidecar_errors.py create mode 100644 packages/shared/src/analytics/redact-message.ts create mode 100644 tests/unit/ai/bridge-error.test.ts create mode 100644 tests/unit/shared/redact-message.test.ts diff --git a/.env.example b/.env.example index 4b244b6c..142d504d 100644 --- a/.env.example +++ b/.env.example @@ -112,6 +112,16 @@ LOG_DIR=./data/logs # rotating log ring for support bundles # SNAPOTTER_TELEMETRY=1 # Label this instance's error reports (shows as the Sentry environment). # SNAPOTTER_ENV=production +# Opt-in verbose diagnostics for THIS instance only (off by default). When on, +# error reports keep the raw message, request path, and breadcrumb data. Use it +# to reproduce a reported bug, or on your own canonical instance. It never +# re-enables telemetry when analytics is off. +# SNAPOTTER_SENTRY_DIAGNOSTIC=1 +# Send this instance's errors to your OWN Sentry project instead of the baked one. +# SNAPOTTER_SENTRY_DSN_OVERRIDE= +# Performance tracing sample rate (0 = off, the default; 0.05 = light). A sampler +# zeroes db/redis/poll spans so it cannot cause a quota storm. +# SENTRY_TRACES_SAMPLE_RATE=0 # One-time SQLite import on first boot (1.x upgrade path). Leave unset normally. # SQLITE_MIGRATE_PATH=/data/snapotter.db diff --git a/apps/api/src/instrument.ts b/apps/api/src/instrument.ts index 2a46ec0f..b613ed32 100644 --- a/apps/api/src/instrument.ts +++ b/apps/api/src/instrument.ts @@ -1,5 +1,10 @@ import { ANALYTICS_BAKED } from "@snapotter/shared"; -import { analyticsEnabled, gatePrimed, telemetryEnvKilled } from "./lib/analytics-gate.js"; +import { + analyticsEnabled, + gatePrimed, + sentryDiagnostic, + telemetryEnvKilled, +} from "./lib/analytics-gate.js"; import { deployMode } from "./lib/deploy-mode.js"; import { buildBeforeSend } from "./lib/sentry-scrub.js"; import { buildTracesSampler } from "./lib/sentry-tracing.js"; @@ -9,7 +14,9 @@ import { buildTracesSampler } from "./lib/sentry-tracing.js"; // so an opted-out instance never reports even a boot-window crash. const sentryActive = () => gatePrimed() && analyticsEnabled(); -if (ANALYTICS_BAKED.sentryDsn && !telemetryEnvKilled()) { +const dsn = process.env.SNAPOTTER_SENTRY_DSN_OVERRIDE || ANALYTICS_BAKED.sentryDsn; + +if (dsn && !telemetryEnvKilled()) { try { const Sentry = await import("@sentry/node"); const { APP_VERSION } = await import("@snapotter/shared"); @@ -31,7 +38,7 @@ if (ANALYTICS_BAKED.sentryDsn && !telemetryEnvKilled()) { const tracingEnabled = tracesSampleRate > 0 && tracesSampleRate <= 1; Sentry.init({ - dsn: ANALYTICS_BAKED.sentryDsn, + dsn, release, environment: process.env.SNAPOTTER_ENV || "production", sendDefaultPii: false, @@ -56,7 +63,10 @@ if (ANALYTICS_BAKED.sentryDsn && !telemetryEnvKilled()) { // Capture the breadcrumb trail (default 100). beforeSend (sentry-scrub.ts) // sanitizes each breadcrumb before send: urls/paths redacted, data dropped. initialScope: { tags: { deploy_mode: deployMode() } }, - beforeSend: buildBeforeSend(sentryActive) as unknown as SentryOptions["beforeSend"], + beforeSend: buildBeforeSend( + sentryActive, + sentryDiagnostic(), + ) as unknown as SentryOptions["beforeSend"], }); console.log( diff --git a/apps/api/src/jobs/worker.ts b/apps/api/src/jobs/worker.ts index 6e943cd5..286cfde1 100644 --- a/apps/api/src/jobs/worker.ts +++ b/apps/api/src/jobs/worker.ts @@ -1119,6 +1119,10 @@ export function startWorkers(): void { worker.on("failed", (job, err) => { if (!job) return; + const pf = (err as { pythonFrames?: unknown }).pythonFrames; + if (Array.isArray(pf) && pf.length) { + logger.error({ pool, jobId: job.id, pythonFrames: pf }, "sidecar failure"); + } void reportError(err, { source: "worker", pool, diff --git a/apps/api/src/lib/analytics-gate.ts b/apps/api/src/lib/analytics-gate.ts index e2ce5249..032092da 100644 --- a/apps/api/src/lib/analytics-gate.ts +++ b/apps/api/src/lib/analytics-gate.ts @@ -38,6 +38,12 @@ export function telemetryEnvKilled(): boolean { return off(process.env.SNAPOTTER_TELEMETRY) || off(process.env.ANALYTICS_ENABLED); } +/** Opt-in diagnostic verbosity for a single (usually self-hosted) instance. */ +export function sentryDiagnostic(): boolean { + const v = process.env.SNAPOTTER_SENTRY_DIAGNOSTIC; + return v === "1" || v === "true" || v === "on"; +} + /** Compile-time bake, with a NON-PRODUCTION-only override so tests can force it on. */ export function bakedEnabled(): boolean { if (telemetryEnvKilled()) return false; diff --git a/apps/api/src/lib/error-report.ts b/apps/api/src/lib/error-report.ts index c60eb058..d52e50d1 100644 --- a/apps/api/src/lib/error-report.ts +++ b/apps/api/src/lib/error-report.ts @@ -19,7 +19,7 @@ import { isSafeMessageError, isToolInputError, } from "@snapotter/shared"; -import { analyticsEnabled } from "./analytics-gate.js"; +import { analyticsEnabled, sentryDiagnostic } from "./analytics-gate.js"; export type ErrorClass = "expected" | "operational" | "bug"; @@ -74,6 +74,13 @@ export function classifyError(err: unknown, source?: ReportContext["source"]): E if (connectivityClass(err)) return "operational"; if (isEnvironmentalDbError(err)) return "operational"; if (e?.code && OPERATIONAL_CODES.has(e.code)) return "operational"; + if ( + e?.name === "ReplyError" && + typeof e.message === "string" && + /^(OOM|READONLY|MISCONF|NOREPLICAS)\b/.test(e.message) + ) { + return "operational"; + } return "bug"; } @@ -105,7 +112,7 @@ export function errorSignature(err: unknown): string { let frame = "-"; if (typeof e?.stack === "string") { const line = e.stack.split("\n").find((l) => l.includes("/apps/") || l.includes("/packages/")); - const m = line?.match(/([^/\\]+\.[cm]?[jt]sx?):(\d+)/); + const m = line?.slice(0, 300).match(/([^/\\\s():]+):(\d+)/); if (m) frame = `${m[1]}:${m[2]}`; } return `${name}:${code}:${frame}`; @@ -155,7 +162,7 @@ export async function reportError(err: unknown, ctx: ReportContext): Promise").replace(URL_RE, "").replace(PATH_RE, ""); -} - // Sentry event/hint are typed loosely on purpose: this module must not import // @sentry/node (instrument.ts loads the SDK lazily and passes events through). type AnyEvent = Record; @@ -60,7 +49,7 @@ function scrubBreadcrumb(entry: unknown): AnyEvent | null { for (const k of ["type", "category", "level", "timestamp"]) { if (b[k] !== undefined) out[k] = b[k]; } - if (typeof b.message === "string") out.message = scrubText(b.message); + if (typeof b.message === "string") out.message = redactMessage(b.message); // For http breadcrumbs keep the non-PII status_code + method (the url is the // sensitive part, dropped with the rest of `data`): they answer "what request // failed right before the error". @@ -135,7 +124,7 @@ function errorDigest(err: unknown): string { return (h >>> 0).toString(16); } -export function buildBeforeSend(isActive: () => boolean) { +export function buildBeforeSend(isActive: () => boolean, diagnostic = false) { let windowStart = 0; let sentInWindow = 0; @@ -149,6 +138,20 @@ export function buildBeforeSend(isActive: () => boolean) { } if (++sentInWindow > CEILING_PER_HOUR) return null; + if (diagnostic) { + // A consenting instance: keep the raw message, request, and breadcrumb data. + // Still drop identity, and cap message length via redactMessage raw mode. + event.user = undefined; + const values = asObj(event.exception)?.values; + if (Array.isArray(values)) { + for (const entry of values) { + const ex = asObj(entry); + if (ex && typeof ex.value === "string") ex.value = redactMessage(ex.value, { raw: true }); + } + } + return event; + } + // Dropped: these surfaces can carry user data or PII. event.message = undefined; event.logentry = undefined; @@ -176,6 +179,22 @@ export function buildBeforeSend(isActive: () => boolean) { } if (Object.keys(safe).length) keep.tool = safe; } + const py = asObj(ctx?.python); + if (py) { + const type = typeof py.type === "string" ? py.type.slice(0, 64) : undefined; + const frames = Array.isArray(py.frames) + ? py.frames + .map((f) => asObj(f)) + .filter((f): f is AnyEvent => !!f) + .map((f) => ({ + file: typeof f.file === "string" ? f.file.slice(0, 64) : "?", + line: typeof f.line === "number" ? f.line : 0, + func: typeof f.func === "string" ? f.func.slice(0, 64) : "?", + })) + .slice(0, 20) + : []; + if (frames.length) keep.python = { type, frames }; + } event.contexts = Object.keys(keep).length ? keep : undefined; const tags = asObj(event.tags); diff --git a/apps/docs/guide/telemetry.md b/apps/docs/guide/telemetry.md index 8f68e243..0ad869d2 100644 --- a/apps/docs/guide/telemetry.md +++ b/apps/docs/guide/telemetry.md @@ -12,7 +12,7 @@ Anonymous Product Analytics is on by default and set for the whole instance by a - pipeline_executed: step count, tool ids, batch flag, file count, duration, status. - ai_bundle_action: bundle id, action, duration. - Frontend usage: which tool pages open, files added (counts only), tool started, downloads, saves, search (result count only), batch processed. -- Crash reports: error type and a source stack with file basenames only. +- Crash reports: error type, a redacted error message (file names, paths, emails, and quoted values removed), a source stack with file basenames, and for AI tools a Python traceback with file basenames only. ## What we never collect {#what-we-never-collect} diff --git a/apps/web/src/lib/sentry-scrub.ts b/apps/web/src/lib/sentry-scrub.ts index 540c7f29..91c3efa8 100644 --- a/apps/web/src/lib/sentry-scrub.ts +++ b/apps/web/src/lib/sentry-scrub.ts @@ -1,11 +1,12 @@ /** - * Sentry beforeSend for the web app. Same allowlist-first stance as the API - * scrubber (apps/api/src/lib/sentry-scrub.ts), adapted for browsers: native - * error messages are usually safe and highly diagnostic, so they pass through - * with url/path redaction; anything else falls back to type-only. Frame paths - * keep the host-less pathname so debug-id source maps still resolve. + * Sentry beforeSend for the web app. Mirrors the API scrubber + * (apps/api/src/lib/sentry-scrub.ts): the shared rebuildErrorValue ladder keeps + * a redacted message for any non-empty error (the rich default), so there is no + * browser-native allowlist here. Breadcrumb text goes through the same shared + * redactMessage. Frame paths keep the host-less pathname so debug-id source maps + * still resolve. */ -import { rebuildErrorValue } from "@snapotter/shared"; +import { rebuildErrorValue, redactMessage } from "@snapotter/shared"; export const IGNORE_ERRORS: (string | RegExp)[] = [ /^AbortError/, @@ -30,71 +31,11 @@ export const DENY_URLS: RegExp[] = [ /^safari-web-extension:\/\//, ]; -const NATIVE_ERRORS = new Set([ - "TypeError", - "RangeError", - "SyntaxError", - "ReferenceError", - "DOMException", - // DOMExceptions report their specific name via err.name, not "DOMException", - // so the base entry alone dropped the diagnostic browser message for the - // whole family (WEB-3/4/6 arrived as "NotFoundError: NotFoundError"). This is - // the full WebIDL DOMException name table: every message is browser-authored - // and still passes through scrubText's url/path redaction. - "AbortError", - "ConstraintError", - "DataCloneError", - "DataError", - "EncodingError", - "HierarchyRequestError", - "IndexSizeError", - "InUseAttributeError", - "InvalidAccessError", - "InvalidCharacterError", - "InvalidModificationError", - "InvalidNodeTypeError", - "InvalidStateError", - "NamespaceError", - "NetworkError", - "NoModificationAllowedError", - "NotAllowedError", - "NotFoundError", - "NotReadableError", - "NotSupportedError", - "OperationError", - "QuotaExceededError", - "ReadOnlyError", - "SecurityError", - "TimeoutError", - "TransactionInactiveError", - "UnknownError", - "URLMismatchError", - "VersionError", - "WrongDocumentError", -]); - // Per-session runaway guard (Sentry de-dupes by fingerprint server-side, so 500 // distinct events/hour is ample), not a quota lever under the sponsored plan. const CEILING_PER_HOUR = 500; const HOUR_MS = 3600_000; -// BLOB_RE must be applied before URL_RE: "blob:http://..." would otherwise -// partially match URL_RE and leave a dangling "blob:" prefix behind. -const URL_RE = /https?:\/\/[^\s"')]+/g; -const BLOB_RE = /blob:[^\s"')]+/g; -const PATH_RE = /(?:\/Users|\/home|[A-Za-z]:\\)[^\s"')]*/g; - -/** Redact urls, blob refs, and absolute paths from free text. */ -function scrubText(s: string): string { - return s.replace(BLOB_RE, "").replace(URL_RE, "").replace(PATH_RE, ""); -} - -/** Redacted native-error message, or null when the name is not allowlisted. */ -export function scrubBrowserMessage(name: string, message: string): string | null { - if (!NATIVE_ERRORS.has(name)) return null; - return scrubText(message); -} - const TAG_ALLOWLIST = new Set(["route", "tool_id", "locale", "error_class"]); // Sentry event/hint are typed loosely on purpose: this module must not import @@ -119,7 +60,7 @@ function scrubBreadcrumb(entry: unknown): AnyEvent | null { for (const k of ["type", "category", "level", "timestamp"]) { if (b[k] !== undefined) out[k] = b[k]; } - if (typeof b.message === "string") out.message = scrubText(b.message); + if (typeof b.message === "string") out.message = redactMessage(b.message); // For network breadcrumbs keep the non-PII status_code + method (the url is // dropped with the rest of `data`): "what request failed before the crash". if (b.category === "fetch" || b.category === "xhr") { @@ -176,16 +117,7 @@ export function buildWebBeforeSend(isActive: () => boolean) { } } - const orig = hint?.originalException; - let rebuilt = rebuildErrorValue(orig); - if (rebuilt === null) { - const err = asObj(orig); - const name = err?.name; - const message = err?.message; - if (typeof name === "string" && typeof message === "string") { - rebuilt = scrubBrowserMessage(name, message); - } - } + const rebuilt = rebuildErrorValue(hint?.originalException); const values = asObj(event.exception)?.values; if (Array.isArray(values)) { diff --git a/packages/ai/python/dispatcher.py b/packages/ai/python/dispatcher.py index 93150f10..8e01d314 100644 --- a/packages/ai/python/dispatcher.py +++ b/packages/ai/python/dispatcher.py @@ -27,6 +27,8 @@ import io import os import traceback +from sidecar_errors import build_error_envelope + # ── Optional OpenTelemetry tracing (enterprise only) ───────────── _tracer = None @@ -294,10 +296,14 @@ def _run_script_main(script_name, args): except SystemExit as e: exit_code = e.code if isinstance(e.code, int) else 1 except Exception as e: - # Log full traceback to stderr for diagnostics + # Log full traceback to stderr for local diagnostics (unchanged). traceback.print_exc(file=sys.stderr) - # Write error to the captured stdout - sys.stdout.write(json.dumps({"success": False, "error": str(e)}) + "\n") + info = build_error_envelope(e) + # Keep `error` as the redacted string for back-compatible consumers; + # add `errorInfo` (type + our frames) for the structured Sentry path. + sys.stdout.write( + json.dumps({"success": False, "error": info["message"], "errorInfo": info}) + "\n" + ) sys.stdout.flush() exit_code = 1 finally: diff --git a/packages/ai/python/sidecar_errors.py b/packages/ai/python/sidecar_errors.py new file mode 100644 index 00000000..e568ef14 --- /dev/null +++ b/packages/ai/python/sidecar_errors.py @@ -0,0 +1,78 @@ +""" +Structured error envelope for the sidecar. Mirrors the Node redactMessage so a +Python failure reaches Sentry with its type, a redacted message, and our own +stack frames (basename only) instead of a bare "Error: Error". +""" +import os +import re +import traceback + +# _CTRL: matches ASCII control chars 0x00-0x1F and 0x7F only. Written with \x +# hex escapes so no literal control bytes appear in this file. It must NOT match +# spaces, "/", ".", or any printable character. +_CTRL = re.compile(r"[\x00-\x1f\x7f]") +_BLOB = re.compile(r"blob:[^\s\"')]+") +_DATA = re.compile(r"data:[^\s\"')]+") +_URL = re.compile(r"https?://[^\s\"')]+") +_PATH = re.compile(r"(?:/(?:Users|home|root|data|tmp|var|app|opt|mnt|srv)|[A-Za-z]:\\)[^\s\"')]*") +# Relative object-storage keys (uploads//…, outputs/…, previews/…) carry a +# user-supplied filename tail; mask them like absolute paths. Runs after _PATH, +# which already swallows the absolute /data/uploads/… form. +_RELKEY = re.compile(r"\b(?:uploads|outputs|previews)/[^\s\"')]+") +_IP = re.compile(r"\b\d{1,3}(?:\.\d{1,3}){3}\b") +# IPv6: a full 8-group form, or any ::-compressed form (::1, fe80::…, …::). The +# negative lookbehind/lookahead ((?", s) + s = _DATA.sub("", s) + s = _URL.sub("", s) + s = _PATH.sub("", s) + s = _RELKEY.sub("", s) + s = _IP.sub("", s) + s = _IPV6.sub("", s) + s = _EMAIL.sub("", s) + s = _QUOTED.sub(lambda m: m.group(1) + "" + m.group(1), s) + s = _HEX.sub("", s) + s = _FILE.sub("", s) + s = re.sub(r"\s+", " ", s).strip() + return (s[:_MAX_LEN] + "…") if len(s) > _MAX_LEN else s + + +def _our_frames(exc): + frames = [] + for fr in traceback.extract_tb(exc.__traceback__): + # Keep only our sidecar-script frames; drop stdlib and venv/site-packages. + if os.path.dirname(os.path.abspath(fr.filename)) != _SIDECAR_DIR: + continue + frames.append({"file": os.path.basename(fr.filename), "line": fr.lineno, "func": fr.name}) + return frames[-20:] + + +def build_error_envelope(exc): + """A JSON-serializable {type, message, frames} describing a caught exception.""" + return { + "type": type(exc).__name__, + "message": redact(str(exc)), + "frames": _our_frames(exc), + } diff --git a/packages/ai/python/test_sidecar_errors.py b/packages/ai/python/test_sidecar_errors.py new file mode 100644 index 00000000..ece6a6e6 --- /dev/null +++ b/packages/ai/python/test_sidecar_errors.py @@ -0,0 +1,31 @@ +import os +import sys + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +from sidecar_errors import build_error_envelope, redact # noqa: E402 + + +def test_redact_masks_paths_and_files(): + assert redact("open /data/uploads/9f/in.bin") == "open " + assert redact("cannot read family_photo.JPG") == "cannot read " + assert redact("torch 2.2.0 ok") == "torch 2.2.0 ok" + + +def test_envelope_shape_and_frames(): + try: + raise RuntimeError("CUDA out of memory for /data/x.png") + except RuntimeError as exc: + env = build_error_envelope(exc) + assert env["type"] == "RuntimeError" + assert env["message"] == "CUDA out of memory for " + assert isinstance(env["frames"], list) and len(env["frames"]) >= 1 + top = env["frames"][-1] + assert top["file"] == "test_sidecar_errors.py" + assert isinstance(top["line"], int) + assert top["func"] == "test_envelope_shape_and_frames" + + +if __name__ == "__main__": + test_redact_masks_paths_and_files() + test_envelope_shape_and_frames() + print("ok") diff --git a/packages/ai/src/bridge.ts b/packages/ai/src/bridge.ts index 5b782f20..9ef176a2 100644 --- a/packages/ai/src/bridge.ts +++ b/packages/ai/src/bridge.ts @@ -108,7 +108,12 @@ const OOM_EXIT_TEXT = * memory-aware callers still retry on a lighter model. Any other non-zero exit * is a bug, with the extracted reason (or an OOM reported in-band) preserved. */ -function pythonExitError(code: number | null, signal: string | null, extracted: string): SafeError { +function pythonExitError( + code: number | null, + signal: string | null, + extracted: string, + info?: PythonErrorInfo | null, +): SafeError { if (signal === "SIGSEGV" || code === 139) { return new SafeError("Process crashed (segmentation fault)", { kind: "operational", @@ -122,10 +127,14 @@ function pythonExitError(code: number | null, signal: string | null, extracted: }); } const message = extracted || `Python script exited with code ${code}`; - return new SafeError(message, { + const err = new SafeError(message, { kind: OOM_EXIT_TEXT.test(message) ? "operational" : "bug", code: `exit-${code ?? "unknown"}`, }); + if (info) { + Object.assign(err, { pythonType: info.type, pythonFrames: info.frames }); + } + return err; } /** @@ -186,6 +195,41 @@ function extractPythonError(error: unknown): string { return String(error); } +export interface PythonErrorInfo { + type?: string; + frames: Array<{ file: string; line: number; func: string }>; +} + +/** Read the structured {type, frames} envelope from a sidecar failure, or null. */ +export function extractPythonErrorInfo(error: unknown): PythonErrorInfo | null { + if (!error || typeof error !== "object") return null; + const pErr = error as { stdout?: string; stderr?: string }; + for (const output of [pErr.stdout, pErr.stderr]) { + if (!output) continue; + try { + const parsed = JSON.parse(output.trim()) as { errorInfo?: unknown }; + const info = parsed.errorInfo as PythonErrorInfo | undefined; + if (info && Array.isArray(info.frames)) { + return { + type: typeof info.type === "string" ? info.type : undefined, + frames: info.frames + .filter( + (f): f is { file: string; line: number; func: string } => + !!f && + typeof (f as { file?: unknown }).file === "string" && + typeof (f as { line?: unknown }).line === "number" && + typeof (f as { func?: unknown }).func === "string", + ) + .slice(0, 20), + }; + } + } catch { + // not JSON; nothing structured to read + } + } + return null; +} + export type ProgressCallback = (percent: number, stage: string) => void; export interface PythonRunOptions { @@ -420,11 +464,11 @@ export class PythonDispatcher { if (req) { this.pending.delete(reqId); if (response.exitCode !== 0) { - const extracted = extractPythonError({ - stdout: response.stdout, - stderr: req.stderrLines.join("\n"), - }); - req.reject(pythonExitError(response.exitCode, null, extracted)); + const raw = { stdout: response.stdout, stderr: req.stderrLines.join("\n") }; + const extracted = extractPythonError(raw); + req.reject( + pythonExitError(response.exitCode, null, extracted, extractPythonErrorInfo(raw)), + ); } else { req.resolve({ stdout: response.stdout || "", @@ -737,8 +781,9 @@ export class PythonDispatcher { const stderr = stderrLines.join("\n"); if (code !== 0) { + const raw = { stdout: stdout.trim(), stderr }; rejectOnce( - pythonExitError(code, signal, extractPythonError({ stdout: stdout.trim(), stderr })), + pythonExitError(code, signal, extractPythonError(raw), extractPythonErrorInfo(raw)), ); return; } diff --git a/packages/shared/src/analytics/error-sanitize.ts b/packages/shared/src/analytics/error-sanitize.ts index 87809d7c..670cb836 100644 --- a/packages/shared/src/analytics/error-sanitize.ts +++ b/packages/shared/src/analytics/error-sanitize.ts @@ -5,6 +5,7 @@ * author). Returning null means "no safe rebuild known, use type-only". */ import { isSafeMessageError } from "../tool-errors.js"; +import { redactMessage } from "./redact-message.js"; interface ErrLike { name?: unknown; @@ -56,13 +57,39 @@ function looksLikePg(links: ErrLike[]): boolean { ); } +/** "at file.ts:NN" from the first in-app stack frame, mirroring errorSignature. */ +function frameHint(err: unknown): string | null { + const stack = (err as { stack?: unknown } | null)?.stack; + if (typeof stack !== "string") return null; + const line = stack.split("\n").find((l) => l.includes("/apps/") || l.includes("/packages/")); + const m = line?.slice(0, 300).match(/([^/\\\s():]+):(\d+)/); + return m ? `at ${m[1]}:${m[2]}` : null; +} + /** Safe replacement for exception.value, or null for type-only fallback. */ export function rebuildErrorValue(err: unknown): string | null { try { - if (isSafeMessageError(err)) return err.message; const links = chain(err); - if (links.length === 0) return null; + // 1. SafeError: our authored (or toSidecarError-wrapped) message, redacted, + // plus the redacted immediate cause so a wrapper title never hides detail. + if (isSafeMessageError(err)) { + let out = redactMessage(err.message); + for (let i = 1; i < links.length; i++) { + const m = links[i].message; + if (typeof m === "string" && m.trim()) { + const detail = redactMessage(m); + if (detail && !out.includes(detail)) out = `${out}: ${detail}`; + break; + } + } + return out; + } + + if (links.length === 0) return null; + const top = links[0]; + + // 2. Structured rebuilds: pg SQLSTATE, node E-code, reply, zod, http status. for (const l of links) { if (typeof l.code === "string" && SQLSTATE.test(l.code) && !NODE_CODE.test(l.code)) { return typeof l.routine === "string" && PG_ROUTINE.test(l.routine) @@ -72,11 +99,9 @@ export function rebuildErrorValue(err: unknown): string | null { } for (const l of links) { if (typeof l.code === "string" && NODE_CODE.test(l.code)) { - // syscall is libuv vocabulary or "spawn ", never user args. return typeof l.syscall === "string" ? `${l.code} ${l.syscall}` : l.code; } } - const top = links[0]; if (top.name === "ReplyError" && typeof top.message === "string") { const token = top.message.split(" ")[0]; return REPLY_TOKEN.test(token) ? `reply ${token}` : "reply"; @@ -96,6 +121,17 @@ export function rebuildErrorValue(err: unknown): string | null { typeof top.name === "string" && SAFE_NAME.test(top.name) ? top.name : "HttpError"; return `${name} ${top.status}`; } + + // 3. Non-empty message: keep it, redacted (the rich default). + if (typeof top.message === "string" && top.message.trim().length > 0) { + return redactMessage(top.message); + } + + // 4. Empty message with a stack: derive a title from the first in-app frame. + const hint = frameHint(err); + if (hint) return hint; + + // 5. Nothing safe to surface: type-only. return null; } catch { return null; diff --git a/packages/shared/src/analytics/redact-message.ts b/packages/shared/src/analytics/redact-message.ts new file mode 100644 index 00000000..73655c73 --- /dev/null +++ b/packages/shared/src/analytics/redact-message.ts @@ -0,0 +1,56 @@ +/** + * The single redactor for error text sent to Sentry. Denylist-shaped, so it is + * only trusted for messages we already gate (SafeError, our own throws, known + * libraries) and for the redacted fallback in rebuildErrorValue. It keeps the + * published never-collect promise (no file names / paths / contents) true while + * still surfacing the human-readable message. + */ +// biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally strip ASCII C0 controls + DEL from error text. +const CTRL_RE = /[\x00-\x1f\x7f]/g; +const BLOB_RE = /blob:[^\s"')]+/g; +const DATA_RE = /data:[^\s"')]+/g; +const URL_RE = /https?:\/\/[^\s"')]+/g; +const PATH_RE = /(?:\/(?:Users|home|root|data|tmp|var|app|opt|mnt|srv)|[A-Za-z]:\\)[^\s"')]*/g; +// Relative object-storage keys (uploads//…, outputs/…, previews/…) carry a +// user-supplied filename tail, so mask them like absolute paths. Runs after +// PATH_RE, which already swallows the absolute /data/uploads/… form. +const RELKEY_RE = /\b(?:uploads|outputs|previews)\/[^\s"')]+/g; +const IP_RE = /\b\d{1,3}(?:\.\d{1,3}){3}\b/g; +// IPv6: a full 8-group form, or any ::-compressed form (::1, fe80::…, …::). The +// negative lookbehind/lookahead ((?") + .replace(DATA_RE, "") + .replace(URL_RE, "") + .replace(PATH_RE, "") + .replace(RELKEY_RE, "") + .replace(IP_RE, "") + .replace(IPV6_RE, "") + .replace(EMAIL_RE, "") + .replace(QUOTED_RE, (_m, q) => `${q}${q}`) + .replace(HEX_RE, "") + .replace(FILE_RE, ""); + } + s = s.replace(/\s+/g, " ").trim(); + return s.length > MAX_LEN ? `${s.slice(0, MAX_LEN)}…` : s; +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index b20c5cfa..2f8df674 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -2,6 +2,7 @@ export * from "./analytics/baked.js"; export * from "./analytics/error-sanitize.js"; export * from "./analytics/events.js"; export * from "./analytics/feedback.js"; +export { redactMessage } from "./analytics/redact-message.js"; export * from "./analytics/types.js"; export * from "./audit-events.js"; export * from "./constants.js"; diff --git a/tests/unit/ai/bridge-error.test.ts b/tests/unit/ai/bridge-error.test.ts new file mode 100644 index 00000000..ae6fc07e --- /dev/null +++ b/tests/unit/ai/bridge-error.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; +import { extractPythonErrorInfo } from "../../../packages/ai/src/bridge.js"; + +describe("extractPythonErrorInfo", () => { + it("reads the structured envelope from stdout JSON", () => { + const stdout = JSON.stringify({ + success: false, + error: "CUDA out of memory for ", + errorInfo: { + type: "RuntimeError", + frames: [{ file: "remove_bg.py", line: 88, func: "run" }], + }, + }); + const info = extractPythonErrorInfo({ stdout, stderr: "" }); + expect(info).toEqual({ + type: "RuntimeError", + frames: [{ file: "remove_bg.py", line: 88, func: "run" }], + }); + }); + + it("returns null when there is no envelope (back-compat)", () => { + expect(extractPythonErrorInfo({ stdout: "boom", stderr: "" })).toBeNull(); + }); +}); diff --git a/tests/unit/api/capture-path.test.ts b/tests/unit/api/capture-path.test.ts index 486a0ce9..8332214b 100644 --- a/tests/unit/api/capture-path.test.ts +++ b/tests/unit/api/capture-path.test.ts @@ -39,6 +39,7 @@ vi.mock("@sentry/node", () => ({ vi.mock("../../../apps/api/src/lib/analytics-gate.js", () => ({ analyticsEnabled: () => true, + sentryDiagnostic: () => false, })); beforeEach(() => { diff --git a/tests/unit/api/error-report.test.ts b/tests/unit/api/error-report.test.ts index 3a1dc683..0aafd607 100644 --- a/tests/unit/api/error-report.test.ts +++ b/tests/unit/api/error-report.test.ts @@ -113,6 +113,17 @@ describe("classifyError", () => { }); }); +describe("classifyError redis", () => { + it("classifies a Redis OOM ReplyError as operational", () => { + const err = Object.assign(new Error("OOM command not allowed"), { name: "ReplyError" }); + expect(classifyError(err, "worker")).toBe("operational"); + }); + it("classifies a Redis READONLY ReplyError as operational", () => { + const err = Object.assign(new Error("READONLY You can't write"), { name: "ReplyError" }); + expect(classifyError(err, "worker")).toBe("operational"); + }); +}); + describe("throttle", () => { beforeEach(() => resetThrottleForTests()); it("operational: 1 per signature per hour; bug: 10", () => { diff --git a/tests/unit/api/sentry-scrub.test.ts b/tests/unit/api/sentry-scrub.test.ts index f1a57005..a79b7814 100644 --- a/tests/unit/api/sentry-scrub.test.ts +++ b/tests/unit/api/sentry-scrub.test.ts @@ -40,6 +40,18 @@ describe("buildBeforeSend (api)", () => { it("returns null when the gate is off", () => { expect(buildBeforeSend(() => false)(evt(), {})).toBeNull(); }); + it("keeps the raw message and request when diagnostic is on", () => { + const diag = buildBeforeSend(() => true, true); + const event = { + exception: { values: [{ type: "Error", value: "open /data/uploads/a/report.pdf" }] }, + request: { method: "POST", url: "https://host/api/v1/tools/image/rounded-crop" }, + }; + const out = diag(event as never, { + originalException: new Error("open /data/uploads/a/report.pdf"), + }) as never as { exception: { values: Array<{ value: string }> }; request?: unknown }; + expect(out.exception.values[0].value).toBe("open /data/uploads/a/report.pdf"); + expect(out.request).toBeDefined(); + }); it("strips high-risk surfaces but keeps full stack paths for debugging", () => { const hint = { originalException: Object.assign(new Error("x"), { code: "EACCES", syscall: "mkdir" }), @@ -78,9 +90,9 @@ describe("buildBeforeSend (api)", () => { { message: "reading ", category: "console", level: "info" }, ]); }); - it("falls back to type-only for unknown errors", () => { + it("keeps a redacted message for unknown errors", () => { const out = send(evt(), { originalException: new Error("user path /tmp/z") })!; - expect(out.exception.values[0].value).toBe("Error"); + expect(out.exception.values[0].value).toBe("user path "); }); it("applies the rebuilt value to the last (original) exception entry only", () => { const event = evt({ @@ -125,6 +137,25 @@ describe("buildBeforeSend (api)", () => { const out = send(evt({ contexts: { device: { hostname: "leak" } } }), {})!; expect(out.contexts).toBeUndefined(); }); + it("keeps a vetted python context and drops overlong fields", () => { + const event = { + ...evt(), + contexts: { + python: { + type: "RuntimeError", + frames: [ + { file: "remove_bg.py", line: 88, func: "run" }, + { file: "x".repeat(200), line: 1, func: "y".repeat(200) }, + ], + }, + }, + }; + const out = send(event, { originalException: new Error("x") })!; + expect(out.contexts.python.type).toBe("RuntimeError"); + expect(out.contexts.python.frames).toHaveLength(2); + expect(out.contexts.python.frames[1].file.length).toBeLessThanOrEqual(64); + expect(out.contexts.python.frames[1].func.length).toBeLessThanOrEqual(64); + }); it("enforces the 500-events-per-hour ceiling", () => { for (let i = 0; i < 500; i++) expect(send(evt(), {})).not.toBeNull(); expect(send(evt(), {})).toBeNull(); diff --git a/tests/unit/shared/error-sanitize.test.ts b/tests/unit/shared/error-sanitize.test.ts index 7c17863d..81c62fca 100644 --- a/tests/unit/shared/error-sanitize.test.ts +++ b/tests/unit/shared/error-sanitize.test.ts @@ -66,8 +66,11 @@ describe("rebuildErrorValue", () => { }); expect(rebuildErrorValue(err)).toBe("zod invalid_type at files.~.0"); }); - it("returns null for unknown errors (caller falls back to type-only)", () => { - expect(rebuildErrorValue(new Error("user file /tmp/x.pdf broke"))).toBeNull(); + it("keeps a redacted message for an unknown Error, null only for non-objects", () => { + // Was type-only before the fallback ladder; now the message survives, redacted. + expect(rebuildErrorValue(new Error("user file /tmp/x.pdf broke"))).toBe( + "user file broke", + ); expect(rebuildErrorValue("string")).toBeNull(); expect(rebuildErrorValue(null)).toBeNull(); }); @@ -78,10 +81,61 @@ describe("rebuildErrorValue", () => { }); expect(rebuildErrorValue(err)).toBe("HttpError 502"); }); - it("returns null for a circular cause chain without hanging", () => { + it("keeps the redacted message for a circular cause chain without hanging", () => { const err = new Error("loop") as Error & { cause?: unknown }; err.cause = err; - expect(rebuildErrorValue(err)).toBeNull(); + // chain()'s max cap breaks the cycle; the top message still surfaces. + expect(rebuildErrorValue(err)).toBe("loop"); + }); +}); + +describe("rebuildErrorValue ladder", () => { + it("keeps a redacted message for an unknown error (was type-only)", () => { + expect(rebuildErrorValue(new Error("Background removal failed"))).toBe( + "Background removal failed", + ); + expect(rebuildErrorValue(new Error("open /data/uploads/9f/in.bin"))).toBe("open "); + }); + + it("redacts a SafeError message (NODE-4W path leak)", () => { + const e = new SafeError("[Errno 13] Permission denied: '/root/.u2net/x.onnx'", { kind: "bug" }); + expect(rebuildErrorValue(e)).toBe("[Errno 13] Permission denied: ''"); + }); + + it("appends the redacted cause to a SafeError title (NODE-3D)", () => { + const cause = new Error("unsupported image format for /data/x.heic"); + const e = new SafeError("Image conversion failed", { kind: "bug", cause }); + expect(rebuildErrorValue(e)).toBe( + "Image conversion failed: unsupported image format for ", + ); + }); + + it("derives a frame title for an empty-message error (NODE-3C)", () => { + const e = new Error(""); + e.stack = "Error\n at Object.process (/app/apps/api/src/routes/tools/rounded-crop.ts:96:10)"; + expect(rebuildErrorValue(e)).toBe("at rounded-crop.ts:96"); + }); + + it("still prefers a pg SQLSTATE rebuild over the raw message", () => { + const e = Object.assign(new Error(`password authentication failed for user "x"`), { + code: "28P01", + routine: "auth_failed", + }); + expect(rebuildErrorValue(e)).toBe("pg 28P01 auth_failed"); + }); + + it("returns null only when there is no message and no stack", () => { + expect(rebuildErrorValue({ name: "Weird" })).toBeNull(); + }); + + it("handles a pathological stack without catastrophic backtracking", () => { + const e = new Error(""); + e.stack = `Error\n at x (/apps/${".".repeat(100000)})`; + const start = performance.now(); + const out = rebuildErrorValue(e); + const elapsed = performance.now() - start; + expect(elapsed).toBeLessThan(500); + expect(out).toBeNull(); }); }); diff --git a/tests/unit/shared/redact-message.test.ts b/tests/unit/shared/redact-message.test.ts new file mode 100644 index 00000000..24228e9d --- /dev/null +++ b/tests/unit/shared/redact-message.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from "vitest"; +import { redactMessage } from "../../../packages/shared/src/analytics/redact-message.js"; + +describe("redactMessage (default)", () => { + it("masks absolute paths", () => { + expect(redactMessage("ENOENT open /data/uploads/9f/input.bin")).toBe("ENOENT open "); + }); + it("masks a user filename token by known extension", () => { + expect(redactMessage("cannot read family_photo.JPG")).toBe("cannot read "); + }); + it("keeps a source filename (code extension, not a user file)", () => { + expect(redactMessage("failed in rounded-crop.ts")).toBe("failed in rounded-crop.ts"); + }); + it("masks a non-ASCII user filename", () => { + expect(redactMessage("写真.jpg not found")).toBe(" not found"); + }); + it("masks a data: URI so base64 content cannot leak", () => { + expect(redactMessage("bad img data:image/png;base64,iVBORw0KGgoAAAA end")).toBe( + "bad img end", + ); + }); + it("masks emails", () => { + expect(redactMessage("login failed for a.b+x@example.com")).toBe("login failed for "); + }); + it("masks a long quoted literal but keeps the quotes", () => { + expect(redactMessage(`bad value "this is a long user supplied caption here"`)).toBe( + `bad value ""`, + ); + }); + it("masks urls and blob refs, blob before url", () => { + expect(redactMessage("fetch blob:https://x/y then https://a.b/c")).toBe( + "fetch then ", + ); + }); + it("keeps a version string intact", () => { + expect(redactMessage("torch 2.2.0 cannot access GPU")).toBe("torch 2.2.0 cannot access GPU"); + }); + it("caps length", () => { + const long = redactMessage("x".repeat(500)); + expect(long.length).toBeLessThanOrEqual(301); + expect(long.endsWith("…")).toBe(true); + }); +}); + +describe("redactMessage (raw)", () => { + it("keeps paths and filenames, strips only control chars and caps", () => { + expect(redactMessage("open /data/x/report.pdf", { raw: true })).toBe("open /data/x/report.pdf"); + }); +}); + +describe("redactMessage adversarial", () => { + it("masks an IPv4 address (never-collect: IP)", () => { + expect(redactMessage("connect to 192.168.10.5:5432 refused")).toBe( + "connect to :5432 refused", + ); + }); + it("masks a windows path", () => { + expect(redactMessage("open C:\\Users\\jane\\photo.png failed")).toBe("open failed"); + }); + it("masks an email inside a long quoted parameter", () => { + expect( + redactMessage(`Failed query: update where email = 'averylonguseraddress@example.com'`), + ).toContain(""); + }); +}); + +describe("redactMessage IPv6 and relative keys", () => { + it("masks a link-local IPv6 address", () => { + expect(redactMessage("connect to fe80::1ff:fe23:4567:890a failed")).toBe( + "connect to failed", + ); + }); + it("masks a bracketed IPv6 with port", () => { + expect(redactMessage("peer [2001:db8::8a2e:370:7334]:443 down")).toBe("peer []:443 down"); + }); + it("masks the IPv6 loopback", () => { + expect(redactMessage("bind ::1 ok")).toBe("bind ok"); + }); + it("masks a full 8-group IPv6", () => { + expect(redactMessage("host 2001:db8:0:0:0:0:0:1 up")).toBe("host up"); + }); + it("still masks IPv4 and leaves versions intact", () => { + expect(redactMessage("host 10.0.0.1 up")).toBe("host up"); + expect(redactMessage("torch 2.2.0 ok")).toBe("torch 2.2.0 ok"); + }); + it("masks a relative object-storage key", () => { + expect(redactMessage("ENOENT uploads/3f2a/input.bin missing")).toBe("ENOENT missing"); + expect(redactMessage("wrote outputs/9b7c/result.dat")).toBe("wrote "); + }); + it("does not mangle C++/Rust scope resolution", () => { + expect(redactMessage("terminate called: std::bad_alloc")).toBe( + "terminate called: std::bad_alloc", + ); + expect(redactMessage("panic in core::result::unwrap")).toBe("panic in core::result::unwrap"); + }); +}); diff --git a/tests/unit/web/sentry-scrub.test.ts b/tests/unit/web/sentry-scrub.test.ts index 29e443e0..1eb89175 100644 --- a/tests/unit/web/sentry-scrub.test.ts +++ b/tests/unit/web/sentry-scrub.test.ts @@ -1,60 +1,5 @@ import { describe, expect, it } from "vitest"; -import { - buildWebBeforeSend, - DENY_URLS, - IGNORE_ERRORS, - scrubBrowserMessage, -} from "@/lib/sentry-scrub"; - -describe("scrubBrowserMessage", () => { - it("keeps browser-native messages with urls/paths redacted", () => { - expect(scrubBrowserMessage("TypeError", "Failed to fetch https://intra.host/x?q=1")).toBe( - "Failed to fetch ", - ); - expect(scrubBrowserMessage("TypeError", "cannot read /Users/bob/file.png")).toBe( - "cannot read ", - ); - expect(scrubBrowserMessage("RangeError", "Invalid array length")).toBe("Invalid array length"); - }); - - it("redacts blob urls and windows paths", () => { - expect(scrubBrowserMessage("DOMException", "load blob:http://x/abc failed")).toBe( - "load failed", - ); - expect(scrubBrowserMessage("TypeError", "open C:\\Users\\bob\\tax.pdf")).toBe("open "); - }); - - it("drops messages for non-native error names", () => { - expect(scrubBrowserMessage("CustomerDataError", "contains secret.pdf")).toBeNull(); - }); - - // DOMExceptions report their specific name ("NotFoundError"), not - // "DOMException", so listing only the base name dropped the diagnostic - // browser message for the whole family (WEB-3/4/6 showed as - // "NotFoundError: NotFoundError" with no way to tell which DOM call failed). - it("keeps messages for specific DOMException names, still redacted", () => { - expect( - scrubBrowserMessage( - "NotFoundError", - "Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.", - ), - ).toBe( - "Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.", - ); - expect(scrubBrowserMessage("InvalidStateError", "The object is in an invalid state.")).toBe( - "The object is in an invalid state.", - ); - expect(scrubBrowserMessage("NotAllowedError", "Write permission denied.")).toBe( - "Write permission denied.", - ); - expect(scrubBrowserMessage("NotReadableError", "error reading /Users/bob/file.png")).toBe( - "error reading ", - ); - expect(scrubBrowserMessage("DataCloneError", "could not be cloned.")).toBe( - "could not be cloned.", - ); - }); -}); +import { buildWebBeforeSend, DENY_URLS, IGNORE_ERRORS } from "@/lib/sentry-scrub"; describe("static filter lists", () => { it("deny extension frames and ignore noisy network errors", () => { @@ -119,11 +64,19 @@ describe("buildWebBeforeSend", () => { ]); }); - it("falls back to type-only for non-native exceptions without a rebuild", () => { + it("keeps a redacted message for a non-native error", () => { const send = buildWebBeforeSend(() => true); const custom = Object.assign(new Error("user secret"), { name: "WeirdLibError" }); const out = send(baseEvent(), { originalException: custom })!; - expect(out.exception.values[0].value).toBe("TypeError"); + expect(out.exception.values[0].value).toBe("user secret"); + }); + + it("keeps a redacted message for a non-native app error", () => { + const send = buildWebBeforeSend(() => true); + const out = send(baseEvent(), { + originalException: new Error("upload failed for report.pdf"), + })!; + expect(out.exception.values[0].value).toBe("upload failed for "); }); it("enforces the 500-per-hour ceiling", () => {