Skip to content

Commit 11aafb6

Browse files
patrickanglepatrickangle
committed
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. 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
1 parent dfe9241 commit 11aafb6

10 files changed

Lines changed: 102 additions & 18 deletions

File tree

‎Source/JavaScriptCore/API/JSRemoteInspector.cpp‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2015 Apple Inc. All rights reserved.
2+
* Copyright (C) 2015-2023 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -41,6 +41,7 @@
4141
using namespace Inspector;
4242

4343
static std::optional<bool> remoteInspectionEnabledByDefault = std::nullopt;
44+
static bool inspectionFollowsInternalPolicies = true;
4445

4546
void JSRemoteInspectorDisableAutoStart(void)
4647
{
@@ -128,3 +129,13 @@ void JSRemoteInspectorSetInspectionEnabledByDefault(bool enabledByDefault)
128129
{
129130
remoteInspectionEnabledByDefault = enabledByDefault;
130131
}
132+
133+
bool JSRemoteInspectorGetInspectionFollowsInternalPolicies(void)
134+
{
135+
return inspectionFollowsInternalPolicies;
136+
}
137+
138+
void JSRemoteInspectorSetInspectionFollowsInternalPolicies(bool followsInternalPolicies)
139+
{
140+
inspectionFollowsInternalPolicies = followsInternalPolicies;
141+
}

‎Source/JavaScriptCore/API/JSRemoteInspector.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2015 Apple Inc. All rights reserved.
2+
* Copyright (C) 2015-2023 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -50,6 +50,9 @@ JS_EXPORT void JSRemoteInspectorSetLogToSystemConsole(bool) JSC_API_AVAILABLE(ma
5050
JS_EXPORT bool JSRemoteInspectorGetInspectionEnabledByDefault(void) JSC_API_AVAILABLE(macos(10.11), ios(9.0));
5151
JS_EXPORT void JSRemoteInspectorSetInspectionEnabledByDefault(bool) JSC_API_DEPRECATED("Use JSGlobalContextSetInspectable on a single JSGlobalContextRef.", macos(10.11, JSC_MAC_TBA), ios(9.0, JSC_IOS_TBA));
5252

53+
JS_EXPORT bool JSRemoteInspectorGetInspectionFollowsInternalPolicies(void) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));
54+
JS_EXPORT void JSRemoteInspectorSetInspectionFollowsInternalPolicies(bool) JSC_API_AVAILABLE(macos(JSC_MAC_TBA), ios(JSC_IOS_TBA));
55+
5356
#ifdef __cplusplus
5457
}
5558
#endif

‎Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ bool JSGlobalObjectInspectorController::developerExtrasEnabled() const
211211
if (!RemoteInspector::singleton().enabled())
212212
return false;
213213

214-
if (!m_globalObject.inspectorDebuggable().inspectable())
214+
if (!m_globalObject.inspectorDebuggable().allowsInspectionByPolicy())
215215
return false;
216216
#endif
217217

