Bug 238262 - REGRESSION(r286590): Links with URL schemes are not clickable in Mail
Summary: REGRESSION(r286590): Links with URL schemes are not clickable in Mail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on: 238839
Blocks:
  Show dependency treegraph
 
Reported: 2022-03-23 08:20 PDT by Per Arne Vollan
Modified: 2022-04-08 15:02 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.04 KB, patch)
2022-03-23 08:24 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.03 KB, patch)
2022-03-23 10:55 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2022-03-24 08:58 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2022-03-23 08:20:26 PDT
Links with URL schemes are not clickable in Mail on iOS.
Comment 1 Per Arne Vollan 2022-03-23 08:21:32 PDT
<rdar://89145552>
Comment 2 Per Arne Vollan 2022-03-23 08:24:15 PDT
Created attachment 455501 [details]
Patch
Comment 3 Per Arne Vollan 2022-03-23 10:55:20 PDT
Created attachment 455519 [details]
Patch
Comment 4 Geoffrey Garen 2022-03-23 11:02:33 PDT
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?
Comment 5 Per Arne Vollan 2022-03-23 12:00:54 PDT
(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!
Comment 6 Geoffrey Garen 2022-03-23 14:08:42 PDT
> 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?
Comment 7 Per Arne Vollan 2022-03-23 15:16:55 PDT
(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!
Comment 8 Per Arne Vollan 2022-03-24 08:58:45 PDT
Created attachment 455645 [details]
Patch
Comment 9 Per Arne Vollan 2022-03-24 09:01:25 PDT
(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!
Comment 10 Geoffrey Garen 2022-03-24 10:28:01 PDT
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.
Comment 11 Per Arne Vollan 2022-03-24 14:17:34 PDT
(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!
Comment 12 EWS 2022-03-24 16:15:25 PDT
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].
Comment 13 WebKit Commit Bot 2022-04-05 14:47:08 PDT
Re-opened since this is blocked by bug 238839
Comment 14 Per Arne Vollan 2022-04-08 15:02:56 PDT
This was re-landed in https://trac.webkit.org/changeset/292632/webkit.