W3C referrer policy attribute is not passed to image

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tnguyen, Assigned: tnguyen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [domsecurity-backlog])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Running web platform test [1] I found that setting referrer policy took no effect on image. Enabling preference enablePerElementReferer ( in about:config or rebuild as Bug 1223838), I see in [2], GetImageReferrerPolicy() never returns the value we passed to image element.
Changes content attribute and referrerpolicy or IDL attribute referrerPolicy has no effect.

[1] https://github.com/w3c/web-platform-tests/tree/master/referrer-policy
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#865

(Iframe element works with IDL attribute referrerPolicy)
(Assignee)

Comment 1

2 years ago
(In reply to Thomas Nguyen[:tnguyen][:thomas][:nguyen] from comment #0)
> about:config or rebuild as Bug 1223838), I see in [2],
> GetImageReferrerPolicy() never returns the value we passed to image element.

Edit: fail in many cases, mostly in "no-referrer", and "origin-only" with img-tag, and GetImageReferrerPolicy() does not get correctly what we passed. (using default).
Probably this is similar to Bug 1261003.
(Assignee)

Updated

2 years ago
Blocks: 1168540
(Assignee)

Comment 2

2 years ago
First look:
Referrer policy is set in [1], at that time, there's no referrerpolicy attribute (or referrerpolicy has not set yet).
Then after setting referrerpolicy attribute, LoadImage(this pass referrerpolicy to channel) has not been called again.
Should find a nice way to pass referrerpolicy after setting attribute (at least reloading image) 
[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsImageLoadingContent.cpp#865
Whiteboard: [domsecurity-backlog]
(Assignee)

Updated

2 years ago
QA Whiteboard: /loadimage
(Assignee)

Comment 3

2 years ago
The situation is if we have an attribute setting order like

img.src = xxx
img.referrerPolicy = xxx

The problem is img.src = xxx triggered image loading (and passed referrerPolicy, default value to httpChannel). I think img.referrrerPolicy = xxx probably should reload the image with new policy (because we created a channel and started loading image with default policy).
That's what we did with crossOrigin in [1] as specs [2]
Could we do similarly to referrer policy?
[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLImageElement.cpp#462
[2] https://html.spec.whatwg.org/multipage/embedded-content.html#the-img-element

Hi Josh
I am not very sure about image loading. Sorry if I flag you here, I don't know who to flag in :). Could you please tell anything about that?
Flags: needinfo?(josh)
It makes sense to me to treat referrerPolicy similarly to crossOrigin, but I have filed https://github.com/w3c/webappsec-referrer-policy/issues/33 to clarify the intentions of the specification authors.
Flags: needinfo?(josh)
(Assignee)

Comment 5

2 years ago
Well, I see referrer-policy is not supposed to use as relevant mutations as [1]. It means setting  referrerPolicy does not change policy/referrer of loading channel.
Should we change order of attribute setting in [2] to 
img.referrerPolicy = xxx then 
img.src = xxx

Currently I am using this way to pass failed test locally.
[1] https://github.com/w3c/webappsec-referrer-policy/pull/35
[2] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/referrer-policy/generic/common.js#46
Flags: needinfo?(josh)
Flags: needinfo?(franziskuskiefer)
This is directly linked to bug 1174921 and [1]. I think I'm ok with the proposed change in comment 5 as this is the consensus. You can then also fix the image loader and remove the referrer policy check [2].

[1] https://github.com/w3c/webappsec-referrer-policy/issues/25
[2] https://dxr.mozilla.org/mozilla-central/source/image/imgLoader.cpp#671
Flags: needinfo?(franziskuskiefer)
Given https://github.com/w3c/webappsec-referrer-policy/pull/35#issuecomment-213375044, I suspect that modifying the test harness code is not the right solution here.
Flags: needinfo?(josh)
(Assignee)

Comment 9

2 years ago
Created attachment 8744829 [details] [diff] [review]
ForceReload when updating referrerpolicy

Thanks for looking into this.
Hmm, I saw the discussion in github, and finally referrerPolicy is not treated as relevant mutation.

I did debugging a while and I found that we triggered image loading right after setting img.src=AAA and then ForceReload when crossOrigin was updated. Probably, our image loading is not up-to-date to new embedded element specs.
I think bug 1076583 was filed for that but it's not in active state at the time being. And I am not sure when it would be fixed because it's inactive for more 1 year, therefore, I would like to do as comment 4 until our image loading algorithm is updated (at least bug 1076583 is fixed).
Attachment #8744829 - Flags: feedback?(josh)
Attachment #8744829 - Flags: feedback?(franziskuskiefer)
Assignee: nobody → tnguyen
Comment on attachment 8744829 [details] [diff] [review]
ForceReload when updating referrerpolicy

Review of attachment 8744829 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with the comment updated along the lines I suggested.

::: dom/html/HTMLImageElement.cpp
@@ +465,5 @@
> +             aNameSpaceID == kNameSpaceID_None &&
> +             aNotify) {
> +      // XXX: Bug 1076583 - We still use the older synchronous algorithm
> +      // Because referrerPolicy is not treated as relevant mutations,
> +      // setting the attribute will not trigger reloading/updating

.. will neither trigger a reload nor update the referrer policy of the loading channel.
Attachment #8744829 - Flags: feedback?(franziskuskiefer) → feedback+
Attachment #8744829 - Flags: feedback?(josh) → feedback+
(Assignee)

Comment 11

2 years ago
Created attachment 8748543 [details]
MozReview Request: Bug 1261298 - W3C referrer policy attribute is not passed to image. r=jdm

Review commit: https://reviewboard.mozilla.org/r/50385/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50385/
Attachment #8748543 - Flags: review?(josh)
(Assignee)

Updated

2 years ago
Attachment #8744829 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
We still have tests that are expected to fail in img-tag due to bug 1257249.
(Assignee)

Comment 13

2 years ago
Comment on attachment 8748543 [details]
MozReview Request: Bug 1261298 - W3C referrer policy attribute is not passed to image. r=jdm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50385/diff/1-2/
(Assignee)

Comment 14

2 years ago
Updated and rebased to the new changes.
Hi Josh,
Sorry for adding a reviewing flag to you. Could you please take a look on this? Or, do you know anyone I could ask for reviewing this bug?
Thank you so much
Comment on attachment 8748543 [details]
MozReview Request: Bug 1261298 - W3C referrer policy attribute is not passed to image. r=jdm

https://reviewboard.mozilla.org/r/50385/#review48487

::: dom/base/test/img_referrer_testserver.sjs:199
(Diff revision 1)
>  
>      result["tests"][name] = test;
>  
>      setSharedState(sharedKey, JSON.stringify(result));
> +
> +    if (content === 'img' || content === 'image') {

I don't think we ever use 'img'?
Attachment #8748543 - Flags: review?(josh) → review+
(Assignee)

Comment 16

2 years ago
Comment on attachment 8748543 [details]
MozReview Request: Bug 1261298 - W3C referrer policy attribute is not passed to image. r=jdm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50385/diff/2-3/
Attachment #8748543 - Attachment description: MozReview Request: Bug 1261298 - W3C referrer policy attribute is not passed to image. r?jdm → MozReview Request: Bug 1261298 - W3C referrer policy attribute is not passed to image. r=jdm
(Assignee)

Comment 17

2 years ago
Great. Thanks for your reviewing Josh :)
(Assignee)

Comment 19

2 years ago
Try looks good.
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e78d7772c3e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Thomas, thanks for fixing this bug!
We are one more step close to enabling the referrer policy attribute. :)
You need to log in before you can comment on or make changes to this bug.