Bug 73339 – Add an internal targetOrigin member to MessageEvent

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Karl Koscher]

To support cross-process postMessage in Chromium (bug 73337), we need to be able to check the postMessage's target origin in Chromium to avoid time-of-check-to-time-of-use security bugs. However, the MessageEvent generated by WebKit doesn't contain the target origin, so the event listeners in Chromium currently can't get this information. We need to add a targetOrigin member to track this. This change will not affect the MessageEvent IDL, so none of the bindings will be aware of this change.

[Karl Koscher]

Created attachment 117007 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117007 [details]
Patch

Attachment 117007 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10695048

New failing tests:
http/tests/inspector/extensions-headers.html
http/tests/appcache/offline-access.html
http/tests/appcache/main-resource-hash.html
http/tests/appcache/deferred-events-delete-while-raising.html
http/tests/appcache/reload.html
http/tests/messaging/cross-domain-message-event-dispatch.html
http/tests/appcache/cyrillic-uri.html
http/tests/appcache/top-frame-4.html
http/tests/appcache/xhr-foreign-resource.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector/extensions-ignore-cache.html
http/tests/appcache/top-frame-3.html
http/tests/appcache/remove-cache.html
http/tests/appcache/top-frame-1.html
http/tests/cache/history-only-cached-subresource-loads-max-age-https.html
http/tests/inspector/extensions-useragent.html
http/tests/cache/history-only-cached-subresource-loads.html
http/tests/history/cross-origin-replace-history-object.html
http/tests/messaging/cross-domain-message-send.html

[Adam Barth]

That looks like an unlikely list of failing tests.

[Alexey Proskuryakov]

Most or all of these tests should be using postMessage, so it seems quite possible that they are genuinely broken by this patch.

[Karl Koscher]

Created attachment 117451 [details]
Patch

[Darin Adler]

Comment on attachment 117451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117451&action=review

> Source/WebCore/dom/MessageEvent.cpp:87
>      , m_origin("")
>      , m_lastEventId("")
> +    , m_targetOrigin("")

Passing "" is unnecessarily inefficient. We should use emptyString() instead.

> Source/WebCore/dom/MessageEvent.h:138
> +    // For internal use only -- not exposed to any bindings

I think this is a confusing comment. There are many data members in many classes with this property.

A more helpful comment would explain the purpose of this, why it exists, and that might make it clear why it would not be exposed. Generally, comments should say “why” rather than “what”.

[Karl Koscher]

Created attachment 117467 [details]
Patch

[Karl Koscher]

Comment on attachment 117467 [details]
Patch

Marking this patch as potentially obsolete. If we're not using native event handlers (see bug 73359 for a discussion of that), we can probably just pass the target origin directly.

[Karl Koscher]

We're now going to ask the embedder if it wants to deliver the postMessage, making this bug obsolete.