Block execution of failed 'crossorigin' <script>s.

Block execution of failed 'crossorigin' <script>s.

The HTML spec tells us that if a script element has a 'crossorigin'
attribute, a potentially CORS enabled fetch of its source URL must be
performed:

 http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html#dfnReturnLink-39
 http://www.whatwg.org/specs/web-apps/current-work/multipage/fetching-resources.html#potentially-cors-enabled-fetch

If the CORS cross-origin request status of that fetch is not a success
due to a failed CORS resource sharing check, the implementation must
act as if no script resource has been obtained.

No such CORS check was in place for parser inserted elements, allowing
a cross-origin CORS resource that either didn't permit the script
access or the resource wasn't CORS enabled, to load in the presence of
'crossorigin'.

Address this here by interposing a CORS check prior to starting script
execution. This (tries) to mirror how the operations are sequenced in
the underlying spec.

Specifically,

  ScriptLoader::executePotentiallyCrossOriginScript()

now performs the needed access check for fetched scripts _if_ the
fetch was initiated as potentially CORS-enabled. That is, if the
script was fetched from same-origin or the CORS check passes in the
cross-origin case, then script execution can go ahead. If not and a
cross-origin script, a console error message is added and an error
event is dispatched. Script execution does not go ahead.

To support the above, the ResourceFetcher::canAccess() predicate over
resources now needs to be informed if the resource fetch was initiated
in a potentially CORS enabled manner. If it was, the required CORS
resource sharing check will be performed.

By making the "CORS enabled" property of a fetch something that a
loader keeps separate from the (potentially cached) resource, we can
simplify the underlying structure a bit, removing the
RequestOriginPolicy portion from the resource options.

As a result, FetchRequest now instead keeps track of any origin
restrictions of the fetch resource + the canAccess() predicate must be
supplied with the "CORS enabled" setting to use when checking access
to the actual resource.

R=mkwst,abarth
BUG=286684
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162417

[sof, 2013-10-29 06:53:43]

Please take a look, or suggest reviewers (even) better placed for CORS + loader
related changes. Thanks :)

[Mike West, 2013-10-29 11:00:53]

Thanks for taking a look at this. The approach seems pretty reasonable, but
checking the DOM at execution time won't work. See below:

https://codereview.chromium.org/47923008/diff/1/Source/core/dom/ScriptLoader.cpp
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/1/Source/core/dom/ScriptLoader....
Source/core/dom/ScriptLoader.cpp:374: &&
!m_element->fastGetAttribute(HTMLNames::crossoriginAttr).isNull()
Another script could alter the value while the script is loading, so this check
comes too late.

Perhaps we could store that cached value on the ScriptSourceCode object instead
of reading it from the DOM here? We check the crossorigin status of the script
when generating the request in ScriptLoader::fetchScript, and pass it through
the fetcher on the Request object. I believe it should therefore already be
available in the ScriptSourceCode constructor.

[sof, 2013-10-29 12:04:41]

https://codereview.chromium.org/47923008/diff/1/Source/core/dom/ScriptLoader.cpp
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/1/Source/core/dom/ScriptLoader....
Source/core/dom/ScriptLoader.cpp:374: &&
!m_element->fastGetAttribute(HTMLNames::crossoriginAttr).isNull()
On 2013/10/29 11:00:53, Mike West wrote:
> Another script could alter the value while the script is loading, so this
check
> comes too late.
> 
> Perhaps we could store that cached value on the ScriptSourceCode object
instead
> of reading it from the DOM here? We check the crossorigin status of the script
> when generating the request in ScriptLoader::fetchScript, and pass it through
> the fetcher on the Request object. I believe it should therefore already be
> available in the ScriptSourceCode constructor.

Let's do that; I found the spec text not 100% clear about when you should sample
the state of the content attribute wrt performing the resource sharing check,
but doing it this way does reduce non-determinism.

[sof, 2013-10-29 16:49:16]

On 2013/10/29 12:04:41, sof wrote:
>
https://codereview.chromium.org/47923008/diff/1/Source/core/dom/ScriptLoader.cpp
> File Source/core/dom/ScriptLoader.cpp (right):
> 
>
https://codereview.chromium.org/47923008/diff/1/Source/core/dom/ScriptLoader....
> Source/core/dom/ScriptLoader.cpp:374: &&
> !m_element->fastGetAttribute(HTMLNames::crossoriginAttr).isNull()
> On 2013/10/29 11:00:53, Mike West wrote:
...
> > 
> > Perhaps we could store that cached value on the ScriptSourceCode object
> > instead of reading it from the DOM here? 

Entirely possible that I'm missing something about how this fits together,
but I think the recording of the potentially-CORS-enabled flag needs to be
kept on the loader itself.

If on a ScriptSourceCode, as suggested, I don't see a way for prepareScript()
to supply that flag when constructing such code objects.

[sof, 2013-10-29 17:25:11]

Come to think of it, the current potentially-CORS check in
ResourceFetcher::canAccess(Resource *) against the origin
policy of the resource looks suspicious if the resource
happens to be used from the cache. The CORS check will not
apply in that case; intentional?

e.g.

<script src="http://cross.origin.org/non-cors-script.js"></script>
<script>
var script = document.createElement("script");
script.crossOrigin = "anonymous";
script.src= "http://cross.origin.org/non-cors-script.js";
document.appendChild(script);
</script>

It should not be allowed to execute 2nd time around, I'd argue.

[abarth-chromium, 2013-10-29 20:50:11]

Shouldn't we model CORS failures as network errors?  I'm not sure why we need
special code in ScriptLoader to understand this case.  It should just act as if
it got a network error of some sort.

[sof, 2013-10-29 22:31:41]

On 2013/10/29 20:50:11, abarth wrote:
> Shouldn't we model CORS failures as network errors?  I'm not sure why we need
> special code in ScriptLoader to understand this case.  It should just act as
if
> it got a network error of some sort.

Yes, that would be consistent to dispatch error events rather than just log
to the console. Added in Patch Set 3 + desired behavior covered by a test.

I've tried to minimize the access check additions in ScriptLoader further, but
perhaps it can be reduced further still? (I can't see how to completely join
the parser-inserted case from the script inserted one. At least not right now.)

[sof, 2013-10-29 22:35:18]

On 2013/10/29 17:25:11, sof wrote:
> Come to think of it, the current potentially-CORS check in
> ResourceFetcher::canAccess(Resource *) against the origin
> policy of the resource looks suspicious if the resource
> happens to be used from the cache. The CORS check will not
> apply in that case; intentional?
> 
> e.g.
> 
> <script src="http://cross.origin.org/non-cors-script.js"></script>
> <script>
> var script = document.createElement("script");
> script.crossOrigin = "anonymous";
> script.src= "http://cross.origin.org/non-cors-script.js";
> document.appendChild(script);
> </script>
> 
> It should not be allowed to execute 2nd time around, I'd argue.

Covered by
http/tests/security/script-crossorigin-loads-correctly-credentials-2.html and
addressed in Patch Set 3.

[sof, 2013-11-01 09:51:27]

If there's anything I can do to improve the shape of this change, do let me
know.

i.e., as long as resources associated with potentially-CORS-enabled fetches
aren't aggressively reloaded (always?), using information on the resource to
trigger the CORS check seems insufficient. Hence keeping that bit of information
on the ScriptLoader here.

[sof, 2013-11-14 15:07:24]

I'm keen to invigorate this patch & review -- I'll do what it takes to advance
it :)

Please do let me know if there's just an intrinsic problem with the approach, or
it's not worth spending limited review bandwidth on it right now, or others are
better placed to have a look, or ...

[eseidel, 2013-11-14 15:54:36]

Adam Barth is really your best reviewer for CORS related changes.  He should be
reachable in #blink as abarth.

