Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

feat: enforce async request redirect URL is in the snaps 'allowedOrigins' #228

Merged
merged 8 commits into from
Mar 5, 2024

Conversation

k-g-j
Copy link
Contributor

@k-g-j k-g-j commented Feb 28, 2024

Description

Currently, in the async flow, we do not check that the redirect URL matches a domain of one of the allowed origins present in the Snap's manifest, and this has to be checked during the Snap review process. This change will add an additional check when receiving an async request that verifies whether or not the URL domain is within the Snap's allowed origins.

Testing

Coverage remains at 100%

Test Coverage Report

File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 100 100 100 100
CaseInsensitiveMap.ts 100 100 100 100
DeferredPromise.ts 100 100 100 100
KeyringSnapControllerClient.ts 100 100 100 100
SnapIdMap.ts 100 100 100 100
SnapKeyring.ts 100 100 100 100
index.ts 100 100 100 100
types.ts 100 100 100 100
util.ts 100 100 100 100

Jest

yarn jest src/SnapKeyring.test.ts -t "throws an error if async request redirect url is not an allowed origin"
yarn jest src/SnapKeyring.test.ts -t "throws an error if no allowed origins and async request redirect url"

@k-g-j k-g-j requested a review from a team as a code owner February 28, 2024 19:48
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see that coverage at a 100%. Left a big comment

src/SnapKeyring.ts Outdated Show resolved Hide resolved
@danroc danroc changed the title feat: enforce async request redirect url is in the snaps 'allowedOrigins' feat: enforce async request redirect URL is in the snaps 'allowedOrigins' Feb 29, 2024
@gantunesr gantunesr added the team-accounts This should be handled by the Accounts Team label Mar 4, 2024
src/SnapKeyring.test.ts Outdated Show resolved Hide resolved
@danroc danroc added this pull request to the merge queue Mar 5, 2024
Merged via the queue into main with commit b82f2ab Mar 5, 2024
19 checks passed
@danroc danroc deleted the feat/enforce-allowed-redirect-url-origin branch March 5, 2024 14:58
Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :) Thanks for those cosmetic changes!

Note

The main description is faulty for the jest tests, I tested using:

yarn jest src/SnapKeyring.test.ts -t "throws an error if async request redirect url is not an allowed origin"
yarn jest src/SnapKeyring.test.ts -t "throws an error if no allowed origins"
yarn jest src/SnapKeyring.test.ts -t "throws an error if the snap is undefined"

and also re-run jest globally to make sure the coverage is still 100%, so all good 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-accounts This should be handled by the Accounts Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants