Skip to content
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.

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

feat: allow wildcards in allowedOrigins #2458

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

FrederikBolding
Copy link
Member

Allow wildcards in allowedOrigins by generating a RegExp based on each allowed origin and testing the origin against it.

Closes #2457

@FrederikBolding FrederikBolding requested a review from a team as a code owner June 3, 2024 13:17
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.33%. Comparing base (e95e670) to head (b5654d4).

Current head b5654d4 differs from pull request most recent head 8c1d6a4

Please upload reports for the commit 8c1d6a4 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2458   +/-   ##
=======================================
  Coverage   94.32%   94.33%           
=======================================
  Files         438      438           
  Lines        9021     9032   +11     
  Branches     1382     1383    +1     
=======================================
+ Hits         8509     8520   +11     
  Misses        512      512           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/snaps-utils/src/json-rpc.ts Outdated Show resolved Hide resolved
allowedOrigins: ['https://*', 'npm:*'],
};

expect(
Copy link
Member

Choose a reason for hiding this comment

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

These tests are maybe a bit more readable with it.each.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't find it.each that readable personally tbh 😅

Comment on lines 132 to 134
origins.allowedOrigins
?.map(createOriginRegExp)
.some((regex) => regex.test(origin))
Copy link
Member

Choose a reason for hiding this comment

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

To avoid creating regex unnecessarily, what do you think of

function checkOrigin(origin: string, originSpecifier: string) {
  if (originSpecifier === '*') {
    return true;
  }

  // Create regex and test.
}


// ...
origins.allowedOrigins?.some((specifier) => checkOrigin(origin, specifier))
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I added one more optimization to this

@FrederikBolding FrederikBolding merged commit 2919879 into main Jun 4, 2024
151 checks passed
@FrederikBolding FrederikBolding deleted the fb/allowed-origins-wildcard branch June 4, 2024 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants