From 21bba1b762f806d8beaac4fbcc6e8eb3b0b2445e Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 1 Sep 2026 09:49:55 +0000 Subject: [PATCH] fix: report extra only when it serializes, replacing the key blocklist Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01H9xMqWcQBvJwrvMb5Tw3nH --- backend/tests/bun_jobs.rs | 48 ++++++++++++--------------- backend/windmill-worker/src/common.rs | 29 +++++++++------- 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index 3bd9ac7f7e..bd3457352c 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -401,29 +401,6 @@ async fn test_bun_job_non_error_throws(db: Pool) -> anyhow::Result<()> assert_eq!(result["error"]["message"], serde_json::json!("null")); } - // Circular object: several HTTP clients throw errors that link two objects - // both ways, which a plain JSON.stringify cannot serialize. - { - let job = bun_job( - r#" -export function main() { - const a: any = { name: "circular", message: "boom" }; - a.self = a; - throw a; -} -"#, - ); - let completed = run_job_in_new_worker_until_complete(&db, false, job, port).await; - assert!(!completed.success); - let result = completed.json_result().unwrap(); - let error = &result["error"]; - assert_eq!(error["message"], serde_json::json!("boom")); - assert_eq!( - error["extra"]["self"]["self"], - serde_json::json!("[Circular]") - ); - } - // A thrown string is a string, not an index map of its characters. { let job = bun_job(r#"export function main() { throw "nur ein string"; }"#); @@ -436,8 +413,10 @@ export function main() { ); } - // Serializing a cycle is what first brings an HTTP client's whole request into - // `extra`, so the transport credentials on it must not reach the stored result. + // An HTTP client links its errors both ways, which is what a plain JSON.stringify + // could not serialize. `extra` is reported only when it serializes on its own, so the + // request the client hangs off the error, credentials included, stays out of the + // result exactly as it did when the wrapper crashed. { let job = bun_job( r#" @@ -446,6 +425,7 @@ export function main() { url: "/x", headers: { Authorization: "Bearer sentinel-tkn" }, auth: { username: "svc", password: "sentinel-tkn" }, + httpsAgent: { options: { key: "-----BEGIN PRIVATE KEY-----sentinel-tkn" } }, }; const request: any = { path: "/x", _header: "GET /x HTTP/1.1\r\nAuthorization: Bearer sentinel-tkn\r\n\r\n" }; const response: any = { status: 401, config, request }; @@ -467,13 +447,29 @@ export function main() { error["message"], serde_json::json!("Request failed with status code 401") ); - assert_eq!(error["extra"]["response"]["status"], serde_json::json!(401)); + assert_eq!(error["name"], serde_json::json!("AxiosError")); + assert_eq!( + error["extra"], + serde_json::json!("[extra omitted: not serializable]") + ); assert!( !error.to_string().contains("sentinel-tkn"), "credential reached the result: {error}" ); } + // An acyclic thrown object still reports its own properties, unchanged. + { + let job = bun_job(r#"export function main() { throw { code: 42, hint: "kein Error" }; }"#); + let completed = run_job_in_new_worker_until_complete(&db, false, job, port).await; + assert!(!completed.success); + let result = completed.json_result().unwrap(); + assert_eq!( + result["error"]["extra"], + serde_json::json!({ "code": 42, "hint": "kein Error" }) + ); + } + Ok(()) } diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 223c426ae1..00e08201cb 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -79,20 +79,19 @@ pub const DEV_CONF_NSJAIL: &str = ""; /// Enabling it there would newly persist whatever an HTTP client hangs off its errors, /// credential-bearing headers included, into existing scripts' run history. /// -/// `wmErrDescribe` renders a thrown object's contents, so it stays behind the same line: -/// it runs only for a value exposing none of message/name/stack, which an HTTP client's -/// error never is, and only to fill a message that would otherwise be `[object Object]`. +/// `extra` is reported only when it serializes on its own, which is exactly when the +/// pre-existing bare `JSON.stringify` would have succeeded. Anything else is what used to +/// crash, so persisting it now would be new exposure: an HTTP client hangs its whole +/// request off a cyclic error, credentials and mTLS keys included, and no blocklist of key +/// names can be complete. Keeping the condition rather than the key list is what bounds it. /// -/// Making a cyclic graph serializable is what puts an HTTP client's whole request under -/// `extra` for the first time, so the replacer drops transport-credential keys and the -/// walk is capped: `console.error` prints those nested objects as `[Object ...]`, and the -/// crash kept them out of the result entirely, so neither had reached a stored result. +/// `wmErrDescribe` also renders a thrown object's contents, so it stays behind the same +/// line: it fills a message only for a value that yielded no field at all, never for one +/// carrying `message`/`name`/`stack` or reportable `extra`. /// /// Comment-free on purpose: it is written into each job's wrapper source. pub const JS_ERROR_SERIALIZER: &str = r#" const wmErrOwnKeys = ['line', 'name', 'stack', 'column', 'message', 'sourceURL', 'originalLine', 'originalColumn']; -const wmErrSecretKeys = /^(authorization|proxy-authorization|cookie|set-cookie|x-api-key|api[-_]?key|auth|passphrase|_header)$/i; -const wmErrMaxExtra = 100000; function wmErrString(v) { if (typeof v === 'string') return v; @@ -111,6 +110,14 @@ function wmErrProp(e, key) { } } +function wmErrSerializable(v) { + try { + return JSON.stringify(v) !== undefined; + } catch (_) { + return false; + } +} + function wmErrDescribe(v) { if (v === null || typeof v !== 'object') return wmErrString(v); let s; @@ -142,8 +149,7 @@ function wmToErrorObject(e, collectExtra) { if (v !== undefined) extra[key] = v; } if (Object.keys(extra).length > 0) { - const s = wmSerializeError(extra); - err.extra = s !== undefined && s.length <= wmErrMaxExtra ? extra : '[extra omitted: too large]'; + err.extra = wmErrSerializable(extra) ? extra : '[extra omitted: not serializable]'; } } if (Object.keys(err).length === 0) { @@ -157,7 +163,6 @@ function wmSerializeError(err) { const ancestors = []; try { return JSON.stringify(err, function (key, v) { - if (wmErrSecretKeys.test(key)) return '[redacted]'; if (v === null || typeof v !== 'object') return v; while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) ancestors.pop(); if (ancestors.indexOf(v) !== -1) return '[Circular]';