From 6df8687c9ee7da37c2c3fe39328b7eb422329263 Mon Sep 17 00:00:00 2001 From: Daniel Huigens Date: Fri, 23 May 2025 13:19:43 +0200 Subject: [PATCH] Apply suggestions from code review Co-authored-by: larabr --- src/packet/grammar.ts | 4 +- src/packet/packetlist.js | 41 ++++++++----------- .../sym_encrypted_integrity_protected_data.js | 5 ++- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/packet/grammar.ts b/src/packet/grammar.ts index e2991273..ae1e75f1 100644 --- a/src/packet/grammar.ts +++ b/src/packet/grammar.ts @@ -52,12 +52,12 @@ const isValidSignedMessage = (tagList: enums.packet[], acceptPartial: boolean) = /** * Implements grammar checks based on https://www.rfc-editor.org/rfc/rfc9580.html#section-10.3 . - * @param packetList - list of packet tags to validate + * @param packetList - list of packet tags to validate; marker/padding/unknown packet tags are expected to have been already filtered out. * @param acceptPartial - whether the list of tags corresponds to a partially-parsed message * @returns whether the list of tags is valid */ const isValidOpenPGPMessage = ( - packetList: number[], + packetList: enums.packet[], acceptPartial: boolean ): boolean => { return isValidLiteralMessage(packetList) || diff --git a/src/packet/packetlist.js b/src/packet/packetlist.js index e291c878..e48677b9 100644 --- a/src/packet/packetlist.js +++ b/src/packet/packetlist.js @@ -46,13 +46,15 @@ class PacketList extends Array { * @param {Uint8Array | ReadableStream} bytes - binary data to parse * @param {Object} allowedPackets - mapping where keys are allowed packet tags, pointing to their Packet class * @param {Object} [config] - full configuration, defaults to openpgp.config + * @param {function(enums.packet[], boolean, Object): void} [grammarValidator] + * @param {Boolean} [delayErrors] - delay errors until the input stream has been read completely * @returns {PacketList} parsed list of packets * @throws on parsing errors * @async */ - static async fromBinary(bytes, allowedPackets, config = defaultConfig, grammarValidator = null) { + static async fromBinary(bytes, allowedPackets, config = defaultConfig, grammarValidator = null, delayErrors = false) { const packets = new PacketList(); - await packets.read(bytes, allowedPackets, config, grammarValidator); + await packets.read(bytes, allowedPackets, config, grammarValidator, delayErrors); return packets; } @@ -62,10 +64,11 @@ class PacketList extends Array { * @param {Object} allowedPackets - mapping where keys are allowed packet tags, pointing to their Packet class * @param {Object} [config] - full configuration, defaults to openpgp.config * @param {function(enums.packet[], boolean, Object): void} [grammarValidator] + * @param {Boolean} [delayErrors] - delay errors until the input stream has been read completely * @throws on parsing errors * @async */ - async read(bytes, allowedPackets, config = defaultConfig, grammarValidator = null) { + async read(bytes, allowedPackets, config = defaultConfig, grammarValidator = null, delayErrors = false) { if (config.additionalAllowedPackets.length) { allowedPackets = { ...allowedPackets, ...util.constructAllowedPackets(config.additionalAllowedPackets) }; } @@ -77,7 +80,7 @@ class PacketList extends Array { let useStreamType = util.isStream(readable); while (true) { await writer.ready; - let error; + let unauthenticatedError; let wasStream; await readPacket(reader, useStreamType, async parsed => { try { @@ -126,7 +129,7 @@ class PacketList extends Array { // Those are also the ones we want to be more strict about and throw on all errors // (since we likely cannot process the message without these packets anyway). const throwDataPacketError = supportsStreaming(parsed.tag); - // Throw all other errors, including `GrammarError`s and unexpected errors. + // Throw all other errors, including `GrammarError`s, disallowed packet errors, and unexpected errors. const throwOtherError = !( e instanceof UnknownPacketError || e instanceof UnsupportedError || @@ -139,8 +142,8 @@ class PacketList extends Array { throwDataPacketError || throwOtherError ) { - if (bytes.unauthenticated) { - error = e; + if (delayErrors) { + unauthenticatedError = e; } else { await writer.abort(e); } @@ -159,26 +162,18 @@ class PacketList extends Array { // If there was a parse error, read the entire input first // in case there's an MDC error, which should take precedence. - if (error) { + if (unauthenticatedError) { await reader.readToEnd(); // eslint-disable-next-line @typescript-eslint/no-throw-literal - throw error; + throw unauthenticatedError; } // We peek to check whether this was the last packet. - // - // If this was not a packet that supports streaming, we peek 2 - // bytes instead of 1 because `readPacket` also peeks 2 bytes, - // and we want to cut a `subarray` of the correct length into - // `web-stream-tools`' `externalBuffer` as a tiny optimization - // here. - // - // If it *was* a streaming packet (i.e. the data packets), we - // peek at the entire remainder of the stream, in order to - // forward errors in the remainder of the stream to the packet - // stream. This throws MDC errors, for example, so that they - // take precedence over parse errors. - const nextPacket = await reader.peekBytes(wasStream ? Infinity : 2); + // We peek 2 bytes instead of 1 because `readPacket` also + // peeks 2 bytes, and we want to cut a `subarray` of the + // correct length into `web-stream-tools`' `externalBuffer` + // as a tiny optimization here. + const nextPacket = await reader.peekBytes(2); const done = !nextPacket || !nextPacket.length; if (done) { // Here we are past the MDC check for SEIPDv1 data, hence @@ -205,7 +200,7 @@ class PacketList extends Array { break; } if (supportsStreaming(value.constructor.tag)) { - if (!bytes.unauthenticated) { + if (!delayErrors) { grammarValidator?.(tagsRead, true, config); } break; diff --git a/src/packet/sym_encrypted_integrity_protected_data.js b/src/packet/sym_encrypted_integrity_protected_data.js index 9cc22bcf..de04511e 100644 --- a/src/packet/sym_encrypted_integrity_protected_data.js +++ b/src/packet/sym_encrypted_integrity_protected_data.js @@ -184,6 +184,7 @@ class SymEncryptedIntegrityProtectedDataPacket { if (isArrayStream(encrypted)) encrypted = await streamReadToEnd(encrypted); let packetbytes; + let delayErrors = false; if (this.version === 2) { if (this.cipherAlgorithm !== sessionKeyAlgorithm) { // sanity check @@ -213,7 +214,7 @@ class SymEncryptedIntegrityProtectedDataPacket { packetbytes = streamSlice(bytes, 0, -2); // Remove MDC packet packetbytes = streamConcat([packetbytes, streamFromAsync(() => verifyHash)]); if (util.isStream(encrypted) && config.allowUnauthenticatedStream) { - packetbytes.unauthenticated = true; + delayErrors = true; } else { packetbytes = await streamReadToEnd(packetbytes); } @@ -223,7 +224,7 @@ class SymEncryptedIntegrityProtectedDataPacket { // MUST yield a valid OpenPGP Message. // - Decrypting a version 2 Symmetrically Encrypted and Integrity Protected Data packet // MUST yield a valid Optionally Padded Message. - this.packets = await PacketList.fromBinary(packetbytes, allowedPackets, config, getMessageGrammarValidator()); + this.packets = await PacketList.fromBinary(packetbytes, allowedPackets, config, getMessageGrammarValidator(), delayErrors); return true; } }