Bug 24853 – Provide a way for WebKit clients to specify a more granular policy for cross-origin XHR access

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Rafael Weinstein]

This change creates a platform-pluggable SecurityOriginDelegate which allows the client to whitelist URLs for access & request in special circumstances. This is necessary for the chromium extensions mechanism to have static, installed pages that have been granted permissions to make out-bound XHR requests.

I have consulted with Adam Barth (abarth, cc'd above) on the design of this change.

[Rafael Weinstein]

Created attachment 28979 [details]
Create SecurityOriginDelegate that is called from SecurityOrigin::canAccess() & canRequest()

[Darin Fisher (:fishd, Google)]

I think there is a desire to not have platform depend on other parts of WebCore.  There was an email from Dave Hyatt on this subject to webkit-dev some while back.

(The Chromium port clearly violates that with ChromiumBridge, but that is fairly isolated.)

[Adam Barth]

Maybe we'd prefer to have this be a client (akin to FrameLoaderClient)?  Note that there can be several frames associated with a single SecurityOrigin.

(Also, we should pass SecurityOrigins by pointer instead of by constant reference.)

[Darin Fisher (:fishd, Google)]

Other issues:  Do we need to worry about SecurityOrigin usage by worker threads?

[Adam Barth]

Here is some discussion we had by email:

On Thu, Mar 26, 2009 at 2:30 PM, abarth wrote:
> We could also have this discussion in the bug so it will be archived.
> I wonder if we should somehow separate the "policy" part of
> SecurityOrigin (who can access whom) from the "data" part (who am I).
>
> Adam
>
>
>On Thu, Mar 26, 2009 at 2:24 PM, fishd wrote:
>> That could work.  By the way, is SecurityOrigin used on worker threads?  How
>> should this kind of thing work there?
>> +weinig since I'm sure he may be interested in any proposed changes to
>> SecurityOrigin.
>> -Darin
>>
>>
>> On Thu, Mar 26, 2009 at 2:22 PM, abarth wrote:
>>>
>>> I'm not sure we want to add these functions to FrameLoaderClient.
>>> Maybe FrameLoaderClient can give us a SecurityOriginClient that
>>> SecurityOrigin can hang on to and use to answer these questions?

[Alexey Proskuryakov]

Yes, it seems wrong to have delegate calls that can be invoked on arbitrary threads (which would happen here).

[Alexey Proskuryakov]

Comment on attachment 28979 [details]
Create SecurityOriginDelegate that is called from SecurityOrigin::canAccess() & canRequest()

> This is necessary for the chromium extensions mechanism to have static,
> installed pages that have been granted permissions to make out-bound XHR
> requests.

Perhaps you could use a custom URL scheme for such static pages? These are registered via FrameLoader::registerURLSchemeAsLocal() (it needs to be invoked before secondary threads are spawned).

r-, as this feature doesn't seem to warrant introducing callbacks that work from multiple threads.

[Adam Barth]

(In reply to comment #7)
> Perhaps you could use a custom URL scheme for such static pages? These are
> registered via FrameLoader::registerURLSchemeAsLocal() (it needs to be invoked
> before secondary threads are spawned).

I'm not sure that will work for this use case because extensions aren't given universal access.  They're granted access to specific domains.

[Aaron Boodman]

So, what should be our next step with this enhancement?

To back up and give context, we are working on extensions ("add-ons") for Chromium. One feature we would like to provide these extensions is the ability to use XHR with multiple origins. But instead of giving every extension blanket access to all origins, we want the extension to declare the sets of origins it wants access to.

In order to implement this, we need WebCore to ask us whether it is OK for a particular page to use XHR with a particular origin, instead of deciding based only on its own built-in policy.

Perhaps a more targeted change, that only affects XHR would be better. For example, we don't really need (and probably don't want) the ability to do cross-frame scripting. It seems like this change would allow that.

To answer Darin Fisher's question: we do want this to affect workers. Extensions should be able to use workers, and those workers should be able to perform cross-origin XHR, if the extension has declared it needs access to those origins.

[Sam Weinig]

Would passing in the set of allowed origins work?

[Aaron Boodman]

(In reply to comment #10)
> Would passing in the set of allowed origins work?

That is a possibility, but note that the set is defined using a syntax, it isn't just a list (http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/url_pattern.h?revision=9813&view=markup).

To me it seems cleaner to have WebCore call out for this. But if we wanted to pass it in, were would be the right place to do that, and where would we store the state?

[Alexey Proskuryakov]

(In reply to comment #11)
> To me it seems cleaner to have WebCore call out for this. 

I agree that this is cleaner, too bad this code is executed from multiple threads. Perhaps the real solution is to move it out of XMLHttpRequest, so that it would only ever work on the main thread? We need to do eventually this anyway, see bug 24462.

> But if we wanted to pass it in, were would be the right place to do that, 
> and where would we store the state?

FrameLoader::registerURLSchemeAsLocal() is clearly the example to follow in that case, for consistency is nothing else.

[Sam Weinig]

If we really do need to use a client here, we should probably follow the strategy of passing in the client into the origin.

[Aaron Boodman]

Picking this up again...

(In reply to comment #7)
> Perhaps you could use a custom URL scheme for such static pages? These are
> registered via FrameLoader::registerURLSchemeAsLocal() (it needs to be invoked
> before secondary threads are spawned).
> 
> r-, as this feature doesn't seem to warrant introducing callbacks that work
> from multiple threads.

Our feature requires a client as we want to implement logic to decide whether an origin is allowed, not just a static set (and it doesn't seem right to pull that logic into WebCore).

It is unfortunate to have client callbacks happening on multiple threads, but we can document this in comments. Also, when the work to move preflight into the main thread lands, it seems like it will automatically solve that issue, no matter what we do now. It doesn't seem like we need to block this issue on that one.


(In reply to comment #2)
> I think there is a desire to not have platform depend on other parts of
> WebCore.  There was an email from Dave Hyatt on this subject to webkit-dev some
> while back.
> 
> (The Chromium port clearly violates that with ChromiumBridge, but that is
> fairly isolated.)


(In reply to comment #13)
> If we really do need to use a client here, we should probably follow the
> strategy of passing in the client into the origin.


So here is a proposal that I think addresses these two issues. Sam and Darin, can you comment?

a) Move SecurityOriginDelegate.h into page/, next to SecurityOrigin.h (and rename it SecurityOriginClient). That way we don't have the issue Darin brought up.
b) Add a setClient() method to SecurityOrigin.
c) Add a securityOriginClient() method to FrameLoaderClient
d) Use frameLoader->client()->securityOriginClient() in Document.cpp and WorkerContext.cpp to populate the SecurityOrigins they create 
with client
s.

Thoughts?

[Alexey Proskuryakov]

> c) Add a securityOriginClient() method to FrameLoaderClient

Can't we just add these methods to FrameLoaderClient? They seem to fit well there.

[Aaron Boodman]

(In reply to comment #15)
> > c) Add a securityOriginClient() method to FrameLoaderClient
> 
> Can't we just add these methods to FrameLoaderClient? They seem to fit well
> there.
> 

It isn't currently possible to access FrameLoaderClient from the XHR code where these checks are being done. There is an idea to refactor things such that the checking would be done in DocumentThreadableLoader, where FrameLoaderClient is accessible, but that hasn't been done yet.

On IRC, Alexey and I worked out essentially two possible paths forward:

a) Do the refactor to move access checking for XHR into DocumentThreadableLoader, then add methods to FrameLoaderClient to fill these needs.

b) Push static data into WebCore, similar to how FrameLoader::registerURLSchemesAsLocal() works today. We'd have to push the data in a form that WebCore understands, either by teaching it how to understand Chromium URL patterns, or pre-processing to something like a regex.

