Skip to content

Commit 200ab4c

Browse files
runway-github[bot]davidmurdoch
runway-github[bot]
and
davidmurdoch
authored
release(runway): cherry-pick fix: cp-13.10.4 parse signed deep links with empty sig_params correctly (#38269)
- 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] > Handle empty `sig_params` in deep-link canonicalization/parsing and add a gated `/test` route with updated tests and helpers. > > - **Deep links**: > - Canonicalization: Treat `sig_params` as present even when empty; preserve only listed params (none if empty), always append `sig_params`, and keep URL immutable. > - Parsing: Remove `sig_params` before invoking handlers; enforce filtering to only listed params. > - **Routes**: > - Add `DEVELOPER_OPTIONS_ROUTE` export and a gated `/test` route (enabled in tests/dev options) that forwards query params. > - **E2E/Tests**: > - Add/expand unit and e2e tests for empty `sig_params` and param filtering; switch relevant tests to `/test` route. > - Helpers: `signDeepLink` now always appends `sig_params` (possibly empty); simplify `getHashParams` parsing. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 2f054e9. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> [8b80bc2](8b80bc2) Co-authored-by: David Murdoch <[email protected]>
1 parent 6a92b3e commit 200ab4c

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
@@ -592,16 +592,14 @@ and we'll take you to the right place.`
592592
const homePage = new HomePage(driver);
593593
await homePage.checkPageIsLoaded();
594594

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

599598
await driver.openNewURL(signedUrl);
600599
const deepLink = new DeepLink(driver);
601600
await deepLink.checkPageIsLoaded();
602-
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
603601
await deepLink.clickContinueButton();
604-
await homePage.checkPageIsLoaded();
602+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
605603

606604
assert.deepStrictEqual(hashParams.getAll('foo'), ['0', '1']);
607605
assert.deepStrictEqual(hashParams.getAll('bar'), ['2']);
@@ -610,7 +608,7 @@ and we'll take you to the right place.`
610608
);
611609
});
612610

613-
it('signed without sig_params exposes all params (foo, bar, baz)', async function () {
611+
it('signed with empty sig_params, but url has extra params added, does not expose extra params', async function () {
614612
await withFixtures(
615613
await getConfig(this.test?.fullTitle()),
616614
async ({ driver }: { driver: Driver }) => {
@@ -621,16 +619,65 @@ and we'll take you to the right place.`
621619
const homePage = new HomePage(driver);
622620
await homePage.checkPageIsLoaded();
623621

624-
const rawUrl = 'https://link.metamask.io/home?foo=1&bar=2&baz=3';
625-
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl, false);
622+
const rawUrl = 'https://link.metamask.io/test';
623+
const signedUrl = `${await signDeepLink(keyPair.privateKey, rawUrl)}&foo=0&foo=1&bar=2&baz=3`;
626624

627625
await driver.openNewURL(signedUrl);
628626
const deepLink = new DeepLink(driver);
629627
await deepLink.checkPageIsLoaded();
628+
await deepLink.clickContinueButton();
630629
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
630+
631+
assert.deepStrictEqual(hashParams.size, 0);
632+
},
633+
);
634+
});
635+
636+
it('signed with sig_params, url has no extra params added, works', async function () {
637+
await withFixtures(
638+
await getConfig(this.test?.fullTitle()),
639+
async ({ driver }: { driver: Driver }) => {
640+
await driver.navigate();
641+
const loginPage = new LoginPage(driver);
642+
await loginPage.checkPageIsLoaded();
643+
await loginPage.loginToHomepage();
644+
const homePage = new HomePage(driver);
645+
await homePage.checkPageIsLoaded();
646+
647+
const rawUrl = 'https://link.metamask.io/test';
648+
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl);
649+
650+
await driver.openNewURL(signedUrl);
651+
const deepLink = new DeepLink(driver);
652+
await deepLink.checkPageIsLoaded();
631653
await deepLink.clickContinueButton();
654+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
655+
656+
assert.deepStrictEqual(hashParams.size, 0);
657+
},
658+
);
659+
});
660+
661+
it('signed without sig_params exposes all params (foo, bar, baz)', async function () {
662+
await withFixtures(
663+
await getConfig(this.test?.fullTitle()),
664+
async ({ driver }: { driver: Driver }) => {
665+
await driver.navigate();
666+
const loginPage = new LoginPage(driver);
667+
await loginPage.checkPageIsLoaded();
668+
await loginPage.loginToHomepage();
669+
const homePage = new HomePage(driver);
632670
await homePage.checkPageIsLoaded();
633671

672+
const rawUrl = 'https://link.metamask.io/test?foo=1&bar=2&baz=3';
673+
const signedUrl = await signDeepLink(keyPair.privateKey, rawUrl, false);
674+
675+
await driver.openNewURL(signedUrl);
676+
const deepLink = new DeepLink(driver);
677+
await deepLink.checkPageIsLoaded();
678+
await deepLink.clickContinueButton();
679+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
680+
634681
assert.deepStrictEqual(hashParams.getAll('foo'), ['1']);
635682
assert.deepStrictEqual(hashParams.getAll('bar'), ['2']);
636683
assert.deepStrictEqual(hashParams.getAll('baz'), ['3']);
@@ -649,14 +696,13 @@ and we'll take you to the right place.`
649696
const homePage = new HomePage(driver);
650697
await homePage.checkPageIsLoaded();
651698

652-
const rawUrl = 'https://link.metamask.io/home?foo=1&foo=2&bar=3';
699+
const rawUrl = 'https://link.metamask.io/test?foo=1&foo=2&bar=3';
653700

654701
await driver.openNewURL(rawUrl);
655702
const deepLink = new DeepLink(driver);
656703
await deepLink.checkPageIsLoaded();
657-
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
658704
await deepLink.clickContinueButton();
659-
await homePage.checkPageIsLoaded();
705+
const hashParams = getHashParams(new URL(await driver.getCurrentUrl()));
660706

661707
assert.deepStrictEqual(hashParams.getAll('foo'), ['1', '2']);
662708
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)