[webkit-reviews] review denied: [Bug 24853] Provide a way for WebKit clients to specify a more granular policy for cross-origin XHR access : [Attachment 35037] Implements the concept of 'security origin access white lists'.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Aaron Boodman <aa at chromium.org>'s
request for review:
Bug 24853: Provide a way for WebKit clients to specify a more granular policy
for cross-origin XHR access
https://bugs.webkit.org/show_bug.cgi?id=24853

Attachment 35037: Implements the concept of 'security origin access white
lists'.
https://bugs.webkit.org/attachment.cgi?id=35037&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.