Permissions.query should return 'prompt' for unique origins #3575

youennf · 2022-08-23T12:04:12Z

`fde28e0`

Permissions.query should return 'prompt' for unique origins
https://bugs.webkit.org/show_bug.cgi?id=244246
rdar://98990618

Reviewed by Chris Dumez.

Unique origins do not have a host so it is not useful to call the UI delegate to get the permission.
Instead, we do as if the delegate returns prompt.

Coverdd by added API test.

* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::isOriginUnique):
(WebKit::WebPageProxy::queryPermission):
* Tools/TestWebKitAPI/SourcesCocoa.txt:
* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/PermissionsAPI.mm: Added.
(-[PermissionsAPIMessageHandler userContentController:didReceiveScriptMessage:]):
(-[PermissionsAPIUIDelegate _webView:queryPermission:forOrigin:completionHandler:]):
(TestWebKitAPI::urlEncodeIfNeeded):
(TestWebKitAPI::TEST):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/UserContentController.mm:

Canonical link: https://commits.webkit.org/253785@main

8120f61

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-08-23T12:04:18Z

EWS run on previous version of this PR (hash d176605)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	❌ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

cdumez

This doesn't look great. How do we end up with a security origin data that has data members that are empty but not null?
Maybe we should fix that instead?
Having 2 states for security origin data (null & empty) adds complexity and I don't understand why we'd need both (and why we'd need to differentiate them)

Tools/TestWebKitAPI/Tests/WebKitCocoa/PermissionsAPI.mm

geoffreygaren · 2022-08-24T17:13:28Z

I kind of agree with Chris: What is the null security origin, and how does it differ from the empty security origin? At first glance the distinction is super subtle -- and subtle is not good for security!

cdumez · 2022-08-24T17:16:35Z

I kind of agree with Chris: What is the null security origin, and how does it differ from the empty security origin? At first glance the distinction is super subtle -- and subtle is not good for security!

My proposal offline was to:

Do a SecurityOrigin::isUnique() check in WebCore (Permissions::query()) and resolve the promise then
Add a MESSAGE_CHECK() in the UIProcess to make sure the SecurityOriginData is safe for the UIProcess to deal with (i.e. make sure the WebProcess cannot cause the UIProcess to crash with a bad IPC).

youennf · 2022-08-25T07:49:15Z

I kind of agree with Chris: What is the null security origin, and how does it differ from the empty security origin?

Right, this is probably too subtle.
An empty security origin data is derived from a unique security origin.
A null security origin is unexpected and considered invalid.

The patch should probably have added a isUnique instead of isNullOrEmpty.

My proposal offline was to:

Do a SecurityOrigin::isUnique() check in WebCore (Permissions::query()) and resolve the promise then

Add a MESSAGE_CHECK() in the UIProcess to make sure the SecurityOriginData is safe for the UIProcess to deal with (i.e. make sure the WebProcess cannot cause the UIProcess to crash with a bad IPC).

I think a check in UIProcess better integrates with how we handle permission computation.
I'll make it a more specific patch.

webkit-early-warning-system · 2022-08-25T07:53:06Z

EWS run on previous version of this PR (hash 4ea0d4c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 api-mac	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk1	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2022-08-25T09:47:29Z

8120f61

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
✅ 🛠 🧪 merge	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

cdumez · 2022-08-25T15:00:53Z

Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -8840,6 +8840,12 @@ void WebPageProxy::revokeGeolocationAuthorizationToken(const String& authorizati
    m_geolocationPermissionRequestManager.revokeAuthorizationToken(authorizationToken);
 }

+static bool isOriginUnique(const SecurityOriginData& origin)


Like I mentioned in my previous review, I think we should do the isUnique() check in WebCore's query(), on a SecurityOrigin.

Relying on data members of a SecurityOriginData to be empty (but not null) to determine that the origin is unique is a bit obscure and fragile.

The UIProcess side check can then be a MESSAGE_CHECK and this function can be name something like isValidOrigin().

Besides clarity, another reason for my proposal is that there is no good reason to do IPC or involve the UIProcess for unique/opaque origins. It is more efficient to deal with them in the WebProcess.

I think it is best to return 'denied' if possible (say application does not have the camera entitlement), or reject with NotSupported (say permission name is background-fetch).
This is best done in UIProcess.

I don't understand what that means, you are returning Prompt unconditionally so...

Also, whatever decisions about entitlements or not supporting a particular API should be doable at WebProcess-level if we wanted to.

There are cases where WebPageProxy::queryPermission is called but delegate is not called at all (leading to returning denied or rejecting with NotSupportedError depending on the permission name and/or app entitlements).
There are cases where even though delegate returns prompt, we will IPC grant to the WebProcess (camera and microphone).

This is existing logic in WebPageProxy that seems good to keep consistent.
All this patch does is, instead of calling delegate, we make it as if delegate returns Prompt for unique origins.

We also have plenty of checks in the WebProcess that resolve/reject the promise early in WebCore's query().

Also, we normally don't pass opaque origins to other processes since they are only valid within a single process (requires pointer identity). This is why we have a concept of unique/opaque SecurityOrigin but we don't have one for SecurityOriginData. Relying of SecurityOriginData having non-null / empty data members to make such determination is not robust or clear.

cdumez

LGTM but please add isUnique() member function to SecurityOriginData.

cdumez · 2022-08-25T16:40:44Z

Source/WebKit/UIProcess/WebPageProxy.cpp

-    });
+    };
+
+    if (isOriginUnique(clientOrigin.topOrigin)) {


Ok, I discussed it offline with Youenn and it sounds like we want to deal with these unique origins on the UIProcess side because we don't always return "Prompt" for these, unlike what the next line makes it look like. There is some logic above in this function that could cause us to return Granted or Denied for unique origins and we wouldn't want to duplicate that logic in the WebProcess.

I think we should add a isUnique() member function to SecurityOriginData (which makes sure that the members are empty and non-null) instead of this new isOriginUnique() free function. It might be useful in other places as it sounds like we sometimes want to determine in other processes than the WebProcess if an origin is unique (even though we cannot at the moment determine if 2 unique security origins are the same).

Agreed, I think we should change isEmpty to isNull and add isUnique. Will do it in a follow-up.

webkit-commit-queue · 2022-08-25T19:11:51Z

Committed 253785@main (fde28e0): https://commits.webkit.org/253785@main

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

youennf requested a review from cdumez as a code owner August 23, 2022 12:04

youennf self-assigned this Aug 23, 2022

youennf added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels Aug 23, 2022

youennf requested review from szewai and achristensen07 August 23, 2022 12:20

cdumez reviewed Aug 24, 2022

View reviewed changes

szewai reviewed Aug 24, 2022

View reviewed changes

Tools/TestWebKitAPI/Tests/WebKitCocoa/PermissionsAPI.mm Outdated Show resolved Hide resolved

youennf force-pushed the eng/Permissions-query-should-return-prompt-for-unique-origins branch from d176605 to 4ea0d4c Compare August 25, 2022 07:53

youennf force-pushed the eng/Permissions-query-should-return-prompt-for-unique-origins branch from 4ea0d4c to 8120f61 Compare August 25, 2022 09:45

youennf requested review from cdumez and szewai August 25, 2022 14:53

cdumez requested changes Aug 25, 2022

View reviewed changes

cdumez approved these changes Aug 25, 2022

View reviewed changes

youennf added the merge-queue Applied to send a pull request to merge-queue label Aug 25, 2022

webkit-early-warning-system merged commit fde28e0 into WebKit:main Aug 25, 2022

webkit-early-warning-system force-pushed the eng/Permissions-query-should-return-prompt-for-unique-origins branch from 8120f61 to fde28e0 Compare August 25, 2022 19:11

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissions.query should return 'prompt' for unique origins #3575

Permissions.query should return 'prompt' for unique origins #3575

youennf commented Aug 23, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Aug 23, 2022 •

edited

cdumez left a comment •

edited

geoffreygaren commented Aug 24, 2022

cdumez commented Aug 24, 2022 •

edited

youennf commented Aug 25, 2022

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

cdumez Aug 25, 2022

cdumez Aug 25, 2022

cdumez Aug 25, 2022

youennf Aug 25, 2022

cdumez Aug 25, 2022

cdumez Aug 25, 2022

youennf Aug 25, 2022 •

edited

cdumez Aug 25, 2022

cdumez Aug 25, 2022

cdumez left a comment

cdumez Aug 25, 2022

youennf Aug 25, 2022

webkit-commit-queue commented Aug 25, 2022

Permissions.query should return 'prompt' for unique origins #3575

Permissions.query should return 'prompt' for unique origins #3575

Conversation

youennf commented Aug 23, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Aug 23, 2022 • edited

cdumez left a comment • edited

Choose a reason for hiding this comment

geoffreygaren commented Aug 24, 2022

cdumez commented Aug 24, 2022 • edited

youennf commented Aug 25, 2022

webkit-early-warning-system commented Aug 25, 2022 • edited

webkit-early-warning-system commented Aug 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youennf Aug 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdumez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-commit-queue commented Aug 25, 2022

youennf commented Aug 23, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Aug 23, 2022 •

edited

cdumez left a comment •

edited

cdumez commented Aug 24, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

webkit-early-warning-system commented Aug 25, 2022 •

edited

youennf Aug 25, 2022 •

edited