Diff
Modified: branches/safari-613.1.17.0-branch/LayoutTests/ChangeLog (289904 => 289905)
--- branches/safari-613.1.17.0-branch/LayoutTests/ChangeLog 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/LayoutTests/ChangeLog 2022-02-16 18:12:13 UTC (rev 289905)
@@ -1,3 +1,69 @@
+2022-02-16 Russell Epstein <repst...@apple.com>
+
+ Cherry-pick r289892. rdar://problem/88696673
+
+ REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()
+ https://bugs.webkit.org/show_bug.cgi?id=236618
+ <rdar://88696673>
+
+ Reviewed by Chris Dumez.
+
+ Source/WebCore:
+
+ After r287293, if an inline event handler was replaced with a JSFunction, its execution was
+ still disallowed by the CSP policy.
+
+ This change fixes detection of inline event handlers (ones that were created from markup)
+ by introducing JSEventListener::m_wasCreatedFromMarkup and unsetting it during replacement
+ of an attribute event listener.
+
+ Since no virtual calls are added to the hot path, the Speedometer2/Inferno-TodoMVC performance
+ gain is kept. Also, a virtual call is removed from JSEventListener::handleEvent(), which is nice.
+ `sizeof(JSEventListener)` is unchanged.
+
+ Test: http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html
+
+ * bindings/js/JSErrorHandler.cpp:
+ (WebCore::JSErrorHandler::JSErrorHandler):
+ * bindings/js/JSEventListener.cpp:
+ (WebCore::JSEventListener::JSEventListener):
+ (WebCore::JSEventListener::create):
+ (WebCore::JSEventListener::replaceJSFunctionForAttributeListener):
+ * bindings/js/JSEventListener.h:
+ (WebCore::JSEventListener::wasCreatedFromMarkup const):
+ (WebCore::JSEventListener::wasCreatedFromMarkup):
+ * bindings/js/JSLazyEventListener.cpp:
+ (WebCore::JSLazyEventListener::JSLazyEventListener):
+ * bindings/js/JSLazyEventListener.h:
+ * dom/EventListener.h:
+ (WebCore::EventListener::wasCreatedFromMarkup const): Deleted.
+ * dom/EventListenerMap.cpp:
+ (WebCore::removeFirstListenerCreatedFromMarkup):
+ (WebCore::copyListenersNotCreatedFromMarkupToTarget):
+ * dom/EventTarget.cpp:
+ (WebCore::EventTarget::addEventListener):
+ * svg/SVGElement.cpp:
+ (WebCore::SVGElement::removeEventListener):
+
+ LayoutTests:
+
+ * http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced-expected.txt: Added.
+ * http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289892 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-02-16 Alexey Shvayka <ashva...@apple.com>
+
+ REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()
+ https://bugs.webkit.org/show_bug.cgi?id=236618
+ <rdar://88696673>
+
+ Reviewed by Chris Dumez.
+
+ * http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced-expected.txt: Added.
+ * http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html: Added.
+
2022-02-15 Russell Epstein <repst...@apple.com>
Revert r289828. rdar://problem/88656665
Added: branches/safari-613.1.17.0-branch/LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced-expected.txt (0 => 289905)
--- branches/safari-613.1.17.0-branch/LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced-expected.txt (rev 0)
+++ branches/safari-613.1.17.0-branch/LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced-expected.txt 2022-02-16 18:12:13 UTC (rev 289905)
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: Refused to execute a script for an inline event handler because 'unsafe-inline' appears in neither the script-src directive nor the default-src directive of the Content Security Policy.
+CONSOLE MESSAGE: PASS: clicked is 1
+This test checks that if an inline handler was replaced with a JSFunction, CSP doesn't prevent it from being invoked. It passes if there is one SecurityError and 'PASS' message, with no 'FAIL' logs appearing.
Added: branches/safari-613.1.17.0-branch/LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html (0 => 289905)
--- branches/safari-613.1.17.0-branch/LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html (rev 0)
+++ branches/safari-613.1.17.0-branch/LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html 2022-02-16 18:12:13 UTC (rev 289905)
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<head>
+<body>
+ <script>
+ document.head.innerHTML = `<meta http-equiv="Content-Security-Policy" content="default-src 'self'">`;
+
+ if (window.testRunner)
+ testRunner.dumpAsText();
+
+ var clicked = 0;
+ var a = document.createElement('a')
+ a.setAttribute('onclick', 'throw new Error("FAIL: should be unreached!")');
+ a.click();
+
+ a._onclick_ = function() { clicked++ };
+ a.click();
+
+ if (clicked === 1) {
+ console.log(`PASS: clicked is 1`);
+ } else {
+ console.log(`FAIL: clicked was ${clicked}, should be 1`);
+ }
+ </script>
+ <p>This test checks that if an inline handler was replaced with a JSFunction, CSP doesn't prevent it from being invoked. It passes if there is one SecurityError and 'PASS' message, with no 'FAIL' logs appearing.</p>
+</body>
+</html>
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/ChangeLog (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/ChangeLog 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/ChangeLog 2022-02-16 18:12:13 UTC (rev 289905)
@@ -1,3 +1,101 @@
+2022-02-16 Russell Epstein <repst...@apple.com>
+
+ Cherry-pick r289892. rdar://problem/88696673
+
+ REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()
+ https://bugs.webkit.org/show_bug.cgi?id=236618
+ <rdar://88696673>
+
+ Reviewed by Chris Dumez.
+
+ Source/WebCore:
+
+ After r287293, if an inline event handler was replaced with a JSFunction, its execution was
+ still disallowed by the CSP policy.
+
+ This change fixes detection of inline event handlers (ones that were created from markup)
+ by introducing JSEventListener::m_wasCreatedFromMarkup and unsetting it during replacement
+ of an attribute event listener.
+
+ Since no virtual calls are added to the hot path, the Speedometer2/Inferno-TodoMVC performance
+ gain is kept. Also, a virtual call is removed from JSEventListener::handleEvent(), which is nice.
+ `sizeof(JSEventListener)` is unchanged.
+
+ Test: http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html
+
+ * bindings/js/JSErrorHandler.cpp:
+ (WebCore::JSErrorHandler::JSErrorHandler):
+ * bindings/js/JSEventListener.cpp:
+ (WebCore::JSEventListener::JSEventListener):
+ (WebCore::JSEventListener::create):
+ (WebCore::JSEventListener::replaceJSFunctionForAttributeListener):
+ * bindings/js/JSEventListener.h:
+ (WebCore::JSEventListener::wasCreatedFromMarkup const):
+ (WebCore::JSEventListener::wasCreatedFromMarkup):
+ * bindings/js/JSLazyEventListener.cpp:
+ (WebCore::JSLazyEventListener::JSLazyEventListener):
+ * bindings/js/JSLazyEventListener.h:
+ * dom/EventListener.h:
+ (WebCore::EventListener::wasCreatedFromMarkup const): Deleted.
+ * dom/EventListenerMap.cpp:
+ (WebCore::removeFirstListenerCreatedFromMarkup):
+ (WebCore::copyListenersNotCreatedFromMarkupToTarget):
+ * dom/EventTarget.cpp:
+ (WebCore::EventTarget::addEventListener):
+ * svg/SVGElement.cpp:
+ (WebCore::SVGElement::removeEventListener):
+
+ LayoutTests:
+
+ * http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced-expected.txt: Added.
+ * http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html: Added.
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289892 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2022-02-16 Alexey Shvayka <ashva...@apple.com>
+
+ REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()
+ https://bugs.webkit.org/show_bug.cgi?id=236618
+ <rdar://88696673>
+
+ Reviewed by Chris Dumez.
+
+ After r287293, if an inline event handler was replaced with a JSFunction, its execution was
+ still disallowed by the CSP policy.
+
+ This change fixes detection of inline event handlers (ones that were created from markup)
+ by introducing JSEventListener::m_wasCreatedFromMarkup and unsetting it during replacement
+ of an attribute event listener.
+
+ Since no virtual calls are added to the hot path, the Speedometer2/Inferno-TodoMVC performance
+ gain is kept. Also, a virtual call is removed from JSEventListener::handleEvent(), which is nice.
+ `sizeof(JSEventListener)` is unchanged.
+
+ Test: http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html
+
+ * bindings/js/JSErrorHandler.cpp:
+ (WebCore::JSErrorHandler::JSErrorHandler):
+ * bindings/js/JSEventListener.cpp:
+ (WebCore::JSEventListener::JSEventListener):
+ (WebCore::JSEventListener::create):
+ (WebCore::JSEventListener::replaceJSFunctionForAttributeListener):
+ * bindings/js/JSEventListener.h:
+ (WebCore::JSEventListener::wasCreatedFromMarkup const):
+ (WebCore::JSEventListener::wasCreatedFromMarkup):
+ * bindings/js/JSLazyEventListener.cpp:
+ (WebCore::JSLazyEventListener::JSLazyEventListener):
+ * bindings/js/JSLazyEventListener.h:
+ * dom/EventListener.h:
+ (WebCore::EventListener::wasCreatedFromMarkup const): Deleted.
+ * dom/EventListenerMap.cpp:
+ (WebCore::removeFirstListenerCreatedFromMarkup):
+ (WebCore::copyListenersNotCreatedFromMarkupToTarget):
+ * dom/EventTarget.cpp:
+ (WebCore::EventTarget::addEventListener):
+ * svg/SVGElement.cpp:
+ (WebCore::SVGElement::removeEventListener):
+
2022-02-15 Russell Epstein <repst...@apple.com>
Revert r289828. rdar://problem/88656665
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSErrorHandler.cpp (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSErrorHandler.cpp 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSErrorHandler.cpp 2022-02-16 18:12:13 UTC (rev 289905)
@@ -49,7 +49,7 @@
using namespace JSC;
inline JSErrorHandler::JSErrorHandler(JSObject& listener, JSObject& wrapper, bool isAttribute, DOMWrapperWorld& world)
- : JSEventListener(&listener, &wrapper, isAttribute, world)
+ : JSEventListener(&listener, &wrapper, isAttribute, CreatedFromMarkup::No, world)
{
}
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSEventListener.cpp (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSEventListener.cpp 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSEventListener.cpp 2022-02-16 18:12:13 UTC (rev 289905)
@@ -46,10 +46,12 @@
namespace WebCore {
using namespace JSC;
-JSEventListener::JSEventListener(JSObject* function, JSObject* wrapper, bool isAttribute, DOMWrapperWorld& isolatedWorld)
+JSEventListener::JSEventListener(JSObject* function, JSObject* wrapper, bool isAttribute, CreatedFromMarkup createdFromMarkup, DOMWrapperWorld& isolatedWorld)
: EventListener(JSEventListenerType)
+ , m_isAttribute(isAttribute)
+ , m_wasCreatedFromMarkup(createdFromMarkup == CreatedFromMarkup::Yes)
+ , m_isInitialized(false)
, m_wrapper(wrapper)
- , m_isAttribute(isAttribute)
, m_isolatedWorld(isolatedWorld)
{
if (function) {
@@ -63,7 +65,7 @@
Ref<JSEventListener> JSEventListener::create(JSC::JSObject& listener, JSC::JSObject& wrapper, bool isAttribute, DOMWrapperWorld& world)
{
- return adoptRef(*new JSEventListener(&listener, &wrapper, isAttribute, world));
+ return adoptRef(*new JSEventListener(&listener, &wrapper, isAttribute, CreatedFromMarkup::No, world));
}
RefPtr<JSEventListener> JSEventListener::create(JSC::JSValue listener, JSC::JSObject& wrapper, bool isAttribute, DOMWrapperWorld& world)
@@ -85,6 +87,7 @@
ASSERT(function);
ASSERT(wrapper);
+ m_wasCreatedFromMarkup = false;
m_jsFunction = Weak { function };
m_wrapper = wrapper;
m_isInitialized = true;
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSEventListener.h (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSEventListener.h 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSEventListener.h 2022-02-16 18:12:13 UTC (rev 289905)
@@ -46,6 +46,8 @@
// Returns true if this event listener was created for an event handler attribute, like "onload" or "onclick".
bool isAttribute() const final { return m_isAttribute; }
+ bool wasCreatedFromMarkup() const { return m_wasCreatedFromMarkup; }
+
JSC::JSObject* ensureJSFunction(ScriptExecutionContext&) const;
DOMWrapperWorld& isolatedWorld() const { return m_isolatedWorld; }
@@ -59,6 +61,10 @@
String functionName() const;
void replaceJSFunctionForAttributeListener(JSC::JSObject* function, JSC::JSObject* wrapper);
+ static bool wasCreatedFromMarkup(const EventListener& listener)
+ {
+ return is<JSEventListener>(listener) && downcast<JSEventListener>(listener).wasCreatedFromMarkup();
+ }
private:
virtual JSC::JSObject* initializeJSFunction(ScriptExecutionContext&) const;
@@ -69,16 +75,20 @@
virtual String code() const { return String(); }
protected:
- JSEventListener(JSC::JSObject* function, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld&);
+ enum class CreatedFromMarkup : bool { No, Yes };
+
+ JSEventListener(JSC::JSObject* function, JSC::JSObject* wrapper, bool isAttribute, CreatedFromMarkup, DOMWrapperWorld&);
void handleEvent(ScriptExecutionContext&, Event&) override;
void setWrapperWhenInitializingJSFunction(JSC::VM&, JSC::JSObject* wrapper) const { m_wrapper = JSC::Weak<JSC::JSObject>(wrapper); }
private:
+ bool m_isAttribute : 1;
+ bool m_wasCreatedFromMarkup : 1;
+
+ mutable bool m_isInitialized : 1;
mutable JSC::Weak<JSC::JSObject> m_jsFunction;
mutable JSC::Weak<JSC::JSObject> m_wrapper;
- mutable bool m_isInitialized { false };
- bool m_isAttribute;
Ref<DOMWrapperWorld> m_isolatedWorld;
};
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSLazyEventListener.cpp (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSLazyEventListener.cpp 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSLazyEventListener.cpp 2022-02-16 18:12:13 UTC (rev 289905)
@@ -68,7 +68,7 @@
}
JSLazyEventListener::JSLazyEventListener(CreationArguments&& arguments, const URL& sourceURL, const TextPosition& sourcePosition)
- : JSEventListener(nullptr, arguments.wrapper, true, mainThreadNormalWorld())
+ : JSEventListener(nullptr, arguments.wrapper, true, CreatedFromMarkup::Yes, mainThreadNormalWorld())
, m_functionName(arguments.attributeName.localName().string())
, m_eventParameterName(eventParameterName(arguments.shouldUseSVGEventName))
, m_code(arguments.attributeValue)
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSLazyEventListener.h (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSLazyEventListener.h 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/bindings/js/JSLazyEventListener.h 2022-02-16 18:12:13 UTC (rev 289905)
@@ -53,7 +53,6 @@
#endif
JSC::JSObject* initializeJSFunction(ScriptExecutionContext&) const final;
- bool wasCreatedFromMarkup() const final { return true; }
String m_functionName;
const String& m_eventParameterName;
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventListener.h (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventListener.h 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventListener.h 2022-02-16 18:12:13 UTC (rev 289905)
@@ -51,7 +51,6 @@
virtual ~EventListener() = default;
virtual bool operator==(const EventListener&) const = 0;
virtual void handleEvent(ScriptExecutionContext&, Event&) = 0;
- virtual bool wasCreatedFromMarkup() const { return false; }
virtual void visitJSFunction(JSC::AbstractSlotVisitor&) { }
virtual void visitJSFunction(JSC::SlotVisitor&) { }
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventListenerMap.cpp (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventListenerMap.cpp 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventListenerMap.cpp 2022-02-16 18:12:13 UTC (rev 289905)
@@ -36,6 +36,7 @@
#include "AddEventListenerOptions.h"
#include "Event.h"
#include "EventTarget.h"
+#include "JSEventListener.h"
#include <wtf/MainThread.h>
#include <wtf/StdLibExtras.h>
#include <wtf/Vector.h>
@@ -191,7 +192,7 @@
static void removeFirstListenerCreatedFromMarkup(EventListenerVector& listenerVector)
{
bool foundListener = listenerVector.removeFirstMatching([] (const auto& registeredListener) {
- if (registeredListener->callback().wasCreatedFromMarkup()) {
+ if (JSEventListener::wasCreatedFromMarkup(registeredListener->callback())) {
registeredListener->markAsRemoved();
return true;
}
@@ -220,7 +221,7 @@
{
for (auto& registeredListener : listenerVector) {
// Event listeners created from markup have already been transfered to the shadow tree during cloning.
- if (registeredListener->callback().wasCreatedFromMarkup())
+ if (JSEventListener::wasCreatedFromMarkup(registeredListener->callback()))
continue;
target->addEventListener(eventType, registeredListener->callback(), registeredListener->useCapture());
}
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventTarget.cpp (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventTarget.cpp 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/dom/EventTarget.cpp 2022-02-16 18:12:13 UTC (rev 289905)
@@ -92,7 +92,7 @@
if (!passive.has_value() && Quirks::shouldMakeEventListenerPassive(*this, eventType, listener.get()))
passive = true;
- bool listenerCreatedFromScript = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();
+ bool listenerCreatedFromScript = is<JSEventListener>(listener) && !downcast<JSEventListener>(listener.get()).wasCreatedFromMarkup();
if (!ensureEventTargetData().eventListenerMap.add(eventType, listener.copyRef(), { options.capture, passive.value_or(false), options.once }))
return false;
Modified: branches/safari-613.1.17.0-branch/Source/WebCore/svg/SVGElement.cpp (289904 => 289905)
--- branches/safari-613.1.17.0-branch/Source/WebCore/svg/SVGElement.cpp 2022-02-16 18:10:52 UTC (rev 289904)
+++ branches/safari-613.1.17.0-branch/Source/WebCore/svg/SVGElement.cpp 2022-02-16 18:12:13 UTC (rev 289905)
@@ -34,6 +34,7 @@
#include "HTMLElement.h"
#include "HTMLNames.h"
#include "HTMLParserIdioms.h"
+#include "JSEventListener.h"
#include "RenderAncestorIterator.h"
#include "RenderSVGResourceFilter.h"
#include "RenderSVGResourceMasker.h"
@@ -462,7 +463,7 @@
continue;
// This case can only be hit for event listeners created from markup
- ASSERT(listener.wasCreatedFromMarkup());
+ ASSERT(JSEventListener::wasCreatedFromMarkup(listener));
// If the event listener 'listener' has been created from markup and has been fired before
// then JSLazyEventListener::parseCode() has been called and m_jsFunction of that listener
