Bug 123114

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.

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.

Created attachment 214977 [details] Patch Fixed a style-o.

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.

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.

Comment on attachment 215188 [details] Patch Looks cool. r=me

Comment on attachment 215188 [details] Patch Clearing flags on attachment: 215188 Committed r158044: <http://trac.webkit.org/changeset/158044>

All reviewed patches have been landed. Closing bug.