Bug 217294 – CSS image-orientation: none should be ignored for cross-origin images

• Component: Images
• Status: RESOLVED FIXED

[Noam Rosenthal]

See https://github.com/w3c/csswg-drafts/issues/5165
To avoid leaking metadata to the embedder, images that can't be accessed by the document's security origin should always render with their default (from-image) orientation.

[Noam Rosenthal]

Equivalent chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1110330

[Noam Rosenthal]

Created attachment 410502 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Noam Rosenthal]

Created attachment 410622 [details]
Patch

[Noam Rosenthal]

Created attachment 410623 [details]
Patch

[Noam Rosenthal]

The iOS layout test failure is a slight image-diff... I think the patch can be reviewed.

[youenn fablet]

Comment on attachment 410623 [details]
Patch

iOS bot fails http/tests/security/canvas-remote-image-orientation.html.

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

> Source/WebCore/html/HTMLImageElement.h:143
> +    bool allowsOrientationOverride() const override;

final?

> Source/WebCore/rendering/RenderElement.cpp:2151
> +    return orientation;

Why not:
return element() && !element()->allowsOrientationOverride() ? ImageOrientation::FromImage : style().imageOrientation();

[Noam Rosenthal]

Created attachment 410831 [details]
Patch

[Noam Rosenthal]

Created attachment 410847 [details]
Patch

[youenn fablet]

Comment on attachment 410847 [details]
Patch

LGTM overall but I hope we can change http/tests/security/canvas-remote-image-orientation.html so that it passes on iOS.

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

> Source/WebCore/ChangeLog:31
> +                Only apply orientation for eligible images. 

Strange indentation here and above.

> Source/WebCore/html/HTMLImageElement.cpp:690
> +    return image->isOriginClean(&(document().securityOrigin()));

Could be a one liner too.

> LayoutTests/ChangeLog:11
> +                Added a test to ensure image-orientation: none is not respected for remote images in canvas

Strange indentation.

> LayoutTests/imported/w3c/ChangeLog:10
> +                Imported a new W3C test for remote image with image-orientation.

Strange indentation.

> LayoutTests/platform/ios-simulator-wk2/TestExpectations:11
> +http/tests/security/canvas-remote-image-orientation.html [ ImageOnlyFailure ]

Isn't there a way we could change the test slightly?
It seems the WPT tests are passing, so maybe the idea is to change the canvas size?

Also, we could migrate that test to http/wpt so that we would more easily reuse WPT exif-orientation-3-lr.jpg (or even contribute it to WPT at some point).

[Noam Rosenthal]

Comment on attachment 410847 [details]
Patch

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

>> LayoutTests/platform/ios-simulator-wk2/TestExpectations:11
>> +http/tests/security/canvas-remote-image-orientation.html [ ImageOnlyFailure ]
> 
> Isn't there a way we could change the test slightly?
> It seems the WPT tests are passing, so maybe the idea is to change the canvas size?
> 
> Also, we could migrate that test to http/wpt so that we would more easily reuse WPT exif-orientation-3-lr.jpg (or even contribute it to WPT at some point).

There is already a wpt test for this. I wasn't aware of http/wpt, I'll import it there instead of the webkit-specific test. Should I land this with the test working on ios, or do you want to re-review once I have that ready?

[youenn fablet]

> There is already a wpt test for this. I wasn't aware of http/wpt, I'll
> import it there instead of the webkit-specific test. Should I land this with
> the test working on ios, or do you want to re-review once I have that ready?

I think the patch is good to land once the test works on iOS.

[Noam Rosenthal]

Created attachment 410861 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 410861 [details]
Patch

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

> Source/WebCore/dom/Element.h:340
> +    virtual bool allowsOrientationOverride() const { return true; }

I do not think it is worthy adding this as a virtual method because it is overridden by one superclass which is HTMLImageElement. All it does is it saves you a casting to HTMLImageElement in RenderElement::imageOrientation().

> Source/WebCore/rendering/RenderElement.cpp:2143
> +    return (element() && !element()->allowsOrientationOverride()) ? ImageOrientation(ImageOrientation::FromImage) : style().imageOrientation();

Can be written like this:

    auto* imageElement = is<HTMLImageElement>(element()) ? downcast<HTMLImageElement>(element()) : nullptr;
    return (imageElement && ...

[Noam Rosenthal]

Created attachment 410920 [details]
Patch for landing

[EWS]

Committed r268249: <https://trac.webkit.org/changeset/268249>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410920 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70130571>