From 5174c66ca9d4c6f6b091f1cfdab6de83ca2b99d4 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Thu, 28 May 2026 08:43:50 -0700 Subject: [PATCH 1/5] stream: serialize concurrent share consumer reads Ensure overlapping next() calls on a single share() consumer resolve in the same order they were requested. Fixes: https://github.com/nodejs/node/issues/63477 Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: openai:gpt-5.5 PR-URL: https://github.com/nodejs/node/pull/63478 Fixes: https://github.com/nodejs/node/issues/63477 Reviewed-By: James M Snell --- lib/internal/streams/iter/share.js | 107 ++++++++++-------- test/parallel/test-stream-iter-share-async.js | 19 ++++ 2 files changed, 78 insertions(+), 48 deletions(-) diff --git a/lib/internal/streams/iter/share.js b/lib/internal/streams/iter/share.js index 3cd24409222712..f755550712efa7 100644 --- a/lib/internal/streams/iter/share.js +++ b/lib/internal/streams/iter/share.js @@ -113,6 +113,7 @@ class ShareImpl { resolve: null, reject: null, detached: false, + pendingNext: PromiseResolve(), }; this.#consumers.add(state); @@ -129,62 +130,72 @@ class ShareImpl { return { __proto__: null, [SymbolAsyncIterator]() { - return { - __proto__: null, - async next() { - if (self.#sourceError) { - state.detached = true; - self.#consumers.delete(state); - throw self.#sourceError; + const getNext = async () => { + if (self.#sourceError) { + state.detached = true; + self.#consumers.delete(state); + throw self.#sourceError; + } + + // Loop until we get data, source is exhausted, or + // consumer is detached. Multiple consumers may be woken + // after a single pull - those that find no data at their + // cursor must re-pull rather than terminating prematurely. + for (;;) { + if (state.detached) { + if (self.#sourceError) throw self.#sourceError; + return { __proto__: null, done: true, value: undefined }; } - // Loop until we get data, source is exhausted, or - // consumer is detached. Multiple consumers may be woken - // after a single pull - those that find no data at their - // cursor must re-pull rather than terminating prematurely. - for (;;) { - if (state.detached) { - if (self.#sourceError) throw self.#sourceError; - return { __proto__: null, done: true, value: undefined }; - } + if (self.#cancelled) { + state.detached = true; + self.#deleteConsumer(state); + return { __proto__: null, done: true, value: undefined }; + } - if (self.#cancelled) { - state.detached = true; - self.#deleteConsumer(state); - return { __proto__: null, done: true, value: undefined }; + // Check if data is available in buffer + const bufferIndex = state.cursor - self.#bufferStart; + if (bufferIndex < self.#buffer.length) { + const chunk = self.#buffer.get(bufferIndex); + const cursor = state.cursor; + state.cursor++; + if (cursor === self.#cachedMinCursor && + --self.#cachedMinCursorConsumers === 0) { + self.#tryTrimBuffer(); } + return { __proto__: null, done: false, value: chunk }; + } - // Check if data is available in buffer - const bufferIndex = state.cursor - self.#bufferStart; - if (bufferIndex < self.#buffer.length) { - const chunk = self.#buffer.get(bufferIndex); - const cursor = state.cursor; - state.cursor++; - if (cursor === self.#cachedMinCursor && - --self.#cachedMinCursorConsumers === 0) { - self.#tryTrimBuffer(); - } - return { __proto__: null, done: false, value: chunk }; - } + if (self.#sourceExhausted) { + state.detached = true; + self.#deleteConsumer(state); + if (self.#sourceError) throw self.#sourceError; + return { __proto__: null, done: true, value: undefined }; + } - if (self.#sourceExhausted) { - state.detached = true; - self.#deleteConsumer(state); - if (self.#sourceError) throw self.#sourceError; - return { __proto__: null, done: true, value: undefined }; - } + // Need to pull from source - check buffer limit + const canPull = await self.#waitForBufferSpace(); + if (!canPull) { + state.detached = true; + self.#deleteConsumer(state); + if (self.#sourceError) throw self.#sourceError; + return { __proto__: null, done: true, value: undefined }; + } - // Need to pull from source - check buffer limit - const canPull = await self.#waitForBufferSpace(); - if (!canPull) { - state.detached = true; - self.#deleteConsumer(state); - if (self.#sourceError) throw self.#sourceError; - return { __proto__: null, done: true, value: undefined }; - } + await self.#pullFromSource(); + } + }; - await self.#pullFromSource(); - } + return { + __proto__: null, + next() { + const next = PromisePrototypeThen( + state.pendingNext, + getNext, + getNext); + state.pendingNext = + PromisePrototypeThen(next, undefined, () => {}); + return next; }, async return() { diff --git a/test/parallel/test-stream-iter-share-async.js b/test/parallel/test-stream-iter-share-async.js index c10ac71a7b3ead..7e1c06b6328f19 100644 --- a/test/parallel/test-stream-iter-share-async.js +++ b/test/parallel/test-stream-iter-share-async.js @@ -306,6 +306,24 @@ async function testShareMultipleConsumersConcurrentPull() { assert.strictEqual(t3, expected); } +async function testShareConsumerConcurrentNextCalls() { + async function* source() { + const enc = new TextEncoder(); + yield [enc.encode('first')]; + yield [enc.encode('second')]; + } + + const shared = share(source()); + const it = shared.pull()[Symbol.asyncIterator](); + const first = it.next(); + const second = it.next(); + + const [r1, r2] = await Promise.all([first, second]); + const dec = new TextDecoder(); + assert.strictEqual(dec.decode(r1.value[0]), 'first'); + assert.strictEqual(dec.decode(r2.value[0]), 'second'); +} + // share() accepts string source directly (normalized via from()) async function testShareStringSource() { const shared = share('hello-share'); @@ -327,5 +345,6 @@ Promise.all([ testShareLateJoiningConsumer(), testShareConsumerBreak(), testShareMultipleConsumersConcurrentPull(), + testShareConsumerConcurrentNextCalls(), testShareStringSource(), ]).then(common.mustCall()); From 4d21e86700f2932ccc09ceb1115af50903ece876 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Thu, 28 May 2026 08:44:05 -0700 Subject: [PATCH 2/5] tools: add lint rule for aborted AbortController Refs: https://github.com/nodejs/node/pull/63489 Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Co-authored-by: Antoine du Hamel Assisted-by: openai:gpt-5.5 PR-URL: https://github.com/nodejs/node/pull/63541 Reviewed-By: Ethan Arrowood Reviewed-By: Antoine du Hamel Reviewed-By: Stephen Belanger --- test/eslint.config_partial.mjs | 1 + test/parallel/test-abortcontroller.js | 2 + .../test-eslint-prefer-abort-signal-abort.js | 118 ++++++++++++++ .../test-quic-writer-abort-signal.mjs | 5 +- .../eslint-rules/prefer-abort-signal-abort.js | 154 ++++++++++++++++++ 5 files changed, 277 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-eslint-prefer-abort-signal-abort.js create mode 100644 tools/eslint-rules/prefer-abort-signal-abort.js diff --git a/test/eslint.config_partial.mjs b/test/eslint.config_partial.mjs index 6fbdf277044679..70c00ef5d4dcda 100644 --- a/test/eslint.config_partial.mjs +++ b/test/eslint.config_partial.mjs @@ -162,6 +162,7 @@ export default [ 'node-core/require-common-first': 'error', 'node-core/no-duplicate-requires': 'off', 'node-core/must-call-assert': 'error', + 'node-core/prefer-abort-signal-abort': 'error', }, }, { diff --git a/test/parallel/test-abortcontroller.js b/test/parallel/test-abortcontroller.js index 948bd208cd2b90..95eea21437ead9 100644 --- a/test/parallel/test-abortcontroller.js +++ b/test/parallel/test-abortcontroller.js @@ -161,6 +161,7 @@ test('AbortController inspection depth 1 or null works', () => { test('AbortSignal reason is set correctly', () => { // Test AbortSignal.reason + // eslint-disable-next-line node-core/prefer-abort-signal-abort const ac = new AbortController(); ac.abort('reason'); assert.strictEqual(ac.signal.reason, 'reason'); @@ -235,6 +236,7 @@ test('AbortSignal.reason should default', () => { assert.ok(signal.reason instanceof DOMException); assert.strictEqual(signal.reason.code, 20); + // eslint-disable-next-line node-core/prefer-abort-signal-abort const ac = new AbortController(); ac.abort(); assert.ok(ac.signal.reason instanceof DOMException); diff --git a/test/parallel/test-eslint-prefer-abort-signal-abort.js b/test/parallel/test-eslint-prefer-abort-signal-abort.js new file mode 100644 index 00000000000000..954d7df83dab02 --- /dev/null +++ b/test/parallel/test-eslint-prefer-abort-signal-abort.js @@ -0,0 +1,118 @@ +'use strict'; + +const common = require('../common'); +if ((!common.hasCrypto) || (!common.hasIntl)) { + common.skip('ESLint tests require crypto and Intl'); +} + +common.skipIfEslintMissing(); + +const RuleTester = require('../../tools/eslint/node_modules/eslint').RuleTester; +const rule = require('../../tools/eslint-rules/prefer-abort-signal-abort'); + +const message = 'Use AbortSignal.abort() instead of creating and aborting an AbortController.'; + +new RuleTester().run('prefer-abort-signal-abort', rule, { + valid: [ + 'const signal = AbortSignal.abort();', + ` + const controller = new AbortController(); + controller.abort(); + controller.abort(); + fn(controller.signal); + `, + ` + const controller = new AbortController(); + controller.abort(); + console.log(controller); + fn(controller.signal); + `, + ` + const controller = new AbortController(); + // This comment should not be removed. + controller.abort(); + fn(controller.signal); + `, + ` + const controller = new AbortController(); + setImmediate(() => controller.abort()); + fn(controller.signal); + `, + ` + const controller = new AbortController(); + controller.abort('reason', 'extra'); + fn(controller.signal); + `, + ], + invalid: [ + { + code: ` + const controller = new AbortController(); + controller.abort(); + fn(controller.signal); + `, + errors: [{ message }], + output: ` + fn(AbortSignal.abort()); + `, + }, + { + code: ` + const abortController = new AbortController(); + abortController.abort(new Error('aborted')); + fn({ signal: abortController.signal }); + `, + errors: [{ message }], + output: ` + fn({ signal: AbortSignal.abort(new Error('aborted')) }); + `, + }, + { + code: ` + { + const ac = new AbortController(); + ac.abort(); + await wait({ signal: ac.signal }); + } + `, + errors: [{ message }], + output: ` + { + await wait({ signal: AbortSignal.abort() }); + } + `, + }, + { + code: ` + { + const controller = new AbortController(); + controller.abort(); + fn(controller.signal, controller.signal); + } + `, + errors: [{ message }], + output: ` + { + const controller = AbortSignal.abort(); + fn(controller, controller); + } + `, + }, + { + code: ` + { + const controller = new AbortController(); + controller.abort("reason"); + fn(controller.signal, controller.signal); + } + `, + errors: [{ message }], + output: ` + { + const controller = AbortSignal.abort("reason"); + fn(controller, controller); + } + `, + }, + ] +}); diff --git a/test/parallel/test-quic-writer-abort-signal.mjs b/test/parallel/test-quic-writer-abort-signal.mjs index 8d0dbb0351d931..a00e5904f670d1 100644 --- a/test/parallel/test-quic-writer-abort-signal.mjs +++ b/test/parallel/test-quic-writer-abort-signal.mjs @@ -33,12 +33,11 @@ const stream = await clientSession.createBidirectionalStream(); const w = stream.writer; // Create an already-aborted signal. -const ac = new AbortController(); -ac.abort(new Error('already aborted')); +const signal = AbortSignal.abort(new Error('already aborted')); // write() with an already-aborted signal should reject immediately. await rejects( - w.write(encoder.encode('data'), { signal: ac.signal }), + w.write(encoder.encode('data'), { signal }), { message: 'already aborted' }, ); diff --git a/tools/eslint-rules/prefer-abort-signal-abort.js b/tools/eslint-rules/prefer-abort-signal-abort.js new file mode 100644 index 00000000000000..e5b182ab73de95 --- /dev/null +++ b/tools/eslint-rules/prefer-abort-signal-abort.js @@ -0,0 +1,154 @@ +/** + * @file Prefer AbortSignal.abort() for already-aborted signals. + */ +'use strict'; + +const message = 'Use AbortSignal.abort() instead of creating and aborting an AbortController.'; + +function isAbortControllerConstruction(node) { + return node?.type === 'NewExpression' && + node.callee.type === 'Identifier' && + node.callee.name === 'AbortController' && + node.arguments.length === 0; +} + +function isIdentifier(node, name) { + return node?.type === 'Identifier' && node.name === name; +} + +function isProperty(node, name) { + return !node.computed && isIdentifier(node.property, name); +} + +function isAbortCallStatement(node, name) { + const expression = node?.expression; + const callee = expression?.callee; + return node?.type === 'ExpressionStatement' && + expression.type === 'CallExpression' && + callee.type === 'MemberExpression' && + isIdentifier(callee.object, name) && + isProperty(callee, 'abort') && + expression.arguments.length <= 1; +} + +function isSignalReference(reference, name) { + const { identifier } = reference; + const parent = identifier.parent; + return isIdentifier(identifier, name) && + parent?.type === 'MemberExpression' && + parent.object === identifier && + isProperty(parent, 'signal'); +} + +function isAbortReference(reference, abortStatement, name) { + const { identifier } = reference; + const parent = identifier.parent; + return isIdentifier(identifier, name) && + parent?.type === 'MemberExpression' && + parent.object === identifier && + isProperty(parent, 'abort') && + parent.parent === abortStatement.expression; +} + +module.exports = { + meta: { + fixable: 'code', + }, + + create(context) { + const sourceCode = context.sourceCode; + const candidates = []; + + function hasCommentsBetween(left, right) { + return sourceCode.getCommentsBefore(right) + .some((comment) => comment.range[0] > left.range[1]); + } + + function rangeIncludingTrailingLine(statement) { + const tokenAfter = sourceCode.getTokenAfter(statement, { includeComments: true }); + if (tokenAfter && tokenAfter.loc.start.line > statement.loc.end.line) { + return [statement.range[0], tokenAfter.range[0]]; + } + return statement.range; + } + + return { + VariableDeclarator(node) { + if (node.id.type !== 'Identifier' || + !isAbortControllerConstruction(node.init) || + node.parent.declarations.length !== 1) { + return; + } + + const variableDeclaration = node.parent; + const parent = variableDeclaration.parent; + if (parent.type !== 'BlockStatement' && parent.type !== 'Program') { + return; + } + + const index = parent.body.indexOf(variableDeclaration); + const abortStatement = parent.body[index + 1]; + if (!isAbortCallStatement(abortStatement, node.id.name) || + hasCommentsBetween(variableDeclaration, abortStatement)) { + return; + } + + candidates.push({ + abortStatement, + declarator: node, + variableDeclaration, + }); + }, + + 'Program:exit'() { + for (const { abortStatement, declarator, variableDeclaration } of candidates) { + const [variable] = sourceCode.scopeManager.getDeclaredVariables(declarator); + if (!variable) { + continue; + } + + const name = declarator.id.name; + const references = variable.references.filter((reference) => { + return reference.identifier !== declarator.id; + }); + const signalReferences = references.filter((reference) => { + return isSignalReference(reference, name); + }); + const abortReferences = references.filter((reference) => { + return isAbortReference(reference, abortStatement, name); + }); + + if (references.length !== (1 + signalReferences.length) || + abortReferences.length !== 1) { + continue; + } + + const signalNode = signalReferences[0].identifier.parent; + const abortArguments = abortStatement.expression.arguments; + const abortReason = abortArguments.length === 0 ? + '' : sourceCode.getText(abortArguments[0]); + + context.report({ + node: declarator, + message, + fix(fixer) { + const abortSignalCreationCall = `AbortSignal.abort(${abortReason})`; + if (signalReferences.length > 1) { + return [ + fixer.replaceText(declarator.init, abortSignalCreationCall), + fixer.removeRange(rangeIncludingTrailingLine(abortStatement)), + ...signalReferences.map(({ identifier: { parent } }) => fixer.replaceText(parent, name)), + ]; + } + return [ + fixer.removeRange(rangeIncludingTrailingLine(variableDeclaration)), + fixer.removeRange(rangeIncludingTrailingLine(abortStatement)), + fixer.replaceText(signalNode, abortSignalCreationCall), + ]; + }, + }); + } + }, + }; + }, +}; From 5c78025549cddce8b4a61f6b2d058934b571fea8 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 28 May 2026 17:44:18 +0200 Subject: [PATCH 3/5] doc: update `git node land` instructions for security releases Signed-off-by: Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/63586 Refs: https://github.com/nodejs/node-core-utils/pull/1043 Reviewed-By: Marco Ippolito Reviewed-By: Luigi Pinca Reviewed-By: Rafael Gonzaga Reviewed-By: Colin Ihrig Reviewed-By: Filip Skokan --- doc/contributing/releases.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/doc/contributing/releases.md b/doc/contributing/releases.md index 5299b0026298e3..e2eba8880b1db9 100644 --- a/doc/contributing/releases.md +++ b/doc/contributing/releases.md @@ -272,11 +272,12 @@ $ git reset --hard upstream/vN.x The list of patches to include should be listed in the "Next Security Release" issue in `nodejs-private`. Ask the security release steward if you're unsure. -The `git node land` tool does not work with the `nodejs-private` -organization. To land a PR in Node.js private, use `git cherry-pick` to apply -each commit from the PR. You will also need to manually apply the PR -metadata (`PR-URL`, `Reviewed-by`, etc.) by amending the commit messages. If +To use the `git node land` tool to land Pull Requests in the `nodejs-private` +organization, you need to specify the full URL to the Pull Request and make sure +you provide a GitHub token with read permission to the private repository. If known, additionally include `CVE-ID: CVE-XXXX-XXXXX` in the commit metadata. +Make sure to sign and push to resulting commit to the private repository and not +the public one. **Note**: Do not run CI on the PRs in `nodejs-private` until CI is locked down. You can integrate the PRs into the proposal without running full CI. From 54f7e892d5eafa96552e9dd1b1ff1830162781cf Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 28 May 2026 15:15:16 -0400 Subject: [PATCH 4/5] build: def `NODE_USE_NODE_CODE_CACHE` only used in node_mksnapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Chengzhong Wu PR-URL: https://github.com/nodejs/node/pull/63588 Reviewed-By: Juan José Arboleda Reviewed-By: Luigi Pinca Reviewed-By: Joyee Cheung --- node.gyp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/node.gyp b/node.gyp index 9c6d44d90bbba1..28db2a955b74d6 100644 --- a/node.gyp +++ b/node.gyp @@ -1033,11 +1033,6 @@ '@rpath/lib<(node_core_target_name).<(shlib_suffix)' }, }], - [ 'node_use_node_code_cache=="true"', { - 'defines': [ - 'NODE_USE_NODE_CODE_CACHE=1', - ], - }], ['node_shared=="true" and OS in "aix os400"', { 'product_name': 'node_base', }], From e1ae3a56f9ca252df6e559cdee9e5d39bc61d536 Mon Sep 17 00:00:00 2001 From: Tim Perry <1526883+pimterry@users.noreply.github.com> Date: Thu, 28 May 2026 22:02:05 +0200 Subject: [PATCH 5/5] quic: add proper error codes & messages for QUIC failures Signed-off-by: Tim Perry PR-URL: https://github.com/nodejs/node/pull/63198 Reviewed-By: Ethan Arrowood Reviewed-By: Stephen Belanger --- lib/eslint.config_partial.mjs | 2 +- lib/internal/errors.js | 2 - lib/internal/quic/quic.js | 85 ++++++++++++++----- src/quic/bindingdata.cc | 8 ++ src/quic/bindingdata.h | 5 ++ src/quic/data.cc | 68 ++++++++++++++- src/quic/data.h | 3 + src/quic/session.cc | 11 ++- test/parallel/test-quic-alpn-mismatch.mjs | 10 +-- .../test-quic-session-close-error-code.mjs | 16 +++- .../test-quic-stream-destroy-emits-reset.mjs | 10 +-- .../test-quic-stream-destroy-options-code.mjs | 5 +- ...est-quic-stream-writer-fail-error-code.mjs | 19 +---- 13 files changed, 186 insertions(+), 58 deletions(-) diff --git a/lib/eslint.config_partial.mjs b/lib/eslint.config_partial.mjs index cdc2cb0dcbf8db..005cd0a410c2f6 100644 --- a/lib/eslint.config_partial.mjs +++ b/lib/eslint.config_partial.mjs @@ -23,7 +23,7 @@ const noRestrictedSyntax = [ message: "`btoa` supports only latin-1 charset, use Buffer.from(str).toString('base64') instead", }, { - selector: 'NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError|AbortError|NodeAggregateError|QuotaExceededError)$/])', + selector: 'NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError|AbortError|NodeAggregateError|QuicError|QuotaExceededError)$/])', message: "Use an error exported by 'internal/errors' instead.", }, { diff --git a/lib/internal/errors.js b/lib/internal/errors.js index f989bc8fe60a6e..ba632359fbc185 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1689,14 +1689,12 @@ E('ERR_PERFORMANCE_INVALID_TIMESTAMP', E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError); E('ERR_PROXY_INVALID_CONFIG', '%s', Error); E('ERR_PROXY_TUNNEL', '%s', Error); -E('ERR_QUIC_APPLICATION_ERROR', 'A QUIC application error occurred. %d [%s]', Error); E('ERR_QUIC_CONNECTION_FAILED', 'QUIC connection failed', Error); E('ERR_QUIC_ENDPOINT_CLOSED', 'QUIC endpoint closed: %s (%d)', Error); E('ERR_QUIC_OPEN_STREAM_FAILED', 'Failed to open QUIC stream', Error); E('ERR_QUIC_STREAM_ABORTED', '%s', Error); E('ERR_QUIC_STREAM_RESET', 'The QUIC stream was reset by the peer with error code %d', Error); -E('ERR_QUIC_TRANSPORT_ERROR', 'A QUIC transport error occurred. %d [%s]', Error); E('ERR_QUIC_VERSION_NEGOTIATION_ERROR', 'The QUIC session requires version negotiation', Error); E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parentFilename) { let message = 'require() cannot be used on an ESM ' + diff --git a/lib/internal/quic/quic.js b/lib/internal/quic/quic.js index fe9e223bdce4d5..d237adccd448cc 100644 --- a/lib/internal/quic/quic.js +++ b/lib/internal/quic/quic.js @@ -9,6 +9,7 @@ const { ArrayPrototypePush, BigInt, DataViewPrototypeGetByteLength, + ErrorCaptureStackTrace, FunctionPrototypeBind, Number, ObjectDefineProperties, @@ -108,13 +109,11 @@ const { ERR_INVALID_THIS, ERR_MISSING_ARGS, ERR_OUT_OF_RANGE, - ERR_QUIC_APPLICATION_ERROR, ERR_QUIC_CONNECTION_FAILED, ERR_QUIC_ENDPOINT_CLOSED, ERR_QUIC_OPEN_STREAM_FAILED, ERR_QUIC_STREAM_ABORTED, ERR_QUIC_STREAM_RESET, - ERR_QUIC_TRANSPORT_ERROR, ERR_QUIC_VERSION_NEGOTIATION_ERROR, }, } = require('internal/errors'); @@ -738,10 +737,12 @@ setCallbacks({ * @param {number} errorType * @param {number} code * @param {string} [reason] + * @param {string} [errorName] Decoded TLS alert name when `code` is a + * CRYPTO_ERROR; otherwise undefined. */ - onSessionClose(errorType, code, reason) { - debug('session close callback', errorType, code, reason); - this[kOwner][kFinishClose](errorType, code, reason); + onSessionClose(errorType, code, reason, errorName) { + debug('session close callback', errorType, code, reason, errorName); + this[kOwner][kFinishClose](errorType, code, reason, errorName); }, /** @@ -931,8 +932,12 @@ setCallbacks({ // was an abnormal termination even if the session closed cleanly. const resetCode = getQuicStreamState(this[kOwner]).resetCode; if (resetCode !== undefined && resetCode > 0n) { - error = new ERR_QUIC_APPLICATION_ERROR( - resetCode, `stream reset with code ${resetCode}`); + error = makeQuicError( + 'ERR_QUIC_APPLICATION_ERROR', + 'QUIC application error', + 'application', + resetCode, + `stream reset with code ${resetCode}`); } } debug(`stream ${this[kOwner].id} closed callback with error: ${error}`); @@ -1054,21 +1059,50 @@ class QuicError extends Error { } } -// Converts a raw QuicError array [type, code, reason] from C++ into a -// proper Node.js Error object. +// Build the human-readable message for an ERR_QUIC_TRANSPORT_ERROR or +// ERR_QUIC_APPLICATION_ERROR. `errorName` is the symbolic name for +// the wire code when known: either the OpenSSL-decoded TLS alert +// (CRYPTO_ERROR; 0x100..0x1ff) or one of the named transport codes +// from RFC 9000 (e.g. PROTOCOL_VIOLATION). Otherwise undefined. +// `reason` is the peer-supplied UTF-8 reason string from the +// CONNECTION_CLOSE / RESET_STREAM frame, often empty. +function quicErrorMessage(prefix, errorCode, reason, errorName) { + let msg = `${prefix} `; + msg += errorName ? `${errorName} (${errorCode})` : `${errorCode}`; + if (reason) msg += `: ${reason}`; + return msg; +} + +function makeQuicError(code, prefix, type, errorCode, reason, errorName) { + const err = new QuicError( + quicErrorMessage(prefix, errorCode, reason, errorName), + { errorCode, code, type }); + ErrorCaptureStackTrace(err, makeQuicError); + if (reason) err.reason = reason; + if (errorName) err.errorName = errorName; + return err; +} + function convertQuicError(error) { const type = error[0]; const code = error[1]; const reason = error[2]; + const errorName = error[3]; switch (type) { case 'transport': - return new ERR_QUIC_TRANSPORT_ERROR(code, reason); + return makeQuicError('ERR_QUIC_TRANSPORT_ERROR', + 'QUIC transport error', + 'transport', code, reason, errorName); case 'application': - return new ERR_QUIC_APPLICATION_ERROR(code, reason); + return makeQuicError('ERR_QUIC_APPLICATION_ERROR', + 'QUIC application error', + 'application', code, reason, errorName); case 'version_negotiation': return new ERR_QUIC_VERSION_NEGOTIATION_ERROR(); default: - return new ERR_QUIC_TRANSPORT_ERROR(code, reason); + return makeQuicError('ERR_QUIC_TRANSPORT_ERROR', + 'QUIC transport error', + 'transport', code, reason, errorName); } } @@ -3575,7 +3609,7 @@ class QuicSession { * @param {number} code * @param {string} [reason] */ - [kFinishClose](errorType, code, reason) { + [kFinishClose](errorType, code, reason, errorName) { // If code is zero, then we closed without an error. Yay! We can destroy // safely without specifying an error. if (code === 0n) { @@ -3584,7 +3618,8 @@ class QuicSession { return; } - debug('finishing closing the session with an error', errorType, code, reason); + debug('finishing closing the session with an error', + errorType, code, reason, errorName); // If the local side initiated this close with an error code (via // close({ code })), this is an intentional shutdown; not an error. @@ -3611,10 +3646,14 @@ class QuicSession { // session would leak with `closed` hanging forever. switch (errorType) { case 0: /* Transport Error */ - this.destroy(new ERR_QUIC_TRANSPORT_ERROR(code, reason)); + this.destroy(makeQuicError('ERR_QUIC_TRANSPORT_ERROR', + 'QUIC transport error', + 'transport', code, reason, errorName)); break; case 1: /* Application Error */ - this.destroy(new ERR_QUIC_APPLICATION_ERROR(code, reason)); + this.destroy(makeQuicError('ERR_QUIC_APPLICATION_ERROR', + 'QUIC application error', + 'application', code, reason, errorName)); break; case 2: /* Version Negotiation Error */ this.destroy(new ERR_QUIC_VERSION_NEGOTIATION_ERROR()); @@ -3623,7 +3662,9 @@ class QuicSession { this.destroy(); break; default: - this.destroy(new ERR_QUIC_TRANSPORT_ERROR(code, reason)); + this.destroy(makeQuicError('ERR_QUIC_TRANSPORT_ERROR', + 'QUIC transport error', + 'transport', code, reason, errorName)); break; } } @@ -3874,9 +3915,13 @@ class QuicSession { // decide. In 'strict' mode, the handshake already failed at the C++ // level (SSL_VERIFY_PEER) so we won't reach here. if (inner.verifyPeer === 'auto' && validationErrorReason !== undefined) { - const err = new ERR_QUIC_TRANSPORT_ERROR( - 0, `Peer certificate validation failed: ${validationErrorReason}` + - ` [${validationErrorCode}]`); + const err = makeQuicError( + 'ERR_QUIC_TRANSPORT_ERROR', + 'QUIC transport error', + 'transport', + 0n, + `Peer certificate validation failed: ${validationErrorReason}` + + ` [${validationErrorCode}]`); inner.pendingOpen.reject?.(err); inner.pendingOpen.resolve = undefined; inner.pendingOpen.reject = undefined; diff --git a/src/quic/bindingdata.cc b/src/quic/bindingdata.cc index f029f0e8eb83da..31467a8477a792 100644 --- a/src/quic/bindingdata.cc +++ b/src/quic/bindingdata.cc @@ -435,6 +435,14 @@ QUIC_JS_CALLBACKS(V) #undef V +Local BindingData::error_name_string(const char* name) { + auto& slot = error_name_strings_[name]; + if (slot.IsEmpty()) { + slot.Set(env()->isolate(), OneByteString(env()->isolate(), name)); + } + return slot.Get(env()->isolate()); +} + JS_METHOD_IMPL(BindingData::SetCallbacks) { auto env = Environment::GetCurrent(args); auto isolate = env->isolate(); diff --git a/src/quic/bindingdata.h b/src/quic/bindingdata.h index 978dad44467585..2116a35c158ea6 100644 --- a/src/quic/bindingdata.h +++ b/src/quic/bindingdata.h @@ -305,6 +305,8 @@ class BindingData final std::unordered_map> listening_endpoints; + v8::Local error_name_string(const char* name); + size_t current_ngtcp2_memory_ = 0; // The following set up various storage and accessors for common strings, @@ -357,6 +359,9 @@ class BindingData final QUIC_JS_CALLBACKS(V) #undef V + // Lazy cache backing error_name_string() + std::unordered_map> error_name_strings_; + std::unique_ptr session_manager_; // Type-erased arena storage. The concrete AliasedStructArena types diff --git a/src/quic/data.cc b/src/quic/data.cc index 4c515d8931291e..9599adec62f805 100644 --- a/src/quic/data.cc +++ b/src/quic/data.cc @@ -1,13 +1,15 @@ #if HAVE_OPENSSL && HAVE_QUIC #include "guard.h" #ifndef OPENSSL_NO_QUIC -#include "data.h" #include #include #include #include +#include #include #include +#include "bindingdata.h" +#include "data.h" #include "defs.h" #include "util.h" @@ -363,6 +365,62 @@ std::optional QuicError::get_crypto_error() const { return code() & ~NGTCP2_CRYPTO_ERROR; } +const char* QuicError::name() const { + // CRYPTO_ERROR carries a TLS alert in its low byte (RFC 9001 sec. 4.8). + // OpenSSL's SSL_alert_desc_string_long owns a stable string for every + // alert it knows about; we filter out the "unknown" placeholder so the + // JS side can present `errorName` as undefined for unrecognised alerts. + if (auto alert = get_crypto_error()) { + const char* n = SSL_alert_desc_string_long(*alert); + if (n != nullptr && std::string_view(n) != "unknown") return n; + return nullptr; + } + // Named transport-layer error codes from RFC 9000 sec. 20.1 (and the + // RFC 9368 version-negotiation extension). Application error codes are + // opaque to QUIC, so we only decode for transport. + if (type() != Type::TRANSPORT) return nullptr; + switch (code()) { + case NGTCP2_NO_ERROR: + return "NO_ERROR"; + case NGTCP2_INTERNAL_ERROR: + return "INTERNAL_ERROR"; + case NGTCP2_CONNECTION_REFUSED: + return "CONNECTION_REFUSED"; + case NGTCP2_FLOW_CONTROL_ERROR: + return "FLOW_CONTROL_ERROR"; + case NGTCP2_STREAM_LIMIT_ERROR: + return "STREAM_LIMIT_ERROR"; + case NGTCP2_STREAM_STATE_ERROR: + return "STREAM_STATE_ERROR"; + case NGTCP2_FINAL_SIZE_ERROR: + return "FINAL_SIZE_ERROR"; + case NGTCP2_FRAME_ENCODING_ERROR: + return "FRAME_ENCODING_ERROR"; + case NGTCP2_TRANSPORT_PARAMETER_ERROR: + return "TRANSPORT_PARAMETER_ERROR"; + case NGTCP2_CONNECTION_ID_LIMIT_ERROR: + return "CONNECTION_ID_LIMIT_ERROR"; + case NGTCP2_PROTOCOL_VIOLATION: + return "PROTOCOL_VIOLATION"; + case NGTCP2_INVALID_TOKEN: + return "INVALID_TOKEN"; + case NGTCP2_APPLICATION_ERROR: + return "APPLICATION_ERROR"; + case NGTCP2_CRYPTO_BUFFER_EXCEEDED: + return "CRYPTO_BUFFER_EXCEEDED"; + case NGTCP2_KEY_UPDATE_ERROR: + return "KEY_UPDATE_ERROR"; + case NGTCP2_AEAD_LIMIT_REACHED: + return "AEAD_LIMIT_REACHED"; + case NGTCP2_NO_VIABLE_PATH: + return "NO_VIABLE_PATH"; + case NGTCP2_VERSION_NEGOTIATION_ERROR: + return "VERSION_NEGOTIATION_ERROR"; + default: + return nullptr; + } +} + MaybeLocal QuicError::ToV8Value(Environment* env) const { if ((type() == Type::TRANSPORT && code() == NGTCP2_NO_ERROR) || (type() == Type::APPLICATION && @@ -384,6 +442,7 @@ MaybeLocal QuicError::ToV8Value(Environment* env) const { type_str, BigInt::NewFromUnsigned(env->isolate(), code()), Undefined(env->isolate()), + Undefined(env->isolate()), }; // Note that per the QUIC specification, the reason, if present, is @@ -397,6 +456,13 @@ MaybeLocal QuicError::ToV8Value(Environment* env) const { return {}; } + // Attach a human-readable name for known wire codes (RFC 9000 sec. 20.1 + // names and OpenSSL TLS alert descriptions for CRYPTO_ERROR). Unknown + // codes leave the slot as undefined. + if (const char* n = name()) { + argv[3] = BindingData::Get(env).error_name_string(n); + } + return Array::New(env->isolate(), argv, arraysize(argv)).As(); } diff --git a/src/quic/data.h b/src/quic/data.h index ec8d40cbc4c7a0..9c92a30c1ddf4c 100644 --- a/src/quic/data.h +++ b/src/quic/data.h @@ -265,6 +265,9 @@ class QuicError final : public MemoryRetainer { bool is_crypto_error() const; std::optional get_crypto_error() const; + // Returns a human-readable name for this error if known, or nullptr + const char* name() const; + // Note that since application errors are application-specific and we // don't know which application is being used here, it is possible that // the comparing two different QuicError instances from different applications diff --git a/src/quic/session.cc b/src/quic/session.cc index 04a69d32cd9a09..bb94b6a7400766 100644 --- a/src/quic/session.cc +++ b/src/quic/session.cc @@ -3065,7 +3065,7 @@ void Session::CheckStreamIdleTimeout(uint64_t now) { // Without this, the peer's stream sits orphaned until the // session closes. auto error = - QuicError::ForTransport(NGTCP2_ERR_PROTO, "stream idle timeout"); + QuicError::ForNgtcp2Error(NGTCP2_ERR_PROTO, "stream idle timeout"); ShutdownStream(id, error); stream->Destroy(error); STAT_INCREMENT(Stats, streams_idle_timed_out); @@ -3451,12 +3451,21 @@ void Session::EmitClose(const QuicError& error) { Integer::New(env()->isolate(), static_cast(error.type())), BigInt::NewFromUnsigned(env()->isolate(), error.code()), Undefined(env()->isolate()), + Undefined(env()->isolate()), }; if (error.reason().length() > 0 && !ToV8Value(env()->context(), error.reason()).ToLocal(&argv[2])) { return; } + // Attach a human-readable name for known wire codes (RFC 9000 sec. 20.1 + // names and OpenSSL TLS alert descriptions for CRYPTO_ERROR). Unknown + // codes leave the slot as undefined. See QuicError::name() for the + // matching path on stream-level errors. + if (const char* n = error.name()) { + argv[3] = BindingData::Get(env()).error_name_string(n); + } + MakeCallback( BindingData::Get(env()).session_close_callback(), arraysize(argv), argv); diff --git a/test/parallel/test-quic-alpn-mismatch.mjs b/test/parallel/test-quic-alpn-mismatch.mjs index 14c9dfe39bf2f3..64d8e1f0006380 100644 --- a/test/parallel/test-quic-alpn-mismatch.mjs +++ b/test/parallel/test-quic-alpn-mismatch.mjs @@ -6,9 +6,7 @@ // // The QUIC transport error code for a CRYPTO_ERROR carrying a TLS alert is // 0x100 | . For `no_application_protocol` (alert 120 / 0x78) this -// is 0x178 == 376. ERR_QUIC_TRANSPORT_ERROR formats the wire code into its -// message as a bigint, so we match `376n` to assert the specific alert was -// sent rather than some other handshake failure. +// is 0x178 == 376. import { hasQuic, skip, mustCall } from '../common/index.mjs'; import assert from 'node:assert'; @@ -23,12 +21,14 @@ const { listen, connect } = await import('../common/quic.mjs'); const expected = { code: 'ERR_QUIC_TRANSPORT_ERROR', - message: /\b376n\b/, + errorCode: 376n, + message: /no application protocol/ }; const onerror = mustCall((err) => { strictEqual(err.code, 'ERR_QUIC_TRANSPORT_ERROR'); - match(err.message, /\b376n\b/); + strictEqual(err.errorCode, 376n); + match(err.message, /no application protocol/); }, 2); const transportParams = { maxIdleTimeout: 1 }; diff --git a/test/parallel/test-quic-session-close-error-code.mjs b/test/parallel/test-quic-session-close-error-code.mjs index e907cd672c19d4..5d0392bf102585 100644 --- a/test/parallel/test-quic-session-close-error-code.mjs +++ b/test/parallel/test-quic-session-close-error-code.mjs @@ -29,13 +29,18 @@ const { listen, connect } = await import('../common/quic.mjs'); const serverEndpoint = await listen(mustCall(async (serverSession) => { serverSession.onerror = mustCall((err) => { strictEqual(err.code, 'ERR_QUIC_APPLICATION_ERROR'); - strictEqual(err.message.includes('42n'), true, + strictEqual(err.message.includes('42'), true, 'error message should contain the code'); strictEqual(err.message.includes('client shutdown'), true, 'error message should contain the reason'); + strictEqual(err.errorCode, 42n); + strictEqual(err.type, 'application'); + strictEqual(err.reason, 'client shutdown'); }); await rejects(serverSession.closed, { code: 'ERR_QUIC_APPLICATION_ERROR', + errorCode: 42n, + reason: 'client shutdown', }); serverGot.resolve(); })); @@ -71,8 +76,10 @@ const { listen, connect } = await import('../common/quic.mjs'); const serverEndpoint = await listen(mustCall(async (serverSession) => { serverSession.onerror = mustCall((err) => { strictEqual(err.code, 'ERR_QUIC_TRANSPORT_ERROR'); - strictEqual(err.message.includes('1n'), true, + strictEqual(err.message.includes('1'), true, 'error message should contain the code'); + strictEqual(err.errorCode, 1n); + strictEqual(err.type, 'transport'); }); await rejects(serverSession.closed, { code: 'ERR_QUIC_TRANSPORT_ERROR', @@ -102,7 +109,10 @@ const { listen, connect } = await import('../common/quic.mjs'); const serverEndpoint = await listen(mustCall(async (serverSession) => { serverSession.onerror = mustCall((err) => { strictEqual(err.code, 'ERR_QUIC_APPLICATION_ERROR'); - strictEqual(err.message.includes('99n'), true); + strictEqual(err.message.includes('99'), true); + strictEqual(err.errorCode, 99n); + strictEqual(err.type, 'application'); + strictEqual(err.reason, 'destroy with code'); }); await rejects(serverSession.closed, { code: 'ERR_QUIC_APPLICATION_ERROR', diff --git a/test/parallel/test-quic-stream-destroy-emits-reset.mjs b/test/parallel/test-quic-stream-destroy-emits-reset.mjs index e8289e13ec84b7..e565f5e9ff7b8f 100644 --- a/test/parallel/test-quic-stream-destroy-emits-reset.mjs +++ b/test/parallel/test-quic-stream-destroy-emits-reset.mjs @@ -12,12 +12,12 @@ // code is the negotiated application's "internal error" code: for // the test fixture's non-h3 ALPN (`quic-test`) the C++ // DefaultApplication reports `1n`, which propagates to the server -// as `ERR_QUIC_APPLICATION_ERROR` carrying `1n` in its message. +// as `ERR_QUIC_APPLICATION_ERROR` exposing `errorCode === 1n`. import { hasQuic, skip, mustCall } from '../common/index.mjs'; import assert from 'node:assert'; -const { strictEqual, ok, rejects } = assert; +const { strictEqual, rejects } = assert; if (!hasQuic) { skip('QUIC is not enabled'); @@ -31,10 +31,8 @@ const serverEndpoint = await listen(mustCall((serverSession) => { serverSession.onstream = mustCall(async (stream) => { stream.onreset = mustCall((err) => { strictEqual(err.code, 'ERR_QUIC_APPLICATION_ERROR'); - // The DefaultApplication's internal error code is 0x1n, which - // util.format renders as `1n` (BigInt) in the message text. - ok(err.message.includes('1n'), - `expected '1n' in message, got: ${err.message}`); + // The DefaultApplication's internal error code is 0x1n. + strictEqual(err.errorCode, 1n); serverResetSeen.resolve(); }); diff --git a/test/parallel/test-quic-stream-destroy-options-code.mjs b/test/parallel/test-quic-stream-destroy-options-code.mjs index 6caf9ef794bd3b..b1fd70018d456b 100644 --- a/test/parallel/test-quic-stream-destroy-options-code.mjs +++ b/test/parallel/test-quic-stream-destroy-options-code.mjs @@ -13,7 +13,7 @@ import { hasQuic, skip, mustCall } from '../common/index.mjs'; import assert from 'node:assert'; -const { strictEqual, ok, rejects } = assert; +const { strictEqual, rejects } = assert; if (!hasQuic) { skip('QUIC is not enabled'); @@ -27,8 +27,7 @@ const serverEndpoint = await listen(mustCall((serverSession) => { serverSession.onstream = mustCall(async (stream) => { stream.onreset = mustCall((err) => { strictEqual(err.code, 'ERR_QUIC_APPLICATION_ERROR'); - ok(err.message.includes('66n'), - `expected '66n' in message, got: ${err.message}`); + strictEqual(err.errorCode, 66n); serverResetSeen.resolve(); }); diff --git a/test/parallel/test-quic-stream-writer-fail-error-code.mjs b/test/parallel/test-quic-stream-writer-fail-error-code.mjs index 4bf2c2365e9cfe..ff3c18f69c5d03 100644 --- a/test/parallel/test-quic-stream-writer-fail-error-code.mjs +++ b/test/parallel/test-quic-stream-writer-fail-error-code.mjs @@ -17,11 +17,8 @@ // the QuicError fast path. // // The peer-side observation goes through `stream.onreset(err)` where -// `err` is `ERR_QUIC_APPLICATION_ERROR` carrying the wire code in its -// message string. We extract the code via regex; once -// `ERR_QUIC_APPLICATION_ERROR` exposes the numeric code as a property -// (a planned follow-up), this test can switch to direct property -// access. +// `err` is `ERR_QUIC_APPLICATION_ERROR` exposing the wire code on +// `err.errorCode` (a BigInt). import { hasQuic, skip, mustCall } from '../common/index.mjs'; import assert from 'node:assert'; @@ -35,19 +32,9 @@ if (!hasQuic) { const { listen, connect } = await import('../common/quic.mjs'); const { QuicError } = await import('node:quic'); -// Extract the numeric wire code from an ERR_QUIC_APPLICATION_ERROR -// message of the form -// "A QUIC application error occurred. n []" -// where the trailing `n` on the code is the BigInt formatting from -// `util.format('%d', bigint)`. RESET_STREAM frames do not carry a -// reason string, so the bracketed value is typically `undefined`. function wireCodeOf(err) { strictEqual(err.code, 'ERR_QUIC_APPLICATION_ERROR'); - const match = err.message.match(/A QUIC application error occurred\. (\d+)n /); - if (!match) { - throw new Error(`Could not extract code from message: ${err.message}`); - } - return BigInt(match[1]); + return err.errorCode; } // Server: capture the next two streams. Each stream receives an