mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
fix(jobs): strip internal paths from all worker SSE error frames
Closes #71. Several error paths in the worker could leak internal filesystem paths (/tmp/workspace, /data/ai/venv, /app) through SSE frames, resultPayload objects, and Redis batch-error lists. The existing stripInternalPaths call at worker.ts line 345 only covered the single-file processToolJob catch block. Wrapped 6 additional call sites: - processPipelineStep: prevError from DB and catch errorMsg - processPipelineFinalize: composed errorMsg reaching SSE, recordChildOutcome, and resultPayload - processBatchChild: catch error reaching recordChildOutcome and resultPayload - processBatchFinalize: manifest errorMsg from DB rows - recordChildOutcome (batch-progress.ts): defense-in-depth strip before Redis rpush Added 10 unit tests for stripInternalPaths covering /tmp, /data, /app, /opt, /home, /workspace, multi-path messages, safe passthrough, and pipeline-step wrapping.
This commit is contained in:
@@ -12,6 +12,7 @@
|
|||||||
* "failed" only when every file failed
|
* "failed" only when every file failed
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
import { stripInternalPaths } from "../lib/errors.js";
|
||||||
import { updateJobProgress } from "../routes/progress.js";
|
import { updateJobProgress } from "../routes/progress.js";
|
||||||
import { sharedRedis } from "./connection.js";
|
import { sharedRedis } from "./connection.js";
|
||||||
import { bullPrefix } from "./types.js";
|
import { bullPrefix } from "./types.js";
|
||||||
@@ -37,7 +38,8 @@ export async function recordChildOutcome(
|
|||||||
const base = `${bullPrefix()}:batch:${parentId}`;
|
const base = `${bullPrefix()}:batch:${parentId}`;
|
||||||
const done = await r.incr(`${base}:${error ? "failed" : "done"}`);
|
const done = await r.incr(`${base}:${error ? "failed" : "done"}`);
|
||||||
const other = Number((await r.get(`${base}:${error ? "done" : "failed"}`)) ?? 0);
|
const other = Number((await r.get(`${base}:${error ? "done" : "failed"}`)) ?? 0);
|
||||||
if (error) await r.rpush(`${base}:errors`, JSON.stringify({ filename, error }));
|
if (error)
|
||||||
|
await r.rpush(`${base}:errors`, JSON.stringify({ filename, error: stripInternalPaths(error) }));
|
||||||
await r.expire(`${base}:done`, 3600);
|
await r.expire(`${base}:done`, 3600);
|
||||||
await r.expire(`${base}:failed`, 3600);
|
await r.expire(`${base}:failed`, 3600);
|
||||||
await r.expire(`${base}:errors`, 3600);
|
await r.expire(`${base}:errors`, 3600);
|
||||||
|
|||||||
@@ -387,10 +387,11 @@ async function processPipelineStep(job: Job<ToolJobData>): Promise<ToolJobResult
|
|||||||
|
|
||||||
if (!prevRow || prevRow.status === "failed" || !prevRow.outputRefs?.[0]) {
|
if (!prevRow || prevRow.status === "failed" || !prevRow.outputRefs?.[0]) {
|
||||||
// Previous step failed -- propagate the error without processing.
|
// Previous step failed -- propagate the error without processing.
|
||||||
const prevError =
|
const prevError = stripInternalPaths(
|
||||||
prevRow?.status === "failed"
|
prevRow?.status === "failed"
|
||||||
? ((prevRow.error as { message?: string } | null)?.message ?? "Processing failed")
|
? ((prevRow.error as { message?: string } | null)?.message ?? "Processing failed")
|
||||||
: "Previous step has no output";
|
: "Previous step has no output",
|
||||||
|
);
|
||||||
await db
|
await db
|
||||||
.update(schema.jobs)
|
.update(schema.jobs)
|
||||||
.set({ status: "failed", completedAt: new Date(), error: { message: prevError } })
|
.set({ status: "failed", completedAt: new Date(), error: { message: prevError } })
|
||||||
@@ -426,7 +427,7 @@ async function processPipelineStep(job: Job<ToolJobData>): Promise<ToolJobResult
|
|||||||
// Step failed -- return failure marker. processToolJob already
|
// Step failed -- return failure marker. processToolJob already
|
||||||
// updated the DB row to "failed" and emitted a terminal event
|
// updated the DB row to "failed" and emitted a terminal event
|
||||||
// on the step's own progress channel.
|
// on the step's own progress channel.
|
||||||
const errorMsg = err instanceof Error ? err.message : String(err);
|
const errorMsg = stripInternalPaths(err instanceof Error ? err.message : String(err));
|
||||||
return {
|
return {
|
||||||
outputRefs: [],
|
outputRefs: [],
|
||||||
filename: data.filename,
|
filename: data.filename,
|
||||||
@@ -494,7 +495,7 @@ async function processPipelineFinalize(job: Job<ToolJobData>): Promise<ToolJobRe
|
|||||||
|
|
||||||
// ── Failure path ────────────────────────────────────────────
|
// ── Failure path ────────────────────────────────────────────
|
||||||
if (failedAtStep !== null) {
|
if (failedAtStep !== null) {
|
||||||
const errorMsg = `Step ${failedAtStep + 1}: ${failError}`;
|
const errorMsg = stripInternalPaths(`Step ${failedAtStep + 1}: ${failError}`);
|
||||||
|
|
||||||
await db
|
await db
|
||||||
.update(schema.jobs)
|
.update(schema.jobs)
|
||||||
@@ -596,7 +597,7 @@ async function processBatchChild(job: Job<ToolJobData>): Promise<ToolJobResult>
|
|||||||
await recordChildOutcome(job.data.parentId!, job.data.totalFiles!, job.data.filename);
|
await recordChildOutcome(job.data.parentId!, job.data.totalFiles!, job.data.filename);
|
||||||
return result;
|
return result;
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
const error = err instanceof Error ? err.message : String(err);
|
const error = stripInternalPaths(err instanceof Error ? err.message : String(err));
|
||||||
await recordChildOutcome(job.data.parentId!, job.data.totalFiles!, job.data.filename, error);
|
await recordChildOutcome(job.data.parentId!, job.data.totalFiles!, job.data.filename, error);
|
||||||
// Return a completed job with a failure marker so the parent runs.
|
// Return a completed job with a failure marker so the parent runs.
|
||||||
return {
|
return {
|
||||||
@@ -647,7 +648,7 @@ async function processBatchFinalize(job: Job<ToolJobData>): Promise<ToolJobResul
|
|||||||
} else {
|
} else {
|
||||||
const errorMsg = (row.error as { message?: string } | null)?.message ?? "Processing failed";
|
const errorMsg = (row.error as { message?: string } | null)?.message ?? "Processing failed";
|
||||||
const inputFilename = row.inputRefs?.[0]?.split("/").pop() ?? `file-${i}`;
|
const inputFilename = row.inputRefs?.[0]?.split("/").pop() ?? `file-${i}`;
|
||||||
manifest.push({ index: i, filename: inputFilename, error: errorMsg });
|
manifest.push({ index: i, filename: inputFilename, error: stripInternalPaths(errorMsg) });
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
import { describe, expect, it } from "vitest";
|
import { describe, expect, it } from "vitest";
|
||||||
import { formatZodErrors } from "../../../apps/api/src/lib/errors.js";
|
import { formatZodErrors, stripInternalPaths } from "../../../apps/api/src/lib/errors.js";
|
||||||
|
|
||||||
describe("formatZodErrors", () => {
|
describe("formatZodErrors", () => {
|
||||||
it("formats single issue with path", () => {
|
it("formats single issue with path", () => {
|
||||||
@@ -34,3 +34,58 @@ describe("formatZodErrors", () => {
|
|||||||
expect(result).toBe("settings.quality: Too high");
|
expect(result).toBe("settings.quality: Too high");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("stripInternalPaths", () => {
|
||||||
|
it("strips /tmp scratch paths", () => {
|
||||||
|
const msg = "ENOENT: no such file or directory, open '/tmp/workspace/abc123/input.png'";
|
||||||
|
expect(stripInternalPaths(msg)).toBe("ENOENT: no such file or directory, open '[internal]'");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips /data/ai/venv Python traceback paths", () => {
|
||||||
|
const msg = 'File "/data/ai/venv/lib/python3.11/site-packages/rembg/bg.py", line 42, in remove';
|
||||||
|
expect(stripInternalPaths(msg)).toBe('File "[internal]", line 42, in remove');
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips /app container paths", () => {
|
||||||
|
const msg = "Error loading model from /app/models/realesrgan-x4.pth";
|
||||||
|
expect(stripInternalPaths(msg)).toBe("Error loading model from [internal]");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips /opt paths", () => {
|
||||||
|
const msg = "Cannot find /opt/libreoffice/program/soffice";
|
||||||
|
expect(stripInternalPaths(msg)).toBe("Cannot find [internal]");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips /home paths", () => {
|
||||||
|
const msg = "Permission denied: /home/node/.cache/sharp";
|
||||||
|
expect(stripInternalPaths(msg)).toBe("Permission denied: [internal]");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips /workspace paths", () => {
|
||||||
|
const msg = "failed at /workspace/snapotter/packages/image-engine/src/convert.ts:42";
|
||||||
|
expect(stripInternalPaths(msg)).toBe("failed at [internal]");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("strips multiple paths in one message", () => {
|
||||||
|
const msg = "cp: /tmp/scratch/a.png -> /data/output/b.png failed";
|
||||||
|
expect(stripInternalPaths(msg)).toBe("cp: [internal] -> [internal] failed");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("leaves safe messages untouched", () => {
|
||||||
|
const msg = "Invalid image format: expected PNG or JPEG";
|
||||||
|
expect(stripInternalPaths(msg)).toBe(msg);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("leaves timeout messages untouched", () => {
|
||||||
|
const msg = "Timed out after 120s";
|
||||||
|
expect(stripInternalPaths(msg)).toBe(msg);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("handles pipeline step error wrapping", () => {
|
||||||
|
const msg = "Step 2: Command failed: /tmp/workspace/job123/ffmpeg -i /data/input.mp4";
|
||||||
|
const result = stripInternalPaths(msg);
|
||||||
|
expect(result).not.toContain("/tmp/");
|
||||||
|
expect(result).not.toContain("/data/");
|
||||||
|
expect(result).toBe("Step 2: Command failed: [internal] -i [internal]");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user