From eae7d15d7f670d60d0e0e598a7cac6b63adf447b Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Tue, 20 Feb 2024 12:30:43 +0100 Subject: [PATCH 1/5] Add sizing limits for UI inputs --- .../interface/SnapInterfaceController.test.ts | 112 ++++++++++++++++++ .../src/interface/SnapInterfaceController.ts | 21 +++- packages/snaps-utils/src/ui.test.ts | 26 +++- packages/snaps-utils/src/ui.ts | 24 ++++ 4 files changed, 179 insertions(+), 4 deletions(-) diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts index 04a34eafb4..44a57d9305 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts @@ -104,6 +104,52 @@ describe('SnapInterfaceController', () => { 'foo.bar', ); }); + + it('throws if UI content is too large', async () => { + const rootMessenger = getRootSnapInterfaceControllerMessenger(); + const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger( + rootMessenger, + false, + ); + + /* eslint-disable-next-line no-new */ + new SnapInterfaceController({ + messenger: controllerMessenger, + }); + + const components = panel([text('[foo](https://foo.bar)'.repeat(100000))]); + + await expect( + rootMessenger.call( + 'SnapInterfaceController:createInterface', + MOCK_SNAP_ID, + components, + ), + ).rejects.toThrow('UI content is unreasonably large.'); + }); + + it('throws if text content is too large', async () => { + const rootMessenger = getRootSnapInterfaceControllerMessenger(); + const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger( + rootMessenger, + false, + ); + + /* eslint-disable-next-line no-new */ + new SnapInterfaceController({ + messenger: controllerMessenger, + }); + + const components = panel([text('[foo](https://foo.bar)'.repeat(2500))]); + + await expect( + rootMessenger.call( + 'SnapInterfaceController:createInterface', + MOCK_SNAP_ID, + components, + ), + ).rejects.toThrow('UI text content is unreasonably large.'); + }); }); describe('getInterface', () => { @@ -292,6 +338,72 @@ describe('SnapInterfaceController', () => { ); }); + it('throws if UI content is too large', async () => { + const rootMessenger = getRootSnapInterfaceControllerMessenger(); + const controllerMessenger = + getRestrictedSnapInterfaceControllerMessenger(rootMessenger); + + /* eslint-disable-next-line no-new */ + new SnapInterfaceController({ + messenger: controllerMessenger, + }); + + const components = form({ + name: 'foo', + children: [input({ name: 'bar' })], + }); + + const newContent = panel([text('[foo](https://foo.bar)'.repeat(100000))]); + + const id = await rootMessenger.call( + 'SnapInterfaceController:createInterface', + MOCK_SNAP_ID, + components, + ); + + await expect( + rootMessenger.call( + 'SnapInterfaceController:updateInterface', + MOCK_SNAP_ID, + id, + newContent, + ), + ).rejects.toThrow('UI content is unreasonably large.'); + }); + + it('throws if text content is too large', async () => { + const rootMessenger = getRootSnapInterfaceControllerMessenger(); + const controllerMessenger = + getRestrictedSnapInterfaceControllerMessenger(rootMessenger); + + /* eslint-disable-next-line no-new */ + new SnapInterfaceController({ + messenger: controllerMessenger, + }); + + const components = form({ + name: 'foo', + children: [input({ name: 'bar' })], + }); + + const newContent = panel([text('[foo](https://foo.bar)'.repeat(2500))]); + + const id = await rootMessenger.call( + 'SnapInterfaceController:createInterface', + MOCK_SNAP_ID, + components, + ); + + await expect( + rootMessenger.call( + 'SnapInterfaceController:updateInterface', + MOCK_SNAP_ID, + id, + newContent, + ), + ).rejects.toThrow('UI text content is unreasonably large.'); + }); + it('throws if the interface does not exist', async () => { const rootMessenger = getRootSnapInterfaceControllerMessenger(); const controllerMessenger = diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index 1dd0cecae1..419668663c 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -5,12 +5,18 @@ import type { TestOrigin, } from '@metamask/phishing-controller'; import type { Component, InterfaceState, SnapId } from '@metamask/snaps-sdk'; -import { validateComponentLinks } from '@metamask/snaps-utils'; -import { assert } from '@metamask/utils'; +import { + getTotalTextLength, + validateComponentLinks, +} from '@metamask/snaps-utils'; +import { assert, getJsonSize } from '@metamask/utils'; import { nanoid } from 'nanoid'; import { constructState } from './utils'; +const MAX_UI_CONTENT_SIZE = 256000; // 250 kb +const MAX_TEXT_LENGTH = 51200; // 50 kb + const controllerName = 'SnapInterfaceController'; export type CreateInterface = { @@ -252,6 +258,17 @@ export class SnapInterfaceController extends BaseController< * @param content - The components to verify. */ async #validateContent(content: Component) { + const size = getJsonSize(content); + + assert(size <= MAX_UI_CONTENT_SIZE, 'UI content is unreasonably large.'); + + const textSize = getTotalTextLength(content); + + assert( + textSize <= MAX_TEXT_LENGTH, + 'UI text content is unreasonably large.', + ); + await this.#triggerPhishingListUpdate(); validateComponentLinks(content, this.#checkPhishingList.bind(this)); diff --git a/packages/snaps-utils/src/ui.test.ts b/packages/snaps-utils/src/ui.test.ts index 43e0c5c98b..612eebce18 100644 --- a/packages/snaps-utils/src/ui.test.ts +++ b/packages/snaps-utils/src/ui.test.ts @@ -1,6 +1,10 @@ -import { panel, text, row, address } from '@metamask/snaps-sdk'; +import { panel, text, row, address, image } from '@metamask/snaps-sdk'; -import { validateTextLinks, validateComponentLinks } from './ui'; +import { + validateTextLinks, + validateComponentLinks, + getTotalTextLength, +} from './ui'; describe('validateTextLinks', () => { it('passes for valid links', () => { @@ -85,3 +89,21 @@ describe('validateComponentLinks', () => { ).toThrow('Invalid URL: The specified URL is not allowed.'); }); }); + +describe('getTotalTextLength', () => { + it('calculates total length', () => { + expect(getTotalTextLength(text('foo'))).toBe(3); + }); + + it('calculates total length for nested text', () => { + expect( + getTotalTextLength( + panel([text('foo'), panel([text('bar'), text('baz')])]), + ), + ).toBe(9); + }); + + it('ignores non text components', () => { + expect(getTotalTextLength(panel([text('foo'), image('')]))).toBe(3); + }); +}); diff --git a/packages/snaps-utils/src/ui.ts b/packages/snaps-utils/src/ui.ts index 6bcbb25d1e..7ca2260d26 100644 --- a/packages/snaps-utils/src/ui.ts +++ b/packages/snaps-utils/src/ui.ts @@ -82,3 +82,27 @@ export function validateComponentLinks( break; } } + +/** + * Calculate the total length of all text in the component. + * + * @param component - A custom UI component. + * @returns The total length of all text components in the component. + */ +export function getTotalTextLength(component: Component): number { + const { type } = component; + if (type === NodeType.Panel) { + return component.children.reduce( + // This is a bug in TypeScript: https://github.com/microsoft/TypeScript/issues/48313 + // eslint-disable-next-line @typescript-eslint/restrict-plus-operands + (sum, node) => sum + getTotalTextLength(node), + 0, + ); + } + + if (type === NodeType.Text) { + return component.value.length; + } + + return 0; +} From 4fa3de336384a1e569c3e82cd09d1bed0a5ea02e Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 21 Feb 2024 14:37:13 +0100 Subject: [PATCH 2/5] Count text in rows --- packages/snaps-utils/coverage.json | 8 ++++---- packages/snaps-utils/src/ui.test.ts | 6 ++++++ packages/snaps-utils/src/ui.ts | 26 ++++++++++++++------------ 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/packages/snaps-utils/coverage.json b/packages/snaps-utils/coverage.json index f0306f9138..3c6a7adf7f 100644 --- a/packages/snaps-utils/coverage.json +++ b/packages/snaps-utils/coverage.json @@ -1,6 +1,6 @@ { - "branches": 96.4, - "functions": 98.62, - "lines": 98.73, - "statements": 94.45 + "branches": 96.44, + "functions": 98.63, + "lines": 98.74, + "statements": 94.49 } diff --git a/packages/snaps-utils/src/ui.test.ts b/packages/snaps-utils/src/ui.test.ts index 612eebce18..4c9a8c9ffd 100644 --- a/packages/snaps-utils/src/ui.test.ts +++ b/packages/snaps-utils/src/ui.test.ts @@ -103,6 +103,12 @@ describe('getTotalTextLength', () => { ).toBe(9); }); + it('calculates total length for nested text in rows', () => { + expect( + getTotalTextLength(panel([row('1', text('foo')), row('2', text('bar'))])), + ).toBe(6); + }); + it('ignores non text components', () => { expect(getTotalTextLength(panel([text('foo'), image('')]))).toBe(3); }); diff --git a/packages/snaps-utils/src/ui.ts b/packages/snaps-utils/src/ui.ts index 7ca2260d26..c1bfe3982d 100644 --- a/packages/snaps-utils/src/ui.ts +++ b/packages/snaps-utils/src/ui.ts @@ -91,18 +91,20 @@ export function validateComponentLinks( */ export function getTotalTextLength(component: Component): number { const { type } = component; - if (type === NodeType.Panel) { - return component.children.reduce( - // This is a bug in TypeScript: https://github.com/microsoft/TypeScript/issues/48313 - // eslint-disable-next-line @typescript-eslint/restrict-plus-operands - (sum, node) => sum + getTotalTextLength(node), - 0, - ); - } - if (type === NodeType.Text) { - return component.value.length; + switch (type) { + case NodeType.Panel: + return component.children.reduce( + // This is a bug in TypeScript: https://github.com/microsoft/TypeScript/issues/48313 + // eslint-disable-next-line @typescript-eslint/restrict-plus-operands + (sum, node) => sum + getTotalTextLength(node), + 0, + ); + case NodeType.Row: + return getTotalTextLength(component.value); + case NodeType.Text: + return component.value.length; + default: + return 0; } - - return 0; } From 9785f788c1e9ae5db189a536e634e5ece1865910 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 21 Feb 2024 14:37:33 +0100 Subject: [PATCH 3/5] Update packages/snaps-controllers/src/interface/SnapInterfaceController.ts Co-authored-by: Maarten Zuidhoorn --- .../snaps-controllers/src/interface/SnapInterfaceController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index 419668663c..cc81ac9988 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -260,7 +260,7 @@ export class SnapInterfaceController extends BaseController< async #validateContent(content: Component) { const size = getJsonSize(content); - assert(size <= MAX_UI_CONTENT_SIZE, 'UI content is unreasonably large.'); + assert(size <= MAX_UI_CONTENT_SIZE, `A Snap UI may not be larger than ${MAX_UI_CONTENT_SIZE / 1000} kB.`); const textSize = getTotalTextLength(content); From 1afad886b1b0ba4cdd1f4211da45fd7305f59396 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 21 Feb 2024 14:46:48 +0100 Subject: [PATCH 4/5] Update error messaging --- .../src/interface/SnapInterfaceController.test.ts | 8 ++++---- .../src/interface/SnapInterfaceController.ts | 13 +++++++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts index 44a57d9305..34349537b4 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.ts @@ -125,7 +125,7 @@ describe('SnapInterfaceController', () => { MOCK_SNAP_ID, components, ), - ).rejects.toThrow('UI content is unreasonably large.'); + ).rejects.toThrow('A Snap UI may not be larger than 250 kB.'); }); it('throws if text content is too large', async () => { @@ -148,7 +148,7 @@ describe('SnapInterfaceController', () => { MOCK_SNAP_ID, components, ), - ).rejects.toThrow('UI text content is unreasonably large.'); + ).rejects.toThrow('The text in a Snap UI may not be larger than 50 kB.'); }); }); @@ -368,7 +368,7 @@ describe('SnapInterfaceController', () => { id, newContent, ), - ).rejects.toThrow('UI content is unreasonably large.'); + ).rejects.toThrow('A Snap UI may not be larger than 250 kB.'); }); it('throws if text content is too large', async () => { @@ -401,7 +401,7 @@ describe('SnapInterfaceController', () => { id, newContent, ), - ).rejects.toThrow('UI text content is unreasonably large.'); + ).rejects.toThrow('The text in a Snap UI may not be larger than 50 kB.'); }); it('throws if the interface does not exist', async () => { diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index cc81ac9988..ec57137c0f 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -14,8 +14,8 @@ import { nanoid } from 'nanoid'; import { constructState } from './utils'; -const MAX_UI_CONTENT_SIZE = 256000; // 250 kb -const MAX_TEXT_LENGTH = 51200; // 50 kb +const MAX_UI_CONTENT_SIZE = 250_000; // 250 kb +const MAX_TEXT_LENGTH = 50_000; // 50 kb const controllerName = 'SnapInterfaceController'; @@ -260,13 +260,18 @@ export class SnapInterfaceController extends BaseController< async #validateContent(content: Component) { const size = getJsonSize(content); - assert(size <= MAX_UI_CONTENT_SIZE, `A Snap UI may not be larger than ${MAX_UI_CONTENT_SIZE / 1000} kB.`); + assert( + size <= MAX_UI_CONTENT_SIZE, + `A Snap UI may not be larger than ${MAX_UI_CONTENT_SIZE / 1000} kB.`, + ); const textSize = getTotalTextLength(content); assert( textSize <= MAX_TEXT_LENGTH, - 'UI text content is unreasonably large.', + `The text in a Snap UI may not be larger than ${ + MAX_TEXT_LENGTH / 1000 + } kB.`, ); await this.#triggerPhishingListUpdate(); From ebadbd373fc5e588480e5a62e73131f19ce244db Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Wed, 21 Feb 2024 15:19:17 +0100 Subject: [PATCH 5/5] Fix coverage --- packages/snaps-controllers/coverage.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 4364775227..419d5c4bff 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,6 +1,6 @@ { "branches": 91.01, "functions": 96.56, - "lines": 97.79, - "statements": 97.45 + "lines": 97.8, + "statements": 97.46 }