Skip to content

Commit 8b80bc2

Browse files
davidmurdochdavidmurdoch
authored
fix: cp-13.10.4 parse signed deep links with empty sig_params correctly (#38142)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** In previous versions, this link would not verify: https://link.metamask.io/home?sig_params=&sig=OHXtQkiw6eH0yxVZb3wFabCEahlcimocFuh8I90u2nEF2Ats3ETf_fQ2qO4zEx8PYr5CXzU0PQFEOgXuGpmHvQ&utm_medium=github Now it does. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/38142?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: signed deep links with empty `sig_params` with extra params as valid <!-- ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Fixes deep link canonicalization/parsing for empty `sig_params`, introduces a test-only `/test` route, and updates e2e/unit tests accordingly. > > - **Deep Links**: > - **Canonicalization**: `canonicalize` now treats present-but-empty `sig_params` as valid, retaining only `sig_params` and removing all other params; preserves original URL; sorts consistently. > - **Parsing**: Ensures `sig_params` is stripped from handler params and filters params strictly to those listed in `sig_params`. > - **Helpers**: `signDeepLink` always appends `sig_params` (can be empty) before signing; `getHashParams` simplified to read query directly from hash. > - **Routes**: > - Adds test-only route `shared/lib/deep-links/routes/test-route.ts` and conditionally registers it in `routes/index.ts` when `ENABLE_SETTINGS_PAGE_DEV_OPTIONS` or `IN_TEST` is set; re-exports `DEVELOPER_OPTIONS_ROUTE`. > - **Tests**: > - Unit: New cases for empty `sig_params` in `canonicalize.test.ts` and handler param filtering in `parse.test.ts`. > - E2E: Switch deep-link tests to `/test` route and add scenarios covering empty `sig_params`, strict filtering, and unsigned flows. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 10d704d. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 7ecb41b commit 8b80bc2

File tree

8 files changed

+157
-26
lines changed

8 files changed

+157
-26
lines changed

‎shared/lib/deep-links/canonicalize.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,36 @@ describe('canonicalize', () => {
8282
expect(original.searchParams.get(SIG_PARAM)).toBe('abc');
8383
expect(original.searchParams.get(SIG_PARAMS_PARAM)).toBe('a,b');
8484
});
85+
86+
it('removes all params when `sig_params` is present but empty (no params signed)', () => {
87+
const url = new URL(
88+
`https://example.com/path?a=2&b=1&${SIG_PARAM}=abc&${SIG_PARAMS_PARAM}`,
89+
);
90+
// No a or b params should be included, only sig_params= should remain
91+
expect(canonicalize(url)).toBe('https://example.com/path?sig_params=');
92+
});
93+
94+
it('removes all params when `sig_params=` is present but empty (no params signed)', () => {
95+
const url = new URL(
96+
`https://example.com/path?a=2&b=1&${SIG_PARAM}=abc&${SIG_PARAMS_PARAM}=`,
97+
);
98+
// No a or b params should be included, only sig_params= should remain
99+
expect(canonicalize(url)).toBe('https://example.com/path?sig_params=');
100+
});
101+
102+
it('treats URLs with `sig_params` (when no other params are included) as valid sig_params', () => {
103+
const url = new URL(
104+
`https://example.com/path?${SIG_PARAM}=abc&${SIG_PARAMS_PARAM}`,
105+
);
106+
// No params were included, but sig_params should still remain
107+
expect(canonicalize(url)).toBe('https://example.com/path?sig_params=');
108+
});
109+
110+
it('treats URLs with `sig_params=` (when no other params are included) as valid sig_params', () => {
111+
const url = new URL(
112+
`https://example.com/path?${SIG_PARAM}=abc&${SIG_PARAMS_PARAM}=`,
113+
);
114+
// No params were included, but sig_params should still remain
115+
expect(canonicalize(url)).toBe('https://example.com/path?sig_params=');
116+
});
85117
});

‎shared/lib/deep-links/canonicalize.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,18 @@ export function canonicalize(url: URL): string {
1313

1414
const sigParams = url.searchParams.get(SIG_PARAMS_PARAM);
1515

16-
if (sigParams) {
17-
const allowedParams = sigParams.split(',');
16+
if (typeof sigParams === 'string') {
1817
const signedParams = new URLSearchParams();
18+
// sigParams might be "" (empty), in which case we
19+
// don't need to split and search
20+
if (sigParams) {
21+
const allowedParams = sigParams.split(',');
1922

20-
for (const allowedParam of allowedParams) {
21-
const values = url.searchParams.getAll(allowedParam);
22-
for (const value of values) {
23-
signedParams.append(allowedParam, value);
23+
for (const allowedParam of allowedParams) {
24+
const values = url.searchParams.getAll(allowedParam);
25+
for (const value of values) {
26+
signedParams.append(allowedParam, value);
27+
}
2428
}
2529
}
2630

‎shared/lib/deep-links/parse.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,4 +132,39 @@ describe('parse', () => {
132132

133133
expect(mockVerify).toHaveBeenCalledWith(new URL(urlStr));
134134
});
135+
136+
it("removes the SIG_PARAMS_PARAM from the handler's query parameters", async () => {
137+
mockRoutes.set('/test', { handler: mockHandler } as unknown as Route);
138+
mockVerify.mockResolvedValue(VALID);
139+
140+
const urlStr1 = 'https://example.com/test?sig=bar&sig_params=';
141+
await parse(new URL(urlStr1));
142+
// `sig_params` should be removed from the handler's searchParams
143+
expect(mockHandler).toHaveBeenCalledWith(new URLSearchParams());
144+
145+
const urlStr2 =
146+
'https://example.com/test?sig=bar&sig_params=value&value=123';
147+
await parse(new URL(urlStr2));
148+
// `sig_params` should be removed from the handler's searchParams, `value`
149+
// should remain because it is listed in `sig_params`
150+
expect(mockHandler).toHaveBeenCalledWith(
151+
new URLSearchParams([['value', '123']]),
152+
);
153+
154+
const urlStr3 = 'https://example.com/test?sig=bar&sig_params=&value=123';
155+
await parse(new URL(urlStr3));
156+
// `sig_params` should be removed from the handler's searchParams, `value`
157+
// should also be removed because it is not in `sig_params`
158+
expect(mockHandler).toHaveBeenCalledWith(new URLSearchParams());
159+
160+
const urlStr4 =
161+
'https://example.com/test?sig=bar&sig_params=value&value=123&foo=bar';
162+
await parse(new URL(urlStr4));
163+
// `sig_params` should be removed from the handler's searchParams, `value`
164+
// should remain because it is listed in `sig_params`, `foo` should be removed
165+
// because it is not listed in `sig_params`
166+
expect(mockHandler).toHaveBeenCalledWith(
167+
new URLSearchParams([['value', '123']]),
168+
);
169+
});
135170
});

‎shared/lib/deep-links/routes/index.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ export function addRoute(route: Route) {
3333
routes.set(route.pathname, route);
3434
}
3535

36+
if (process.env.ENABLE_SETTINGS_PAGE_DEV_OPTIONS || process.env.IN_TEST) {
37+
// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires, node/global-require
38+
addRoute(require('./test-route').default);
39+
}
40+
3641
addRoute(buy);
3742
addRoute(home);
3843
addRoute(notifications);

‎shared/lib/deep-links/routes/route.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export {
1111
NOTIFICATIONS_ROUTE,
1212
SHIELD_PLAN_ROUTE,
1313
SETTINGS_ROUTE,
14+
DEVELOPER_OPTIONS_ROUTE,
1415
} from '../../../../ui/helpers/constants/routes';
1516

1617
/**

‎shared/lib/deep-links/routes/test-route.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import { DEVELOPER_OPTIONS_ROUTE, Route } from './route';
2+
3+
export default new Route({
4+
pathname: '/test',
5+
getTitle: (_: URLSearchParams) => 'deepLink_thePerpsPage',
6+
handler: function handler(params: URLSearchParams) {
7+
return {
8+
// we use the developer options route for testing purposes
9+
// because it doesn't rewrite query params
10+
path: DEVELOPER_OPTIONS_ROUTE,
11+
query: params,
12+
};
13+
},
14+
});

‎test/e2e/tests/deep-link/deep-link.spec.ts‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -544,16 +544,14 @@ and we'll take you to the right place.`
544544
const homePage = new HomePage(driver);
545545
await homePage.checkPageIsLoaded();
546546

547-
const rawUrl =
548-
'https://link.metamask.io/home?foo=0&foo=1&bar=2&baz=3&sig_params=foo,bar';
549-
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl, false);
547+
const rawUrl = 'https://link.metamask.io/test?foo=0&foo=1&bar=2';
548+
const signedUrl = `${await signDeepLink(keyPair.privateKey, rawUrl)}&baz=3`;
550549

551550
await driver.openNewURL(signedUrl);
552551
const deepLink = new DeepLink(driver);
553552
await deepLink.checkPageIsLoaded();
554-
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
555553
await deepLink.clickContinueButton();
556-
await homePage.checkPageIsLoaded();
554+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
557555

558556
assert.deepStrictEqual(hashParams.getAll('foo'), ['0', '1']);
559557
assert.deepStrictEqual(hashParams.getAll('bar'), ['2']);
@@ -562,7 +560,7 @@ and we'll take you to the right place.`
562560
);
563561
});
564562

565-
it('signed without sig_params exposes all params (foo, bar, baz)', async function () {
563+
it('signed with empty sig_params, but url has extra params added, does not expose extra params', async function () {
566564
await withFixtures(
567565
await getConfig(this.test?.fullTitle()),
568566
async ({ driver }: { driver: Driver }) => {
@@ -573,16 +571,65 @@ and we'll take you to the right place.`
573571
const homePage = new HomePage(driver);
574572
await homePage.checkPageIsLoaded();
575573

576-
const rawUrl = 'https://link.metamask.io/home?foo=1&bar=2&baz=3';
577-
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl, false);
574+
const rawUrl = 'https://link.metamask.io/test';
575+
const signedUrl = `${await signDeepLink(keyPair.privateKey, rawUrl)}&foo=0&foo=1&bar=2&baz=3`;
578576

579577
await driver.openNewURL(signedUrl);
580578
const deepLink = new DeepLink(driver);
581579
await deepLink.checkPageIsLoaded();
580+
await deepLink.clickContinueButton();
582581
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
582+
583+
assert.deepStrictEqual(hashParams.size, 0);
584+
},
585+
);
586+
});
587+
588+
it('signed with sig_params, url has no extra params added, works', async function () {
589+
await withFixtures(
590+
await getConfig(this.test?.fullTitle()),
591+
async ({ driver }: { driver: Driver }) => {
592+
await driver.navigate();
593+
const loginPage = new LoginPage(driver);
594+
await loginPage.checkPageIsLoaded();
595+
await loginPage.loginToHomepage();
596+
const homePage = new HomePage(driver);
597+
await homePage.checkPageIsLoaded();
598+
599+
const rawUrl = 'https://link.metamask.io/test';
600+
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl);
601+
602+
await driver.openNewURL(signedUrl);
603+
const deepLink = new DeepLink(driver);
604+
await deepLink.checkPageIsLoaded();
583605
await deepLink.clickContinueButton();
606+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
607+
608+
assert.deepStrictEqual(hashParams.size, 0);
609+
},
610+
);
611+
});
612+
613+
it('signed without sig_params exposes all params (foo, bar, baz)', async function () {
614+
await withFixtures(
615+
await getConfig(this.test?.fullTitle()),
616+
async ({ driver }: { driver: Driver }) => {
617+
await driver.navigate();
618+
const loginPage = new LoginPage(driver);
619+
await loginPage.checkPageIsLoaded();
620+
await loginPage.loginToHomepage();
621+
const homePage = new HomePage(driver);
584622
await homePage.checkPageIsLoaded();
585623

624+
const rawUrl = 'https://link.metamask.io/test?foo=1&bar=2&baz=3';
625+
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl, false);
626+
627+
await driver.openNewURL(signedUrl);
628+
const deepLink = new DeepLink(driver);
629+
await deepLink.checkPageIsLoaded();
630+
await deepLink.clickContinueButton();
631+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
632+
586633
assert.deepStrictEqual(hashParams.getAll('foo'), ['1']);
587634
assert.deepStrictEqual(hashParams.getAll('bar'), ['2']);
588635
assert.deepStrictEqual(hashParams.getAll('baz'), ['3']);
@@ -601,14 +648,13 @@ and we'll take you to the right place.`
601648
const homePage = new HomePage(driver);
602649
await homePage.checkPageIsLoaded();
603650

604-
const rawUrl = 'https://link.metamask.io/home?foo=1&foo=2&bar=3';
651+
const rawUrl = 'https://link.metamask.io/test?foo=1&foo=2&bar=3';
605652

606653
await driver.openNewURL(rawUrl);
607654
const deepLink = new DeepLink(driver);
608655
await deepLink.checkPageIsLoaded();
609-
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
610656
await deepLink.clickContinueButton();
611-
await homePage.checkPageIsLoaded();
657+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
612658

613659
assert.deepStrictEqual(hashParams.getAll('foo'), ['1', '2']);
614660
assert.deepStrictEqual(hashParams.getAll('bar'), ['3']);

‎test/e2e/tests/deep-link/helpers.ts‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,8 @@ export async function signDeepLink(
3838
if (withSigParams) {
3939
const sigParams = [...new Set(signedUrl.searchParams.keys())];
4040

41-
if (sigParams.length) {
42-
signedUrl.searchParams.append(SIG_PARAMS_PARAM, sigParams.join(','));
43-
signedUrl.searchParams.sort();
44-
}
41+
signedUrl.searchParams.append(SIG_PARAMS_PARAM, sigParams.join(','));
42+
signedUrl.searchParams.sort();
4543
}
4644

4745
const signed = await crypto.subtle.sign(
@@ -98,9 +96,5 @@ export function cartesianProduct<T extends unknown[][]>(...sets: T) {
9896
export function getHashParams(url: URL) {
9997
const hash = url.hash.slice(1); // remove leading '#'
10098
const hashQuery = hash.split('?')[1] ?? '';
101-
const hashParams = new URLSearchParams(hashQuery);
102-
const encodedUrl = hashParams.get('u') ?? '';
103-
const decodedUrl = decodeURIComponent(encodedUrl);
104-
const decodedQuery = decodedUrl.split('?')[1] ?? '';
105-
return new URLSearchParams(decodedQuery);
99+
return new URLSearchParams(hashQuery);
106100
}

0 commit comments

Comments
 (0)