Bug 123114 - [CSS Shapes] CORS-enabled fetch for shape image values
Summary: [CSS Shapes] CORS-enabled fetch for shape image values
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
Depends on:
Blocks: 116348
  Show dependency treegraph
Reported: 2013-10-21 12:26 PDT by Hans Muller
Modified: 2013-10-25 13:34 PDT (History)
12 users (show)

See Also:

Patch (19.90 KB, patch)
2013-10-23 11:44 PDT, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (19.94 KB, patch)
2013-10-23 11:51 PDT, Hans Muller
kling: review-
Details | Formatted Diff | Diff
Patch (19.70 KB, patch)
2013-10-25 10:18 PDT, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 2013-10-21 12:26:49 PDT

"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.
Comment 1 Hans Muller 2013-10-23 11:44:47 PDT
Created attachment 214976 [details]

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.
Comment 2 WebKit Commit Bot 2013-10-23 11:46:42 PDT
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.
Comment 3 Hans Muller 2013-10-23 11:51:50 PDT
Created attachment 214977 [details]

Fixed a style-o.
Comment 4 Andreas Kling 2013-10-25 06:36:38 PDT
Comment on attachment 214977 [details]

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.
Comment 5 Hans Muller 2013-10-25 10:18:13 PDT
Created attachment 215188 [details]

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.
Comment 6 Andreas Kling 2013-10-25 11:25:06 PDT
Comment on attachment 215188 [details]

Looks cool. r=me
Comment 7 WebKit Commit Bot 2013-10-25 13:34:39 PDT
Comment on attachment 215188 [details]

Clearing flags on attachment: 215188

Committed r158044: <http://trac.webkit.org/changeset/158044>
Comment 8 WebKit Commit Bot 2013-10-25 13:34:41 PDT
All reviewed patches have been landed.  Closing bug.