From b501eba9b714cc6353173416eaa77ac22ec402f5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 19 Dec 2022 03:08:27 +0000 Subject: [PATCH 1/3] More robust envelope parser --- packages/utils/src/envelope.ts | 47 ++++++++++++++++++++ packages/utils/test/clientreport.test.ts | 10 +++-- packages/utils/test/envelope.test.ts | 32 +++++++++----- packages/utils/test/testutils.ts | 56 ------------------------ 4 files changed, 73 insertions(+), 72 deletions(-) diff --git a/packages/utils/src/envelope.ts b/packages/utils/src/envelope.ts index 92037d18ddfe..2fecdd5f6af5 100644 --- a/packages/utils/src/envelope.ts +++ b/packages/utils/src/envelope.ts @@ -1,6 +1,8 @@ import { Attachment, AttachmentItem, + BaseEnvelopeHeaders, + BaseEnvelopeItemHeaders, DataCategory, DsnComponents, Envelope, @@ -110,6 +112,51 @@ function concatBuffers(buffers: Uint8Array[]): Uint8Array { return merged; } +interface TextDecoderInternal { + decode(input?: Uint8Array): string; +} + +/** + * Parses an envelope + */ +export function parseEnvelope( + env: string | Uint8Array, + textEncoder: TextEncoderInternal, + textDecoder: TextDecoderInternal, +): Envelope { + let buffer = typeof env === 'string' ? textEncoder.encode(env) : env; + + function readBinary(length: number): Uint8Array { + const bin = buffer.slice(0, length); + // Replace the buffer with the remaining data and newline + buffer = buffer.slice(length + 1); + return bin; + } + + function readJson(): T { + let i = buffer.indexOf(0xa); + // If we couldn't find a newline, we must have found the end of the buffer + if (i < 0) { + i = buffer.length; + } + + return JSON.parse(textDecoder.decode(readBinary(i))) as T; + } + + const header = readJson(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const items: [any, any][] = []; + + while (buffer.length) { + const itemHeader = readJson(); + const binaryLength = typeof itemHeader.length === 'number' ? itemHeader.length : undefined; + + items.push([itemHeader, binaryLength ? readBinary(binaryLength) : readJson()]); + } + + return [header, items]; +} + /** * Creates attachment envelope items */ diff --git a/packages/utils/test/clientreport.test.ts b/packages/utils/test/clientreport.test.ts index 442df49c6edf..03026fd16eaf 100644 --- a/packages/utils/test/clientreport.test.ts +++ b/packages/utils/test/clientreport.test.ts @@ -1,9 +1,11 @@ import { ClientReport } from '@sentry/types'; -import { TextEncoder } from 'util'; +import { TextDecoder, TextEncoder } from 'util'; import { createClientReportEnvelope } from '../src/clientreport'; -import { serializeEnvelope } from '../src/envelope'; -import { parseEnvelope } from './testutils'; +import { parseEnvelope, serializeEnvelope } from '../src/envelope'; + +const encoder = new TextEncoder(); +const decoder = new TextDecoder(); const DEFAULT_DISCARDED_EVENTS: ClientReport['discarded_events'] = [ { @@ -44,7 +46,7 @@ describe('createClientReportEnvelope', () => { it('serializes an envelope', () => { const env = createClientReportEnvelope(DEFAULT_DISCARDED_EVENTS, MOCK_DSN, 123456); - const [headers, items] = parseEnvelope(serializeEnvelope(env, new TextEncoder())); + const [headers, items] = parseEnvelope(serializeEnvelope(env, encoder), encoder, decoder); expect(headers).toEqual({ dsn: 'https://public@example.com/1' }); expect(items).toEqual([ diff --git a/packages/utils/test/envelope.test.ts b/packages/utils/test/envelope.test.ts index 5fa7102281b3..b88400dbe503 100644 --- a/packages/utils/test/envelope.test.ts +++ b/packages/utils/test/envelope.test.ts @@ -1,8 +1,16 @@ import { EventEnvelope } from '@sentry/types'; -import { TextEncoder } from 'util'; +import { TextDecoder, TextEncoder } from 'util'; -import { addItemToEnvelope, createEnvelope, forEachEnvelopeItem, serializeEnvelope } from '../src/envelope'; -import { parseEnvelope } from './testutils'; +const encoder = new TextEncoder(); +const decoder = new TextDecoder(); + +import { + addItemToEnvelope, + createEnvelope, + forEachEnvelopeItem, + parseEnvelope, + serializeEnvelope, +} from '../src/envelope'; describe('envelope', () => { describe('createEnvelope()', () => { @@ -18,17 +26,17 @@ describe('envelope', () => { }); }); - describe('serializeEnvelope()', () => { + describe('serializeEnvelope and parseEnvelope', () => { it('serializes an envelope', () => { const env = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, []); - const serializedEnvelope = serializeEnvelope(env, new TextEncoder()); + const serializedEnvelope = serializeEnvelope(env, encoder); expect(typeof serializedEnvelope).toBe('string'); - const [headers] = parseEnvelope(serializedEnvelope); + const [headers] = parseEnvelope(serializedEnvelope, encoder, decoder); expect(headers).toEqual({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }); }); - it('serializes an envelope with attachments', () => { + it.only('serializes an envelope with attachments', () => { const items: EventEnvelope[1] = [ [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }], [{ type: 'attachment', filename: 'bar.txt', length: 6 }, Uint8Array.from([1, 2, 3, 4, 5, 6])], @@ -42,10 +50,10 @@ describe('envelope', () => { expect.assertions(6); - const serializedEnvelope = serializeEnvelope(env, new TextEncoder()); + const serializedEnvelope = serializeEnvelope(env, encoder); expect(serializedEnvelope).toBeInstanceOf(Uint8Array); - const [parsedHeaders, parsedItems] = parseEnvelope(serializedEnvelope); + const [parsedHeaders, parsedItems] = parseEnvelope(serializedEnvelope, encoder, decoder); expect(parsedHeaders).toEqual({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }); expect(parsedItems).toHaveLength(3); expect(items[0]).toEqual([{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }]); @@ -68,7 +76,7 @@ describe('envelope', () => { [{ type: 'event' }, egg], ]); - const serializedEnvelope = serializeEnvelope(env, new TextEncoder()); + const serializedEnvelope = serializeEnvelope(env, encoder); const [, , serializedBody] = serializedEnvelope.toString().split('\n'); expect(serializedBody).toBe('{"chicken":{"egg":"[Circular ~]"}}'); @@ -78,7 +86,7 @@ describe('envelope', () => { describe('addItemToEnvelope()', () => { it('adds an item to an envelope', () => { const env = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, []); - let [envHeaders, items] = parseEnvelope(serializeEnvelope(env, new TextEncoder())); + let [envHeaders, items] = parseEnvelope(serializeEnvelope(env, encoder), encoder, decoder); expect(items).toHaveLength(0); expect(envHeaders).toEqual({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }); @@ -87,7 +95,7 @@ describe('envelope', () => { { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }, ]); - [envHeaders, items] = parseEnvelope(serializeEnvelope(newEnv, new TextEncoder())); + [envHeaders, items] = parseEnvelope(serializeEnvelope(newEnv, encoder), encoder, decoder); expect(envHeaders).toEqual({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }); expect(items).toHaveLength(1); expect(items[0]).toEqual([{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }]); diff --git a/packages/utils/test/testutils.ts b/packages/utils/test/testutils.ts index b708d77064eb..6130ee7ca365 100644 --- a/packages/utils/test/testutils.ts +++ b/packages/utils/test/testutils.ts @@ -1,6 +1,3 @@ -import { BaseEnvelopeHeaders, BaseEnvelopeItemHeaders, Envelope } from '@sentry/types'; -import { TextDecoder, TextEncoder } from 'util'; - export const testOnlyIfNodeVersionAtLeast = (minVersion: number): jest.It => { const currentNodeVersion = process.env.NODE_VERSION; @@ -14,56 +11,3 @@ export const testOnlyIfNodeVersionAtLeast = (minVersion: number): jest.It => { return it; }; - -/** - * A naive binary envelope parser - */ -export function parseEnvelope(env: string | Uint8Array): Envelope { - let buf = typeof env === 'string' ? new TextEncoder().encode(env) : env; - - let envelopeHeaders: BaseEnvelopeHeaders | undefined; - let lastItemHeader: BaseEnvelopeItemHeaders | undefined; - const items: [any, any][] = []; - - let binaryLength = 0; - while (buf.length) { - // Next length is either the binary length from the previous header - // or the next newline character - let i = binaryLength || buf.indexOf(0xa); - - // If no newline was found, assume this is the last block - if (i < 0) { - i = buf.length; - } - - // If we read out a length in the previous header, assume binary - if (binaryLength > 0) { - const bin = buf.slice(0, binaryLength); - binaryLength = 0; - items.push([lastItemHeader, bin]); - } else { - const json = JSON.parse(new TextDecoder().decode(buf.slice(0, i + 1))); - - if (typeof json.length === 'number') { - binaryLength = json.length; - } - - // First json is always the envelope headers - if (!envelopeHeaders) { - envelopeHeaders = json; - } else { - // If there is a type property, assume this is an item header - if ('type' in json) { - lastItemHeader = json; - } else { - items.push([lastItemHeader, json]); - } - } - } - - // Replace the buffer with the previous block and newline removed - buf = buf.slice(i + 1); - } - - return [envelopeHeaders as BaseEnvelopeHeaders, items]; -} From 3f175c5606d12e46660a7fd6818592f9c8fbe2d4 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 19 Dec 2022 14:35:01 +0000 Subject: [PATCH 2/3] Use subarray rather than slice --- packages/utils/src/envelope.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/utils/src/envelope.ts b/packages/utils/src/envelope.ts index 2fecdd5f6af5..3b9e0e374b53 100644 --- a/packages/utils/src/envelope.ts +++ b/packages/utils/src/envelope.ts @@ -127,9 +127,9 @@ export function parseEnvelope( let buffer = typeof env === 'string' ? textEncoder.encode(env) : env; function readBinary(length: number): Uint8Array { - const bin = buffer.slice(0, length); - // Replace the buffer with the remaining data and newline - buffer = buffer.slice(length + 1); + const bin = buffer.subarray(0, length); + // Replace the buffer with the remaining data excluding trailing newline + buffer = buffer.subarray(length + 1); return bin; } @@ -143,7 +143,7 @@ export function parseEnvelope( return JSON.parse(textDecoder.decode(readBinary(i))) as T; } - const header = readJson(); + const envelopeHeader = readJson(); // eslint-disable-next-line @typescript-eslint/no-explicit-any const items: [any, any][] = []; @@ -154,7 +154,7 @@ export function parseEnvelope( items.push([itemHeader, binaryLength ? readBinary(binaryLength) : readJson()]); } - return [header, items]; + return [envelopeHeader, items]; } /** From cd5b1c7f9c51e36e3b3ac3a6c7ce8b9901796a36 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 19 Dec 2022 15:17:07 +0000 Subject: [PATCH 3/3] Fix test --- packages/core/test/lib/attachments.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/test/lib/attachments.test.ts b/packages/core/test/lib/attachments.test.ts index 610518225793..34385bc61123 100644 --- a/packages/core/test/lib/attachments.test.ts +++ b/packages/core/test/lib/attachments.test.ts @@ -1,5 +1,5 @@ -import { parseEnvelope } from '@sentry/utils/test/testutils'; -import { TextEncoder } from 'util'; +import { parseEnvelope } from '@sentry/utils'; +import { TextDecoder, TextEncoder } from 'util'; import { createTransport } from '../../src/transports/base'; import { getDefaultTestClientOptions, TestClient } from '../mocks/client'; @@ -22,7 +22,7 @@ describe('Attachments', () => { enableSend: true, transport: () => createTransport({ recordDroppedEvent: () => undefined, textEncoder: new TextEncoder() }, async req => { - const [, items] = parseEnvelope(req.body); + const [, items] = parseEnvelope(req.body, new TextEncoder(), new TextDecoder()); expect(items.length).toEqual(2); // Second envelope item should be the attachment expect(items[1][0]).toEqual({ type: 'attachment', length: 50000, filename: 'empty.bin' });