[Darin Fisher (:fishd, Google)]

> > Can't we just add these methods to FrameLoaderClient? They seem to fit well
> > there.

That's what I think we should do too.


> It isn't currently possible to access FrameLoaderClient from the XHR code 
> where these checks are being done. There is an idea to refactor things such 
> that the checking would be done in DocumentThreadableLoader, where
> FrameLoaderClient is accessible, but that hasn't been done yet.

I believe that refactoring is required anyways for workers.  Presently, SecurityOrigin is not safe to use on multiple threads.  It calls FrameLoader::shouldTreatURLSchemeAsLocal, which is not thread safe.  That method uses a lazily allocated static hash map that can also be modified at runtime.  So, using SecurityOrigin from multiple threads is clearly a bug.

I'm pretty convinced that the correct solution to both that problem and the one that is the subject of this bug, is to move XHR security checking off of the worker thread.


> b) Push static data into WebCore, similar to how
> FrameLoader::registerURLSchemesAsLocal() works today. We'd have to push the
> data in a form that WebCore understands, either by teaching it how to
> understand Chromium URL patterns, or pre-processing to something like a regex.

This solution has the subtle downside that the rules would apply to any frame in the system that loads the URL of the privileged extension.  It would be more secure to only grant the special rights to the particular frame(s) that you are loading the extension into.

