Bug 217294 - CSS image-orientation: none should be ignored for cross-origin images
Summary: CSS image-orientation: none should be ignored for cross-origin images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2020-10-04 07:13 PDT by Noam Rosenthal
Modified: 2020-10-16 10:49 PDT (History)
15 users (show)

See Also:


Attachments
Patch (23.84 KB, patch)
2020-10-04 23:05 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (23.80 KB, patch)
2020-10-06 03:02 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (23.85 KB, patch)
2020-10-06 03:09 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (24.51 KB, patch)
2020-10-08 06:00 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (24.53 KB, patch)
2020-10-08 08:44 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (21.37 KB, patch)
2020-10-08 10:49 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (20.91 KB, patch)
2020-10-09 00:36 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2020-10-04 07:13:39 PDT
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.
Comment 1 Noam Rosenthal 2020-10-04 07:17:56 PDT
Equivalent chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1110330
Comment 2 Noam Rosenthal 2020-10-04 23:05:33 PDT
Created attachment 410502 [details]
Patch
Comment 3 EWS Watchlist 2020-10-04 23:06:32 PDT
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
Comment 4 Noam Rosenthal 2020-10-06 03:02:15 PDT
Created attachment 410622 [details]
Patch
Comment 5 Noam Rosenthal 2020-10-06 03:09:59 PDT
Created attachment 410623 [details]
Patch
Comment 6 Noam Rosenthal 2020-10-06 10:06:04 PDT
The iOS layout test failure is a slight image-diff... I think the patch can be reviewed.
Comment 7 youenn fablet 2020-10-08 01:16:45 PDT
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();
Comment 8 Noam Rosenthal 2020-10-08 06:00:33 PDT
Created attachment 410831 [details]
Patch
Comment 9 Noam Rosenthal 2020-10-08 08:44:20 PDT
Created attachment 410847 [details]
Patch
Comment 10 youenn fablet 2020-10-08 08:59:24 PDT
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).
Comment 11 Noam Rosenthal 2020-10-08 09:07:27 PDT
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?
Comment 12 youenn fablet 2020-10-08 09:08:53 PDT
> 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.
Comment 13 Noam Rosenthal 2020-10-08 10:49:50 PDT
Created attachment 410861 [details]
Patch
Comment 14 Said Abou-Hallawa 2020-10-08 11:25:35 PDT
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 && ...
Comment 15 Noam Rosenthal 2020-10-09 00:36:04 PDT
Created attachment 410920 [details]
Patch for landing
Comment 16 EWS 2020-10-09 01:19:05 PDT
Committed r268249: <https://trac.webkit.org/changeset/268249>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410920 [details].
Comment 17 Radar WebKit Bug Importer 2020-10-09 01:20:21 PDT
<rdar://problem/70130571>