From b8d6166b5d86408096ae79011ec5ef0b29f36f8b Mon Sep 17 00:00:00 2001 From: Adam Mcgrath Date: Fri, 16 Jan 2026 11:45:11 +0000 Subject: [PATCH 1/5] 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/5] 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]); }); }); From 2ce5f86823fb6b5db2b392ec9e9620df512bd20d Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 10 Sep 2026 14:25:40 -0500 Subject: [PATCH 3/5] Add test for `computeSignature` callback on signing failure Ensures the `computeSignature` callback is invoked exactly once with an error, and `getSignedXml()` returns an empty string, when the cryptographic signing operation fails. This test complements existing error handling tests by specifically covering failures originating from the signing process itself, such as using an invalid key. --- test/types-tests.spec.ts | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/test/types-tests.spec.ts b/test/types-tests.spec.ts index 348e7ad6..115b79b6 100644 --- a/test/types-tests.spec.ts +++ b/test/types-tests.spec.ts @@ -3,11 +3,11 @@ import * as fs from "fs"; import { expect } from "chai"; 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 xml = ``; + + function createSigner(privateKey: Buffer): SignedXml { const sig = new SignedXml(); - sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.privateKey = privateKey; sig.addReference({ xpath: "//*[local-name(.)='x']", digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1", @@ -15,6 +15,12 @@ describe("Callback invocation", function () { }); sig.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#"; sig.signatureAlgorithm = "http://www.w3.org/2000/09/xmldsig#rsa-sha1"; + return sig; + } + + // https://github.com/node-saml/xml-crypto/issues/527 + it("invokes the callback once when the callback throws", function () { + const sig = createSigner(fs.readFileSync("./test/static/client.pem")); const errorsSeen: (string | null)[] = []; const callback: ErrorFirstCallback = (err) => { @@ -25,4 +31,14 @@ describe("Callback invocation", function () { expect(() => sig.computeSignature(xml, callback)).to.throw("Error Thrown"); expect(errorsSeen).to.deep.equal([null]); }); + + it("invokes the callback once, with an error, when signing fails", function () { + const sig = createSigner(fs.readFileSync("./test/static/client_public.pem")); + + const outcomes: string[] = []; + sig.computeSignature(xml, (err) => outcomes.push(err ? "error" : "success")); + + expect(outcomes).to.deep.equal(["error"]); + expect(sig.getSignedXml()).to.equal(""); + }); }); From abfa93e1f160f1c193673f1356e4baec9ce5770f Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 10 Sep 2026 14:54:46 -0500 Subject: [PATCH 4/5] Remove pointless comment --- src/types.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/types.ts b/src/types.ts index decd170e..322273fe 100644 --- a/src/types.ts +++ b/src/types.ts @@ -254,9 +254,6 @@ export function createOptionalCallbackFunction( 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 { result = syncVersion(...(args.slice(0, -1) as A)); } catch (err) { From 71ccb63e54d8ec4f0ac7734dedbf9c64fab611b5 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Thu, 10 Sep 2026 15:10:58 -0500 Subject: [PATCH 5/5] Remove comment --- test/types-tests.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/types-tests.spec.ts b/test/types-tests.spec.ts index 115b79b6..037b4e47 100644 --- a/test/types-tests.spec.ts +++ b/test/types-tests.spec.ts @@ -18,7 +18,6 @@ describe("Callback invocation", function () { return sig; } - // https://github.com/node-saml/xml-crypto/issues/527 it("invokes the callback once when the callback throws", function () { const sig = createSigner(fs.readFileSync("./test/static/client.pem"));