Skip to content

feat(clerk-js,types): Validate query param redirect urls with the allowedRedirectOrigins prop#1096

Merged
nikosdouvlis merged 7 commits into
mainfrom
george/js-268-whitelist-redirections-to-prevent-open
May 11, 2023
Merged

feat(clerk-js,types): Validate query param redirect urls with the allowedRedirectOrigins prop#1096
nikosdouvlis merged 7 commits into
mainfrom
george/js-268-whitelist-redirections-to-prevent-open

Conversation

@desiprisg

@desiprisg desiprisg commented Apr 21, 2023

Copy link
Copy Markdown
Contributor

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

The allowedRedirectOrigins prop has been added to the ClerkProvider. Using this prop, the developer can limit the urls that can be used for redirects via query parameters. If an invalid redirect url is extracted, then it will be ignored and a warning will be printed in the console. The default allowed urls are:

  • Relative URLs
  • Absolute URLs with https protocol where the domain matches, or is a subdomain of, the eTLD+1.

A couple of utils have been added to implement this validation. This takes place inside the pickRedirectionProp util.

@jit-ci jit-ci Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

@desiprisg
desiprisg force-pushed the george/js-268-whitelist-redirections-to-prevent-open branch 2 times, most recently from 06a4071 to 40f7e8a Compare April 27, 2023 07:54
@desiprisg desiprisg changed the title feat(clerk-js,types): Fallback to default redirect urls if isAllowedRedirect returns false feat(clerk-js,types): Validate query param redirect urls with the allowedRedirectOrigins prop Apr 27, 2023
@desiprisg
desiprisg force-pushed the george/js-268-whitelist-redirections-to-prevent-open branch 4 times, most recently from a42831d to 01d06c0 Compare April 28, 2023 12:19
@desiprisg
desiprisg marked this pull request as ready for review April 28, 2023 12:21
@desiprisg
desiprisg requested review from anagstef and dimkl April 28, 2023 12:21
const secondaryQueryParamRedirectUrl =
accessRedirectUrl && typeof queryParams?.redirect_url === 'string' ? queryParams.redirect_url : null;

let queryParamUrl: string | null | undefined = primaryQueryParamRedirectUrl || secondaryQueryParamRedirectUrl;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❓ could we reduce the potential values of queryParamUrl to string | undefined by simplifying the types of the values used to calculate it ?

Comment thread packages/clerk-js/src/utils/authPropHelpers.ts
@nikosdouvlis
nikosdouvlis force-pushed the george/js-268-whitelist-redirections-to-prevent-open branch from 01d06c0 to db4afe8 Compare May 9, 2023 08:04
@nikosdouvlis
nikosdouvlis force-pushed the george/js-268-whitelist-redirections-to-prevent-open branch from 424eabe to fcc9f29 Compare May 9, 2023 14:11

@anagstef anagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💯

@nikosdouvlis
nikosdouvlis merged commit 1ed28b1 into main May 11, 2023
@nikosdouvlis
nikosdouvlis deleted the george/js-268-whitelist-redirections-to-prevent-open branch May 11, 2023 11:32
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants