Auth Code w PKCE using popup mode and reponse_mode set to query

[m-mohr]

I've implemented the Implicit Flow in my SPA using the popup mode with the following options:

response_type: 'token id_token' automaticSilentRenew: true and of course I've set client_id: ..., redirect_uri: ..., authority: ... and scope: ....

This works flawlessly. Now I'd like to switch to the Authorization Code Flow with PKCE and I had assumed it's as easy as changing the response_type to code.

Unfortunately, this doesn't work.

Some debugging brought up the following error message: PopupWindow.notifyOpener: no state found in response url for the following URL: http://localhost/?state=...&scope=email+openid+https%3A%2F%2Fwww.googleapis.com%2Fauth%2Fuserinfo.email&authuser=0&prompt=consent

The difference I spot is that for implicit the fragment contains the state and for auth code it's the query. I'd assumed that this is handled automatically by the oidc-client. Looking at the oidc-client code, it seems in popups the delimiter is always # (i.e. expecting the state in the fragment) and it doesn't change to ? if the response type is code. Is this observation correct and it's a bug or is it just me and I've understood something wrong / need to set parameters differently?

Thanks in advance...

[brockallen]

The callback page (for your popup) needs to also add a response_mode config value since the code is now coming as a query param (and not a hash fragment value):

https://github.com/IdentityModel/oidc-client-js/blob/dev/samples/VanillaJS/public/code-identityserver-sample-callback.js#L3

[m-mohr]

@brockallen I did that during my tests, but it was not working either.

[m-mohr]

I tried it once again and setting response_mode to query for popups doesn't seem to have an effect. Still seems to be a bug to me.

[m-mohr]

@brockallen I've debugged the code and there's a bug in the library.

PopupWindow.js, line 112: The delimiter is always # regardless of the response_mode/type set. Hardcoding a ? makes it work.

Replacing _signinCallback in UserManager.js with

    _signinCallback(url, navigator) {
        Log.debug("UserManager._signinCallback");
        let useQuery = this._settings.response_mode === "query" || (!this._settings.response_mode && SigninRequest.isCode(this._settings.response_type));
        let delimiter = useQuery ? "?" : "#";
        return navigator.callback(url, undefined, delimiter);
    }

also works, but I'm not sure it's the right way to fix the issue. I've copied the new code from OidcClient.readSigninResponseState. I'll issue a PR although I'm not sure at all it actually is working for other cases than popups.

[brockallen]

I know this has gone stale, but now that I'm digging into this -- why are you using a hash fragment for the response_mode for code flow? Why not just use the query string?

[m-mohr]

@brockallen Phew, hard to remember, but I think I couldn't set query to be used with popups because it was hardcoded to hashes. At least that's how I understand my PR looking at it again. Is there anything wrong with the PR?

[brockallen]

I've just run thru it and both fragment and query is working in the popup. So I'm not sure what situation you were running into. Since it seems this isn't affecting you any more, I think I'm going to close. If it does still pose a problem, then feel free to reopen, ok?

Thanks.

[m-mohr]

@brockallen Wait, it's still NOT working for me, and from reading the code in December you must use different settings than me or the code has been fixed since December (looking at commits, it doesn't seem to be fixed). In popup mode with AuthCode w PKCE it's not working. The PR has the fix for it, which I'm waiting for to be merged so that I can deprecate Implicit Flow finally.

I can't re-open this issue. Do you re-open? Otherwise, I have to create a new issue.

[m-mohr]

If you have a running solution for popups with response_type set to code, I'm happy to use it. The response_mode option had no impact for me at all.

[brockallen]

Yes, it works for me with this config:

var config = {
    authority: "https://localhost:5001/",
    client_id: "js_oidc",
    redirect_uri: window.location.origin + "/callback.html",
    post_logout_redirect_uri: window.location.origin + "/index.html",

    // if we choose to use popup window instead for logins
    popup_redirect_uri: window.location.origin + "/popup.html",
    popupWindowFeatures: "menubar=yes,location=yes,toolbar=yes,width=1200,height=800,left=100,top=100;resizable=yes",

    // these two will be done dynamically from the buttons clicked, but are
    // needed if you want to use the silent_renew
    response_type: "code",
    response_mode:"fragment",
    scope: "openid profile email resource1.scope1 resource2.scope1",
...

From this sample: https://github.com/IdentityServer/IdentityServer4/tree/main/samples/Clients/src/JsOidc/wwwroot

[m-mohr]

@brockallen Thank you! I did some further investigations by comparing my implementation to your example.

1. I've used signinCallback instead of signinPopupCallback. While signinCallback is undocumented at the moment (see #1253), it seems that this is an "all-in one" solution so that you don't need to call the specific methods like signinPopupCallback.
2. I can use response_mode set to fragment and it works (yay!). With response_mode set to query it does not work for me.

What do you think?

[brockallen]

I'll have a look. I'm bothered that it seems to work here, but not for you.

[m-mohr]

@brockallen Just to clarify: I can confirm that your example works! Your example works with response_mode set to fragment. Does your setup also work with response_mode set to query? That would really be confusing...

With response_mode set to query the popup stays open with the state (as a query) in the URL. Changing to fragment makes it work without any additional change.

And last but not least (if this is important), I'm authenticating against a KeyCloak server.

[brockallen]

With response_mode set to query the popup stays open with the state (as a query) in the URL.

Which page do you set this in? The calling page, or the callback page?

[brockallen]

I debugged thru all of this. The UrlUtility is working in both cases in the code running in the popup. Do you find that the state is passed in both the query and the fragment, perhaps?

[brockallen]

Also, as a workaround -- you can always pass the section of the URL into the signinPopupCallback API yourself (either fragment or query).

I'm not willing to accept the PR until I can repo the problem myself, and I can't seem to.

[m-mohr]

Which page do you set this in? The calling page, or the callback page?

Good point, I now remember that I had this set in both in my tests months ago, but not today. So I've changed this to be set in both places again. Unfortunately, it's still only fragment which is working. Can't get query to work yet. But now that I'm back on track again, let me quickly check all cases again and report back... I'm also wondering whether this may be an issue with the identity provider?

[brockallen]

Which page do you set this in? The calling page, or the callback page?

Having debugged thru it again (it's been years since I wrote it, so I had forgotten how it all worked), it's the calling page that needs the response_mode:"fragment". The popup page just needs to call signinPopupCallback (or the newer signinCallback).

The popup page will parse the URL correctly regardless of the response_mode, unless state is in there twice for some reason.

[brockallen]

Does your setup also work with response_mode set to query

query is the default for code flow, FWIW.

[m-mohr]

Thanks for looking into it @brockallen, highly appreciated.

I've checked responses (i.e. the urls after redirect) from the IP. They look the same, except for the ? and # - so I doubt it's a problem with the IP. Also, it's not containing both query and fragment.

I'm now trying to get the oidc-client-js repo running again myself so that I can debug myself, too. Not working yet, but will continue to try. Still, only fragment is working for me, not query. I would think it should just work when changing the response_mode config...

Good to know that I don't need to pass the config to the callback page.

I'll further investigate myself and get back to you. Thanks again.

[brockallen]

I would think it should just work when changing the response_mode config...

Yes, that's all I need to do in my sample that seems to be working.

[m-mohr]

For me it's still that my PR fixes the issue. Maybe some details how you can actually try to reproduce:

1. The URL that is returned by the IP has this form http://localhost/?state=...&session_state=...&code=...
2. To the callback UserManager I'm passing the following options: { response_mode: 'query', response_type: 'code' }
3. I'm using signinCallback in the callback.
4. To the caller I'm passing the following options: {client_id: "...", redirect_uri: "http://localhost/", authority: "https://sso.vgt.vito.be/auth/realms/terrascope", scope: "openid", response_type: "code", response_mode: "query", automaticSilentRenew: true} (left the authority in so that you can check it out)
5. The error I'm getting is: PopupWindow.notifyOpener: no state found in response url
6. In UrlUtility.parseUrlFragment the delimiter for the URL above is # (see post below)
7. ~In the popup I'm running through the _signinCallback method where navigator.callback is called. The _signincallback method just passes the URL to the navigator.callback. navigator is an instance of PopupNavigator. The PopupNavigator eventually arrives in UrlUtility with no delimiter (undefined) and thus the default # is used. I'm really wondering whether you also go through these steps, because I don't see how this could ever work. There's no place that is actually checking for the query parameter. Thus my PR still makes it work for me as the delimiter gets set to ? if the response_mode is query.~ see post below for details
8. Interestingly, there's also an earlier call to parseUrlFragment, but in this case the URL (i.e. the value) is undefined and there the delimiter is ?. It seems to not originate directly from my code (i.e. the stack trace has one entry, which is the oidc library)

[m-mohr]

Step by step how the code runs:

1. signinCallback with the URL above
2. readSigninResponseState is called. Everything works well, the Promise resolves. Interestingly, in readSigninResponseState it can actually get the state (as it sets the delimiter correctly to ?)!
3. It then calls signinPopupCallback
4. There, _signinCallback is called
5. navigator.callback is called with just one parameter (the URL). No delimiter is passed, which is different to how it's done in readSigninResponseState (see 2). That's what the PR fixes.
6. PopupNavigator.callback calls PopupWindow.notifyOpener, which itself calls UrlUtility.parseUrlFragment
7. In UrlUtility.parseUrlFragment the parameters are: value = URL from above, delimiter = # (the delimiter passed was undefined, thus the default value # is used)
8. Thus, no state is found in the URL and PopupWindow.notifyOpener logs the warning: PopupWindow.notifyOpener: no state found in response url to the callback window

Does this help to reproduce, @brockallen ? What is different on your side?

[brockallen]

What's the full redirect URL in the popup?

[m-mohr]

http://localhost/?state=...&session_state=...&code=... with ... being strings containing alphanumeric characters including some hyphens (-). Not sure which of the parameters I can securely share so I have removed the query values. If it's important to have the real URL, I guess I could send it to the gmail adress that is listed in your profile?

[brockallen]

Ok, I see the issue. It works if state is not the first param. I"ll see what the best fix is. As for the PR, that code might make it work properly, but it's now the parsing logic in two places, so I don't think it's the best place for the fix. I'll see... maybe it is the best. Having the settings in two places might just be how it needs to be done.

[brockallen]

Ok, yea, after having gone back into it -- I agree. This is the best way to address it. Thanks for sticking with it and helping me understand the problem.

[m-mohr]

Thank you for the release and also in general the great library.