[abarth-chromium, 2013-11-14 16:34:48]

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:245:
executePotentiallyCrossOriginScript(ScriptSourceCode(scriptContent(), scriptURL,
position));
Does it matter that we're losing the return value here?  In particular, we do
need to return false from this function when we don't execute the script?

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:366: bool
ScriptLoader::executePotentiallyCrossOriginScript(const ScriptSourceCode&
sourceCode)
It looks like the one caller ignores the return value...

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:370: &&
!m_element->document().fetcher()->canAccess(sourceCode.resource(),
asPotentiallyCORSEnabledLoad())) {
It seems strange that we need to check asPotentiallyCORSEnabledLoad given that
canAccess checks "resource->options().requestOriginPolicy ==
PotentiallyCrossOriginEnabled".  Do we need this duplicated state?

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.h:108: bool m_asPotentiallyCORSEnabledLoad : 1;
as?  Di you mean "is" ?

https://codereview.chromium.org/47923008/diff/70002/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/fetch/Resourc...
Source/core/fetch/ResourceFetcher.cpp:545: if (isPotentiallyCORSEnabled
What's the reason for this part of the change?  What's wrong with looking at the
resource's options?  Is the issue that a single resource might be requested with
different options?  If so, does that mean we need to rip requestOriginPolicy out
of the options struct?

https://codereview.chromium.org/47923008/diff/70002/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImportLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/html/HTMLImpo...
Source/core/html/HTMLImportLoader.cpp:116: if
(!m_parent->document()->fetcher()->canAccess(m_resource.get(), true))
If we keep this design, we should use an enum rather than a bare bool.  From
just reading this line, it's very unclear what |true| means in this context.

https://codereview.chromium.org/47923008/diff/70002/Source/core/html/parser/H...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/html/parser/H...
Source/core/html/parser/HTMLScriptRunner.cpp:140: if
(scriptLoader->executePotentiallyCrossOriginScript(sourceCode))
I see, the return value is used here.  I'm surprised we don't need to use it in
ScriptLoader too.

[sof, 2013-11-14 17:12:02]

Thanks abarth! Updated patch incoming a bit later, but wanted to follow up with
some comments right away.

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:245:
executePotentiallyCrossOriginScript(ScriptSourceCode(scriptContent(), scriptURL,
position));
On 2013/11/14 16:34:48, abarth wrote:
> Does it matter that we're losing the return value here?  In particular, we do
> need to return false from this function when we don't execute the script?

It does matter , i.e., the prepareScript() return value indicates if the script
executes as far as I've been able to determine (which makes sense wrt
the underlying spec text.)

XMLDocumentParser is currently the only caller that checks; will test & fix
this.

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:370: &&
!m_element->document().fetcher()->canAccess(sourceCode.resource(),
asPotentiallyCORSEnabledLoad())) {
On 2013/11/14 16:34:48, abarth wrote:
> It seems strange that we need to check asPotentiallyCORSEnabledLoad given that
> canAccess checks "resource->options().requestOriginPolicy ==
> PotentiallyCrossOriginEnabled".  Do we need this duplicated state?

We don't use that resource state any longer in canAccess(), as the resource may
have been cached & wouldn't have that policy set.

https://codereview.chromium.org/47923008/diff/70002/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/fetch/Resourc...
Source/core/fetch/ResourceFetcher.cpp:545: if (isPotentiallyCORSEnabled
On 2013/11/14 16:34:48, abarth wrote:
> What's the reason for this part of the change?  What's wrong with looking at
the
> resource's options?  Is the issue that a single resource might be requested
with
> different options?  If so, does that mean we need to rip requestOriginPolicy
out
> of the options struct?

Yes, that's why. See the test
LayoutTests/http/tests/security/script-crossorigin-loads-correctly-credentials-2.html
for an example.

There are two remaining uses of requestOriginPolicy - style resource loading +
XSLT. I didn't investigate if removal was possible, but it struck my mind as a
desirable outcome of this change. (I could certainly investigate separately.)

[abarth-chromium, 2013-11-14 18:42:17]

If we can remove the option from the struct, that would be better.  It's always
a bit concerning when we duplicate state in two places.

[sof, 2013-11-15 08:05:23]

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.cpp:245:
executePotentiallyCrossOriginScript(ScriptSourceCode(scriptContent(), scriptURL,
position));
On 2013/11/14 17:12:03, sof wrote:
> On 2013/11/14 16:34:48, abarth wrote:
> > Does it matter that we're losing the return value here?  In particular, we
do
> > need to return false from this function when we don't execute the script?
> 
> It does matter , i.e., the prepareScript() return value indicates if the
script
> executes as far as I've been able to determine (which makes sense wrt
> the underlying spec text.)
> 
> XMLDocumentParser is currently the only caller that checks; will test & fix
> this.

prepareScript() now returns 'false' if executePotentiallyCrossOriginScript()
fails.

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/dom/ScriptLoa...
Source/core/dom/ScriptLoader.h:108: bool m_asPotentiallyCORSEnabledLoad : 1;
On 2013/11/14 16:34:48, abarth wrote:
> as?  Di you mean "is" ?

Now shortened to "m_isPotentiallyCORSEnabled" (went back&forth on "as" and "is",
initially.)

https://codereview.chromium.org/47923008/diff/70002/Source/core/html/HTMLImpo...
File Source/core/html/HTMLImportLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/70002/Source/core/html/HTMLImpo...
Source/core/html/HTMLImportLoader.cpp:116: if
(!m_parent->document()->fetcher()->canAccess(m_resource.get(), true))
On 2013/11/14 16:34:48, abarth wrote:
> If we keep this design, we should use an enum rather than a bare bool.  From
> just reading this line, it's very unclear what |true| means in this context.

Definitely agree. Now done, with hopefully comprehensible type and enum tag
names.

[sof, 2013-11-15 08:11:58]

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.h:64: enum OriginRestriction {
Finding appropriate names for this one (and the one below) took quite some time.
I think they make sense now, but if people see better alternatives, I'll happily
rename.

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.h:95: ResourcePtr<ImageResource>
fetchImage(FetchRequest&, OriginRestriction =
UseDefaultOriginRestrictionForType);
Notice that none of the other fetch{ResourceType}() methods take this extra
argument. They could, but there's no need for its functionality by their
callers, so I held off. (And requestResource() provides full control in any
case.)

[abarth-chromium, 2013-11-15 15:47:51]

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.h:95: ResourcePtr<ImageResource>
fetchImage(FetchRequest&, OriginRestriction =
UseDefaultOriginRestrictionForType);
On 2013/11/15 08:11:58, sof wrote:
> Notice that none of the other fetch{ResourceType}() methods take this extra
> argument. They could, but there's no need for its functionality by their
> callers, so I held off. (And requestResource() provides full control in any
> case.) 

These functions used to take a large number of parameters.  We created the
FetchRequest object to hold all these parameters.  Can we add this value to
FetchRequest?  The FetchRequest doesn't get stored in the cache, so different
callers can have different values for the origin restriction.

[sof, 2013-11-15 17:05:18]

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/47923008/diff/300001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.h:95: ResourcePtr<ImageResource>
fetchImage(FetchRequest&, OriginRestriction =
UseDefaultOriginRestrictionForType);
On 2013/11/15 15:47:52, abarth wrote:
> On 2013/11/15 08:11:58, sof wrote:
> > Notice that none of the other fetch{ResourceType}() methods take this extra
> > argument. They could, but there's no need for its functionality by their
> > callers, so I held off. (And requestResource() provides full control in any
> > case.) 
> 
> These functions used to take a large number of parameters.  We created the
> FetchRequest object to hold all these parameters.  Can we add this value to
> FetchRequest?  The FetchRequest doesn't get stored in the cache, so different
> callers can have different values for the origin restriction.

Done. Thank you, sorry for not picking up on that practice. That does remove a
wart added.

[abarth-chromium, 2013-11-20 05:12:31]

lgtm

This looks great.  Sorry for the delay in responding.  Thanks for the patch.

https://codereview.chromium.org/47923008/diff/360001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResourceLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/360001/Source/core/css/resolver...
Source/core/css/resolver/StyleResourceLoader.cpp:120:
shapeValue->setImage(cssImageValue->cachedImage(m_fetcher, options,
FetchRequest::UseDefaultOriginRestrictionForType));
Should setImage take a FetchRequest?

[commit-bot: I haz the power, 2013-11-20 05:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/47923008/360001

[commit-bot: I haz the power, 2013-11-20 05:12:47]

Failed to apply patch for
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A        
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
  Copied LayoutTests/http/tests/security/script-onerror-crossorigin-cors.html ->
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
  patching file
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
  Hunk #1 succeeded at 5 with fuzz 1.
  Hunk #2 FAILED at 14.
  1 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html.rej

Patch:   NR 
LayoutTests/http/tests/security/script-onerror-crossorigin-cors.html->LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
Index:
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
diff --git
a/LayoutTests/http/tests/security/script-onerror-crossorigin-cors.html
b/LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
similarity index 60%
copy from LayoutTests/http/tests/security/script-onerror-crossorigin-cors.html
copy to
LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
index
f0033b9177d3383aac546a49b310341b90d17712..32b1cf839c5b73cfcc93940d0f0507895bc71ad5
100644
--- a/LayoutTests/http/tests/security/script-onerror-crossorigin-cors.html
+++
b/LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html
@@ -5,7 +5,7 @@
     <script src="../../js-test-resources/js-test-pre.js"></script>
     <script>
         window.jsTestIsAsync = true;
-        description("The test passes if 'window.onerror' gets unsanitized
information about an exception thrown in a script loaded with a 'crossorigin'
attribute, and delivered with valid CORS headers.");
+        description("The test passes if 'window.onerror' is invoked with
unsanitized information on a script loaded with a 'crossorigin' attribute, but
loads from same-origin and without valid CORS headers.");
 
         window.onerror = function(msg, url, line, column, error) {
             window.msg = msg;
@@ -14,14 +14,14 @@
             window.column = column;
             window.errorObject = error;
             shouldBeTrue("/SomeError/.test(msg)");
-            shouldBeEqualToString("url",
"http://localhost:8000/security/resources/cors-script.php?fail=true&cors=true");
+            shouldBeEqualToString("url",
"http://127.0.0.1:8000/security/resources/cors-script.php?fail=true&cors=false");
             shouldBe("line", "1");
             shouldBe("column", "1");
             shouldNotBe("window.errorObject", "null");
             finishJSTest();
         }
     </script>
-    <script crossorigin="anonymous"
src="http://localhost:8000/security/resources/cors-script.php?fail=true&cors=true"></script>
+    <script crossorigin="anonymous"
src="resources/cors-script.php?fail=true&cors=false"></script>
     <script src="../../js-test-resources/js-test-post.js"></script>
 </body>
 </html>

[sof, 2013-11-20 09:38:03]

On 2013/11/20 05:12:47, I haz the power (commit-bot) wrote:
> Failed to apply patch for
> LayoutTests/http/tests/security/script-onerror-crossorigin-same-origin.html:
....
>

Failed to apply due to js-test-pre.js renamed to js-test.js (and -post.js gone.)
Rebased and addressed trivial conflicts.

[commit-bot: I haz the power, 2013-11-20 09:38:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/47923008/430001

[sof, 2013-11-20 10:10:30]

Thanks abarth, do appreciate you finding the time.

https://codereview.chromium.org/47923008/diff/360001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResourceLoader.cpp (right):

https://codereview.chromium.org/47923008/diff/360001/Source/core/css/resolver...
Source/core/css/resolver/StyleResourceLoader.cpp:120:
shapeValue->setImage(cssImageValue->cachedImage(m_fetcher, options,
FetchRequest::UseDefaultOriginRestrictionForType));
On 2013/11/20 05:12:31, abarth wrote:
> Should setImage take a FetchRequest?

I could well be misunderstanding, but do you mean cachedImage() ? It currently
constructs a FetchRequest instead, adding some local CSSImageValue state, but it
appears possible to have the caller do that construction here & supply it. 

More than happy to tidy this up in a followup.

[commit-bot: I haz the power, 2013-11-20 10:45:25]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[sof, 2013-11-20 12:22:55]

On 2013/11/20 10:10:30, sof wrote:
> Thanks abarth, do appreciate you finding the time.
> 
>
https://codereview.chromium.org/47923008/diff/360001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleResourceLoader.cpp (right):
> 
>
https://codereview.chromium.org/47923008/diff/360001/Source/core/css/resolver...
> Source/core/css/resolver/StyleResourceLoader.cpp:120:
> shapeValue->setImage(cssImageValue->cachedImage(m_fetcher, options,
> FetchRequest::UseDefaultOriginRestrictionForType));
> On 2013/11/20 05:12:31, abarth wrote:
> > Should setImage take a FetchRequest?
> 
> I could well be misunderstanding, but do you mean cachedImage() ? It currently
> constructs a FetchRequest instead, adding some local CSSImageValue state, but
it
> appears possible to have the caller do that construction here & supply it. 
> 
> More than happy to tidy this up in a followup.

Wait a minute, this is not right. The CSS Shapes image fetching is about doing
it in a CORS enabled manner, and not about origin restrictions. i.e.,
cachedImage() should take a CrossOriginEnabled argument, not an origin
restriction. How bad, misreading the code's use of the previous
RequestOriginPolicy field.

Uploaded a new patchset to address (CSSImageValue + StyleResourceLoader
changed). 

(I don't understand why, but the rebase caused this bug to make
http/tests/misc/acid{2,3} to fail, not allowing data: images to load. They most
certainly haven't been failing before; good thing it was caught.)

[abarth-chromium, 2013-11-20 17:32:33]

On 2013/11/20 12:22:55, sof wrote:
> (I don't understand why, but the rebase caused this bug to make
> http/tests/misc/acid{2,3} to fail, not allowing data: images to load. They
most
> certainly haven't been failing before; good thing it was caught.)

Can you add a separate test for that case?  acid2 and acid3 are annoying to
debug, so it's useful to duplicate their testing in smaller test cases.

[abarth-chromium, 2013-11-20 17:34:05]

lgtm

https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
File Source/core/css/CSSImageValue.h (right):

https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
Source/core/css/CSSImageValue.h:42: StyleFetchedImage*
cachedImage(ResourceFetcher* fetcher) { return cachedImage(fetcher,
ResourceFetcher::defaultResourceOptions(), NotCrossOriginEnabled); }
NotCrossOriginEnabled -> This name is confusing.  Does this mean
RestrictToSameOrigin or AllowAllOrigins ?

[sof, 2013-11-20 17:49:07]

https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
File Source/core/css/CSSImageValue.h (right):

https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
Source/core/css/CSSImageValue.h:42: StyleFetchedImage*
cachedImage(ResourceFetcher* fetcher) { return cachedImage(fetcher,
ResourceFetcher::defaultResourceOptions(), NotCrossOriginEnabled); }
On 2013/11/20 17:34:06, abarth wrote:
> NotCrossOriginEnabled -> This name is confusing.  Does this mean
> RestrictToSameOrigin or AllowAllOrigins ?

It means the image fetch isn't CORS enabled if out of origin, no more.

[abarth-chromium, 2013-11-20 17:50:35]

On 2013/11/20 17:49:07, sof wrote:
>
https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
> File Source/core/css/CSSImageValue.h (right):
> 
>
https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
> Source/core/css/CSSImageValue.h:42: StyleFetchedImage*
> cachedImage(ResourceFetcher* fetcher) { return cachedImage(fetcher,
> ResourceFetcher::defaultResourceOptions(), NotCrossOriginEnabled); }
> On 2013/11/20 17:34:06, abarth wrote:
> > NotCrossOriginEnabled -> This name is confusing.  Does this mean
> > RestrictToSameOrigin or AllowAllOrigins ?
> 
> It means the image fetch isn't CORS enabled if out of origin, no more.

Can we rename it to NotCORSEnabled?  We usually try to avoid acronyms, but in
this case it might be a bit clearer.

[sof, 2013-11-20 18:01:08]

On 2013/11/20 17:50:35, abarth wrote:
> On 2013/11/20 17:49:07, sof wrote:
> >
>
https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
> > File Source/core/css/CSSImageValue.h (right):
> > 
> >
>
https://codereview.chromium.org/47923008/diff/610001/Source/core/css/CSSImage...
> > Source/core/css/CSSImageValue.h:42: StyleFetchedImage*
> > cachedImage(ResourceFetcher* fetcher) { return cachedImage(fetcher,
> > ResourceFetcher::defaultResourceOptions(), NotCrossOriginEnabled); }
> > On 2013/11/20 17:34:06, abarth wrote:
> > > NotCrossOriginEnabled -> This name is confusing.  Does this mean
> > > RestrictToSameOrigin or AllowAllOrigins ?
> > 
> > It means the image fetch isn't CORS enabled if out of origin, no more.
> 
> Can we rename it to NotCORSEnabled?  We usually try to avoid acronyms, but in
> this case it might be a bit clearer.

Certainly; shorten PotentiallyCrossOriginEnabled also?

[abarth-chromium, 2013-11-20 18:18:41]

On 2013/11/20 18:01:08, sof wrote:
> Certainly; shorten PotentiallyCrossOriginEnabled also?

Yeah

[sof, 2013-11-20 19:17:34]

On 2013/11/20 17:32:33, abarth wrote:
> On 2013/11/20 12:22:55, sof wrote:
> > (I don't understand why, but the rebase caused this bug to make
> > http/tests/misc/acid{2,3} to fail, not allowing data: images to load. They
> most
> > certainly haven't been failing before; good thing it was caught.)
> 
> Can you add a separate test for that case?  acid2 and acid3 are annoying to
> debug, so it's useful to duplicate their testing in smaller test cases.

Done, added to http/tests/security/cross-origin-css & verified to fail with the
referred-to change reverted.

[sof, 2013-11-20 19:20:41]

On 2013/11/20 18:18:41, abarth wrote:
> On 2013/11/20 18:01:08, sof wrote:
> > Certainly; shorten PotentiallyCrossOriginEnabled also?
> 
> Yeah

ok, renamed. A bit studlycaps looking, but I'll get used to it :)

[abarth-chromium, 2013-11-20 20:24:36]

Thanks :)

[commit-bot: I haz the power, 2013-11-20 20:25:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/47923008/700001

[commit-bot: I haz the power, 2013-11-20 20:25:54]

Failed to apply patch for Source/core/html/HTMLImportLoader.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLImportLoader.cpp
  Hunk #1 FAILED at 113.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/html/HTMLImportLoader.cpp.rej

Patch:       Source/core/html/HTMLImportLoader.cpp
Index: Source/core/html/HTMLImportLoader.cpp
diff --git a/Source/core/html/HTMLImportLoader.cpp
b/Source/core/html/HTMLImportLoader.cpp
index
3f8ca2f319328300201db22fb28a9fca9d5d3a35..877cfe583977ff9bdec09e5c590096710b460208
100644
--- a/Source/core/html/HTMLImportLoader.cpp
+++ b/Source/core/html/HTMLImportLoader.cpp
@@ -113,7 +113,7 @@ void HTMLImportLoader::didFinish()
 HTMLImportLoader::State HTMLImportLoader::startWritingAndParsing(const
ResourceResponse& response)
 {
     // Current canAccess() implementation isn't sufficient for catching
cross-domain redirects: http://crbug.com/256976
-    if (!m_parent->document()->fetcher()->canAccess(m_resource.get()))
+    if (!m_parent->document()->fetcher()->canAccess(m_resource.get(),
PotentiallyCORSEnabled))
         return StateError;
 
     DocumentInit init = DocumentInit(response.url(), 0,
root()->document()->contextDocument(), this)

[sof, 2013-11-20 20:56:20]

Rebased and resolved straightforward HTMLImportLoader conflict (m_parent vs
parent())

[commit-bot: I haz the power, 2013-11-20 20:56:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/47923008/780001

[commit-bot: I haz the power, 2013-11-20 23:48:06]

Change committed as 162417