dotMorten / WinUIEx

WinUI Extensions
https://dotmorten.github.io/WinUIEx
MIT License
572 stars 36 forks source link

WebAuthenticator not working with Mozilla Firefox #117

Closed shv07 closed 9 months ago

shv07 commented 1 year ago

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).

benny-ayote commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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.

RobinS-S commented 1 year ago

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 commented 1 year ago

@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.