From b8d6166b5d86408096ae79011ec5ef0b29f36f8b Mon Sep 17 00:00:00 2001 From: Adam Mcgrath Date: Fri, 16 Jan 2026 11:45:11 +0000 Subject: [PATCH 1/2] Fix double callback invoke on unhandled exception --- src/types.ts | 2 +- test/types-tests.spec.ts | 52 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 test/types-tests.spec.ts diff --git a/src/types.ts b/src/types.ts index 89c0b304..160a801a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -256,7 +256,7 @@ export function createOptionalCallbackFunction( if (isErrorFirstCallback(possibleCallback)) { try { const result = syncVersion(...(args.slice(0, -1) as A)); - possibleCallback(null, result); + process.nextTick(() => possibleCallback(null, result)); } catch (err) { possibleCallback(err instanceof Error ? err : new Error("Unknown error")); } diff --git a/test/types-tests.spec.ts b/test/types-tests.spec.ts new file mode 100644 index 00000000..4bcbb7c6 --- /dev/null +++ b/test/types-tests.spec.ts @@ -0,0 +1,52 @@ +/// +import * as types from "../src/types"; +import { expect } from "chai"; + +describe("createOptionalCallbackFunction", function () { + it("should not execute callback twice when callback throws unhandled exception", function (done) { + const syncFn = (a: number, b: number) => a + b; + const flexibleFn = types.createOptionalCallbackFunction(syncFn); + + let callbackExecutionCount = 0; + + // Store and remove existing unhandled exception listeners + const existingListeners = process.rawListeners("uncaughtException"); + process.removeAllListeners("uncaughtException"); + + process.once("uncaughtException", (err) => { + // Restore unhandled exception listeners + existingListeners.forEach((listener) => { + process.on("uncaughtException", listener as NodeJS.UncaughtExceptionListener); + }); + + expect(err.message).to.equal("Callback threw an error"); + expect(callbackExecutionCount).to.equal(1); + done(); + }); + + flexibleFn(2, 3, (err, result) => { + callbackExecutionCount++; + expect(err).to.be.null; + expect(result).to.equal(5); + + throw new Error("Callback threw an error"); + }); + }); + + it("should defer callback execution in success case", function (done) { + const syncFn = (a: number, b: number) => a + b; + const flexibleFn = types.createOptionalCallbackFunction(syncFn); + + let callbackExecuted = false; + + flexibleFn(2, 3, (err, result) => { + callbackExecuted = true; + expect(err).to.be.null; + expect(result).to.equal(5); + done(); + }); + + // Callback should be asynchronously deferred + expect(callbackExecuted).to.be.false; + }); +}); From b97e5dad98d9245faa6deb87ae43db14453d46af Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Tue, 8 Sep 2026 08:16:07 -0500 Subject: [PATCH 2/2] Fix double callback invocation without changing callback timing The callback was invoked inside the `try`, so an exception thrown by the callback itself landed in the `catch` and invoked it a second time with its own error as `err`. Narrow the `try` to cover only `syncVersion`. This keeps the callback synchronous. Deferring it with `process.nextTick` also stops the double invocation, but changes when the callback runs: `computeSignature(xml, cb)` would return before `cb` fires, so an existing caller reading `getSignedXml()` immediately after gets `""` rather than the signed document -- a silent breaking change for a public, semver-bound API. De-Zalgo-ing these callbacks is worth doing, but as a deliberate major. The `return` in the `catch` is compiler-enforced: without it `result` is not definitely assigned and `tsc --strict` rejects the code, so the error path cannot regress into a double invocation. Replace the helper-level tests with one driving the public `computeSignature(xml, callback)` path from the issue. It is synchronous, so it no longer removes and restores the process `uncaughtException` listeners, which leaked mocha's handler when the test failed. Resolves #527 Co-Authored-By: Claude Opus 5 --- src/types.ts | 9 ++++-- test/types-tests.spec.ts | 66 +++++++++++++--------------------------- 2 files changed, 28 insertions(+), 47 deletions(-) diff --git a/src/types.ts b/src/types.ts index 160a801a..58949622 100644 --- a/src/types.ts +++ b/src/types.ts @@ -254,12 +254,17 @@ export function createOptionalCallbackFunction( return ((...args: A | [...A, ErrorFirstCallback]) => { const possibleCallback = args[args.length - 1]; if (isErrorFirstCallback(possibleCallback)) { + let result: T; + // Only `syncVersion` may run inside the `try`. Invoking the callback there would let an + // exception thrown *by the callback* land in the `catch` and invoke it a second time. + // https://github.com/node-saml/xml-crypto/issues/527 try { - const result = syncVersion(...(args.slice(0, -1) as A)); - process.nextTick(() => possibleCallback(null, result)); + result = syncVersion(...(args.slice(0, -1) as A)); } catch (err) { possibleCallback(err instanceof Error ? err : new Error("Unknown error")); + return; } + possibleCallback(null, result); } else { return syncVersion(...(args as A)); } diff --git a/test/types-tests.spec.ts b/test/types-tests.spec.ts index 4bcbb7c6..348e7ad6 100644 --- a/test/types-tests.spec.ts +++ b/test/types-tests.spec.ts @@ -1,52 +1,28 @@ -/// -import * as types from "../src/types"; +import { SignedXml, type ErrorFirstCallback } from "../src/index"; +import * as fs from "fs"; import { expect } from "chai"; -describe("createOptionalCallbackFunction", function () { - it("should not execute callback twice when callback throws unhandled exception", function (done) { - const syncFn = (a: number, b: number) => a + b; - const flexibleFn = types.createOptionalCallbackFunction(syncFn); - - let callbackExecutionCount = 0; - - // Store and remove existing unhandled exception listeners - const existingListeners = process.rawListeners("uncaughtException"); - process.removeAllListeners("uncaughtException"); - - process.once("uncaughtException", (err) => { - // Restore unhandled exception listeners - existingListeners.forEach((listener) => { - process.on("uncaughtException", listener as NodeJS.UncaughtExceptionListener); - }); - - expect(err.message).to.equal("Callback threw an error"); - expect(callbackExecutionCount).to.equal(1); - done(); +describe("Callback invocation", function () { + // https://github.com/node-saml/xml-crypto/issues/527 + it("invokes the callback once when the callback throws", function () { + const xml = ``; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", + transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"], }); + sig.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#"; + sig.signatureAlgorithm = "http://www.w3.org/2000/09/xmldsig#rsa-sha1"; - flexibleFn(2, 3, (err, result) => { - callbackExecutionCount++; - expect(err).to.be.null; - expect(result).to.equal(5); - - throw new Error("Callback threw an error"); - }); - }); - - it("should defer callback execution in success case", function (done) { - const syncFn = (a: number, b: number) => a + b; - const flexibleFn = types.createOptionalCallbackFunction(syncFn); - - let callbackExecuted = false; - - flexibleFn(2, 3, (err, result) => { - callbackExecuted = true; - expect(err).to.be.null; - expect(result).to.equal(5); - done(); - }); + const errorsSeen: (string | null)[] = []; + const callback: ErrorFirstCallback = (err) => { + errorsSeen.push(err ? err.message : null); + throw new Error("Error Thrown"); + }; - // Callback should be asynchronously deferred - expect(callbackExecuted).to.be.false; + expect(() => sig.computeSignature(xml, callback)).to.throw("Error Thrown"); + expect(errorsSeen).to.deep.equal([null]); }); });