bjerkio / oidc-react

React component to provide OpenID Connect and OAuth2 protocol support. 🌳
MIT License
328 stars 69 forks source link

hasCodeInUrl checks both hashParams and searchParams even though the OIDC Auth is configured for just one of them. #864

Closed john-adatree closed 3 weeks ago

john-adatree commented 2 years ago

Hi

I am hoping you can help with this issue

Using "oidc-react": "^1.5.1",

Please note my Authentication callback is configured to use the query delimiter '?'

In my app, I have a non-authentication callback from a third party to a page which includes the hash parameter 'code' example 'myapp.com/mypage#code=12345'

This page is wrapped in the oidc-react AuthProvider component. And because of that, the following code is executed

  useEffect(() => {
    const getUser = async (): Promise<void> => {
      /**
       * Check if the user is returning back from OIDC.
       */
      if (hasCodeInUrl(location)) {
        const user = await userManager.signinCallback();
        setUserData(user);
        setIsLoading(false);
        onSignIn && onSignIn(user);
        return;
      }
      ...

https://github.com/bjerkio/oidc-react/blob/main/src/AuthContext.tsx#L109

The hasCodeInUrl function sees the hash code parameter in the URL and returns true.

The issue is userManager.signinCallback(); will call readSigninResponseState and it checks to see what delimiter to use and it correctly chooses '?' because this is what is configured but since this a non-authentication callback and it is not using the '?' delimiter, it will always throw an error Error: No state in response

Possible solution

hasCodeInUrl should not blindly look for hashParams 'code' OR searchParams 'code' but instead should check which delimiter has been configured, '?' or '#' and just check for parameters using that delimiter.

Thanks John

simenandre commented 2 years ago

Hello 👋

Thanks for reaching out! You are definitely right, I haven't thought to much about this. Do you have an idea to fix it?

I'm not sure we should introduce the callback component as many others have, since it produces more boilerplate. What do you think? Maybe add the option to use a component like that, and/or disable hadCodeInUrl function?

john-adatree commented 2 years ago

Since we have access to the userManager.settings

I would just use the same check oidc-client-js uses https://github.com/IdentityModel/oidc-client-js/blob/dev/src/OidcClient.js#L103 and https://github.com/IdentityModel/oidc-client-js/blob/dev/src/SigninRequest.js#L96

 const isCode = (response_type) => {
    var result = response_type.split(/\s+/g).filter(function (item) {
      return item === 'code';
    });
    return !!result[0];
  };

  const getDelimiter = (settings: UserManagerSettings): string => {
    let useQuery = settings.response_mode === 'query' || (!settings.response_mode && isCode(settings.response_type));

    let delimiter = useQuery ? '?' : '#';
    return delimiter;
  };

  const isOidcCallback = (location: Location, settings: UserManagerSettings): boolean => {
    let delimiter = getDelimiter(settings);

    if (delimiter === '?') {
      const searchParams = new URLSearchParams(location.search);
      return Boolean(searchParams.get('code') || searchParams.get('id_token') || searchParams.get('session_state'));
    } else {
      const hashParams = new URLSearchParams(location.hash.replace('#', '?'));
      return Boolean(hashParams.get('code') || hashParams.get('id_token') || hashParams.get('session_state'));
    }
  };
john-adatree commented 2 years ago

We also have the redirect_uri in the UserManagerSettings, is there a reason we do not check the path to confirm it is the correct OIDC callback location?

simenandre commented 2 years ago

We also have the redirect_uri in the UserManagerSettings, is there a reason we do not check the path to confirm it is the correct OIDC callback location?

Nice, I haven't thought about that. We can check against that! Do you want to open a pull request on this?

simenandre commented 3 weeks ago

Closing this as stale. Please open a new issue or let me know you still have this issue in a comment 👍