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

PopUpSOAuthorization::initSecretWebView modifies parent web view configuration #1190

Conversation

brentfulgham
Copy link
Contributor

@brentfulgham brentfulgham commented May 31, 2022

82e4b15

PopUpSOAuthorization::initSecretWebView modifies parent web view configuration
https://bugs.webkit.org/show_bug.cgi?id=241155
<rdar://94176551 >

Reviewed by Chris Dumez.

The AppSSO flows that create a new WKWebView pass through the method
'PopUpSOAuthorizationSession::initSecretWebView'. This conducts SSO flows in
an invisible Window for cases where other UI handles the actual authentication,
but a web view is needed to handle server interactions. It deactivates AppSSO
in the hidden view so that normal server authentication can happen without AppSSO
being triggered a second time.

This method made the common mistake of assuming that performing 'copy' on the
configuration member of the paren't WKWebView yielded a deep copy that could bei
manipulated to control the invisible view independently of the parent view. While
the method correctly disabled AppSSO for the hidden view, it also deactivated it
for the parent view.

This bug could lead to cases where someone who mistakenly terminated an AppSSO flow
would be unable to start the process a second time, as the view would now be
configured to block access to AppSSO authentication.

This patch corrects that bug.

Tested by SOAuthorizationPopUp.InterceptionSucceedTwice.

* Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:
(WebKit::PopUpSOAuthorizationSession::initSecretWebView):

Canonical link: https://commits.webkit.org/251163@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295068 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there no test? Change log says it is already covered by a test but that's only true if that test is currently failing on the bots (not sure if that's the case).

@brentfulgham
Copy link
Contributor Author

@cdumez That test is currently a timeout on the bots, and is failing at the moment. This is a fix for that new timeout.

Previously, the test passed because there was always an SOAuthenticationCoordinator object (even if AppSSO was disabled), so it could check permission to do AppSSO.

After our change to make it lazy, it started crashing because this flow didn't expect a nullptr SOAuthenticatorCoordinator.

I fixed that crash by checking if AppSSO was enabled, but that change revealed this long-standing bug where Jiewen was manipulating the AppSSO state of all web views in the process.

I suspect this was the cause of some subtle errors some people reported where AppleConnect would stop working for mysterious reasons when they tried (and cancelled) an AppSSO login.

@brentfulgham brentfulgham added the merge-queue Applied to send a pull request to merge-queue label May 31, 2022
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/PopUpSOAuthorizationSessioninitSecretWebView-performs-a-shallow-copy-leading-to-manipulation-of-parent-view-configuration branch from 2bcb204 to 82e4b15 Compare May 31, 2022 22:09
@webkit-early-warning-system webkit-early-warning-system merged commit 82e4b15 into WebKit:main May 31, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r295068 (251163@main): https://commits.webkit.org/251163@main

Reviewed commits have been landed. Closing PR #1190 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants