Bug 123114 – [CSS Shapes] CORS-enabled fetch for shape image values

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

http://dev.w3.org/csswg/css-shapes/#shape-outside-property

"User agents must use the potentially CORS-enabled fetch method defined by the [HTML5] specification for all URLs in a shape-outside value. When fetching, user agents must use "Anonymous" mode, set the referrer source to the stylesheet's URL and set the origin to the URL of the containing document. If this results in network errors such that there is no valid fallback image, the effect is as if the value auto had been specified."

Currently, shape image values must be same-origin, see https://bugs.webkit.org/show_bug.cgi?id=117610.

[Hans Muller]

Created attachment 214976 [details]
Patch

Access to shape images is now controlled by CORS CSS shape per http://dev.w3.org/csswg/css-shapes/#shape-outside-property. Previously shape images had to be same-origin.

Shape image URL access is defined by the same logic that defines canvas tainting: same-origin and data URLs are allowed and images with a "Access-Control-Allow-Origin:" header that's either "*" or that matches the document's origin.

A PotentiallyCrossOriginEnabled RequestOriginPolicy was added to ResourceLoaderOptions, to indicate that a "potentially CORS-enabled fetch" was to be undertaken. The CSSImageValue::cachedImage() method handles this option by effectively setting the "Origin:" request header (see updateRequestForAccessControl() in CrossOriginAccessControl.cpp).  StyleResolver::loadPendingShapeImage() uses the new ResourceLoaderOption.

The static ShapeInsideInfo and ShapeOutsideInfo isEnabledFor() method now performs the CORS check for image valued shapes. The private isOriginClean() method from CanvasRenderingContext2D has been moved to the CachedImage class so that it can be shared by the Canvas and Shape implementations. It checks the response headers per the CORS spec.

[WebKit Commit Bot]

Attachment 214976 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/security/resources/image-access-control.php', u'LayoutTests/http/tests/security/shape-image-cors-expected.html', u'LayoutTests/http/tests/security/shape-image-cors.html', u'LayoutTests/http/tests/security/shape-inside-image-origin-expected.txt', u'LayoutTests/http/tests/security/shape-inside-image-origin.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSImageValue.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp', u'Source/WebCore/loader/ResourceLoaderOptions.h', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/loader/cache/CachedImage.h', u'Source/WebCore/rendering/shapes/ShapeInfo.cpp', u'Source/WebCore/rendering/shapes/ShapeInfo.h', u'Source/WebCore/rendering/shapes/ShapeInsideInfo.cpp', u'Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp']" exit_code: 1
Source/WebCore/css/CSSImageValue.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hans Muller]

Created attachment 214977 [details]
Patch

Fixed a style-o.

[Andreas Kling]

Comment on attachment 214977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214977&action=review

I think this approach makes sense. I would say r=me, but there are some things I think we can do better:

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:44
> +bool ShapeInfo<RenderType>::checkImageOrigin(const RenderType* renderer, const StyleImage* styleImage)

This function assumes that 'renderer' and 'styleImage' are both non-null, so we should pass them by reference instead.
It only uses the renderer to dig out the Document, so we could just pass the Document from the call sites and avoid templatizing this function.

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:46
> +    ASSERT(styleImage && styleImage->isCachedImage() && styleImage->cachedImage() && styleImage->cachedImage()->image());

Hm. Instead of all this asserting, could we just make the function take a CachedImage& directly?
You're using the StyleImage to grab at the CachedImage inside.

> Source/WebCore/rendering/shapes/ShapeInfo.cpp:49
> +    bool originClean = styleImage->cachedImage()->isOriginClean(renderer->document().securityOrigin());
> +    if (!originClean) {

Why not just return right away?
if (styleImage->cachedImage()->isOriginClean(renderer->document().securityOrigin()))
    return true;
...
return false;

> Source/WebCore/loader/cache/CachedImage.cpp:577
> +bool CachedImage::isOriginClean(SecurityOrigin* securityOrigin)

It's ugly that this takes the SecurityOrigin by pointer, since it could crash if nullptr is passed.
Although this is a larger problem with SecurityContexts in WebCore, and not something we need to fix in this patch.

[Hans Muller]

Created attachment 215188 [details]
Patch

I've made the suggested changes.

I made static function ShapeInfo<RenderType>::checkImageOrigin() a global function called checkShapeImageOrigin() to avoid creating one static per template instantiation. I would have preferred not to add checkShapeImageOrigin() to the WebCore namespace, but I wasn't sure how to avoid doing so.  If you know of a better way to handle this, I'd appreciate the advice.

[Andreas Kling]

Comment on attachment 215188 [details]
Patch

Looks cool. r=me

[WebKit Commit Bot]

Comment on attachment 215188 [details]
Patch

Clearing flags on attachment: 215188

Committed r158044: <http://trac.webkit.org/changeset/158044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.