REGRESSION(r286590): Links with URL schemes are not clickable in Mail

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Links with URL schemes are not clickable in Mail on iOS.

[Per Arne Vollan]

<rdar://89145552>

[Per Arne Vollan]

Created attachment 455501 [details]
Patch

[Per Arne Vollan]

Created attachment 455519 [details]
Patch

[Geoffrey Garen]

Comment on attachment 455519 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:422
> +    if (!WebCore::IOSApplication::isMobileSafari())
> +        parameters.dynamicMachExtensionHandles = SandboxExtension::createHandlesForMachLookup(nonBrowserServices(), std::nullopt);

How do we know that Safari does not want / need to launch apps by URL scheme?

What about other browsers?

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 455519 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455519&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:422
> > +    if (!WebCore::IOSApplication::isMobileSafari())
> > +        parameters.dynamicMachExtensionHandles = SandboxExtension::createHandlesForMachLookup(nonBrowserServices(), std::nullopt);
> 
> How do we know that Safari does not want / need to launch apps by URL scheme?
> 

That is a good point. We have not seen any telemetry for Safari accessing this daemon. We have also been blocking it for the last ~2 years, so I do not believe it is being used by Safari.

> What about other browsers?

That is also a good point! This patch grants access to the service for other browsers.  It would be good if we could avoid that. I will look into that.

Thanks for reviewing!

[Geoffrey Garen]

> We have not seen any telemetry for Safari accessing
> this daemon.

I wonder if there is some preference that Safari sets (or that Mail un-sets) that influences whether we consult lsd for links or not? If so, perhaps we can key off of that preference rather than isMobileSafari(), providing the sandbox win to all apps that have the preference in that state.

> We have also been blocking it for the last ~2 years, so I do
> not believe it is being used by Safari.

Blocking in all apps?

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #6)
> > We have not seen any telemetry for Safari accessing
> > this daemon.
> 
> I wonder if there is some preference that Safari sets (or that Mail un-sets)
> that influences whether we consult lsd for links or not? If so, perhaps we
> can key off of that preference rather than isMobileSafari(), providing the
> sandbox win to all apps that have the preference in that state.
> 

Yes, that is a good point. I will look into this!

> > We have also been blocking it for the last ~2 years, so I do
> > not believe it is being used by Safari.
> 
> Blocking in all apps?

We have only been blocking in Safari.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 455645 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #7)
> (In reply to Geoffrey Garen from comment #6)
> > > We have not seen any telemetry for Safari accessing
> > > this daemon.
> > 
> > I wonder if there is some preference that Safari sets (or that Mail un-sets)
> > that influences whether we consult lsd for links or not? If so, perhaps we
> > can key off of that preference rather than isMobileSafari(), providing the
> > sandbox win to all apps that have the preference in that state.
> > 
> 
> Yes, that is a good point. I will look into this!
> 

I was not able to find such a preference, but in the latest patch I made sure that no browsers will get this sandbox extension. This will provide the sandbox win for all browsers.

> > > We have also been blocking it for the last ~2 years, so I do
> > > not believe it is being used by Safari.
> > 
> > Blocking in all apps?
> 
> We have only been blocking in Safari.
> 
> Thanks for reviewing!

[Geoffrey Garen]

Comment on attachment 455645 [details]
Patch

r=me

I think this is an OK compromise for the time being; and please also file a follow-up bug to investigate how to remove lsd for all clients, since there is some security value left over there.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 455645 [details]
> Patch
> 
> r=me
> 
> I think this is an OK compromise for the time being; and please also file a
> follow-up bug to investigate how to remove lsd for all clients, since there
> is some security value left over there.

I will file a follow-up bug.

Thanks for reviewing!

[EWS]

Committed r291821 (248846@main): <https://commits.webkit.org/248846@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 238839

[Per Arne Vollan]

This was re-landed in https://trac.webkit.org/changeset/292632/webkit.