Skip to content

Commit

Permalink
Improve validateLink error handling (#2702)
Browse files Browse the repository at this point in the history
This improves the error handling of `validateLink` in case of assertion
errors. Previously we used `instanceof`, which can cause false negatives
when importing from libraries. I've changed it to look at the error code
instead.
  • Loading branch information
Mrtenz authored Sep 9, 2024
1 parent ac9af83 commit 338e790
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 4 deletions.
2 changes: 1 addition & 1 deletion packages/snaps-utils/coverage.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"branches": 99.73,
"functions": 98.9,
"lines": 99.43,
"statements": 96.32
"statements": 96.33
}
56 changes: 56 additions & 0 deletions packages/snaps-utils/src/ui.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
walkJsx,
getJsxChildren,
serialiseJsx,
validateLink,
} from './ui';

describe('getTextChildren', () => {
Expand Down Expand Up @@ -541,6 +542,61 @@ describe('getJsxElementFromComponent', () => {
});
});

describe('validateLink', () => {
it('passes for a valid link', () => {
const fn = jest.fn().mockReturnValue(false);

expect(() => validateLink('https://foo.bar', fn)).not.toThrow();
expect(() => validateLink('mailto:foo@bar.com', fn)).not.toThrow();

expect(fn).toHaveBeenCalledTimes(2);
expect(fn).toHaveBeenCalledWith('foo.bar');
expect(fn).toHaveBeenCalledWith('bar.com');
});

it('throws an error for an invalid protocol', () => {
const fn = jest.fn().mockReturnValue(false);

expect(() => validateLink('http://foo.bar', fn)).toThrow(
'Invalid URL: Protocol must be one of: https:, mailto:.',
);

expect(fn).not.toHaveBeenCalled();
});

it('throws an error for an invalid URL', () => {
const fn = jest.fn().mockReturnValue(false);

expect(() => validateLink('foo.bar', fn)).toThrow(
'Invalid URL: Unable to parse URL.',
);

expect(fn).not.toHaveBeenCalled();
});

it('throws an error for a phishing link', () => {
const fn = jest.fn().mockReturnValue(true);

expect(() => validateLink('https://test.metamask-phishing.io', fn)).toThrow(
'Invalid URL: The specified URL is not allowed.',
);

expect(fn).toHaveBeenCalledTimes(1);
expect(fn).toHaveBeenCalledWith('test.metamask-phishing.io');
});

it('throws an error for a phishing email', () => {
const fn = jest.fn().mockReturnValue(true);

expect(() =>
validateLink('mailto:foo@test.metamask-phishing.io', fn),
).toThrow('Invalid URL: The specified URL is not allowed.');

expect(fn).toHaveBeenCalledTimes(1);
expect(fn).toHaveBeenCalledWith('test.metamask-phishing.io');
});
});

describe('validateTextLinks', () => {
it('passes for valid links', () => {
expect(() =>
Expand Down
5 changes: 2 additions & 3 deletions packages/snaps-utils/src/ui.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import {
import {
assert,
assertExhaustive,
AssertionError,
hasProperty,
isPlainObject,
} from '@metamask/utils';
Expand Down Expand Up @@ -332,7 +331,7 @@ function getMarkdownLinks(text: string) {
* @param isOnPhishingList - The function that checks the link against the
* phishing list.
*/
function validateLink(
export function validateLink(
link: string,
isOnPhishingList: (url: string) => boolean,
) {
Expand All @@ -350,7 +349,7 @@ function validateLink(
} catch (error) {
throw new Error(
`Invalid URL: ${
error instanceof AssertionError ? error.message : 'Unable to parse URL.'
error?.code === 'ERR_ASSERTION' ? error.message : 'Unable to parse URL.'
}`,
);
}
Expand Down

0 comments on commit 338e790

Please sign in to comment.