Apply suggestions from code review

Co-authored-by: larabr <larabr+github@protonmail.com>
This commit is contained in:
Daniel Huigens 2025-05-23 13:19:43 +02:00
parent 62c4d9907b
commit 6df8687c9e
No known key found for this signature in database
GPG Key ID: CB064A128FA90686
3 changed files with 23 additions and 27 deletions

View File

@ -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) ||

View File

@ -46,13 +46,15 @@ class PacketList extends Array {
* @param {Uint8Array | ReadableStream<Uint8Array>} 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;

View File

@ -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;
}
}