[webkit-reviews] review denied: [Bug 110880] Cross-origin XSLT requests with CORS should be allowed : [Attachment 191771] Remove some leftover

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Raphael Kubo da Costa (rakuco, Intel)
<rakuco at webkit.org>'s request for review:
Bug 110880: Cross-origin XSLT requests with CORS should be allowed
https://bugs.webkit.org/show_bug.cgi?id=110880

Attachment 191771: Remove some leftover
https://bugs.webkit.org/attachment.cgi?id=191771&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191771&action=review


This looks like a good start.  My main question is why XSLT needs special
handling in subresource loader.

>>> Source/WebCore/loader/SubresourceLoader.cpp:147
>>> +	     if (m_resource->type() == CachedResource::XSLStyleSheet &&
!m_documentLoader->document()->securityOrigin()->canRequest(newRequest.url()))
>>> +		 updateRequestForAccessControl(newRequest,
m_documentLoader->document()->securityOrigin(), DoNotAllowStoredCredentials);
>>>	     m_resource->willSendRequest(newRequest, redirectResponse);
>> 
>> Is this the right place to make this check? It seems like the line below
should get us inside CachedXSLStyleSheet, which would be better for
XSLStyleSheet specific code than SubresourceLoader.
>> 
>> Additionally, this handling of redirects seems unique - I don't see any
similar updateRequestForAccessControl calls elsewhere.
> 
> I thought about putting it in CachedXSLStyleSheet too. This would probably
mean adding some empty method to CachedResource that's only implemented by
CachedXSLStyleSheet. Is that OK?
> 
> WRT this looking unique: other places, such as <img> + <canvas>, or XHR,
provide means for users to specify the behavior their want. XSL was a special
case before by simply discarding cross-origin requests. Were you thinking of
something in specific?

Why is this check needed for XSLStyleSheet but not for CORS-enabled image
fetches?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:238
> +    KURL requestURL = request.resourceRequest().url();
> +    bool sameOriginRequest =
m_document->securityOrigin()->canRequest(requestURL);
> +
> +    if (!sameOriginRequest)
> +	   updateRequestForAccessControl(request.mutableResourceRequest(),
m_document->securityOrigin(), DoNotAllowStoredCredentials);

So, we're only doing an anonymous fetch if the initial URL is cross-origin.  We
need to be sure to test the case where a same-origin request gets redirected to
a cross-origin URL

> LayoutTests/http/tests/security/cross-origin-xsl-BLOCKED-expected.txt:1
> -CONSOLE MESSAGE: line 2: Unsafe attempt to load URL
http://localhost:8000/security/resources/forbidden-stylesheet.xsl from frame
with URL http://127.0.0.1:8000/security/resources/cross-origin-xsl.xml.
Domains, protocols and ports must match.
> +CONSOLE MESSAGE: Unsafe attempt to load URL
http://localhost:8000/security/resources/forbidden-stylesheet.xsl from frame
with URL http://127.0.0.1:8000/security/resources/cross-origin-xsl-BLOCKED.xml.
Domains, protocols and ports must match.

Why are we losing the line number?