‎Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.cpp‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2013, 2015 Apple Inc. All Rights Reserved.
2+
* Copyright (C) 2013-2023 Apple Inc. All Rights Reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -40,24 +40,55 @@ namespace Inspector {
4040

4141
bool RemoteInspectionTarget::remoteControlAllowed() const
4242
{
43-
return inspectable() || hasLocalDebugger();
43+
return allowsInspectionByPolicy() || hasLocalDebugger();
4444
}
4545

46-
bool RemoteInspectionTarget::inspectable() const
46+
bool RemoteInspectionTarget::allowsInspectionByPolicy() const
4747
{
48-
#if PLATFORM(COCOA)
49-
static bool allowInternalSecurityPolicies = os_variant_allows_internal_security_policies("com.apple.WebInspector");
50-
if (allowInternalSecurityPolicies && !RemoteInspector::singleton().isSimulatingCustomerInstall())
48+
switch (m_inspectable) {
49+
case Inspectable::Yes:
5150
return true;
51+
case Inspectable::No:
52+
#if PLATFORM(COCOA)
53+
static bool allowInternalSecurityPolicies = os_variant_allows_internal_security_policies("com.apple.WebInspector");
54+
if (allowInternalSecurityPolicies && !RemoteInspector::singleton().isSimulatingCustomerInstall())
55+
return true;
56+
FALLTHROUGH;
5257
#endif
53-
return m_inspectable;
58+
case Inspectable::NoIgnoringInternalPolicies:
59+
return false;
60+
}
61+
62+
ASSERT_NOT_REACHED();
63+
return false;
64+
}
65+
66+
bool RemoteInspectionTarget::inspectable() const
67+
{
68+
switch (m_inspectable) {
69+
case Inspectable::Yes:
70+
return true;
71+
case Inspectable::No:
72+
case Inspectable::NoIgnoringInternalPolicies:
73+
return false;
74+
}
75+
76+
ASSERT_NOT_REACHED();
77+
return false;
5478
}
5579

5680
void RemoteInspectionTarget::setInspectable(bool inspectable)
5781
{
58-
m_inspectable = inspectable;
82+
if (inspectable)
83+
m_inspectable = Inspectable::Yes;
84+
else {
85+
if (!JSRemoteInspectorGetInspectionFollowsInternalPolicies())
86+
m_inspectable = Inspectable::NoIgnoringInternalPolicies;
87+
else
88+
m_inspectable = Inspectable::No;
89+
}
5990

60-
if (RemoteInspectionTarget::inspectable() && automaticInspectionAllowed())
91+
if (allowsInspectionByPolicy() && automaticInspectionAllowed())
6192
RemoteInspector::singleton().updateAutomaticInspectionCandidate(this);
6293
else
6394
RemoteInspector::singleton().updateTarget(this);
@@ -66,7 +97,7 @@ void RemoteInspectionTarget::setInspectable(bool inspectable)
6697
void RemoteInspectionTarget::pauseWaitingForAutomaticInspection()
6798
{
6899
ASSERT(targetIdentifier());
69-
ASSERT(inspectable());
100+
ASSERT(allowsInspectionByPolicy());
70101
ASSERT(automaticInspectionAllowed());
71102

72103
while (RemoteInspector::singleton().waitingForAutomaticInspection(targetIdentifier())) {

‎Source/JavaScriptCore/inspector/remote/RemoteInspectionTarget.h‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2013, 2015 Apple Inc. All Rights Reserved.
2+
* Copyright (C) 2013-2023 Apple Inc. All Rights Reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -27,6 +27,7 @@
2727

2828
#if ENABLE(REMOTE_INSPECTOR)
2929

30+
#include "JSRemoteInspector.h"
3031
#include "RemoteControllableTarget.h"
3132
#include <wtf/RetainPtr.h>
3233
#include <wtf/TypeCasts.h>
@@ -41,6 +42,8 @@ class JS_EXPORT_PRIVATE RemoteInspectionTarget : public RemoteControllableTarget
4142
bool inspectable() const;
4243
void setInspectable(bool);
4344

45+
bool allowsInspectionByPolicy() const;
46+
4447
#if USE(CF)
4548
CFRunLoopRef targetRunLoop() const final { return m_runLoop.get(); }
4649
void setTargetRunLoop(CFRunLoopRef runLoop) { m_runLoop = runLoop; }
@@ -60,7 +63,16 @@ class JS_EXPORT_PRIVATE RemoteInspectionTarget : public RemoteControllableTarget
6063
bool remoteControlAllowed() const final;
6164

6265
private:
63-
bool m_inspectable { false };
66+
enum class Inspectable : uint8_t {
67+
Yes,
68+
No,
69+
70+
// For WebKit internal proxies and wrappers, we want to always disable inspection even when internal policies
71+
// would otherwise enable inspection.
72+
NoIgnoringInternalPolicies,
73+
};
74+
Inspectable m_inspectable { JSRemoteInspectorGetInspectionFollowsInternalPolicies() ? Inspectable::No : Inspectable::NoIgnoringInternalPolicies };
75+
6476
#if USE(CF)
6577
RetainPtr<CFRunLoopRef> m_runLoop;
6678
#endif

‎Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,7 @@ static bool canAccessWebInspectorMachPort()
458458
// Must collect target information on the WebThread, Main, or Worker thread since RemoteTargets are
459459
// implemented by non-threadsafe JSC / WebCore classes such as JSGlobalObject or WebCore::Page.
460460

461-
if (!target.inspectable())
461+
if (!target.allowsInspectionByPolicy())
462462
return nil;
463463

464464
RetainPtr<NSMutableDictionary> listing = adoptNS([[NSMutableDictionary alloc] init]);

‎Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ static const char* targetDebuggableType(RemoteInspectionTarget::Type type)
170170

171171
TargetListing RemoteInspector::listingForInspectionTarget(const RemoteInspectionTarget& target) const
172172
{
173-
if (!target.inspectable())
173+
if (!target.allowsInspectionByPolicy())
174174
return nullptr;
175175

176176
return g_variant_new("(tsssb)", static_cast<guint64>(target.targetIdentifier()),

‎Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ void RemoteInspector::stopInternal(StopSource)
117117

118118
TargetListing RemoteInspector::listingForInspectionTarget(const RemoteInspectionTarget& target) const
119119
{
120-
if (!target.inspectable())
120+
if (!target.allowsInspectionByPolicy())
121121
return nullptr;
122122

123123
// FIXME: Support remote debugging of a ServiceWorker.

‎Source/WebCore/bindings/js/JSDOMGlobalObject.cpp‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,10 @@
7575
#include <JavaScriptCore/WeakGCMapInlines.h>
7676
#include <wtf/text/StringConcatenateNumbers.h>
7777

78+
#if ENABLE(REMOTE_INSPECTOR)
79+
#include <JavaScriptCore/JSRemoteInspector.h>
80+
#endif
81+
7882
namespace WebCore {
7983
using namespace JSC;
8084

@@ -288,22 +292,40 @@ SUPPRESS_ASAN void JSDOMGlobalObject::addBuiltinGlobals(VM& vm)
288292

289293
void JSDOMGlobalObject::finishCreation(VM& vm)
290294
{
295+
#if ENABLE(REMOTE_INSPECTOR)
296+
bool inspectionPreviouslyFollowedInternalPolicies = JSRemoteInspectorGetInspectionFollowsInternalPolicies();
297+
JSRemoteInspectorSetInspectionFollowsInternalPolicies(false);
298+
#endif
299+
291300
Base::finishCreation(vm);
292301
ASSERT(inherits(info()));
293302

294303
addBuiltinGlobals(vm);
295304

296305
RELEASE_ASSERT(classInfo());
306+
307+
#if ENABLE(REMOTE_INSPECTOR)
308+
JSRemoteInspectorSetInspectionFollowsInternalPolicies(inspectionPreviouslyFollowedInternalPolicies);
309+
#endif
297310
}
298311

299312
void JSDOMGlobalObject::finishCreation(VM& vm, JSObject* thisValue)
300313
{
314+
#if ENABLE(REMOTE_INSPECTOR)
315+
bool inspectionPreviouslyFollowedInternalPolicies = JSRemoteInspectorGetInspectionFollowsInternalPolicies();
316+
JSRemoteInspectorSetInspectionFollowsInternalPolicies(false);
317+
#endif
318+
301319
Base::finishCreation(vm, thisValue);
302320
ASSERT(inherits(info()));
303321

304322
addBuiltinGlobals(vm);
305323

306324
RELEASE_ASSERT(classInfo());
325+
326+
#if ENABLE(REMOTE_INSPECTOR)
327+
JSRemoteInspectorSetInspectionFollowsInternalPolicies(inspectionPreviouslyFollowedInternalPolicies);
328+
#endif
307329
}
308330

309331
ScriptExecutionContext* JSDOMGlobalObject::scriptExecutionContext() const

‎Source/WebKit/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@
4949
{
5050
m_lastUseTime = MonotonicTime::now();
5151
if (!m_context) {
52+
bool inspectionPreviouslyFollowedInternalPolicies = JSRemoteInspectorGetInspectionFollowsInternalPolicies();
53+
JSRemoteInspectorSetInspectionFollowsInternalPolicies(false);
54+
5255
// FIXME: rdar://100738357 Remote Web Inspector: Remove use of JSRemoteInspectorGetInspectionEnabledByDefault
5356
// and JSRemoteInspectorSetInspectionEnabledByDefault once the default state is always false.
5457
ALLOW_DEPRECATED_DECLARATIONS_BEGIN
@@ -58,6 +61,8 @@
5861
JSRemoteInspectorSetInspectionEnabledByDefault(previous);
5962
ALLOW_DEPRECATED_DECLARATIONS_END
6063

64+
JSRemoteInspectorSetInspectionFollowsInternalPolicies(inspectionPreviouslyFollowedInternalPolicies);
65+
6166
m_timer.startOneShot(sharedJSContextMaxIdleTime);
6267
}
6368
return m_context.get();

0 commit comments

Comments
 (0)