Skip to content
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 #8773

Conversation

patrickangle
Copy link
Contributor

@patrickangle patrickangle commented Jan 18, 2023

d08492c

Web Inspector: WebKit-internal JSContexts should not be inspectable, 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/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

37efe1f

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk πŸ’₯ πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ›  jsc-mips
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim βœ… πŸ§ͺ jsc-mips-tests

@patrickangle patrickangle requested review from a team and cdumez as code owners January 18, 2023 17:54
@patrickangle patrickangle self-assigned this Jan 18, 2023
@patrickangle patrickangle added the Web Inspector Bugs related to the WebKit Web Inspector. label Jan 18, 2023
@patrickangle patrickangle force-pushed the eng/b250633-webkit-internal-contexts-should-not-be-inspectable branch from d595bf5 to 37efe1f Compare January 18, 2023 17:59
@patrickangle patrickangle added the merge-queue Applied to send a pull request to merge-queue label Jan 19, 2023
…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
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/b250633-webkit-internal-contexts-should-not-be-inspectable branch from 37efe1f to d08492c Compare January 19, 2023 02:31
@webkit-early-warning-system webkit-early-warning-system merged commit d08492c into WebKit:main Jan 19, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 259064@main (d08492c): https://commits.webkit.org/259064@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Web Inspector Bugs related to the WebKit Web Inspector.
Projects
None yet
3 participants