Fix false positive when check isSafeRedirect and a absolute URL is in URL Params

[dkleber89]

Description

As Described in #545 -> When a absolute URL is in a parameter of redirectURL then we get a false positive. Fixed by check double // only on the beginning of the URL

References

Fix #545

Testing

Added Unit Test to test the changed helper Function (isSafeRedirect)

Checklist

• I have added documentation for new/changed functionality in this PR or in auth0.com/docs
• All active GitHub checks for tests, formatting, and security are passing
• The correct base branch is being used, if not main

[vercel]

@dkleber89 is attempting to deploy a commit to the Auth0 Team on Vercel.

A member of the Team first needs to authorize it.

[adamjmcgrath]

Hi @dkleber89 - thanks for raising this

Unfortunately, this would be vulnerable to an open redirect (visiting /api/auth/login?%20//google.com would redirect to google.com)

To fix #545 - can you update your application logic to add the https:// to the someUrl param?

[dkleber89]

@adamjmcgrath ... Hmm i can´t aggree with that at this time -> Maybe i oversee something but ->

In this helper we check yet "//" at the beginning or something like "http:" or "ftp:" ... at the beginning ->

export default function isSafeRedirect(url: string): boolean {
  if (typeof url !== 'string') {
    throw new TypeError(`Invalid url: ${url}`);
  }

  // Prevent open redirects using the //foo.com format (double forward slash).
  if (/^\/\//.test(url)) {
    return false;
  }

  return !/^[a-zA-Z][a-zA-Z\d+\-.]*:/.test(url);
}

And when i take a look at the login handler i can see also that we check the returnTo Param itself ->

export default function handleLoginFactory(handler: BaseHandleLogin, nextConfig: NextConfig): HandleLogin {
  return async (req, res, options = {}): Promise<void> => {
    try {
      assertReqRes(req, res);
      if (req.query.returnTo) {
        const returnTo = Array.isArray(req.query.returnTo) ? req.query.returnTo[0] : req.query.returnTo;

        if (!isSafeRedirect(returnTo)) {
          throw new Error('Invalid value provided for returnTo, must be a relative url');
        }

Correct me when im wrong ... But i can´t see some vulnerabilities

[adamjmcgrath]

Hi @dkleber89 -

With this change, visiting /api/auth/login?returnTo=%20//malicious-site.com would log the user in then redirect to malicious-site.com, because you are only checking the first character of the string and the Location http response header accepts any number of whitespace characters before the url. I'd encourage you to test this on your own branch.

I know you could change it to check for optional whitespace, but I'm wondering if you could not just change your app to not rely on full urls in the query parameter. If you really have to rely on them, I'd be interested on knowing what the use case was.

[dkleber89]

Hello @adamjmcgrath thanks for explaination.

To your suggestion to change our application / applications -> Yes for sure i can but this seems a little workaround for me ... On the one side i need to remove that Part (From System it comes with Protocol) and on the other side i need to check again against urls with protocol.

I mean this was not complicated but also not really beautiful.

UseCase:
We have multiple applications in one ecosystem and we need in the central app (my account) where we go back after register process / after registration confirming process / after connect contracts and so on. This needs also to work when the user come to identity login mask and then clicks to register on this mask to come back to the my account application to register. And it should also work with Websites that are only static cms sites they implement a "header panel" (Login/Logout Button and User Overlay) from us.

I have added the whitespace check to the isSafeRedirect Function and added another Test to check this properly.

In my oppinion it would improve the functionality of this lib and helps maybe also some other people. So its your choice if you want to merge this or not. Both ways are okay for me :-)

Please let me know what u think.

[adamjmcgrath]

Hey @dkleber89 - thanks for updating your PR and providing me with a use case.

Let me do a little more research into this and get back to you

[adamjmcgrath]

Closing in favour of #557