[Aaron Boodman]

Hello all,

I'm returning to this work now. I've already refactored the access control code out onto the main thread (https://bugs.webkit.org/show_bug.cgi?id=28134), so it should be possible to implement this now without having delegates getting called on multiple threads.

However, Darin Adler and Maciej both have suggested that they have a minor preference that this be implemented with more of the logic in WebKit than outside of it.

So I have a proposal I would like feedback on:

* Add static state to SecurityOrigin.cpp that keeps track of which origins each origin can access. This state would look something like:

map<origin, list<OriginAccessInfo>>

where OriginAccessInfo is something like:

struct OriginAccessInfo {
  string domain;
  bool subdomains;  // whether subdomain access is also allowed
  bool secure;  // http vs https
}

* Add static members to SecurityOrigin.h to manipulate this list

* Plumb this through the mac WebKit API as SPI so that it can be tested

* In SecurityOrigin::canLoad(), consult this state to determine if an origin is accessible.


Is this feasible? Do people prefer it to the earlier one (putting a new callback method on FrameLoaderClient)?

[Aaron Boodman]

Created attachment 35037 [details]
Implements the concept of 'security origin access white lists'.

This patch adds the concept of a whitelist of origins that each origin can access in addition to those that the normal security policies would allow. Clients of WebCore can add items to this list to enable particular origins to break out of same-origin in a controlled way.

The particular features of the whitelist were chosen because they are the features needed by this code's first client -- Chromium extensions. But I think that the feature is pretty useful and might be used by other clients in the future.

I haven't yet implemented the WebKit frontends for any platforms other than mac. I wanted to get feedback on this first.

[Aaron Boodman]

Adding a few Darins. Darin Adler, cc'ing you because you recommend I take this approach in #webkit. Darin Fisher, cc'ing you just to circle back from when we talked about this months ago.

[Darin Adler]

This does sound like what we discussed.

Calling this on a single WebView seems a little strange, because JavaScript often causes one WebView to affect another; they tend to work in groups that need to share the same security settings. We normally use WebPreferences for such settings, to make it easy to share them among many WebView objects.

I do think that it's probably better to code this so it's easier to use correctly. If we did it with a delegate it would provide a lot of flexibility, but it would be a bit too much "do it yourself", and make it less likely that people would get it right.

[Adam Barth]

(In reply to comment #21)
> Calling this on a single WebView seems a little strange, because JavaScript
> often causes one WebView to affect another;

Where do we pipe in the noAccess schemes?  Seems like we should put this API in the same place as that one.

[Darin Adler]

(In reply to comment #22)
> Where do we pipe in the noAccess schemes?  Seems like we should put this API in
> the same place as that one.

Currently those are per-process global.

    +[WebView registerURLAsLocal:]

Doing that may be a mistake we should not repeat. Not sure.

[Aaron Boodman]

(In reply to comment #21)
> This does sound like what we discussed.

Cool.

> Calling this on a single WebView seems a little strange, because JavaScript
> often causes one WebView to affect another; they tend to work in groups that
> need to share the same security settings. We normally use WebPreferences for
> such settings, to make it easy to share them among many WebView objects.

The method is static on WebView (I think, I'm new to Objective-C), so it's not being called for a _single_ WebView.

I'm happy to move it wherever though. Would you prefer it on WebPreferences?

[Darin Adler]

(In reply to comment #24)
> The method is static on WebView (I think, I'm new to Objective-C), so it's not
> being called for a _single_ WebView.

I missed that! You're right. It's a class method.

> I'm happy to move it wherever though. Would you prefer it on WebPreferences?

We can start with this.

[Adam Barth]

Comment on attachment 35037 [details]
Implements the concept of 'security origin access white lists'.

I've only looked at the SecurityOrigin pieces.  In general, this approach seems good.  There are a couple of details that we should change:

1) OriginAccessWhiteListEntry should be in its own file.  In general, WebKit is one class per file.

2) I'm not a big fan of m_secure.  Seems like what you really want to do is pass in m_scheme.  Then you can pass in "https" when you were previously passing m_secure = true and "http" when you were previously passing m_secure = false.

3) Some style issues:

+    OriginAccessWhiteList* list = originAccessMap().get(toString());
+    if (list)
+        for (size_t i = 0; i < list->size(); ++i)
+            if (list->at(i)->matchesOrigin(*targetOrigin))
+                return true;

A more WebKity way to write this would be:

    if (OriginAccessWhiteList* list = originAccessMap().get(toString())) {
        for (size_t i = 0; i < list->size(); ++i) {
            if (list->at(i)->matchesOrigin(*targetOrigin))
                return true;
        }
    }

+    if (!list)
+    {
+        list = new OriginAccessWhiteList();
+        originAccessMap().set(source.toString(), list);
+    }

should be

    if (!list) {
        list = new OriginAccessWhiteList();
        originAccessMap().set(source.toString(), list);
    }

(several other occurances of this same style issue)

4) SecurityOrigin::resetOriginAccessWhiteLists is kind of ugly because it has a lot of delete operators.  Why not make OriginAccessWhiteListEntry a value type and store it directly in the Vector instead of storing pointers?  The current design seems like a premature optimization.

5) WebKit leaks a lot at exit.  I don't think we should be that concerned about it here.

6) Why do we do the work to figure out whether m_host is an IP address ever time we call matchesOrigin?  Seems like we should do that in the constructor.  We should probably make it an error to supply an IP address and includeSubdomains by making this an ASSERT.

[David Levin]

Comment on attachment 35037 [details]
Implements the concept of 'security origin access white lists'.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        * page/SecurityOrigin.cpp: Implement origin acces whitelists.
typo: acces 

Typically these comments are a bit more granular.  For example, see http://trac.webkit.org/changeset/47469
But at the same time not everyone does them.


> diff --git a/WebCore/page/SecurityOrigin.cpp b/WebCore/page/SecurityOrigin.cpp
> +class OriginAccessWhiteListEntry {
> +public:
> +    OriginAccessWhiteListEntry(const String& host, bool includeSubdomains, bool secure) : m_host(host), m_includeSubdomains(includeSubdomains), m_secure(secure) { }
As mentioned before: formatting and bool -> enum.

> +    bool matchesOrigin(const SecurityOrigin& origin)
> +        if (origin.host().length() > m_host.length() && origin.host()[origin.host().length() - m_host.length() - 1] == '.' && origin.host().endsWith(m_host, true))

I think you want "origin.host().endsWith(m_host, false)" (caseSensitive = false).


> +            return true;
> +static OriginAccessMap& originAccessMap()
> +{

It would be nice to add an assert that this is only accessed on the main thread. 

#include <wtf/threading.h>

ASSERT(isMainThread());



> +void SecurityOrigin::whiteListAccessToOrigin(const SecurityOrigin& source, const String& domain, bool includeSubdomains, bool secure)
> +{
> +    // FIXME: We leak these objects at process shutdown. Is that OK?
> +    OriginAccessWhiteList* list = originAccessMap().get(source.toString());

Doing toString multiple times seems non-ideal.

It would be nice to add

Assert(!source.isEmpty());

if (source.m_noAccess)
    return;



> diff --git a/WebCore/page/SecurityOrigin.h b/WebCore/page/SecurityOrigin.h
> +        static void whiteListAccessToOrigin(const SecurityOrigin& source, const String& domain, bool includeSubdomains, bool secure);
Consider s/domain/allowedDomain/


>
> diff --git a/WebKit/mac/WebView/WebViewPrivate.h b/WebKit/mac/WebView/WebViewPrivate.h
> +// Whitelists access from an origin (sourceOrigin) to a set of one or more origins described by the parameters:
> +// - host: The host to grant access to.
> +// - includeSubdomains: If host is a domain, setting this to YES will whitelist host and all its subdomains, recursively.
> +// - secure: Whether to grant access to https or http origins. If yes, only https origins will be whitelisted. If no, only http origins will be.
> ++ (void)_whiteListAccessToOrigin:(NSString*)sourceOrigin host:(NSString*)host includeSubdomains:(BOOL)includeSubdomains secure:(BOOL)secure;
It seems odd that the api name is "_whiteListAccessToOrigin" but the origin that is allowed is called host.

I'd recommend this
  + (void)_whiteListAccessFromOrigin:(NSString*)sourceOrigin allowedHost:(NSString*)host includeSubdomains:(BOOL)includeSubdomains secure:(BOOL)secure;
so that it reads better.  It current reads like the first parameter is the origin that you're allowing access to. (Consider this name change in other places too if you think it reads better.)



> diff --git a/WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm b/WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
> +void LayoutTestController::resetOriginAccessWhiteLists()
> +{
> +    [WebView _resetOriginAccessWhiteLists];
> +}


It seems that this
   [WebView _resetOriginAccessWhiteLists];
should be done in WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
  static void resetWebViewToConsistentStateBeforeTesting()
 
A that point there is no reason to expose this from layout test controller.

[Aaron Boodman]

Created attachment 35151 [details]
New patch

Addressed comments, added WebKit frontends and integration with DRT for qt and gtk. Did not do Windows because it looks pretty hairy and other things (database) also aren't done yet.

[Darin Adler]

Comment on attachment 35151 [details]
New patch

> +OriginAccessEntry::OriginAccessEntry(const String& protocol, const String& host, SubdomainSetting subdomainSetting) : m_protocol(protocol), m_host(host), m_allowSubdomains(subdomainSetting) {

We normally use a vertical format that puts each of these onto a separate line.

> +    ASSERT(protocol == "http" || protocol == "https");

A while back I had a patch that added a protocolIsInHTTPFamily() function for strings to KURL.h. Looking, though, it seems I did not land it.

> +    UChar last = m_host[m_host.length() - 1];

Can host be an empty string? Does this work properly in that case? I guess it does.

> +    m_hostIsIPAddress = last >= '0' && last <= '9';

CType.h's isASCIIDigit is the normal way we'd do this.

> +    if (m_protocol != origin.protocol())

Are these made canonically lowercase?

> +    if (m_host == origin.host())

Same question again.

> +    // FIXME: This hasn't actually been tested as I can't figure out a way to test it with the layout test system.

This is good to note, but maybe not with a comment in the source code.

> +#include "CString.h"

You should be including PlatformString.h here in CrossOriginAccess.h, not CString.h.

> +    OriginAccessEntry(const String& protocol, const String& host, SubdomainSetting subdomainSetting);

Normally we would omit the subdomainSetting argument name.

> +    bool matchesOrigin(const SecurityOrigin& origin);

Normally we would omit the origin argument name. And make this a const member function.

> +// static
> +void SecurityOrigin::whiteListAccessFromOrigin(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomains, bool allowDestinationSubdomains)

We don't normally include that "// static" comment, and while it's nice to know, it's not that useful to have it for this one function and not for the many other static member functions.

> +    String sourceString = sourceOrigin.toString();
> +    OriginAccessWhiteList* list = originAccessMap().get(sourceString);

Applies to other call sites too. If the sourceString is empty or null then this will crash. So we either need to know it's not empty or have a check for empty string. Inelegant, I know.

> +        list = new OriginAccessWhiteList();

No need for the parentheses here.

> +    for (OriginAccessMap::iterator iter = map.begin(); iter != map.end(); ++iter) {
> +        OriginAccessWhiteList* list = iter->second;
> +        delete list;
> +    }

There's a function template that does this. It's called deleteAllValues.

> ++ (void)_whiteListAccessFromOrigin:(NSString*)sourceOrigin destinationProtocol:(NSString*)destinationProtocol destinationHost:(NSString*)destinationHost allowDestinationSubdomains:(BOOL)allowDestinationSubdomains;

The semicolon at the end of this line is incorrect, albeit tolerated by the compiler.

The confusing WebKit style rule is that for Objective-C types such as NSString, we put the * after a space. I know, it's strange, but it's how we do it.

> [phone]  Aaron Boodman  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=24853: Provide a way for WebKit clients to
> +        specify a more granular policy for cross-origin XHR access.
> +
> +        * DumpRenderTree/LayoutTestController.cpp: Expose whiteListAccessFromOrigin() to layout tests.
> +        (whiteListAccessFromOriginCallback): Ditto.
> +        (LayoutTestController::staticFunctions): Ditto.
> +        * DumpRenderTree/LayoutTestController.h: Ditto.
> +        * DumpRenderTree/gtk/LayoutTestControllerGtk.cpp: Ditto.
> +        (LayoutTestController::whiteListAccessToOrigin): Ditto.
> +        * DumpRenderTree/mac/LayoutTestControllerMac.mm: Ditto.
> +        (LayoutTestController::whiteListAccessFromOrigin): Ditto.
> +        * DumpRenderTree/qt/jsobjects.cpp: Ditto.
> +        (LayoutTestController::whiteListAccessFromOrigin): Ditto.
> +        * DumpRenderTree/win/LayoutTestControllerWin.cpp: Stub out whiteListAccessFromOrigin().
> +        (LayoutTestController::whiteListAccessFromOrigin): Ditto.
> +        * DumpRenderTree/gtk/DumpRenderTree.cpp: Reset origin access lists before each test.
> +        (resetWebViewToConsistentStateBeforeTesting): Ditto.
> +        * DumpRenderTree/mac/DumpRenderTree.mm: Ditto.
> +        (resetWebViewToConsistentStateBeforeTesting): Ditto.
> +        * DumpRenderTree/qt/DumpRenderTree.cpp: Ditto.
> +        (WebCore::DumpRenderTree::resetToConsistentStateBeforeTesting): Ditto.
> +
> [phone]  Aaron Boodman  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=24853: Provide a way for WebKit clients to
> +        specify a more granular policy for cross-origin XHR access.
> +
> +        * DumpRenderTree/LayoutTestController.cpp: Add plumbing to be able to manipulate
> +        origin access whitelists from layout tests.
> +        (whiteListAccessToOriginCallback): Ditto.
> +        (resetOriginAccessWhiteListsCallback): Ditto.
> +        (LayoutTestController::staticFunctions): Ditto.
> +        * DumpRenderTree/LayoutTestController.h: Ditto.
> +        * DumpRenderTree/mac/LayoutTestControllerMac.mm: Ditto.
> +        (LayoutTestController::whiteListAccessToOrigin): Ditto.
> +        (LayoutTestController::resetOriginAccessWhiteLists): Ditto.
> +

Double change log.

Looks good.

review- because of the comments above

[David Levin]

Darin's comments include what I would have said and more.

The only thing to add is that the current patch only fixes up the osx build system.  The new files need to be added to the other build systems as well.

[Aaron Boodman]

Created attachment 35161 [details]
Now with even more code!

[Aaron Boodman]

(In reply to comment #29)
> > +    ASSERT(protocol == "http" || protocol == "https");
> 
> A while back I had a patch that added a protocolIsInHTTPFamily() function for
> strings to KURL.h. Looking, though, it seems I did not land it.

Ok, so I just left this as-is.

> > +    UChar last = m_host[m_host.length() - 1];
> 
> Can host be an empty string? Does this work properly in that case? I guess it
> does.

Thanks for pointing this out. It was working that way correctly only by chance, I have fixed it.

> > +    if (m_protocol != origin.protocol())
> 
> Are these made canonically lowercase?
>
> > +    if (m_host == origin.host())
> 
> Same question again.

I have now manually forced the state in OriginAccessEntry to be lower-case. For the SecurityOrigin input to ::matchesOrigin(), SecurityOrigin currently lower-cases all the components on construction, and I want to avoid duplicating that. I've added ASSERTs that this remains true in the future.

> > +// static
> > +void SecurityOrigin::whiteListAccessFromOrigin(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomains, bool allowDestinationSubdomains)
> 
> We don't normally include that "// static" comment, and while it's nice to
> know, it's not that useful to have it for this one function and not for the
> many other static member functions.

I only did this because the rest of this file has a bunch of // static comments. Anyway, I've removed it.

> > +    String sourceString = sourceOrigin.toString();
> > +    OriginAccessWhiteList* list = originAccessMap().get(sourceString);
> 
> Applies to other call sites too. If the sourceString is empty or null then this
> will crash. So we either need to know it's not empty or have a check for empty
> string. Inelegant, I know.

I already had an ASSERT here that sourceOrigin is non-empty. Is there more that I should do?

> > +    for (OriginAccessMap::iterator iter = map.begin(); iter != map.end(); ++iter) {
> > +        OriginAccessWhiteList* list = iter->second;
> > +        delete list;
> > +    }
> 
> There's a function template that does this. It's called deleteAllValues.

Cool, thanks for the pointer.

(In reply to comment #30)
> Darin's comments include what I would have said and more.
> 
> The only thing to add is that the current patch only fixes up the osx build
> system.  The new files need to be added to the other build systems as well.

Myriad build systems updated.


All other comments addressed I think.

[Darin Adler]

(In reply to comment #32)
> > > +    String sourceString = sourceOrigin.toString();
> > > +    OriginAccessWhiteList* list = originAccessMap().get(sourceString);
> > 
> > Applies to other call sites too. If the sourceString is empty or null then this
> > will crash. So we either need to know it's not empty or have a check for empty
> > string. Inelegant, I know.
> 
> I already had an ASSERT here that sourceOrigin is non-empty. Is there more that
> I should do?

If the assert is correct, then the code is fine. But asserting something doesn't make it so. I assume there's a simple reason to know it won't be empty; you don't have to convince me, I just want to be sure you know it to be true.

[Aaron Boodman]

Created attachment 35166 [details]
rebase patch to HEAD

This just updates the patch to apply cleanly to current HEAD.

[David Levin]

Comment on attachment 35166 [details]
rebase patch to HEAD

Looks good to me modulo a few nits....


WebCore/ChangeLog:
  It appears to be missing a few file names.


> diff --git a/WebCore/GNUmakefile.am b/WebCore/GNUmakefile.am
>  	WebCore/page/Navigator.h \
>  	WebCore/page/NavigatorBase.cpp \
>  	WebCore/page/NavigatorBase.h \
> +	WebCore/page/OriginAccessEntry.h \
> +	WebCore/page/OriginAccessEntry.cpp \

cpp then h


> diff --git a/WebCore/WebCore.base.exp b/WebCore/WebCore.base.exp
> +__ZN7WebCore14SecurityOrigin27resetOriginAccessWhiteListsEv
> +__ZN7WebCore14SecurityOrigin25whiteListAccessFromOriginERKS0_RKNS_6StringES5_b

These seem out of sort order 27 after 25.


> diff --git a/WebCore/page/OriginAccessEntry.cpp b/WebCore/page/OriginAccessEntry.cpp
> new file mode 100644
> index 0000000..c0e92fe
> --- /dev/null
> +++ b/WebCore/page/OriginAccessEntry.cpp
> @@ -0,0 +1,80 @@
> +/*
> + * Copyright (C) 2009 Apple Inc. All rights reserved.

Consider using http://trac.webkit.org/browser/trunk/WebCore/loader/ThreadableLoader.h as a basis for your copyright.


> +#include "config.h"
> +#include "CType.h"
> +#include "OriginAccessEntry.h"

OriginAccessEntry.h should be immediately after config.h (like Google style except add in a config.h first).

Then a blank like and CType.h, SecurityOrigin.h

> diff --git a/WebCore/page/OriginAccessEntry.h b/WebCore/page/OriginAccessEntry.h
> +/*
> + * Copyright (C) 2009 Apple Inc. All rights reserved.
Same copyright comment.

> diff --git a/WebCore/page/SecurityOrigin.cpp b/WebCore/page/SecurityOrigin.cpp
>  #include "CString.h"
>  #include "FrameLoader.h"
> +#include "OriginAccessEntry.h"

Should be after KURL.h

>  #include "KURL.h"

> diff --git a/WebCore/page/SecurityOrigin.h b/WebCore/page/SecurityOrigin.h
> +        static void whiteListAccessFromOrigin(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomains, bool allowDestinationSubdomains);

Omit param name: sourceOrigin
since it doesn't add any information.


> diff --git a/WebKit/gtk/webkit/webkitprivate.cpp b/WebKit/gtk/webkit/webkitprivate.cpp
>  #include "ResourceHandleClient.h"
>  #include "ResourceHandleInternal.h"
>  #include <runtime/InitializeThreading.h>
> +#include "SecurityOrigin.h"

This include is out of order.

[Aaron Boodman]

Created attachment 35172 [details]
New patch

[Aaron Boodman]

(In reply to comment #33)
> If the assert is correct, then the code is fine. But asserting something
> doesn't make it so. I assume there's a simple reason to know it won't be empty;
> you don't have to convince me, I just want to be sure you know it to be true.

My thinking was that this is API and that it should just be part of the contract that callers do not pass empty strings.

On second thought though, I think since clients will likely be passing in computed data, it would be nice to fail somewhat more gracefully than crashing.

So I've fixed that.


(In reply to comment #35)

> WebCore/ChangeLog:
>   It appears to be missing a few file names.

I think they are all there, though the formatting was weird. Fixed.

> > diff --git a/WebCore/page/SecurityOrigin.h b/WebCore/page/SecurityOrigin.h
> > +        static void whiteListAccessFromOrigin(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomains, bool allowDestinationSubdomains);
> 
> Omit param name: sourceOrigin
> since it doesn't add any information.

I feel like it clarifies a little, especially in contrast with destination* param names. Do you mind if I leave it?

> > diff --git a/WebKit/gtk/webkit/webkitprivate.cpp b/WebKit/gtk/webkit/webkitprivate.cpp
> >  #include "ResourceHandleClient.h"
> >  #include "ResourceHandleInternal.h"
> >  #include <runtime/InitializeThreading.h>
> > +#include "SecurityOrigin.h"
> 
> This include is out of order.

It is? It seems OK.


All else done.

[Aaron Boodman]

Note: Please do not commit-queue+ this, as I am out for the rest of the night, and am quite certain the non-mac world will catch on fire when this is submitted. I want to be there when it does so I can put it out.

[Adam Barth]

Comment on attachment 35172 [details]
New patch

Marking as commit-queue- per aa's request.  Note that you should feel free to do this yourself.  :)

[Adam Barth]

Comment on attachment 35172 [details]
New patch

The SecurityOrigin and OriginAccessEntry parts look great!  Two nits:

m_allowSubdomains should be m_subdomainSetting

+ // Special case: Include subdomains and empty host means "all hosts, including ip addresses".

Please note this in the header.

[Aaron Boodman]

Created attachment 35181 [details]
New patch

Made abarth's comment.

[David Levin]

Comment on attachment 35181 [details]
New patch

Only one minor nit which would be nice for someone to fix on landing.... (I'm willing to do so).

> diff --git a/WebKit/gtk/webkit/webkitprivate.cpp b/WebKit/gtk/webkit/webkitprivate.cpp
>  #include "ResourceHandleClient.h"
>  #include "ResourceHandleInternal.h"
>  #include <runtime/InitializeThreading.h>
> +#include "SecurityOrigin.h"

The <> headers come after the "" headers.

 

> > Omit param name: sourceOrigin
> I feel like it clarifies...
You are right.  I read it too quickly as securityOrigin.

> I think they are all there, though the formatting was weird. Fixed
Thanks.  I must have read it too quickly again.

[David Levin]

Committed as http://trac.webkit.org/changeset/47548


> The <> headers come after the "" headers.
Once I got in this file, I saw that it didn't follow the standard header ordering and Aaron's change followed the style of the file better so I left this as it was in the patch.