-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Web Inspector: WebKit-internal JSContexts should not be inspectable, even if internal policies would override inspectable
#8666
Conversation
EWS run on previous version of this PR (hash 98d8881) |
98d8881
to
4468221
Compare
EWS run on previous version of this PR (hash 4468221)
|
return true; | ||
case Inspectable::No: | ||
#if PLATFORM(COCOA) | ||
static bool allowInternalSecurityPolicies = os_variant_allows_internal_security_policies("com.apple.WebInspector"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the code in this switch case be done inside of setInspectable? That way, maybe we don't need this new enum with three values, and can keep using a bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback! Halfway through responding to this suggestion I realized we have a bug that will likely require us to do this work in the getter.
There is actually a bug here currently that occurs when internal policies are being applied: the APIs for WKWebView and JSContext will return true
even for contexts that have disabled inspection at the API level, meaning the setting of inspectable
on those objects won't appear to do anything on system following an internal policy, which could lead to confusing or undesirable behavior in applications. The internal policies are meant to be invisible to clients of the libraries, but currently we are technically leaking this implementation detail.
Really what we should have is an inspectable
getter like we have today that ignores internal policies, and an extra allowsInspection
that actually takes internal policy into account, and is used by RemoteInspector* to determine the actual inspectability of the RemoteInspectionTarget
.
For what its worth, the existing bool
couldn't have been packed with any other bits anyway in this class, so the size of RemoteInspectionTarget
should remain the same moving from bool
to a uint8_t
enum.
@@ -41,6 +41,7 @@ | |||
using namespace Inspector; | |||
|
|||
static std::optional<bool> remoteInspectionEnabledByDefault = std::nullopt; | |||
static bool disablingInspectionIgnoresInternalPolicies = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's a better name is without the double negative. Maybe "inspectionFollowsInternalPolicies"? And it defaults to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a much better name for this, yes!
4468221
to
c78b5ff
Compare
EWS run on current version of this PR (hash c78b5ff)
|
c78b5ff
to
c538c37
Compare
β¦even if internal policies would override `inspectable` https://bugs.webkit.org/show_bug.cgi?id=250633 rdar://103312497 Reviewed by Saam Barati. On configurations where `inspectable` can be overriden, there are still some WebKit-internal contexts that should not be inspectable. The first are JSDOMGlobalObjects, which are already inspectable via the WKWebView they exist for by using the context picker in Web Inspector, making these JSContexts redundant and noisy. The second case is APISerializedScriptValueCocoa which creates JSContexts to help serialize values to/from JS/Cocoa. This problem did not exist before the introduction of the `inspectable` API because the default state of `inspectable` was false, which would not be overriden because the decision by the platform as to whether an application was inspectable occurred in a system daemon, which would not override the per-context `inspectable` setting. When unifying the decision logic for what is inspectable into JSC/WebKit, this use case was initially overlooked as the only platform that implements an internal policy for inspection doesn't have any symptoms of this that a user could observe due to the specific policy. However, in use for those working on machines where this policy is applied, the noise of so many JSContexts is making it difficult to sort through usefully inspectable contexts in Safari's Develop menu. This patch also fixes a minor bug where `inspectable` would return `true` for JSContexts and WKWebViews, even if inspection was disabled, when an internal policy is overriding inspection. * Source/JavaScriptCore/API/JSRemoteInspector.cpp: (JSRemoteInspectorGetInspectionFollowsInternalPolicies): (JSRemoteInspectorSetInspectionFollowsInternalPolicies): * Source/JavaScriptCore/API/JSRemoteInspector.h: - Add methods to set and get the new "followsInternalPolicies" state to be applied to new contexts as well as those that change their `inspectable` setting. * Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp: (Inspector::JSGlobalObjectInspectorController::developerExtrasEnabled const): * Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm: (Inspector::RemoteInspector::listingForInspectionTarget const): * Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp: (Inspector::RemoteInspector::listingForInspectionTarget const): * Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp: (Inspector::RemoteInspector::listingForInspectionTarget const): - Use new `allowsInspectionByPolicy` which takes into account internal policies. * Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.cpp: (Inspector::RemoteInspectionTarget::remoteControlAllowed const): (Inspector::RemoteInspectionTarget::allowsInspectionByPolicy const): (Inspector::RemoteInspectionTarget::inspectable const): (Inspector::RemoteInspectionTarget::setInspectable): (Inspector::RemoteInspectionTarget::pauseWaitingForAutomaticInspection): * Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.h: - Use the new `followsInternalPolicies` state to keep track of when a target should be exempt from internal policies for contexts that never make sense to be inspectable. - Introduce `allowsInspectionByPolicy` which takes into account internal policy when determining the inspectability of a target. Previously this was baked into `inspectable`, but that inadvertently leaks the internal policy implementation detail to clients of JSContext and WKWebView. * Source/WebCore/bindings/js/JSDOMGlobalObject.cpp: (WebCore::JSDOMGlobalObject::finishCreation): * Source/WebKit/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm: (API::SharedJSContext::ensureContext): - Adopt new methods to mark the contexts created here as never inspectable since they do not expose any useful information. Canonical link: https://commits.webkit.org/259000@main
c538c37
to
11aafb6
Compare
Committed 259000@main (11aafb6): https://commits.webkit.org/259000@main Reviewed commits have been landed. Closing PR #8666 and removing active labels. |
β¦even if internal policies would override `inspectable` https://bugs.webkit.org/show_bug.cgi?id=250633 rdar://103312497 Reviewed by Saam Barati. Relanding with availability annotation fix. Originally review in github.com/WebKit/pull/8666. On configurations where `inspectable` can be overriden, there are still some WebKit-internal contexts that should not be inspectable. The first are JSDOMGlobalObjects, which are already inspectable via the WKWebView they exist for by using the context picker in Web Inspector, making these JSContexts redundant and noisy. The second case is APISerializedScriptValueCocoa which creates JSContexts to help serialize values to/from JS/Cocoa. This problem did not exist before the introduction of the `inspectable` API because the default state of `inspectable` was false, which would not be overriden because the decision by the platform as to whether an application was inspectable occurred in a system daemon, which would not override the per-context `inspectable` setting. When unifying the decision logic for what is inspectable into JSC/WebKit, this use case was initially overlooked as the only platform that implements an internal policy for inspection doesn't have any symptoms of this that a user could observe due to the specific policy. However, in use for those working on machines where this policy is applied, the noise of so many JSContexts is making it difficult to sort through usefully inspectable contexts in Safari's Develop menu. This patch also fixes a minor bug where `inspectable` would return `true` for JSContexts and WKWebViews, even if inspection was disabled, when an internal policy is overriding inspection. * Source/JavaScriptCore/API/JSRemoteInspector.cpp: (JSRemoteInspectorGetInspectionFollowsInternalPolicies): (JSRemoteInspectorSetInspectionFollowsInternalPolicies): * Source/JavaScriptCore/API/JSRemoteInspector.h: - Add methods to set and get the new "followsInternalPolicies" state to be applied to new contexts as well as those that change their `inspectable` setting. * Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp: (Inspector::JSGlobalObjectInspectorController::developerExtrasEnabled const): * Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm: (Inspector::RemoteInspector::listingForInspectionTarget const): * Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp: (Inspector::RemoteInspector::listingForInspectionTarget const): * Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp: (Inspector::RemoteInspector::listingForInspectionTarget const): - Use new `allowsInspectionByPolicy` which takes into account internal policies. * Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.cpp: (Inspector::RemoteInspectionTarget::remoteControlAllowed const): (Inspector::RemoteInspectionTarget::allowsInspectionByPolicy const): (Inspector::RemoteInspectionTarget::inspectable const): (Inspector::RemoteInspectionTarget::setInspectable): (Inspector::RemoteInspectionTarget::pauseWaitingForAutomaticInspection): * Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.h: - Use the new `followsInternalPolicies` state to keep track of when a target should be exempt from internal policies for contexts that never make sense to be inspectable. - Introduce `allowsInspectionByPolicy` which takes into account internal policy when determining the inspectability of a target. Previously this was baked into `inspectable`, but that inadvertently leaks the internal policy implementation detail to clients of JSContext and WKWebView. * Source/WebCore/bindings/js/JSDOMGlobalObject.cpp: (WebCore::JSDOMGlobalObject::finishCreation): * Source/WebKit/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm: (API::SharedJSContext::ensureContext): - Adopt new methods to mark the contexts created here as never inspectable since they do not expose any useful information. Canonical link: https://commits.webkit.org/259064@main
11aafb6
c78b5ff