Skip to content
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

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

Closed
john-adatree opened this issue Jul 20, 2022 · 5 comments

Comments

@john-adatree
Copy link

john-adatree commented Jul 20, 2022

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
Copy link
Member

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
Copy link
Author

john-adatree commented Jul 21, 2022

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants