Bug 220897 – [WebAuthn] Allow one user gesture free prompt for each navigation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Allow one user gesture free prompt for every navigation.

[Radar WebKit Bug Importer]

<rdar://problem/73541303>

[Jiewen Tan]

Created attachment 418233 [details]
Patch

[Sam Weinig]

Why would we want to allow this without a user gesture?

[Jiewen Tan]

(In reply to Sam Weinig from comment #3)
> Why would we want to allow this without a user gesture?

The spec doesn't suggest it needs a user gesture. We think for any UI that can be triggered by scripts will be safer with some API rate limiter. Therefore, we ask for user gesture. However, most of the old sites don't expect that. This relaxation is more or less a quirk or compromise for those early adopters.

[Jiewen Tan]

Created attachment 418369 [details]
Patch

[Sam Weinig]

(In reply to Jiewen Tan from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Why would we want to allow this without a user gesture?
> 
> The spec doesn't suggest it needs a user gesture. We think for any UI that
> can be triggered by scripts will be safer with some API rate limiter.
> Therefore, we ask for user gesture. However, most of the old sites don't
> expect that. This relaxation is more or less a quirk or compromise for those
> early adopters.

The future is far greater than the past. I can't think of why it would ever be ok to to prompt for this without a user gesture, so I would recommend we just always require it and get early adopters to update their sites or add site specific quirks if needed.

[Jiewen Tan]

Created attachment 418495 [details]
Patch

[Jiewen Tan]

(In reply to Sam Weinig from comment #6)
> (In reply to Jiewen Tan from comment #4)
> > (In reply to Sam Weinig from comment #3)
> > > Why would we want to allow this without a user gesture?
> > 
> > The spec doesn't suggest it needs a user gesture. We think for any UI that
> > can be triggered by scripts will be safer with some API rate limiter.
> > Therefore, we ask for user gesture. However, most of the old sites don't
> > expect that. This relaxation is more or less a quirk or compromise for those
> > early adopters.
> 
> The future is far greater than the past. I can't think of why it would ever
> be ok to to prompt for this without a user gesture, so I would recommend we
> just always require it and get early adopters to update their sites or add
> site specific quirks if needed.

Some really large early adopters have refused to update their sites or have implied it will take years to update their sites. Since there is a number of those websites, this approach seems a bit better than creating quirks for each of them.

[Sam Weinig]

I completely disagree. Creating a bad user experience for our users forever because a few sites refuse to change now does not seem like the right tradeoff here.

[Jiewen Tan]

(In reply to Sam Weinig from comment #9)
> I completely disagree. Creating a bad user experience for our users forever
> because a few sites refuse to change now does not seem like the right
> tradeoff here.

This patch is not just about compromise. It also improves the situation a bit by requiring user gestures for security keys as well. Before we only do that for the platform authenticator.

[Brent Fulgham]

(In reply to Sam Weinig from comment #9)
> I completely disagree. Creating a bad user experience for our users forever
> because a few sites refuse to change now does not seem like the right
> tradeoff here.

Jiewen's change is more about making WebKit behave like Chrome does, which is (sadly) what most web developers have standardized around. I think our description of "a few large sites" is misleading. Literally zero sites work with our current flow, because developers expect to be able to trigger WebAuthn without user gesture as part of the sign-in flow.

As an example, Microsoft's login page asks the user if they wish to sign in. Once the user agrees, they navigate to their authentication origin (I can't remember the exact eTLD's right now) and attempt to trigger WebAuthn when they hit that site. When WebKit does this, the call fails and users can't log in.

Google properties and Facebook work similarly, and we received feedback through various channels from other web developers that requested the behavior we are trying to implement here.

The current plan is also based on discussion with other browser implementors who point out that the specification does not speak to user gesture in this case, and that WebKit is the odd person out.

Jiewen and I argued your side of this for a long time before being (grudgingly) convinced that this was the best option for our users.

Can you suggest another approach that would allow us to preserve the user gesture requirement while being compatible with all of these large web service providers that are designed to work without a gesture? I would be very happy to come up with a way to do so!

[Brent Fulgham]

Comment on attachment 418495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418495&action=review

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:386
> +        transports.clear();

So this change requires user gesture for any transport now? I think that's a good thing.

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
> +    m_requireUserGesture = true;

It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:92
> +    m_requireUserGesture = true;

Having to remember to set this manually seems wrong. It would be better if we just had a method 'processingUserGesture()' that would return the answer and do the book-keeping for us.

[Sam Weinig]

(In reply to Brent Fulgham from comment #11)
> (In reply to Sam Weinig from comment #9)
> > I completely disagree. Creating a bad user experience for our users forever
> > because a few sites refuse to change now does not seem like the right
> > tradeoff here.
> 
> Can you suggest another approach that would allow us to preserve the user
> gesture requirement while being compatible with all of these large web
> service providers that are designed to work without a gesture? I would be
> very happy to come up with a way to do so!

Sure. Make always requiring a user gesture the default and add temporary quirks for the existing providers and let them know that we plan to remove the quirks at some specific time in the future.

[Jiewen Tan]

Comment on attachment 418495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418495&action=review

Thanks Brent for reviewing this patch.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:386
>> +        transports.clear();
> 
> So this change requires user gesture for any transport now? I think that's a good thing.

Yes, that's right. Since we now offer this free prompt quirk, it won't be too much a compatible problem to require user gestures for all.

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
>> +    m_requireUserGesture = true;
> 
> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.

Sure, will do that.

>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:92
>> +    m_requireUserGesture = true;
> 
> Having to remember to set this manually seems wrong. It would be better if we just had a method 'processingUserGesture()' that would return the answer and do the book-keeping for us.

Sure.

[Brent Fulgham]

Comment on attachment 418495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418495&action=review

>>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
>>> +    m_requireUserGesture = true;
>> 
>> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.
> 
> Sure, will do that.

How about:

auto processingUserGesture = UserGestureIndicator::processingUserGestureForMedia();
if (!processingUserGesture) {
    if (<<If This site Needs Quirk>>)
        processingUserGesture |= m_requireUserGesture;
}
m_requireUserGesture = true;

[Sam Weinig]

(In reply to Brent Fulgham from comment #15)
> Comment on attachment 418495 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418495&action=review
> 
> >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
> >>> +    m_requireUserGesture = true;
> >> 
> >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.
> > 
> > Sure, will do that.
> 
> How about:
> 
> auto processingUserGesture =
> UserGestureIndicator::processingUserGestureForMedia();
> if (!processingUserGesture) {
>     if (<<If This site Needs Quirk>>)
>         processingUserGesture |= m_requireUserGesture;
> }
> m_requireUserGesture = true;

I may just not remember the naming here, but why "processingUserGestureForMedia"? Specifically the "Media" part seems odd.

[Brent Fulgham]

(In reply to Sam Weinig from comment #16)
> (In reply to Brent Fulgham from comment #15)
> > Comment on attachment 418495 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=418495&action=review
> > 
> > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
> > >>> +    m_requireUserGesture = true;
> > >> 
> > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.
> > > 
> > > Sure, will do that.
> > 
> > How about:
> > 
> > auto processingUserGesture =
> > UserGestureIndicator::processingUserGestureForMedia();
> > if (!processingUserGesture) {
> >     if (<<If This site Needs Quirk>>)
> >         processingUserGesture |= m_requireUserGesture;
> > }
> > m_requireUserGesture = true;
> 
> I may just not remember the naming here, but why
> "processingUserGestureForMedia"? Specifically the "Media" part seems odd.

This particular method was originally created to support media (I think maybe related to autoplay behavior). We should probably rename it now that we've found a non-media reason for it to exist.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #17)
> (In reply to Sam Weinig from comment #16)
> > (In reply to Brent Fulgham from comment #15)
> > > Comment on attachment 418495 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review
> > > 
> > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
> > > >>> +    m_requireUserGesture = true;
> > > >> 
> > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.
> > > > 
> > > > Sure, will do that.
> > > 
> > > How about:
> > > 
> > > auto processingUserGesture =
> > > UserGestureIndicator::processingUserGestureForMedia();
> > > if (!processingUserGesture) {
> > >     if (<<If This site Needs Quirk>>)
> > >         processingUserGesture |= m_requireUserGesture;
> > > }
> > > m_requireUserGesture = true;
> > 
> > I may just not remember the naming here, but why
> > "processingUserGestureForMedia"? Specifically the "Media" part seems odd.
> 
> This particular method was originally created to support media (I think
> maybe related to autoplay behavior). We should probably rename it now that
> we've found a non-media reason for it to exist.

I filed Bug 221232 to consider better naming.

[Sam Weinig]

(In reply to Brent Fulgham from comment #17)
> (In reply to Sam Weinig from comment #16)
> > (In reply to Brent Fulgham from comment #15)
> > > Comment on attachment 418495 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review
> > > 
> > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
> > > >>> +    m_requireUserGesture = true;
> > > >> 
> > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.
> > > > 
> > > > Sure, will do that.
> > > 
> > > How about:
> > > 
> > > auto processingUserGesture =
> > > UserGestureIndicator::processingUserGestureForMedia();
> > > if (!processingUserGesture) {
> > >     if (<<If This site Needs Quirk>>)
> > >         processingUserGesture |= m_requireUserGesture;
> > > }
> > > m_requireUserGesture = true;
> > 
> > I may just not remember the naming here, but why
> > "processingUserGestureForMedia"? Specifically the "Media" part seems odd.
> 
> This particular method was originally created to support media (I think
> maybe related to autoplay behavior). We should probably rename it now that
> we've found a non-media reason for it to exist.

How does it differ from processingUserGesture()? If the difference isn't that about media, what it is about?

[Jiewen Tan]

Created attachment 418948 [details]
Patch

[Jiewen Tan]

Created attachment 418949 [details]
Patch

[Jiewen Tan]

Created attachment 419034 [details]
Patch

[Jiewen Tan]

Created attachment 419050 [details]
Patch

[Jiewen Tan]

(In reply to Sam Weinig from comment #19)
> (In reply to Brent Fulgham from comment #17)
> > (In reply to Sam Weinig from comment #16)
> > > (In reply to Brent Fulgham from comment #15)
> > > > Comment on attachment 418495 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=418495&action=review
> > > > 
> > > > >>> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:71
> > > > >>> +    m_requireUserGesture = true;
> > > > >> 
> > > > >> It seems like making the above two lines into a method, which you could use here and in 'getAssertion' would be slightly better, since you would have less places to remember to set 'm_requiresUserGesture' to true.
> > > > > 
> > > > > Sure, will do that.
> > > > 
> > > > How about:
> > > > 
> > > > auto processingUserGesture =
> > > > UserGestureIndicator::processingUserGestureForMedia();
> > > > if (!processingUserGesture) {
> > > >     if (<<If This site Needs Quirk>>)
> > > >         processingUserGesture |= m_requireUserGesture;
> > > > }
> > > > m_requireUserGesture = true;
> > > 
> > > I may just not remember the naming here, but why
> > > "processingUserGestureForMedia"? Specifically the "Media" part seems odd.
> > 
> > This particular method was originally created to support media (I think
> > maybe related to autoplay behavior). We should probably rename it now that
> > we've found a non-media reason for it to exist.
> 
> How does it differ from processingUserGesture()? If the difference isn't
> that about media, what it is about?

The difference is that processingUserGestureForMedia will populate the user gesture through XHR and fetch, which is commonly used right before WebAuthn API to fetch nonce. Not sure the media use case though.

[Brent Fulgham]

Comment on attachment 419050 [details]
Patch

r=me. Thanks for switching to the tighter version (with Quirk).

[Jiewen Tan]

(In reply to Brent Fulgham from comment #25)
> Comment on attachment 419050 [details]
> Patch
> 
> r=me. Thanks for switching to the tighter version (with Quirk).

No problems. Thanks for r+ this patch.

[EWS]

Committed r272345: <https://trac.webkit.org/changeset/272345>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419050 [details].

[Tal Peretz]

That's awesome! Sounds like a great solution. Do we know what version of iOS it'll be on?

[Brent Fulgham]

(In reply to Tal Peretz from comment #28)
> That's awesome! Sounds like a great solution. Do we know what version of iOS
> it'll be on?

I believe this change shipped in iOS 14.5, and is definitely in the iOS 15 series. Note that this behavior was limited to a set of Quirked sites (i.e., only some sites were given this behavior).

Another bug tracks work to bring this behavior to all sites, though I don’t have it handy to paste here. If you search in Bugzilla you should find it.