Transfer SecurityOrigin overrides to Workers.

Transfer SecurityOrigin overrides to Workers.

Should a Document be operating with a SecurityOrigin that grants
(powerful) abilities like disabling same-origin policy or allows
file access, have those transfer over to any new Workers created.

R=horo,mkwst,jochen
BUG=254993
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182897

[sof, 2014-09-23 22:03:12]

sigbjornf@opera.com changed reviewers:
+ abarth@chromium.org, horo@chromium.org, jochen@chromium.org

[sof, 2014-09-23 22:03:13]

Please take a look.

The request to have --disable-web-security also apply within Workers, seems a
reasonable one.

I opted in the end for a somewhat general solution to the problem of how to
propagate the required information into new worker global scopes, involving
SecurityOrigin. workable?

The tests here depend on https://codereview.chromium.org/601513002/

[horo, 2014-09-24 05:42:27]

https://codereview.chromium.org/594803002/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/594803002/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:406: if (document->securityOrigin())
I think WebEmbeddedWorkerImpl's m_mainFrame is not affected by the settings.
So m_grantUniversalAccess and m_enforceFilePathSeparation are always false.

https://codereview.chromium.org/594803002/diff/40001/Source/web/WebSharedWork...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/594803002/diff/40001/Source/web/WebSharedWork...
Source/web/WebSharedWorkerImpl.cpp:398: if (document->securityOrigin())
I think WebSharedWorkerImpl's m_mainFrame is not affected by the settings.
So m_grantUniversalAccess and m_enforceFilePathSeparation are always false.

[Mike West, 2014-09-26 11:29:09]

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

[Mike West, 2014-09-26 11:29:10]

This looks like a reasonable approach, though I'm not sure you really need the
generic PolicyOverrides object. You could get away without modifying
SecurityOrigin at all if you just passed around the two boolean values rather
than the new structure.

Do you expect more of these flags to propagate? If so, the object makes sense.
If not, and it's really just these two booleans, I'd avoid the complexity.

https://codereview.chromium.org/594803002/diff/40001/Source/core/workers/Work...
File Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/594803002/diff/40001/Source/core/workers/Work...
Source/core/workers/WorkerMessagingProxy.cpp:121: if
(document->securityOrigin())
When do we ever have Documents without SecurityOrigins? Don't we always
initialize the security context in the document's constructor?

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

https://codereview.chromium.org/594803002/diff/40001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.cpp:541: // Individual overrides does
not have does-not-apply value;
Nit: I think you can drop this clause: "The policy will be applied in its
entirety, without exception." is probably clear enough.

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

https://codereview.chromium.org/594803002/diff/40001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:209: class PolicyOverrides {
It might be simpler to model this as a struct; there's no real reason to push
things through getters and setters into private variables if all you're doing is
passing two values around.

https://codereview.chromium.org/594803002/diff/40001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:227: bool enforceFilePathSeparation()
const { return m_enforceFilePathSeparation; }
These both sound a bit strange. Perhaps "doesGrantUniversalAccess"? Without the
"does" it sounds like an action rather than a getter.

[sof, 2014-09-26 13:52:44]

On 2014/09/26 11:29:10, Mike West wrote:
> This looks like a reasonable approach, though I'm not sure you really need the
> generic PolicyOverrides object. You could get away without modifying
> SecurityOrigin at all if you just passed around the two boolean values rather
> than the new structure.
> 
> Do you expect more of these flags to propagate? If so, the object makes sense.
> If not, and it's really just these two booleans, I'd avoid the complexity.
> 

I'm not aware of any atm.

Separately propagating these two 'security' bits in the startup data seemed like
too much of a quickfix, which is why I steered away from that representation.
But I'd be happy to simplify, and leave any generalization step for when/if we
have to do that here.

[Mike West, 2014-09-26 14:07:25]

On 2014/09/26 13:52:44, sof wrote:
> Separately propagating these two 'security' bits in the startup data seemed
like
> too much of a quickfix, which is why I steered away from that representation.
> But I'd be happy to simplify, and leave any generalization step for when/if we
> have to do that here.

If passing two booleans around seems dirty, how about passing a ref to the
document's SecurityOrigin, and changing the applyPolicyOverrides method you
added to just read the values off of that object?

[sof, 2014-09-26 14:19:15]

On 2014/09/26 14:07:25, Mike West wrote:
> On 2014/09/26 13:52:44, sof wrote:
> > Separately propagating these two 'security' bits in the startup data seemed
> like
> > too much of a quickfix, which is why I steered away from that
representation.
> > But I'd be happy to simplify, and leave any generalization step for when/if
we
> > have to do that here.
> 
> If passing two booleans around seems dirty, how about passing a ref to the
> document's SecurityOrigin, and changing the applyPolicyOverrides method you
> added to just read the values off of that object?

That strikes a good balance, I think; let's try that.

[sof, 2014-09-28 17:07:12]

On 2014/09/26 14:19:15, sof wrote:
> On 2014/09/26 14:07:25, Mike West wrote:
> > On 2014/09/26 13:52:44, sof wrote:
> > > Separately propagating these two 'security' bits in the startup data
seemed
> > like
> > > too much of a quickfix, which is why I steered away from that
> representation.
> > > But I'd be happy to simplify, and leave any generalization step for
when/if
> we
> > > have to do that here.
> > 
> > If passing two booleans around seems dirty, how about passing a ref to the
> > document's SecurityOrigin, and changing the applyPolicyOverrides method you
> > added to just read the values off of that object?
> 
> That strikes a good balance, I think; let's try that.

Switched to that + tried to improve comments surrounding this ability.

[sof, 2014-09-28 17:10:22]

https://codereview.chromium.org/594803002/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/594803002/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:406: if (document->securityOrigin())
On 2014/09/24 05:42:27, horo wrote:
> I think WebEmbeddedWorkerImpl's m_mainFrame is not affected by the settings.
> So m_grantUniversalAccess and m_enforceFilePathSeparation are always false.

Thanks, I missed that -- the document for the shadow page being without it.

For now, I've added a FIXME to try to make it clear what to (not) expect;
acceptable?

[Mike West, 2014-09-28 20:41:00]

At first glance, this looks pretty good. I'll take a more detailed pass in the
morning, thanks for trying this approach!

[Mike West, 2014-09-29 07:37:23]

LGTM. Thanks for taking another pass. A few comments inline (and you'll need a
platform/ and web/ OWNER to sign off on it, of course).

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerMessagingProxy.cpp:121:
OwnPtrWillBeRawPtr<WorkerThreadStartupData> startupData =
WorkerThreadStartupData::create(scriptURL, userAgent, sourceCode, startMode,
document->contentSecurityPolicy()->deprecatedHeader(),
document->contentSecurityPolicy()->deprecatedHeaderType(), starterOrigin,
m_workerClients.release());
Nit: I think you can just inline this.

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThreadStartupData.h:67: // To have those be
transferred to the origin of a new worker
Nit: How about "To ensure that these are transferrer...". It would also be nice
to add a comment about object ownership.

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThreadStartupData.h:79: WorkerThreadStartupData(const
KURL& scriptURL, const String& userAgent, const String& sourceCode,
WorkerThreadStartMode, const String& contentSecurityPolicy,
ContentSecurityPolicyHeaderType contentSecurityPolicyType, const SecurityOrigin*
parentOrigin, PassOwnPtrWillBeRawPtr<WorkerClients>);
s/parentOrigin/starterOrigin/. Or vice-versa, really. I'm not thrilled about
"starter origin". "parent origin" isn't much better though... *shrug* :)

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

https://codereview.chromium.org/594803002/diff/60001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:215: void transferPrivileges(const
SecurityOrigin&);
I think something like `transferPrivilegesFrom()` would make it clearer which
origin is the target, and which the transferee.

https://codereview.chromium.org/594803002/diff/60001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/594803002/diff/60001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:405: // FIXME: this document's origin is
pristine and without any extra privileges.
Can you point to a bug here? I don't understand the context of the comment...

https://codereview.chromium.org/594803002/diff/60001/Source/web/WebSharedWork...
File Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/594803002/diff/60001/Source/web/WebSharedWork...
Source/web/WebSharedWorkerImpl.cpp:397: // FIXME: this document's origin is
pristine and without any extra privileges.
Ditto.

[sof, 2014-09-29 09:42:28]

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
File Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThreadStartupData.h:67: // To have those be
transferred to the origin of a new worker
On 2014/09/29 07:37:22, Mike West wrote:
> Nit: How about "To ensure that these are transferrer...". It would also be
nice
> to add a comment about object ownership.

Oh yes, forgot to mention ownership assumptions.

Added - is the assumption reasonable to all?

https://codereview.chromium.org/594803002/diff/60001/Source/core/workers/Work...
Source/core/workers/WorkerThreadStartupData.h:79: WorkerThreadStartupData(const
KURL& scriptURL, const String& userAgent, const String& sourceCode,
WorkerThreadStartMode, const String& contentSecurityPolicy,
ContentSecurityPolicyHeaderType contentSecurityPolicyType, const SecurityOrigin*
parentOrigin, PassOwnPtrWillBeRawPtr<WorkerClients>);
On 2014/09/29 07:37:22, Mike West wrote:
> s/parentOrigin/starterOrigin/. Or vice-versa, really. I'm not thrilled about
> "starter origin". "parent origin" isn't much better though... *shrug* :)

Oops, I decided not to go with 'parent origin' in the end ('parent' implies more
than 'starter'), but left this in. Removed.

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

https://codereview.chromium.org/594803002/diff/60001/Source/platform/weborigi...
Source/platform/weborigin/SecurityOrigin.h:215: void transferPrivileges(const
SecurityOrigin&);
On 2014/09/29 07:37:22, Mike West wrote:
> I think something like `transferPrivilegesFrom()` would make it clearer which
> origin is the target, and which the transferee.

Yes, disambiguation by its name rather than having to consult its type. Done.

https://codereview.chromium.org/594803002/diff/60001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/594803002/diff/60001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:405: // FIXME: this document's origin is
pristine and without any extra privileges.
On 2014/09/29 07:37:22, Mike West wrote:
> Can you point to a bug here? I don't understand the context of the comment...

Added crbug.com xref.

[horo, 2014-09-29 10:33:41]

lgtm

[Mike West, 2014-09-29 10:35:08]

SLGTM. Thanks!

[sof, 2014-09-29 12:53:12]

jochen, abarth: does the SecurityOrigin change look acceptable to you?

[jochen (gone - plz use gerrit), 2014-09-30 08:28:41]

lgtm

[sof, 2014-09-30 08:38:54]

thanks all for the reviews.

[sof, 2014-09-30 08:39:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-30 08:39:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/594803002/80001

[commit-bot: I haz the power, 2014-09-30 09:09:47]

Committed patchset #5 (id:80001) as 182897