Skip to content

chore!: Simplify SVG validation #2475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 12, 2024
Merged

chore!: Simplify SVG validation #2475

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
114db7a
chore: Simplify SVG validation
FrederikBolding Jun 11, 2024
d9eba41
Simplify
FrederikBolding Jun 11, 2024
7e76196
Remove exports
FrederikBolding Jun 11, 2024
20818fa
Update LavaMoat policies
metamaskbot Jun 11, 2024
afc7e0d
Update example snaps
metamaskbot Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/snaps-sdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
"@metamask/providers": "^17.0.0",
"@metamask/rpc-errors": "^6.2.1",
"@metamask/utils": "^8.3.0",
"fast-xml-parser": "^4.3.4",
"superstruct": "^1.0.3"
},
"devDependencies": {
Expand Down
34 changes: 8 additions & 26 deletions packages/snaps-sdk/src/internals/svg.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,12 @@
import { parseSvg } from './svg';
import { is } from 'superstruct';

const SNAP_ICON =
'<svg width="24" height="25" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M17.037 0H6.975C2.605 0 0 2.617 0 6.987v10.05c0 4.37 2.605 6.975 6.975 6.975h10.05c4.37 0 6.975-2.605 6.975-6.976V6.988C24.012 2.617 21.407 0 17.037 0ZM11.49 17.757c0 .36-.18.684-.492.876a.975.975 0 0 1-.54.156 1.11 1.11 0 0 1-.469-.108l-4.202-2.1a1.811 1.811 0 0 1-.985-1.61v-3.973c0-.36.18-.685.493-.877a1.04 1.04 0 0 1 1.008-.048l4.202 2.101a1.8 1.8 0 0 1 .997 1.609v3.974h-.012Zm-.252-6.423L6.723 8.896a1.045 1.045 0 0 1-.528-.924c0-.384.204-.744.528-.924l4.515-2.438a1.631 1.631 0 0 1 1.524 0l4.515 2.438c.324.18.528.528.528.924s-.204.744-.528.924l-4.515 2.438c-.24.132-.504.192-.768.192a1.54 1.54 0 0 1-.756-.192Zm7.972 3.638c0 .684-.385 1.308-.997 1.608l-4.202 2.101c-.144.072-.3.108-.468.108a.975.975 0 0 1-.54-.156 1.017 1.017 0 0 1-.493-.876v-3.974c0-.684.384-1.309.997-1.609l4.202-2.101a1.04 1.04 0 0 1 1.008.048c.313.192.493.516.493.877v3.974Z" fill="#24272A"/></svg>';
import { svg } from './svg';

describe('parseSvg', () => {
it('parses valid SVGs', () => {
expect(parseSvg(SNAP_ICON)).toMatchInlineSnapshot(`
{
"@_fill": "none",
"@_height": 25,
"@_width": 24,
"@_xmlns": "http://www.w3.org/2000/svg",
"path": {
"@_d": "M17.037 0H6.975C2.605 0 0 2.617 0 6.987v10.05c0 4.37 2.605 6.975 6.975 6.975h10.05c4.37 0 6.975-2.605 6.975-6.976V6.988C24.012 2.617 21.407 0 17.037 0ZM11.49 17.757c0 .36-.18.684-.492.876a.975.975 0 0 1-.54.156 1.11 1.11 0 0 1-.469-.108l-4.202-2.1a1.811 1.811 0 0 1-.985-1.61v-3.973c0-.36.18-.685.493-.877a1.04 1.04 0 0 1 1.008-.048l4.202 2.101a1.8 1.8 0 0 1 .997 1.609v3.974h-.012Zm-.252-6.423L6.723 8.896a1.045 1.045 0 0 1-.528-.924c0-.384.204-.744.528-.924l4.515-2.438a1.631 1.631 0 0 1 1.524 0l4.515 2.438c.324.18.528.528.528.924s-.204.744-.528.924l-4.515 2.438c-.24.132-.504.192-.768.192a1.54 1.54 0 0 1-.756-.192Zm7.972 3.638c0 .684-.385 1.308-.997 1.608l-4.202 2.101c-.144.072-.3.108-.468.108a.975.975 0 0 1-.54-.156 1.017 1.017 0 0 1-.493-.876v-3.974c0-.684.384-1.309.997-1.609l4.202-2.101a1.04 1.04 0 0 1 1.008.048c.313.192.493.516.493.877v3.974Z",
"@_fill": "#24272A",
},
}
`);
});

it('returns an empty object for empty SVGs', () => {
expect(parseSvg('<svg />')).toStrictEqual({});
});

it('throws for invalid SVGs', () => {
expect(() => parseSvg('')).toThrow('Snap icon must be a valid SVG.');
expect(() => parseSvg('foo')).toThrow('Snap icon must be a valid SVG.');
describe('svg', () => {
it('validates an SVG string', () => {
expect(is('<svg />', svg())).toBe(true);
expect(is('<svg></svg>', svg())).toBe(true);
expect(is('<foo/>', svg())).toBe(false);
expect(is(1, svg())).toBe(false);
});
});
52 changes: 12 additions & 40 deletions packages/snaps-sdk/src/internals/svg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,20 @@
import { assert, hasProperty, isObject } from '@metamask/utils';
import { XMLParser } from 'fast-xml-parser';
import { refine, string } from 'superstruct';

/**
* Parse and validate a string as an SVG.
* Get a Struct that validates a string as a valid SVG.
*
* @param svg - An SVG string.
* @returns The SVG, its attributes and children in an object format.
* @returns A Struct that validates a string as a valid SVG.
* @internal
*/
export function parseSvg(svg: string) {
try {
const trimmed = svg.trim();

assert(trimmed.length > 0);

const parser = new XMLParser({
ignoreAttributes: false,
parseAttributeValue: true,
});
const parsed = parser.parse(trimmed, true);

assert(hasProperty(parsed, 'svg'));

// Empty SVGs are not returned as objects
if (!isObject(parsed.svg)) {
return {};
export function svg() {
return refine(string(), 'SVG', (value) => {
// This validation is intentionally very basic, we don't need to be that strict
// and merely have this extra validation as a helpful error if devs aren't
// passing in SVGs.
if (!value.startsWith('<svg')) {
return 'Value is not a valid SVG.';
}

return parsed.svg;
} catch {
throw new Error('Snap icon must be a valid SVG.');
}
}

/**
* Validate that a string is a valid SVG.
*
* @param svg - An SVG string.
* @returns True if the SVG is valid otherwise false.
*/
export function isSvg(svg: string) {
try {
parseSvg(svg);
return true;
} catch {
return false;
}
});
}
56 changes: 28 additions & 28 deletions packages/snaps-sdk/src/jsx/validation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ describe('ButtonStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, ButtonStruct)).toBe(false);
Expand Down Expand Up @@ -195,7 +195,7 @@ describe('InputStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, InputStruct)).toBe(false);
Expand Down Expand Up @@ -237,7 +237,7 @@ describe('FieldStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, FieldStruct)).toBe(false);
Expand Down Expand Up @@ -294,7 +294,7 @@ describe('FormStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, FormStruct)).toBe(false);
Expand All @@ -320,7 +320,7 @@ describe('BoldStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, BoldStruct)).toBe(false);
Expand Down Expand Up @@ -349,7 +349,7 @@ describe('ItalicStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, ItalicStruct)).toBe(false);
Expand Down Expand Up @@ -383,7 +383,7 @@ describe('AddressStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, AddressStruct)).toBe(false);
Expand All @@ -402,19 +402,19 @@ describe('BoxStruct', () => {
<Box>
<Text>foo</Text>
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>
</Box>,
<Box direction="horizontal" alignment="space-between">
<Text>foo</Text>
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>
</Box>,
<Box direction="horizontal">
<Text>foo</Text>
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>
</Box>,
])('validates a box element', (value) => {
Expand All @@ -433,20 +433,20 @@ describe('BoxStruct', () => {
<Box children={[]} />,
<Text>foo</Text>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
// @ts-expect-error - Invalid props.
<Box direction="foo">
<Text>foo</Text>
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>
</Box>,
// @ts-expect-error - Invalid props.
<Box direction="vertical" alignment="foo">
<Text>foo</Text>
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>
</Box>,
<Box>
Expand Down Expand Up @@ -486,7 +486,7 @@ describe('CopyableStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, CopyableStruct)).toBe(false);
Expand All @@ -512,7 +512,7 @@ describe('DividerStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, DividerStruct)).toBe(false);
Expand Down Expand Up @@ -543,7 +543,7 @@ describe('ValueStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, ValueStruct)).toBe(false);
Expand Down Expand Up @@ -575,7 +575,7 @@ describe('HeadingStruct', () => {
<Text>Hello</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, HeadingStruct)).toBe(false);
Expand All @@ -600,13 +600,13 @@ describe('ImageStruct', () => {
// @ts-expect-error - Invalid props.
<Image />,
// @ts-expect-error - Invalid props.
<Image src="src" alt={42} />,
<Image src="<svg />" alt={42} />,
<Text>foo</Text>,
<Box>
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, ImageStruct)).toBe(false);
Expand Down Expand Up @@ -637,7 +637,7 @@ describe('LinkStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, LinkStruct)).toBe(false);
Expand Down Expand Up @@ -669,7 +669,7 @@ describe('TextStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, TextStruct)).toBe(false);
Expand All @@ -682,7 +682,7 @@ describe('RowStruct', () => {
<Text>foo</Text>
</Row>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
<Row label="label" variant="default">
<Address address="0x1234567890abcdef1234567890abcdef12345678" />
Expand Down Expand Up @@ -738,7 +738,7 @@ describe('SpinnerStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, SpinnerStruct)).toBe(false);
Expand Down Expand Up @@ -772,7 +772,7 @@ describe('DropdownStruct', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not validate "%p"', (value) => {
expect(is(value, DropdownStruct)).toBe(false);
Expand All @@ -786,7 +786,7 @@ describe('isJSXElement', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('returns true for a JSX element', (value) => {
expect(isJSXElement(value)).toBe(true);
Expand Down Expand Up @@ -832,10 +832,10 @@ describe('isJSXElementUnsafe', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('returns true for a JSX element', (value) => {
expect(isJSXElement(value)).toBe(true);
expect(isJSXElementUnsafe(value)).toBe(true);
});

it.each([
Expand Down Expand Up @@ -925,7 +925,7 @@ describe('assertJSXElement', () => {
<Text>foo</Text>
</Box>,
<Row label="label">
<Image src="src" alt="alt" />
<Image src="<svg />" alt="alt" />
</Row>,
])('does not throw an error for a JSX element', (value) => {
expect(() => {
Expand Down
4 changes: 2 additions & 2 deletions packages/snaps-sdk/src/jsx/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
import type { ObjectSchema } from 'superstruct/dist/utils';

import type { Describe } from '../internals';
import { literal, nullUnion } from '../internals';
import { literal, nullUnion, svg } from '../internals';
import type { EmptyObject } from '../types';
import type {
GenericSnapElement,
Expand Down Expand Up @@ -278,7 +278,7 @@ export const HeadingStruct: Describe<HeadingElement> = element('Heading', {
* A struct for the {@link ImageElement} type.
*/
export const ImageStruct: Describe<ImageElement> = element('Image', {
src: string(),
src: svg(),
alt: optional(string()),
});

Expand Down
13 changes: 1 addition & 12 deletions packages/snaps-sdk/src/ui/components/image.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,8 @@
import { is } from 'superstruct';

import { NodeType } from '../nodes';
import { image, svg } from './image';
import { image } from './image';

const MOCK_SVG = '<svg />';

describe('svg', () => {
it('validates an SVG string', () => {
expect(is(MOCK_SVG, svg())).toBe(true);
expect(is('<svg></svg>', svg())).toBe(true);
expect(is('<foo/>', svg())).toBe(false);
expect(is(1, svg())).toBe(false);
});
});

describe('image', () => {
it('creates an image component', () => {
expect(image({ value: MOCK_SVG })).toStrictEqual({
Expand Down
Loading
Loading