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

WebAuthenticator not working with Mozilla Firefox #117

Closed
shv07 opened this issue Apr 10, 2023 · 7 comments
Closed

WebAuthenticator not working with Mozilla Firefox #117

shv07 opened this issue Apr 10, 2023 · 7 comments
Labels
bug Something isn't working External bug Bugs caused by other dependencies

Comments

@shv07
Copy link

shv07 commented Apr 10, 2023

Hi,

I am using your WebAuthenticator for authenticating a MAUI windows app. The authenticator works correctly with edge and chrome browsers. But when I set Mozilla as default browser, the browser opens, accepts login credentials, brings the app in the foreground but the control is never returned to the MAUI app.

On Debugging the WebAuthenticator.cs I found this:-

   // WebAuthenticator.cs
    tasks.Add(taskId, tcs);
    var uri = await tcs.Task.ConfigureAwait(false);

Here, the control never returns to the last line after the second last is executed.

Using with Windows 11 and Firefox 111.0.1 (64-bit).

@dotMorten dotMorten added bug Something isn't working help wanted Extra attention is needed labels Apr 13, 2023
@benny-ayote
Copy link

I experienced the same issue. The quotation marks expected in the JSON string value of the state parameter are being removed by Firefox. This did not occur in Chrome.

Firefox:
https://[URL]/auth?redirect=myapp://&state={appInstanceId:",signinId:6fc43c2c-7df7-443d-ba11-4e37e4aa9a4b}

Chrome:
https://[URL]/auth?redirect=myapp://&state={"appInstanceId":"","signinId":"2739798f-d690-4426-abcf-4f1a9dce3ea6"}

As a result, the call to JsonObject.Parse(state) throws an exception due to the improperly formed JSON.

// WebAuthenticator.cs
var jsonObject = System.Text.Json.Nodes.JsonObject.Parse(state) as JsonObject;
if (jsonObject is not null)
...

I was able to resolve by changing this:

query["state"] = stateJson.ToJsonString();

to this:

query["state"] = Uri.EscapeDataString(stateJson.ToJsonString());

@dotMorten
Copy link
Owner

Thanks that's a good find. However the datastring should already be getting encoded, so I don't really get this. This smells like a firefox bug

@oliverholliday
Copy link

oliverholliday commented May 15, 2023

I'm also getting this in MAUI.

I had to register a key otherwise the state appInstance was always a blank string.

Microsoft.Windows.AppLifecycle.AppInstance.FindOrRegisterForKey("hello");

I think maybe this is a bug? The CheckOAuthRedirectionActivation method has code to set that string, but it returns early if the app activation wasn't by protocol handler.

Even after that, with Firefox as default browser the JSON state is coming back with unquoted strings. Presumably Firefox is stripping them somehow.

{appInstanceId:hello,signinId:03a942fd-4e70-4370-8675-881f9b453c41,state:9M3rdrjDoasqASFXF10_Ig}

With the following patches applied the authentication completes:

image

@RobinS-S
Copy link

@dotMorten are you still maintaining this project? This bug is still relevant in the workaround for MAUI's WebAuthenticator in Windows, since the SDK still doesn't handle this properly. If this is fixed, this library can be used as a universal fix for Windows WebAuthenticator.

As far as I can see, the code @shv07 wrote fixes it. However, I would like to suggest another fix: when passing state in the original URL, it does not respect this state and overwrites it. I've changed the code a little bit to restore the original state in the query parameter.

Is his code OK and would you consider merging it? If you do not, I can fork and PR my changes (the same bug fixed and restores original state). What requirements would you have for merging that fix?
image

@dotMorten
Copy link
Owner

dotMorten commented Jun 25, 2023

are you still maintaining this project?

??? That last release was merely a month ago.
I work on it when I have the time and resources. Installing and working around a bug in Firefox hasn't been a huge priority right now. Considering this is a firefox-specific issue, did anyone open an issue with the Firefox people? I'm quite worried workarounds like this will break other browsers, and double-encoding things just feels wrong, and not like the correct fix.

@dotMorten dotMorten added the External bug Bugs caused by other dependencies label Jun 25, 2023
@RobinS-S
Copy link

are you still maintaining this project?

??? That last release was merely a month ago. I work in it when I have the time and resources. Installing and working around a bug in Firefox hasn't been a huge priority right now. Considering this is a firefox-specific issue, did anyone open an issue with the Firefox people? I'm quite worried workarounds like this will break other browsers.

I didn't mean to offend you or say you don't spend enough time on it. Sorry if you understood that from it, I only looked at the last commit.

No, I did not open an issue with Firefox, I'm not very familiar with their bug reporting process and don't fully understand the bug. I've tested with Chrome and Edge, my fix doesn't seem to break those browsers, but it does add unnecessary double encoding. I will try to see if I can reproduce the bug and report it at Firefox.

I understand if you don't want to fix this on your side, for now I think it's easier to use a WebView because even when using this package as a workaround, it doesn't work for all browsers.

Thank you for your work on the package regardless!

@RobinS-S
Copy link

RobinS-S commented Jun 25, 2023

@dotMorten @shv07 @oliverholliday I've tested it out a bit, and found out it is indeed a Firefox problem. Fixing this in your code would be nice to get something working fast, but I agree they should fix it. Reported it at Bugzilla.

@dotMorten dotMorten removed the help wanted Extra attention is needed label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working External bug Bugs caused by other dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants