- 
                Notifications
    You must be signed in to change notification settings 
- Fork 403
feat(clerk-js): Add default Allowed redirect origins #2128
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
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
allowedRedirectOriginsoption, 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 testruns as expected.npm run buildruns as expected.Type of change
Packages affected
@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/clerk-expo@clerk/fastifygatsby-plugin-clerk@clerk/localizations@clerk/nextjs@clerk/clerk-react@clerk/remix@clerk/clerk-sdk-node@clerk/shared@clerk/themes@clerk/typesbuild/tooling/chore