-
Notifications
You must be signed in to change notification settings - Fork 253
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(clerk-js): Add default Allowed redirect origins #2128
feat(clerk-js): Add default Allowed redirect origins #2128
Conversation
🦋 Changeset detectedLatest commit: 419ead7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
0736b69
to
1b8d03b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Based on the definition of the origin from MDN docs, I think that we should only allow passing origin values to the allowedRedirectOrigins
option (eg ${protocol}://${domain}:${port}
). Doing that will also allow us to drop the /*
path default origins. If we want to match paths in allowedRedirectOrigins i would advise we rename the options.
cc: @nikosdouvlis
1b8d03b
to
407ad7b
Compare
!preview |
Hey @octoper, your preview is available.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@octoper this looks good, we need a minor change but apart from that everything is in place. Let's try and merge it today :)
The
/*
Are you suggesting we manually remove |
7a3faa6
to
898fd84
Compare
packages/clerk-js/src/utils/url.ts
Outdated
): (string | RegExp)[] | undefined { | ||
if (!allowedRedirectOrigins || allowedRedirectOrigins.length === 0) { | ||
const origins = []; | ||
if (typeof window !== 'undefined' && !!window.location) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ could we use the inBrowser()
helper instead? @anagstef wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I had that but change it as another utility function in the same file had it like this so I stick with that for consistency, but I can change to the inBrowser
for every function here.
…ter to a utility function
7e94a65
to
a0a7b81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Description
This PR introduces default values for the
allowedRedirectOrigins
option, the current implementation does not provide any defaults, with this change if there is no option provided the default will be similar to the example below.Let's say the host of the application is
test.host
, the origins will behttps://test.host/
https://yourawesomeapp.clerk.accounts.dev/
https://*.yourawesomeapp.clerk.accounts.dev/
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change
Packages affected
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/clerk-expo
@clerk/fastify
gatsby-plugin-clerk
@clerk/localizations
@clerk/nextjs
@clerk/clerk-react
@clerk/remix
@clerk/clerk-sdk-node
@clerk/shared
@clerk/themes
@clerk/types
build/tooling/chore