Bug 17331 – Change postMessage/MessageEvent to match HTML5 wrt. exposing origin vs. domain/uri

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Roben (:aroben)]

The HTML5 working draft recently changed the MessageEvent interface for security reasons. The interface no longer has the 'domain' and 'uri' properties, and instead has an 'origin' property. From Hixie's email explaining the change <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-February/013943.html>:

> While going through the feedback for postMessage(), I noticed a couple of 
> security problems that nobody had raised:
> 
> * message.domain isn't actually enough to verify any security, given that 
>   on shared hosts one IP address can map to several hostnames and thus 
>   people can end up running servers on different ports that respond to 
>   requests from domains they don't own.
> 
> * message.uri can leak information, e.g. if the user's password is in the 
>   query component of the URI.
> 
> Basically, .domain is too little, and .uri is too much.
> 
> I've replaced both with .origin, which is intended to return the 
> scheme://hostname/ or scheme://hostname:port/ (when the port is 
> non-standard) of the origin of the source document.
> 
> It's still vague for data: URIs, etc; I have outstanding feedback on that 
> matter and will address that when I respond to that feedback.

[Adam Roben (:aroben)]

<rdar://problem/5738341>

[Adam Roben (:aroben)]

See <http://html5.org/tools/web-apps-tracker?from=1213&to=1216> for the change to the spec.

[Adam Roben (:aroben)]

The postMessage() API has also been updated to take an origin parameter. <http://html5.org/tools/web-apps-tracker?from=1216&to=1217>

[Adam Roben (:aroben)]

The MessageEvent interface is defined at <http://www.whatwg.org/specs/web-apps/current-work/#messageevent>, and the postMessage() API is defined at <http://www.whatwg.org/specs/web-apps/current-work/#postmessage>.

[Adam Roben (:aroben)]

Step 2.2 of the postMessage() algorithm says:

> If the origin of the target document is not a scheme/host/port tuple, then abort these steps silently.

We don't currently support non-scheme/host/port tuple origins. Bug 17352 covers that issue.

[Adam Roben (:aroben)]

It looks like SecurityOrigin::canAccess [1] is what we should use in place of the substeps of step 2 of the postMessage() API.

1. http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/platform/SecurityOrigin.cpp?rev=30184#L129

[Adam Roben (:aroben)]

(In reply to comment #6)
> It looks like SecurityOrigin::canAccess [1] is what we should use in place of
> the substeps of step 2 of the postMessage() API.

One reason for this is that this will allow scripts in Documents with file: URIs to send messages to Documents with http: URIs.

[Adam Roben (:aroben)]

(In reply to comment #6)
> It looks like SecurityOrigin::canAccess [1] is what we should use in place of
> the substeps of step 2 of the postMessage() API.

I'm now no longer sure this is correct. The purpose of the origin argument and step 2 of the postMessage() API is to allow the script calling postMessage() to specify the one and only origin the message should be delivered to. The motivation for the origin argument was presented by Collin Jackson to the WHATWG mailing list (substituting "domain" and "uri" for "origin") <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-January/013808.html>:

> For non-security-sensitive messages, like "change your font color to
> red", confidentiality might not be needed. However, if the message
> you're trying to send contains a password, it would be nice to be able
> to specify which domain you're trying to send it to.
> 
> The postMessage API could be extended to provide confidentiality by
> adding some optional arguments:

> void postMessage(in DOMString message, [optional] in DOMString domain,
> [optional] in DOMString uri);
> 
> If "domain" or "uri" are specified, the browser would only deliver the
> message if the recipient's location matches the specified domain and/or
> URI. (Being able to specify the URI allows sites to differentiate
> between http and https URIs.) If "domain" and "uri" are not defined,
> the message would be delivered regardless of who the recipient is,
> making this change backwards compatible for sites that aren't aware
> of the optional parameters.

Given this, I think we really do want to ensure that the scheme/host/port of the target Document is identical to the scheme/host/port of the passed-in origin string. If we were to use canAccess then passing a file: URI as the origin argument would be identical to passing no origin argument at all, since we allow file: URIs to access any Document.

[Adam Roben (:aroben)]

Maybe what we really want is the same logic that's in SecurityOrigin::canAccess, but with the additional restriction that the schemes must always match (SecurityOrigin::canAccess allows the schemes to be different if the SecurityOrigin you call canAccess on has a "local" scheme).

[Adam Barth]

One subtlety you should be careful about is that Step 9 requires comparing the desired host with the host of the target document, ignoring the document.domain value of the target document.  On the other hand, canAccess takes the document.domain value (and whether the value has been set) into account.

Additionally, the host() property of SecurityOrigin is actually the document.domain value for that origin (which can differ from the host if document.domain has been modified).  It might be a good idea to add a domain() property to SecurityOrigin that holds the document.domain value and change host() to hold the actual host value.  I'd be happy to review this part of the patch to make sure the canAccess check remains correct.

[Adam Roben (:aroben)]

(In reply to comment #10)
> One subtlety you should be careful about is that Step 9 requires comparing the
> desired host with the host of the target document, ignoring the document.domain
> value of the target document.

Well, there's an ISSUE in the postMessage() algorithm steps:

> Define 'origin' more exactly -- IDN vs no IDN, effect of window.document.domain on its value, etc

So I don't think it's clear whether document.domain should be taken into account or not.

[Jeff Walden (remove +bwo to email)]

(In reply to comment #11)
> So I don't think it's clear whether document.domain should be taken into
> account or not.

My #webkit 2008-01-11 logs have Hixie saying (pre-origin change) that document.domain isn't considered when determining uri/domain; I don't see a reason why this should change post-origin:

[2008-01-11 20:33:45] <Hixie> abarth, collinj_: updated hte spec to be better
[2008-01-11 20:33:51] <Hixie> and more well-defined
[2008-01-11 20:36:51] <abarth> Hixie: so it should be the initial value of document.domain, regardless of whether it had been modified by the page?
[2008-01-11 20:37:28] <abarth> Hixie: i.e. www.facebook.com on http://www.facebook.com/ (even though document.domain == "facebook.com")
[2008-01-11 20:42:26] <Hixie> abarth: right

[Ian 'Hixie' Hickson]

Yeah I don't think we want document.domain to let a document lie, here. Normally document.domain isn't exposed to other people, it's just to let people communicate if they want to. Here, they can anyway, without any change.

[Collin Jackson]

Created attachment 19288 [details]
Updates postMessage implementation to match HTML 5 specification

Here is a patch that:
1) Adds origin parameter to postMessage.
2) Removes domain and uri attributes of MessageEvent in favor of origin attribute.

Also included are 3 updated tests and 1 new one.

[Collin Jackson]

Created attachment 19291 [details]
Updates postMessage implementation to match HTML 5 specification

Added console message to help developers debug postMessage origin mismatch errors.

[Collin Jackson]

Created attachment 19293 [details]
Updates postMessage implementation to match HTML 5 specification

Changed behavior to match spec for syntax errors in the optional origin argument.

Added a test case for this.

[Sam Weinig]

I think this patch would be better if more of the implementation of postMessage was in DOMWindow instead of JSDOMWindow.  More constructively, in JSDOMWindow::postMessage, you should pretty much just become

JSValue* JSDOMWindow::postMessage(ExecState* exec, const List& args)
{
    DOMWindow* window = impl();

    String message = args[0]->toString(exec);
    if (exec->hadException())
        return jsUndefined();
    String origin = valueToStringWithUndefinedOrNullCheck(exec, args[0]);
    if (exec->hadException())
        return jsUndefined();

    ExceptionCode ec = 0;
    window->postMessage(message, origin, source, ec);
    setDOMException(exec, ec);
    return jsUndefined();
}

or something similar (did not actually compile that code).  This might make reporting the error in the right place more difficult. 

[Collin Jackson]

Created attachment 19311 [details]
Updated patch with origin check moved to DOMWindow

This patch moves more of the implementation (checking the desired target origin against the actual target origin) into DOMWindow.cpp and out of JSDOMWindowCustom.cpp. There are also more syntax error tests. The file:// URL behavior now matches Firefox.

[Sam Weinig]

Comment on attachment 19311 [details]
Updated patch with origin check moved to DOMWindow

There are a few remaining but this looks really good. 

There is a tab here
+            String message = String::format("Unable to post message to %s. Recipient has origin %s.\n", 
+	        origin.utf8().data(), actualTargetOrigin.utf8().data());

This should have a raises(DOMException);
+        [DoNotCheckDomainSecurity, Custom] void postMessage(in DOMString message, in [Optional] DOMString origin);

This method could be made more efficient by calling reserveCapacity on the result vector.
+String KURL::origin() const

r=me

[Adam Roben (:aroben)]

Comment on attachment 19311 [details]
Updated patch with origin check moved to DOMWindow

+    Vector<UChar> result;
+    append(result, protocol());
+    append(result, "://");
+    append(result, host());

I worry that this is relying on some strange implicit casting, since protocol() and host() return Strings, but you're appending to a Vector<UChar>. It might be better to figure out if you need to append the port first, then do:

if (needsPort)
    return String::format("%s://%s:%u", protocol(), host(), port());
else
    return String::format("%s://%s", protocol(), host());

or something like that.

+        String desiredTargetOrigin = desiredTargetURL.origin();
+        String actualTargetOrigin = m_frame->loader()->url().origin();
+        if (desiredTargetOrigin.isNull() || desiredTargetOrigin != actualTargetOrigin) {

It seems like it would be better to use our SecurityOrigin class for a case like this. Teaching KURL about what an origin is seems a little strange to me.

[Collin Jackson]

Created attachment 19327 [details]
Updated patch that addresses Sam's comments

Sam, thanks for your comments.

This patch addresses the tab issue, IDL issue, and reserveCapacity suggestion.

[Collin Jackson]

(In reply to comment #20)
> It seems like it would be better to use our SecurityOrigin class for a case
> like this. Teaching KURL about what an origin is seems a little strange to me.

Good point.

[Darin Adler]

(In reply to comment #20)
> +    Vector<UChar> result;
> +    append(result, protocol());
> +    append(result, "://");
> +    append(result, host());
> 
> I worry that this is relying on some strange implicit casting, since protocol()
> and host() return Strings, but you're appending to a Vector<UChar>.

It's not. PlatformString.h defines:

    void append(Vector<UChar>&, const String&);

That's what's being used here.

> It might be
> better to figure out if you need to append the port first, then do:
> 
> if (needsPort)
>     return String::format("%s://%s:%u", protocol(), host(), port());
> else
>     return String::format("%s://%s", protocol(), host());
> 
> or something like that.

String::format does not have a format specifier that works with other strings. The %s one works with char*, not String. This approach is not practical unless we make some enhancements.

[Collin Jackson]

Created attachment 19338 [details]
Updated patch that addresses aroben's comments

Generating these strings from the SecurityOrigin class required reorganizing the SecurityOrigin class to understand the difference between hosts and domains. There are now three ways to compare security origins:

1) equalIgnoringDomain compares hosts, and is used for postMessage
2) equal compares all aspects of the security origin, and is used for hash keys
3) canAccess understands the semantics of schemes such as file:// and data:// URLs, and should be used for scripting access checks.

We changed SecurityOrigin::toString() and SecurityOrigin::stringIdentifier() to generate identifiers that are suitable for being used as a MessageEvent's origin property. In the future, SecurityOrigin::toString() could be used for the Access-Control-Origin header as well. We're now re-using KURL parser to parse serialized SecurityOrigins.

I took Darin's suggestion of using append() rather than String::format() in SecurityOrigin::toString().

[Adam Roben (:aroben)]

(In reply to comment #24)
> Created an attachment (id=19338) [edit]
> Updated patch that addresses aroben's comments
> 
> Generating these strings from the SecurityOrigin class required reorganizing
> the SecurityOrigin class to understand the difference between hosts and
> domains. There are now three ways to compare security origins:
> 
> 1) equalIgnoringDomain compares hosts, and is used for postMessage

Should we just call this isSameSchemeHostPort to match HTML 5's terminology?

> 2) equal compares all aspects of the security origin, and is used for hash keys

Maybe we should get rid of this method entirely and put the implementation in SecurityOriginHash. It seems like it will only confuse people to have this public method on SecurityOrigin that you're never supposed to call.

> We changed SecurityOrigin::toString() and SecurityOrigin::stringIdentifier() to
> generate identifiers that are suitable for being used as a MessageEvent's
> origin property.

Will these changes interfere with any existing uses of SecurityOrigin::toString or SecurityOrigin::stringIdentifier?

[Collin Jackson]

Created attachment 19353 [details]
Updated patch that matches HTML 5 terminology

(In reply to comment #25)
> > 1) equalIgnoringDomain compares hosts, and is used for postMessage
> 
> Should we just call this isSameSchemeHostPort to match HTML 5's terminology?

Done.

> > 2) equal compares all aspects of the security origin, and is used for hash keys
> 
> Maybe we should get rid of this method entirely and put the implementation in
> SecurityOriginHash. It seems like it will only confuse people to have this
> public method on SecurityOrigin that you're never supposed to call.

I am open to the idea of removing equal(), but it seems like it might be useful for something. Sam and I were discussing whether it might make sense to use it for isSecureTransitionTo. I think it might be better to leave it in for the next few weeks until Sam has had a chance to implement data: URLs with GUIDs. I added a FIXME comment describing your suggestion, but I can refactor it out now in this patch if you prefer.

> > We changed SecurityOrigin::toString() and SecurityOrigin::stringIdentifier() to
> > generate identifiers that are suitable for being used as a MessageEvent's
> > origin property.
> 
> Will these changes interfere with any existing uses of SecurityOrigin::toString
> or SecurityOrigin::stringIdentifier?

The main consequence of this change will be that we lose track of any pre-existing storage databases. Since there doesn't seem to be anyone shipping a version of WebKit that supports databases yet, I don't think this change will have a major impact. The advantage is that we remove redundant code: HTML 5 requires the scheme://host:port way of representing origins for postMessage, so there's no reason to have another way of doing it as well.

[Collin Jackson]

Created attachment 19545 [details]
Updated patch to fix bit rot

This version resolves a conflict with recent changes to JSDOMWindowCustom.cpp.

[Adam Roben (:aroben)]

Comment on attachment 19311 [details]
Updated patch with origin check moved to DOMWindow

Clearing review flag to get this out of the commit queue.

[Collin Jackson]

Created attachment 20357 [details]
Updated patch that fixes postMessage without changing database identifiers

Now that the database API is shipping with Safari 3.1, changing the database security origin identifier syntax requires dealing with backwards compatibility. To avoid delaying the postMessage fix, let's defer the unification of toString and database identifiers for a future patch.

[Collin Jackson]

Created attachment 20358 [details]
Updated patch that fixes postMessage without changing database identifiers

Adding change logs to previous patch

[Collin Jackson]

Created attachment 20359 [details]
Updated patch with better change log explanation

Updated the change log to better describe the status of the database string identifier.

[Collin Jackson]

Created attachment 20418 [details]
Updated patch to address compile issues

I tried re-running the layout tests today and ran into some compile issues (a typo in Document.cpp and a wrong symbol in WebCore.base.exp). I'm not sure why these issues didn't manifest before, but it might have been that I needed to do a clean build. Anyway, here's an updated patch that should fix it.

[Collin Jackson]

Created attachment 20562 [details]
Updated patch to fix bit rot

I've updated the patch to fix a conflict in JSDOMWindowCustom.cpp introduced by changeset 31746.

[Adam Roben (:aroben)]

Comment on attachment 20562 [details]
Updated patch to fix bit rot

The changes here look good.

I think the changes to WebSecurityOrigin in WebKit/mac will break Safari 3.1, since it won't be able to find the -domain method. The WebKit/win changes should be fine because you're just renaming the function, not relocating it in the vtable.

The same changes you've made to DRT/mac need to be made to DRT/win.

It would be a little clearer to rename the origin argument to postMessage to "targetOrigin". It looks like HTML 5 is headed in this direction as well (see <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-April/014414.html>).

If you are feeling up to it, it would be nice to see this patch broken up into smaller independent changes. Some pieces that I think could be broken off:

