CSP Suborigins

Basic experimental suborigins implementation.

BUG=336894

[jww, 2013-10-12 00:04:40]

Adam, I didn't get very far this week, but my first swing at this is basically
duplicating the CSP sandbox infrastructure. Obviously, I have some renaming and
whatnot to do, but let me know if you think this isn't the right path to go
down.

[abarth-chromium, 2013-10-13 05:14:50]

It's hard for me to give you much feedback on this CL.  Maybe I should take
another look when you're further along.

https://codereview.chromium.org/27073003/diff/1/Source/core/dom/SecurityConte...
File Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/27073003/diff/1/Source/core/dom/SecurityConte...
Source/core/dom/SecurityContext.h:58: typedef String SuboriginFlags;
We don't usually create typedefs for Strings.  If you want to make a type here,
please make a class with a String as a member.

[jww, 2013-10-14 16:04:28]

On 2013/10/13 05:14:50, abarth wrote:
> It's hard for me to give you much feedback on this CL.  Maybe I should take
> another look when you're further along.
sgtm
> 
>
https://codereview.chromium.org/27073003/diff/1/Source/core/dom/SecurityConte...
> File Source/core/dom/SecurityContext.h (right):
> 
>
https://codereview.chromium.org/27073003/diff/1/Source/core/dom/SecurityConte...
> Source/core/dom/SecurityContext.h:58: typedef String SuboriginFlags;
> We don't usually create typedefs for Strings.  If you want to make a type
here,
> please make a class with a String as a member.

[jww, 2014-01-22 20:10:41]

This is a basic, working prototype. No need to review yet. I want to get
comments from others before we go down that route, not to mention that I
definitely still need tests.

[jww, 2014-06-14 01:06:00]

On 2014/01/22 20:10:41, jww wrote:
> This is a basic, working prototype. No need to review yet. I want to get
> comments from others before we go down that route, not to mention that I
> definitely still need tests.

Adam, I'd love if you can start taking a look at this whenever you get a chance.

One notable issue right off the bat is the best way to put this behind an
experimental flag. Right now, I put it behind the CSP experimental flag.
However, one issue is with how the CORS error messages interact with it. Because
the CSP experimental flag is turned on in LayoutTests, I can't figure out a
great way to integrate the new CORS error messages in
CrossOriginAccessControl.cpp with the already existing tests in LayoutTests
(such as the http/tests/security/script-crossorigin-* tests). For now, I just
explicitly disabled the new CORS messages (but left in the finer-origin
behavior). Any thoughts on the best approach to how to handle interaction with
the cross origin tests?

[jww, 2014-07-11 01:29:58]

On 2014/06/14 01:06:00, jww wrote:
> On 2014/01/22 20:10:41, jww wrote:
> > This is a basic, working prototype. No need to review yet. I want to get
> > comments from others before we go down that route, not to mention that I
> > definitely still need tests.
> 
> Adam, I'd love if you can start taking a look at this whenever you get a
chance.
> 
> One notable issue right off the bat is the best way to put this behind an
> experimental flag. Right now, I put it behind the CSP experimental flag.
> However, one issue is with how the CORS error messages interact with it.
Because
> the CSP experimental flag is turned on in LayoutTests, I can't figure out a
> great way to integrate the new CORS error messages in
> CrossOriginAccessControl.cpp with the already existing tests in LayoutTests
> (such as the http/tests/security/script-crossorigin-* tests). For now, I just
> explicitly disabled the new CORS messages (but left in the finer-origin
> behavior). Any thoughts on the best approach to how to handle interaction with
> the cross origin tests?

Hey Adam. Any chance you can take a look at this?

[abarth-chromium, 2014-07-31 04:56:48]

Don't we need to do something with the securityToken we give to V8?

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
Source/core/dom/ExecutionContext.cpp:69: , m_suboriginName("")
We should just let this automatically initialize to the null string.

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
Source/core/dom/ExecutionContext.h:158: String m_suboriginName;
Why do we store the m_suboriginName here but have suboriginName() grab it out of
m_client?

https://codereview.chromium.org/27073003/diff/67001/Source/core/fetch/CrossOr...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/fetch/CrossOr...
Source/core/fetch/CrossOriginAccessControl.cpp:150: // return
RuntimeEnabledFeatures::experimentalContentSecurityPolicyFeaturesEnabled();
?

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/CSP...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/CSP...
Source/core/frame/csp/CSPDirectiveList.cpp:536:
m_policy->enforceSuborigin(parseSuboriginName(suboriginPolicy, invalidTokens));
parseSuboriginName <-- This function should be with the rest of the CSP parsing
functions, not off in a separate file.

Can parseSuboriginName fail?  If it fails, should we not enforceSuborigin?

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
Source/core/frame/csp/ContentSecurityPolicy.cpp:553: void
ContentSecurityPolicy::enforceSuborigin(String name) const
const String&

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
File Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
Source/core/frame/csp/ContentSecurityPolicy.h:58: typedef String SuboriginFlags;
I'd skip this typedef.

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
Source/platform/weborigin/SecurityOrigin.cpp:128: , m_suboriginName(String())
The compiler will generate this code for you.

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
Source/platform/weborigin/SecurityOrigin.cpp:153: , m_suboriginName(String())
ditto

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
File Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
Source/platform/weborigin/SecurityOrigin.h:177: String suboriginName() const {
return m_suboriginName; }
const String&

[abarth-chromium, 2014-07-31 04:58:26]

Should we include information about the suborigin when generating JavaScript
access failure messages in the console?

[abarth-chromium, 2014-07-31 05:02:06]

https://codereview.chromium.org/27073003/diff/67001/LayoutTests/http/tests/se...
File LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html
(right):

https://codereview.chromium.org/27073003/diff/67001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:1:
<meta http-equiv="Content-Security-Policy" content="suborigin foobar">
I don't understand how this works.  Does this mean a document can change
suborigins in the middle of execution?  For example, if we put a <script> block
above the meta tag, would that script execute outside the suborigin?

If we permit that, we'll be getting into a bunch of the insanity of
document.domain where you can have a JavaScript reference to an object that you
can no longer access.

I thought for |sandbox| we prevented it from being engaged via the meta tag. 
Instead, you have to use the HTTP header, which means the document is in the
sandbox from the start.  IMHO, we should do the same thing here.

[jww, 2014-10-21 23:51:07]

Besides my inline response, here are a few additional responses to your
additional questions:

> Don't we need to do something with the securityToken we give to V8?

I believe this is taken care of for free by SecurityOrigin::buildRawString. When
the security token is set, it is done by calling SecurityOrigin::toString(),
which eventually leads to buildRawString. The modifications in this CL add the
suborigin+<name> prefix to this method. I would love a suggestion on how to unit
test this, though.

> Should we include information about the suborigin when generating JavaScript
access failure messages in the console?

Yes, that makes perfect sense to me. Any suggestions on where I can start
looking to make that happen?

https://codereview.chromium.org/27073003/diff/67001/LayoutTests/http/tests/se...
File LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html
(right):

https://codereview.chromium.org/27073003/diff/67001/LayoutTests/http/tests/se...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:1:
<meta http-equiv="Content-Security-Policy" content="suborigin foobar">
On 2014/07/31 05:02:05, abarth wrote:
> I don't understand how this works.  Does this mean a document can change
> suborigins in the middle of execution?  For example, if we put a <script>
block
> above the meta tag, would that script execute outside the suborigin?
> 
> If we permit that, we'll be getting into a bunch of the insanity of
> document.domain where you can have a JavaScript reference to an object that
you
> can no longer access.
> 
> I thought for |sandbox| we prevented it from being engaged via the meta tag. 
> Instead, you have to use the HTTP header, which means the document is in the
> sandbox from the start.  IMHO, we should do the same thing here.

I 100% agree. I should have mentioned this in my original comments, but for
experimental purposes, I am allowing <meta> suborigin creation for ease of use.
Definitely for the Real Thing, this is not acceptable, and it needs to be header
only. For now, bear with me on this, and I'll separately try to take a stab at
getting this to work header-only.

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
Source/core/dom/ExecutionContext.cpp:69: , m_suboriginName("")
On 2014/07/31 04:56:47, abarth wrote:
> We should just let this automatically initialize to the null string.

No longer relevant (m_suboriginName is gone).

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
File Source/core/dom/ExecutionContext.h (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/dom/Execution...
Source/core/dom/ExecutionContext.h:158: String m_suboriginName;
On 2014/07/31 04:56:47, abarth wrote:
> Why do we store the m_suboriginName here but have suboriginName() grab it out
of
> m_client?

Yup, this was totally unnecessary, made even more clear by some of the
refactoring in ExecutionContext.h. Removed and simplified in the new version.

https://codereview.chromium.org/27073003/diff/67001/Source/core/fetch/CrossOr...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/fetch/CrossOr...
Source/core/fetch/CrossOriginAccessControl.cpp:150: // return
RuntimeEnabledFeatures::experimentalContentSecurityPolicyFeaturesEnabled();
On 2014/07/31 04:56:47, abarth wrote:
> ?

It seems this works better than I thought, so I'm removing this comment and
enabling the RuntimeEnabledFeatures::... check.

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/CSP...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/CSP...
Source/core/frame/csp/CSPDirectiveList.cpp:536:
m_policy->enforceSuborigin(parseSuboriginName(suboriginPolicy, invalidTokens));
On 2014/07/31 04:56:47, abarth wrote:
> parseSuboriginName <-- This function should be with the rest of the CSP
parsing
> functions, not off in a separate file.
Done.
> 
> Can parseSuboriginName fail?  If it fails, should we not enforceSuborigin?
Yes, it can fail, and correct, it doesn't enforce the suborigin in that case.
It's not clear in this code, because they way this is done is by passing a null
suborigin, which implies a lack of suborigin, and thus isn't enforced. I'm
changing this in the new version to make this clearer, though.

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
Source/core/frame/csp/ContentSecurityPolicy.cpp:553: void
ContentSecurityPolicy::enforceSuborigin(String name) const
On 2014/07/31 04:56:47, abarth wrote:
> const String&

Done.

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
File Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/27073003/diff/67001/Source/core/frame/csp/Con...
Source/core/frame/csp/ContentSecurityPolicy.h:58: typedef String SuboriginFlags;
On 2014/07/31 04:56:47, abarth wrote:
> I'd skip this typedef.

Done.

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
Source/platform/weborigin/SecurityOrigin.cpp:128: , m_suboriginName(String())
On 2014/07/31 04:56:47, abarth wrote:
> The compiler will generate this code for you.

Done.

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
Source/platform/weborigin/SecurityOrigin.cpp:153: , m_suboriginName(String())
On 2014/07/31 04:56:47, abarth wrote:
> ditto

Done.

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
File Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/27073003/diff/67001/Source/platform/weborigin...
Source/platform/weborigin/SecurityOrigin.h:177: String suboriginName() const {
return m_suboriginName; }
On 2014/07/31 04:56:48, abarth wrote:
> const String&

Done.

[abarth-chromium, 2014-10-22 01:18:59]

abarth@chromium.org changed reviewers:
- abarth@chromium.org

[abarth-chromium, 2014-10-22 01:19:13]

I'm probably not the best reviewer for this CL anymore...

[jww, 2014-10-22 16:27:13]

jww@chromium.org changed reviewers:
+ mkwst@chromium.org

[jww, 2014-10-22 16:27:54]

mkwst@, abarth@ has taken a once over at this point, but we thought it would
make sense for you to be the first actual reviewer. Would you mind taking a
first pass at this CL?

[Mike West, 2014-10-23 06:30:36]

Sure, I'll take a look today!

In the meantime, this should probably have an Intent to Implement thread on
blink-dev, and it would be worthwhile to clean up the spec you were kicking
around last year
(http://www.chromium.org/developers/design-documents/per-page-suborigins is the
last I've seen, I think) in order to include enough detail about the CSP
interaction for other vendors to start experimenting in a compatible way.

[Mike West, 2014-10-23 12:59:21]

Took a first pass at this. I was expecting to see some work on the
`postMessage()` API as well. Do you intend to bring that in via a subsequent
patch?

On the whole, I'd reiterate the need to have a solid spec for this. It's
difficult to evaluate some of the decisions without a clear understanding of the
end goal. That said, it looks like this is going in a good direction. Looking
forward to the next iteration!

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/contentSecurityPolicy/resources/suborigin.php
(right):

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/resources/suborigin.php:1:
<?php
Nit: I'd suggest moving the tests into http/tests/security/suborigins/ rather
than lumping them in with CSP. CSP is just the delivery mechanism.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html
(right):

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:1:
<meta http-equiv="Content-Security-Policy" content="suborigin foobar">
As I've noted way down somewhere in the code, we shouldn't do this. I'm in the
process of extracting us from layout tests that rely on `<meta>` distribution of
sandbox and etc. We shouldn't let ourselves off the hook here.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:1:
<meta http-equiv="Content-Security-Policy" content="suborigin foobar">
<!doctype html>
<html>
<head>
...
</head>
<body>
...
</body>
</html>

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:16:
testRunner.notifyDone();
Please use testHarness.js instead to make these assertions. That will let you
drop the -expected.txt file entirely.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-blocked-in-http-header.html
(right):

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-blocked-in-http-header.html:21:
<iframe onload="iframeLoaded();" id="iframe"
src="resources/suborigin.php?suborigin=foobar"></iframe>
As a general note, I'd suggest postMessaging the frame's loaded status up to the
parent.

https://codereview.chromium.org/27073003/diff/107001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:269: if (name.isNull())
isNull or isEmpty? Does enforcing an empty suborigin have a reasonable effect?

https://codereview.chromium.org/27073003/diff/107001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:273: if (!origin->hasSuborigin()) {
Should we do something other than fail silently if a suborigin is already set?

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:110: return
RuntimeEnabledFeatures::experimentalContentSecurityPolicyFeaturesEnabled();
For clarity, I'd suggest adding a RuntimeEnabledFeature flag for suborigins.
It's only related to CSP insofar as CSP is the delivery mechanism; the feature
itself is distinct.

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:125:
AtomicallyInitializedStatic(AtomicString&, accessControlAllowFinerOrigin = *new
AtomicString("access-control-allow-finer-origin",
AtomicString::ConstructFromLiteral));
"finer-origin"? Why not "suborigin"?

This is a spec question, obviously... not a question about this CL in
particular.

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:134: if
(accessControlOriginString == starAtom || accessControlAllowFinerOrigin ==
starAtom) {
This seems wrong: as written, either `a-c-allow-origin: *` OR
`a-c-allow-finer-origin: *` will return true for the access check. Shouldn't
both be required? (Again, a spec would be helpful: I may be misinterpreting the
intent entirely)

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:169: errorDescription = "The
'Access-Control-Allow-Origin' or 'Access-Control-Allow-Finer-Origin' header has
a value '" + accessControlOriginString + "' that is not equal to the supplied
origin. Origin '" + securityOrigin->toString() + "' is therefore not allowed
access.";
I'd suggest moving all the occurrences of these error strings out to a helper
function: something like String::format("The %s header has blah blah",
isEnabled() ? "a or b" : "a") in one place would be simpler to read and less
repetitive.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:549: }
abarth@ is right: we shouldn't land this with <meta> support. I tried to take
out <meta> support for sandbox this morning, and it's a pain because layout
tests rely on it.

We should do this right from the beginning: please add an `m_headerType` check
here.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:551: String invalidTokens;
Nit: Unused?

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:553: if (!suboriginName.isNull()) {
Again, isNull or isEmpty?

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:555: }
Nit: No {} for oneliners.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:671: return String();
I think s/String()/emptyString()/ would be marginally more efficient.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:684:
m_policy->reportInvalidSuboriginFlags("Invalid whitespace in suborigin.
Whitespace may only appear at the beginning and end of a suborigin, and it is
ignored.");
1. I think you can clarify this error message: "Whitespace isn't allowed in
suborigin names." perhaps?

2. You need to `return emptyString()` after the error, otherwise you're
returning the first portion of the block.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:732: } else if
(equalIgnoringCase(name, ContentSecurityPolicy::Suborigin)) {
This ought to be guarded by the experimental flag.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.h:78: String parseSuboriginName(const
String& policy);
Nit: Move this up next to the other parse functions.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.h:117: bool m_haveSuboriginPolicy;
Nit: These both should be "hasXXX", not "haveXXX". Sorry about that. Would you
mind cleaning up the existing usage in a separate CL? Or I'll do it if you don't
have time.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/Co...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.cpp:95: // Experimental Directives
(post CSP 1.1)
Nit: Since we changed the name, can you change both this and the above comment
to CSP2?

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.cpp:170: if
(experimentalFeaturesEnabled())
`&& hasSuborigin`?

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:68: return
(!origin1->hasSuborigin() && !origin2->hasSuborigin()) ||
(origin1->suboriginName() == origin2->suboriginName());
Is this any different from `origin1->suboriginName() ==
origin2->suboriginName()`? If neither has a suborigin, null-string ==
null-string.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:255: return false;
I assume that bypassing the `document.domain` checks is intentional?

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:418: return AlwaysDeny;
Why?

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:514: builder.append('+');
Is this the scheme we ended up with on the list?

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:528: PassRefPtr<SecurityOrigin>
SecurityOrigin::createFromString(const String& originString)
You're not handling creation of origins with suborigins?

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:146: bool canAccessDatabase() const {
return !isUnique() && !hasSuborigin(); };
Why are we blocking access to these for suborigins? Shouldn't we instead be
creating new storage areas for these suborigins?

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:157: bool canAccessSessionStorage()
const { return !isUnique(); }
Here too?

[Mike West, 2014-11-04 07:31:59]

Ping. You're coming back to this at some point, right? :)

[jww, 2014-11-04 18:05:40]

On 2014/11/04 07:31:59, Mike West wrote:
> Ping. You're coming back to this at some point, right? :)

For sure. I've been distracted by my password manager work for the last 2 weeks.
While I'll address the comments in this CL, hopefully this week, I expect that
I'll prioritize writing up a draft spec next, as it seems like that's the actual
prerequisite for pushing this forward.

[Mike West, 2014-11-04 18:07:06]

On 2014/11/04 18:05:40, jww wrote:
> On 2014/11/04 07:31:59, Mike West wrote:
> > Ping. You're coming back to this at some point, right? :)
> 
> For sure. I've been distracted by my password manager work for the last 2
weeks.
> While I'll address the comments in this CL, hopefully this week, I expect that
> I'll prioritize writing up a draft spec next, as it seems like that's the
actual
> prerequisite for pushing this forward.

Awesome! If you want help, let me know. :)

[jochen (gone - plz use gerrit), 2015-03-17 18:49:35]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-03-17 18:49:36]

can we add some test how suborigins interact with changing document.domain?

Maybe we should split up the change into

- adding layout tests of the behavior we want to have (they'll all fail at this
point of course)
- teaching SecurityOrigin about having a sub origin (+ unit tests for
serialization)
- access control header handling
- hooking everything up to CSP

[jww, 2015-03-18 00:16:39]

On 2015/03/17 18:49:36, jochen (slow) wrote:
> can we add some test how suborigins interact with changing document.domain?
> 
> Maybe we should split up the change into
> 
> - adding layout tests of the behavior we want to have (they'll all fail at
this
> point of course)
> - teaching SecurityOrigin about having a sub origin (+ unit tests for
> serialization)
> - access control header handling
> - hooking everything up to CSP

All around, sounds like a good idea. Just for completeness, I'm going to address
Mike's comments in this CL, and then I'll start splitting it up into the smaller
ones you suggested.

[jww, 2015-03-20 22:50:04]

Mike, can you take a look at my response to your comments and the respective
fixes?

I expect that, early next week, I'll start breaking this up in the way Jochen
suggested, to get this committed piece-by-piece.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/contentSecurityPolicy/resources/suborigin.php
(right):

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/resources/suborigin.php:1:
<?php
On 2014/10/23 12:59:18, Mike West wrote:
> Nit: I'd suggest moving the tests into http/tests/security/suborigins/ rather
> than lumping them in with CSP. CSP is just the delivery mechanism.

Done.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html
(right):

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:1:
<meta http-equiv="Content-Security-Policy" content="suborigin foobar">
On 2014/10/23 12:59:18, Mike West wrote:
> <!doctype html>
> <html>
> <head>
> ...
> </head>
> <body>
> ...
> </body>
> </html>

Done.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-allow.html:16:
testRunner.notifyDone();
On 2014/10/23 12:59:18, Mike West wrote:
> Please use testHarness.js instead to make these assertions. That will let you
> drop the -expected.txt file entirely.

Done.

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-blocked-in-http-header.html
(right):

https://codereview.chromium.org/27073003/diff/107001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/suborigin-blocked-in-http-header.html:21:
<iframe onload="iframeLoaded();" id="iframe"
src="resources/suborigin.php?suborigin=foobar"></iframe>
On 2014/10/23 12:59:18, Mike West wrote:
> As a general note, I'd suggest postMessaging the frame's loaded status up to
the
> parent.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:269: if (name.isNull())
On 2014/10/23 12:59:18, Mike West wrote:
> isNull or isEmpty? Does enforcing an empty suborigin have a reasonable effect?

You're right, the proposed spec requires at least one character, which should be
taken care of by parsing. Thus I'll add an ASSERT that this isn't empty.

https://codereview.chromium.org/27073003/diff/107001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:273: if (!origin->hasSuborigin()) {
On 2014/10/23 12:59:18, Mike West wrote:
> Should we do something other than fail silently if a suborigin is already set?

This should probably be an assert that there either isn't a suborigin or that
the new and old ones are the same.

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:110: return
RuntimeEnabledFeatures::experimentalContentSecurityPolicyFeaturesEnabled();
On 2014/10/23 12:59:19, Mike West wrote:
> For clarity, I'd suggest adding a RuntimeEnabledFeature flag for suborigins.
> It's only related to CSP insofar as CSP is the delivery mechanism; the feature
> itself is distinct.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:125:
AtomicallyInitializedStatic(AtomicString&, accessControlAllowFinerOrigin = *new
AtomicString("access-control-allow-finer-origin",
AtomicString::ConstructFromLiteral));
On 2014/10/23 12:59:18, Mike West wrote:
> "finer-origin"? Why not "suborigin"?
> 
> This is a spec question, obviously... not a question about this CL in
> particular.

I'll leave this for now since it's what I've said in all prior documents, but
let's bikeshed about this in spec-land ;-)

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:134: if
(accessControlOriginString == starAtom || accessControlAllowFinerOrigin ==
starAtom) {
On 2014/10/23 12:59:18, Mike West wrote:
> This seems wrong: as written, either `a-c-allow-origin: *` OR
> `a-c-allow-finer-origin: *` will return true for the access check. Shouldn't
> both be required? (Again, a spec would be helpful: I may be misinterpreting
the
> intent entirely)

This is on purpose, and I've tried to explain in the spec draft:
https://metromoxie.github.io/webappsec/specs/suborigins/index.html#cors

I'm second guessing it though. It seems reasonable to let * for all. Again,
discussion for spec-land, I think.

https://codereview.chromium.org/27073003/diff/107001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:169: errorDescription = "The
'Access-Control-Allow-Origin' or 'Access-Control-Allow-Finer-Origin' header has
a value '" + accessControlOriginString + "' that is not equal to the supplied
origin. Origin '" + securityOrigin->toString() + "' is therefore not allowed
access.";
On 2014/10/23 12:59:19, Mike West wrote:
> I'd suggest moving all the occurrences of these error strings out to a helper
> function: something like String::format("The %s header has blah blah",
> isEnabled() ? "a or b" : "a") in one place would be simpler to read and less
> repetitive.

Done. It's still a bit ugly, though, so any other ideas of beautifying it would
be welcome.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:549: }
On 2014/10/23 12:59:19, Mike West wrote:
> abarth@ is right: we shouldn't land this with <meta> support. I tried to take
> out <meta> support for sandbox this morning, and it's a pain because layout
> tests rely on it.
> 
> We should do this right from the beginning: please add an `m_headerType` check
> here.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:551: String invalidTokens;
On 2014/10/23 12:59:19, Mike West wrote:
> Nit: Unused?

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:553: if (!suboriginName.isNull()) {
On 2014/10/23 12:59:19, Mike West wrote:
> Again, isNull or isEmpty?

See earlier comment (NULL means no suborigin; empty should not parse).

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:555: }
On 2014/10/23 12:59:19, Mike West wrote:
> Nit: No {} for oneliners.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:671: return String();
On 2014/10/23 12:59:19, Mike West wrote:
> I think s/String()/emptyString()/ would be marginally more efficient.

As per an earlier comment, we're distinguishing a null String as the case where
there is no suborigin from the empty string, which is an invalid suborigin, this
needs to be String() so we get a null (not empty) string.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:684:
m_policy->reportInvalidSuboriginFlags("Invalid whitespace in suborigin.
Whitespace may only appear at the beginning and end of a suborigin, and it is
ignored.");
On 2014/10/23 12:59:19, Mike West wrote:
> 1. I think you can clarify this error message: "Whitespace isn't allowed in
> suborigin names." perhaps?
Done.
> 
> 2. You need to `return emptyString()` after the error, otherwise you're
> returning the first portion of the block.
Yikes! Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:732: } else if
(equalIgnoringCase(name, ContentSecurityPolicy::Suborigin)) {
On 2014/10/23 12:59:19, Mike West wrote:
> This ought to be guarded by the experimental flag.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.h (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.h:78: String parseSuboriginName(const
String& policy);
On 2014/10/23 12:59:19, Mike West wrote:
> Nit: Move this up next to the other parse functions.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.h:117: bool m_haveSuboriginPolicy;
On 2014/10/23 12:59:19, Mike West wrote:
> Nit: These both should be "hasXXX", not "haveXXX". Sorry about that. Would you
> mind cleaning up the existing usage in a separate CL? Or I'll do it if you
don't
> have time.

Done.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/Co...
File Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.cpp:95: // Experimental Directives
(post CSP 1.1)
On 2014/10/23 12:59:20, Mike West wrote:
> Nit: Since we changed the name, can you change both this and the above comment
> to CSP2?

I ended up putting a "Suborigin" comment above it since it's not really CSP, as
you pointed out.

https://codereview.chromium.org/27073003/diff/107001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.cpp:170: if
(experimentalFeaturesEnabled())
On 2014/10/23 12:59:20, Mike West wrote:
> `&& hasSuborigin`?

Enforce only "turns on" Suborigins if the Suborigin is non-null, so this should
be fine.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:68: return
(!origin1->hasSuborigin() && !origin2->hasSuborigin()) ||
(origin1->suboriginName() == origin2->suboriginName());
On 2014/10/23 12:59:20, Mike West wrote:
> Is this any different from `origin1->suboriginName() ==
> origin2->suboriginName()`? If neither has a suborigin, null-string ==
> null-string.

Yup. I think I was considering something tricky with empty strings being equal
to NULL strings, but that's irrelevant now that empty strings are disallowed.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:255: return false;
On 2014/10/23 12:59:20, Mike West wrote:
> I assume that bypassing the `document.domain` checks is intentional?

Definitely.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:418: return AlwaysDeny;
On 2014/10/23 12:59:20, Mike West wrote:
> Why?

Same rationale as above. Start with the limitations of Sandbox and add more
functionality in as needed/once we understand how to turn it on and off.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:514: builder.append('+');
On 2014/10/23 12:59:20, Mike West wrote:
> Is this the scheme we ended up with on the list?

There was no consensus, but it was the compromise I suggested and no one loudly
objected to. I think no one was happy with any of the suggestions, and I
certainly imagine this is something we'll revist.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:528: PassRefPtr<SecurityOrigin>
SecurityOrigin::createFromString(const String& originString)
On 2014/10/23 12:59:20, Mike West wrote:
> You're not handling creation of origins with suborigins?

You're right, I need to add to the SecurityOrigin constructor a parsing of
Suborigins. Done.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:146: bool canAccessDatabase() const {
return !isUnique() && !hasSuborigin(); };
On 2014/10/23 12:59:20, Mike West wrote:
> Why are we blocking access to these for suborigins? Shouldn't we instead be
> creating new storage areas for these suborigins?

The proposal/initial spec calls for these sensitive APIs to be blocked, like
with sandbox. However, I want to work on a way to turn these on. And, in fact,
talking to aaj@google.com, it would actually be more useful to allow same-origin
access, rather than give it a new one.

In any case, my theory is to start in the initial implementation with something
minimal, figure out what is needed by aaj@ et al, and come up with a mechanism
for turning them on.

https://codereview.chromium.org/27073003/diff/107001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:157: bool canAccessSessionStorage()
const { return !isUnique(); }
On 2014/10/23 12:59:20, Mike West wrote:
> Here too?

Yup, good catch. Rebase mistake, I think.

[Mike West, 2015-03-23 07:32:56]

Added some preliminary feedback. All the comments can basically be summarized as
"I'm not comfortable landing this without a large number of additional tests."

I think it would be good to break this into manageable chunks, and add unit
tests for each. For instance, the CrossOriginAccessControl bits are completely
distinct from the CSP bits. Testing each in isolation would raise our confidence
in the whole.

Moreover, Jochen's suggestions regarding layout tests are spot-on. It's not
clear to me which behavior side-effects of the changes to SecurityOrigin are
intentional, and which are really just unforseen side-effects. These
side-effects won't show up in our existing test coverage, as none of the
existing tests sets a suborigin.

Until we have a good deal more test coverage of suborigin's impact, I'd suggest
moving more slowly through this change to ensure that the decisions we lock into
code are deliberate.

https://codereview.chromium.org/27073003/diff/167001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/security/suborigins/suborigin-invalid-names.html
(right):

https://codereview.chromium.org/27073003/diff/167001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/suborigins/suborigin-invalid-names.html:35:
assert_equals(secret, "I am a secret", "The parent frame should always be able
to get the secret value from the child iframe.");
Why is this the correct behavior? Why not lock down the frame (e.g. unique
origin) if an invalid suborigin is asserted?

https://codereview.chromium.org/27073003/diff/167001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:239: ASSERT(!origin->hasSuborigin() ||
origin->suboriginName() == name);
ASSERTing here means that `Content-Security-Policy: suborigin foo, suborigin
bar` will crash the renderer, doesn't it?

https://codereview.chromium.org/27073003/diff/167001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:130: return
String::format(formatString, RuntimeEnabledFeatures::suboriginsEnabled() ?
suboriginsEnabledString : suboriginsDisabledString);
You're only gating the error message on the runtime-enabled feature. You need to
gate all the `...finer-origin` behavioral changes on that flag.

https://codereview.chromium.org/27073003/diff/167001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:133: bool
passesAccessControlCheck(ExecutionContext* context, const ResourceResponse&
response, StoredCredentials includeCredentials, SecurityOrigin* securityOrigin,
String& errorDescription)
I'd recommend adding some unit tests here. We should have some already, of
course, but this is adding enough complexity that I'm not comfortable going much
further without some verification.

https://codereview.chromium.org/27073003/diff/167001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:581: {
Nit: ASSERT(RuntimeEnabledFeatures::suboriginsEnabled()).

https://codereview.chromium.org/27073003/diff/167001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:699: String
CSPDirectiveList::parseSuboriginName(const String& policy)
We really need to add some parser unittests. Would you mind starting a
`CSPDirectiveListTest.cpp` file and adding some test cases for this method?
Sorry I haven't already started that work. :/

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:140: 
ASSERT that the runtime-enabled feature is enabled?

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:156: if
(deserializeSuboriginAndProtocol(m_protocol, suboriginName, m_protocol))
A side-effect of lumping the suborigin into the protocol is that two URLs
produced from the same "real" origin might compare as distinct given their
suborigins. Have you looked into the impact of suborigins on the URL parsing
behavior of things like `Document::completeURL`? What is the expected impact?

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:238: // an effective security tool.
ASSERT that the runtime-enabled feature is enabled?

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:239:
RELEASE_ASSERT(m_suboriginName.isNull() || m_suboriginName == suborigin);
When would the suborigin be set to itself? Could we simply assert that it is
null instead?

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:240: m_suboriginName = suborigin;
Please add unit tests for this change. You can verify that the test crashes via
EXPECT_DEATH or similar from `gtest_death_test.h`.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:279: return false;
Please add unit tests for this change.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:344: if (!sameSuborigin(this,
targetOrigin.get()))
Please add unit tests for this change.

It seems like it's going to do more than you want, however: there are a number
of places that use `canRequest` as a proxy for "is same-origin", but don't do
CORS checks (I'm thinking of CSP violation report `referer`-stripping, but I'm
sure there are a number of other cases in the ~40 callsites). If we make this
change, we'll need to walk through each of them and add tests to verify the
behavior we expect.

(Same for `canAccess`, really, but I suspect that's more likely to be used only
in the context of another "real" SecurityOrigin, not one just generated from a
URL).

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:442: return AlwaysDeny;
There's no test for this behavioral change. Please add unit tests and layout
tests.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:532: inline void
SecurityOrigin::buildRawString(StringBuilder& builder) const
Please add unit tests for the new behavior here.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:538: builder.append('+');
This is very strange syntax.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:567: if (!sameSuborigin(this,
other))
Do we always want the suborigin check when doing a scheme/host/port check? If
so, we should rename the method.

It would be reasonable to audit the ~11 calls to verify that the behavioral
changes this would introduce are indeed reasonable. For instance, this would
appear cause us to clear the referrer for previously same-origin PingLoader
requests. That doesn't seem like the correct behavior.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:152: EXPECT_EQ("foobar",
origin->suboriginName());
Please verify the rest of the origin as well here and elsewhere, to ensure that
parsing a suborigin doesn't screw up parsing the rest of the URL.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:166: 
It would be good to add a number of new unit tests to cover the behavioral
changes in SecurityOrigin. I've noted several in the CPP file.

[Mike West, 2015-03-30 12:03:19]

Ping?

[Mike West, 2015-04-07 06:50:26]

On 2015/03/30 at 12:03:19, Mike West (OOO until 7th) wrote:
> Ping?

Ping ping?

[jww, 2015-04-09 01:09:50]

On 2015/04/07 06:50:26, Mike West wrote:
> On 2015/03/30 at 12:03:19, Mike West (OOO until 7th) wrote:
> > Ping?
> 
> Ping ping?

I am actively working on responses. Hope to have a CL update tomorrow.

[jww, 2015-04-10 01:47:15]

On 2015/04/09 01:09:50, jww wrote:
> On 2015/04/07 06:50:26, Mike West wrote:
> > On 2015/03/30 at 12:03:19, Mike West (OOO until 7th) wrote:
> > > Ping?
> > 
> > Ping ping?
> 
> I am actively working on responses. Hope to have a CL update tomorrow.

Some of these fixes are a little beefier than expected. I'm hopefully for Friday
though.

[jww, 2015-04-11 02:36:25]

On 2015/04/10 01:47:15, jww wrote:
> On 2015/04/09 01:09:50, jww wrote:
> > On 2015/04/07 06:50:26, Mike West wrote:
> > > On 2015/03/30 at 12:03:19, Mike West (OOO until 7th) wrote:
> > > > Ping?
> > > 
> > > Ping ping?
> > 
> > I am actively working on responses. Hope to have a CL update tomorrow.
> 
> Some of these fixes are a little beefier than expected. I'm hopefully for
Friday
> though.

Addressed a bunch of the comments, but a few of the beefier ones to address
remain to work on early next week.

[jww, 2015-04-11 02:52:36]

Oops, almost forgot my responses.

https://codereview.chromium.org/27073003/diff/167001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/security/suborigins/suborigin-invalid-names.html
(right):

https://codereview.chromium.org/27073003/diff/167001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/suborigins/suborigin-invalid-names.html:35:
assert_equals(secret, "I am a secret", "The parent frame should always be able
to get the secret value from the child iframe.");
On 2015/03/23 07:32:55, Mike West wrote:
> Why is this the correct behavior? Why not lock down the frame (e.g. unique
> origin) if an invalid suborigin is asserted?

That's certainly up for debate. I think this is a fail open case, like with
typing in a bad hash algorithm. In the future, if  we change the format for
suborigins, we probably will want to fail open.

https://codereview.chromium.org/27073003/diff/167001/Source/core/dom/Executio...
File Source/core/dom/ExecutionContext.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/core/dom/Executio...
Source/core/dom/ExecutionContext.cpp:239: ASSERT(!origin->hasSuborigin() ||
origin->suboriginName() == name);
On 2015/03/23 07:32:55, Mike West wrote:
> ASSERTing here means that `Content-Security-Policy: suborigin foo, suborigin
> bar` will crash the renderer, doesn't it?

It should never get to that point. In the assignment of suborigins, the last
assigned suborigin should "win" and be the assigned suborigin. If this state is
ever reached in enforce, something very bad has happened, hence the ASSERT. I've
added a test for multiple suborigins to verify that there isn't a renderer
crash.

https://codereview.chromium.org/27073003/diff/167001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:581: {
On 2015/03/23 07:32:55, Mike West wrote:
> Nit: ASSERT(RuntimeEnabledFeatures::suboriginsEnabled()).

Done.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:140: 
On 2015/03/23 07:32:56, Mike West wrote:
> ASSERT that the runtime-enabled feature is enabled?

I instead put a check at the top to make sure we only try to deserialize a
suborigin if the feature is enabled.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:238: // an effective security tool.
On 2015/03/23 07:32:56, Mike West wrote:
> ASSERT that the runtime-enabled feature is enabled?

Done.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:239:
RELEASE_ASSERT(m_suboriginName.isNull() || m_suboriginName == suborigin);
On 2015/03/23 07:32:55, Mike West wrote:
> When would the suborigin be set to itself? Could we simply assert that it is
> null instead?

There are some cases, I believe during script execution, where
applyPolicySideEffectsToExecutionContext() can be called that then call
'addSuborigin()' again. We're cool with that, as long as something funky doesn't
happen and the suborigin name remains the same.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:240: m_suboriginName = suborigin;
On 2015/03/23 07:32:55, Mike West wrote:
> Please add unit tests for this change. You can verify that the test crashes
via
> EXPECT_DEATH or similar from `gtest_death_test.h`.

Done.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:279: return false;
On 2015/03/23 07:32:56, Mike West wrote:
> Please add unit tests for this change.

Done.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:344: if (!sameSuborigin(this,
targetOrigin.get()))
On 2015/03/23 07:32:55, Mike West wrote:
> Please add unit tests for this change.
Added.
> 
> It seems like it's going to do more than you want, however: there are a number
> of places that use `canRequest` as a proxy for "is same-origin", but don't do
> CORS checks (I'm thinking of CSP violation report `referer`-stripping, but I'm
> sure there are a number of other cases in the ~40 callsites). If we make this
> change, we'll need to walk through each of them and add tests to verify the
> behavior we expect.
> 
> (Same for `canAccess`, really, but I suspect that's more likely to be used
only
> in the context of another "real" SecurityOrigin, not one just generated from a
> URL).
So your concern is that this is *too* aggressive? That is, it will cause
'canAcess()' to return false in too many cases? I can see that being a problem,
and yes, I'll probably need to review them. It seems like they should only be a
problem if suborigins is enabled, though, so as long as we're not changing
security in the suborigin-disabled case, I have fewer concerns.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:442: return AlwaysDeny;
On 2015/03/23 07:32:55, Mike West wrote:
> There's no test for this behavioral change. Please add unit tests and layout
> tests.

Done.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:538: builder.append('+');
On 2015/03/23 07:32:55, Mike West wrote:
> This is very strange syntax.

Initially I went with ':', but this causes all sorts of problems with things
then looking at the URL and thinking 'suborigin:' was the protocol. I went with
'+' instead, here, but it now has the double '+'s. This is the syntax I proposed
on WebAppSec, and I am by no means wedded to it, but I'd like to stick with it
or something similar until we get external objection or discussion (I'm happy to
change the particular tokens, though, but the protocol RFC limits us to
alphanumeric plus '+', '-', and '.').

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:567: if (!sameSuborigin(this,
other))
On 2015/03/23 07:32:55, Mike West wrote:
> Do we always want the suborigin check when doing a scheme/host/port check? If
> so, we should rename the method.
Even while it's experimental? It won't be a suborigin check for most people,
since they won't have the flag turned on. I guess we could make the name more
generic? Not sure what though.
> 
> It would be reasonable to audit the ~11 calls to verify that the behavioral
> changes this would introduce are indeed reasonable. For instance, this would
> appear cause us to clear the referrer for previously same-origin PingLoader
> requests. That doesn't seem like the correct behavior.

It depends on how aggressive we want to be. In the initial proposal, I actually
do think it's reasonable. It should be treated as a separate origin, as if it's
untrusted, so the referrer shouldn't be sent. I think a likely outcome is that
this might turn out to be unwarranted or unnecessary, so we might want to change
it, based on feedback from consumers.

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:166: 
On 2015/03/23 07:32:56, Mike West wrote:
> It would be good to add a number of new unit tests to cover the behavioral
> changes in SecurityOrigin. I've noted several in the CPP file.

Yup, added.

[Mike West, 2015-04-13 10:03:35]

The changes between #9 and #10 seem reasonable, but my overarching feedback is
that it's not clear that we're building something that folks like ISE want.

The use case they're very, very interested in is something like sandboxing Maps
away from GMail. I think the side-effects of this patch's approach will make
that infeasible (e.g. GMail isn't going to give up notifications). That's a
small example, but an important one insofar as it strengthens Jochen's
suggestion to work on this feature from the opposite direction: putting together
a set of use-cases that reflect developers' intent mixed in with your security
expertise seems like an important first step so that we know what we're actually
building.

The absence of such a set of tests makes me wary that we'd be doing ourselves a
disservice by landing an implementation that does something (anything!) and then
poking at it. That seems less likely to get us a well-understood outcome.

https://codereview.chromium.org/27073003/diff/187001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/suborigins/suborigin-blocked-notifications.php
(right):

https://codereview.chromium.org/27073003/diff/187001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/suborigins/suborigin-blocked-notifications.php:13:
testRunner.grantWebNotificationPermission(location.origin, true);
It would be good to discuss this kind of thing with folks like aaj@. I think
it's fairly distinct from their understanding of what suborigins' impact will
be.

https://codereview.chromium.org/27073003/diff/187001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOriginTest.cpp (right):

https://codereview.chromium.org/27073003/diff/187001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOriginTest.cpp:178:
EXPECT_FALSE(suborigin1->canAccess(suborigin2.get()));
Nit: I'd suggest splitting `canAccess` and `canRequest` into separate unit
tests, just for clarity.

[jww, 2015-04-13 23:51:08]

On 2015/04/13 10:03:35, Mike West wrote:
> The changes between #9 and #10 seem reasonable, but my overarching feedback is
> that it's not clear that we're building something that folks like ISE want.
> 
> The use case they're very, very interested in is something like sandboxing
Maps
> away from GMail. I think the side-effects of this patch's approach will make
> that infeasible (e.g. GMail isn't going to give up notifications). That's a
> small example, but an important one insofar as it strengthens Jochen's
> suggestion to work on this feature from the opposite direction: putting
together
> a set of use-cases that reflect developers' intent mixed in with your security
> expertise seems like an important first step so that we know what we're
actually
> building.
We did discuss some of these things with ISE folks, namely aaj@ and lcamtuf@. We
didn't discuss notifications in particular, but we did discuss cookies, for
example. They actually wanted cookies to be cut off, iirc, but said they could
work with them being accessible to start with and hopefully we can provide a way
to disconnect them. The general feeling I got was that it would be great to cut
things off by default and provide a mechanism to turn them on.

But like I said before, I'm more than happy to go with the split-up-the-CL
approach, but I was just trying to address the major issues over here before I
went and did that. I guess I'll go and start the split now though :-)
> 
> The absence of such a set of tests makes me wary that we'd be doing ourselves
a
> disservice by landing an implementation that does something (anything!) and
then
> poking at it. That seems less likely to get us a well-understood outcome.
> 
>
https://codereview.chromium.org/27073003/diff/187001/LayoutTests/http/tests/s...
> File
> LayoutTests/http/tests/security/suborigins/suborigin-blocked-notifications.php
> (right):
> 
>
https://codereview.chromium.org/27073003/diff/187001/LayoutTests/http/tests/s...
>
LayoutTests/http/tests/security/suborigins/suborigin-blocked-notifications.php:13:
> testRunner.grantWebNotificationPermission(location.origin, true);
> It would be good to discuss this kind of thing with folks like aaj@. I think
> it's fairly distinct from their understanding of what suborigins' impact will
> be.
> 
>
https://codereview.chromium.org/27073003/diff/187001/Source/platform/weborigi...
> File Source/platform/weborigin/SecurityOriginTest.cpp (right):
> 
>
https://codereview.chromium.org/27073003/diff/187001/Source/platform/weborigi...
> Source/platform/weborigin/SecurityOriginTest.cpp:178:
> EXPECT_FALSE(suborigin1->canAccess(suborigin2.get()));
> Nit: I'd suggest splitting `canAccess` and `canRequest` into separate unit
> tests, just for clarity.

[jww, 2015-04-25 00:41:22]

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/27073003/diff/167001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:156: if
(deserializeSuboriginAndProtocol(m_protocol, suboriginName, m_protocol))
On 2015/03/23 07:32:55, Mike West wrote:
> A side-effect of lumping the suborigin into the protocol is that two URLs
> produced from the same "real" origin might compare as distinct given their
> suborigins. Have you looked into the impact of suborigins on the URL parsing
> behavior of things like `Document::completeURL`? What is the expected impact?

Agreed. For this original implementation, my idea was to "deny" by default and
work out how to "allow" on a case by case basis. Based on my converastion with
aaj@, I'm not sure that's the right approach so I'm reevaluating.