1. Removal of the m_portSet member from SecurityOrigin.
2. Renaming stringIdentifier -> databaseIdentifier
3. Adding m_domain and domain()
4. Changing SecurityOrigin constructor to take a KURL

This isn't required, but would make reviewing easier.

r- so that we can address the WebSecurityOrigin/DRT issues.

[Collin Jackson]

Created attachment 20631 [details]
Updated patch to address aroben's comments

Restored deprecated "domain" API to WebSecurityOrigin.mm
Renamed "origin" argument to "targetOrigin" in DOMWindow and JSDOMWindowCustom

There aren't any changes required for DumpRenderTree/win because it doesn't implement setDatabaseQuota. (It probably should... do we have a bug for that?)

Bitrot fixes:
Updated the new SecurityOrigin constructors to handle the new ThreadSafeShared behavior in r31971.
The addMessageToConsole method disappeared in r31951, changed to call Console::addMessage

[Adam Roben (:aroben)]

Comment on attachment 20631 [details]
Updated patch to address aroben's comments

r31975 added protocolHostAndPortAreEqual to KURL.h. Should we use that instead of adding SecurityOrigin::isSameSchemeHostPort?

+    // Sender is not allowed to see exceptions other than syntax errors
+    ExceptionCode ec; 
+    document()->dispatchEvent(new MessageEvent(message, sourceOrigin, source), ec, true);

Should ec be initialized to 0?

+        [DoNotCheckDomainSecurity, Custom] void postMessage(in DOMString message, in [Optional] DOMString origin)

Let's call it targetOrigin here as well.

+    if (m_port) {
+      append(result, ":");
+      append(result, String::number(m_port));
+    }

Looks like the indentation got messed up here.

+        // Okay deleted value because "invalid-protocol" is not a valid protocol.

What makes it invalid?

You should use svn cp when renaming the tests so that the history is preserved (maybe  you already did this and it didn't come through in the diff).

r=me, but feel free to fix the above. You may also want to get Sam Weinig to give this a quick once-over before landing.

[Collin Jackson]

Created attachment 20703 [details]
Updated patch to address aroben's comments

(In reply to comment #36)
> (From update of attachment 20631 [details] [edit])
> r31975 added protocolHostAndPortAreEqual to KURL.h. Should
> we use that instead of adding SecurityOrigin::isSameSchemeHostPort?

What is protocolHostAndPortAreEqual supposed to be used for? Right now it doesn't seem to be called anywhere. It seems better to have origin-related security checks be done by the SecurityOrigin class.

> +    // Sender is not allowed to see exceptions other than syntax errors
> +    ExceptionCode ec; 
> +    document()->dispatchEvent(new MessageEvent(message, sourceOrigin, source),
> ec, true);
> 
> Should ec be initialized to 0?

Since it's just a placeholder for a return value that we throw away, I don't think it needs to be initialized. There are other examples of this pattern in WebKit.

> +        [DoNotCheckDomainSecurity, Custom] void postMessage(in DOMString
> message, in [Optional] DOMString origin)
> 
> Let's call it targetOrigin here as well.

Fixed.

> +    if (m_port) {
> +      append(result, ":");
> +      append(result, String::number(m_port));
> +    }
> 
> Looks like the indentation got messed up here.

Fixed.

> +        // Okay deleted value because "invalid-protocol" is not a valid
> protocol.
> 
> What makes it invalid?

Only convention. We tightened up the interface to make it hard to construct nonsense security origins, but SecurityOriginHash wants a nonsense security origin anyway. The code was relying on a different convention (file URLs don't have non-zero ports) before, but it's no longer possible to construct that security origin, so we went with a nonsense scheme instead.

I've changed the comment to the following: 
// Ok deleted value assuming "invalid-protocol" is not a valid protocol

> You should use svn cp when renaming the tests so that the history is preserved
> (maybe  you already did this and it didn't come through in the diff).

Fixed.

I think this patch is ready to land.

[Adam Barth]

Ian has updated the HTML5 spec to make postMessage asynchronous.

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-April/014551.html

It probably makes sense to land this change as is and handle the sync -> async conversion in another bug.

[Sam Weinig]

Fix landed (finally, sorry guys) in r32597.