1330487 - Implement referrer policy for CSS

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Thomas Nguyen (:tnguyen)]

See
https://w3c.github.io/webappsec-referrer-policy/#integration-with-css

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Apply referrer policy to reequest resources referenced from stylesheet

MozReview-Commit-ID: 84AoFANESJe

Comment 2

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8920470 [details] [diff] [review]
Apply referrer policy to reequest resources referenced from stylesheet

The basic requirements in this bug are all resources fetching from stylesheet should follow referrer policy. The request should take referrer as 
- Sheet uri if stylesheet has a location (for example external stylesheet)
- Document uri for null location or css declaration
Then apply referrer policy which is defined:
- Referrer-Policy header in response if there's any
- Document's referrer policy for null location and css declaration.
The spec does not mention explicitly, but I think referrer policy should propagate inheritably from parent to child stylesheet.
The patch works fine locally for the wpt test in 
https://github.com/w3c/web-platform-tests/commit/445cf62a150698f8c67fd1757304139f5d462576
The test does not cover the child sheet loading and @font-face and we may have to complement that. Before doing that, I may need to ask a feedback from css expert to see if I am missing anything.
Hi Cameron, could you please take a look at this?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #2)
> The test does not cover the child sheet loading and @font-face and we may
> have to complement that. Before doing that, I may need to ask a feedback
> from css expert to see if I am missing anything.

The only other thing I can think of is SVG resource documents referenced by CSS properties, e.g. filter:url(...), marker-start:url(...), etc.  These loads get initiated nsExternalResourceMap::PendingLoad::StartLoad, though that's also used for non-CSS SVG resource document references too, like <linearGradient xlink:href="...">.  It's probably worth checking what referrer we're actually sending here.

There's also -moz-binding but I guess we don't have to worry about that.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8920470 [details] [diff] [review]
Apply referrer policy to reequest resources referenced from stylesheet

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

Looks good to me.

::: layout/style/FontFaceSet.cpp
@@ +657,5 @@
>  
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
>    if (httpChannel) {
>      rv = httpChannel->SetReferrerWithPolicy(aFontFaceSrc->mReferrer,
> +                                            aFontFaceSrc->mReferrerPolicy);

Should we check if it's RP_Unset and fall back to the document's referrer policy here?

::: layout/style/Loader.cpp
@@ +637,5 @@
> +  }
> +
> +  NS_ConvertUTF8toUTF16 headerValue(tRPHeaderCValue);
> +  net::ReferrerPolicy policy =
> +    nsContentUtils::GetReferrerPolicyFromHeader(headerValue);

It might be good to have an nsContentUtils::GetReferrerPolicyFromChannel function so we don't have to do all this in here.

@@ +940,5 @@
>    }
>    return NS_OK;
>  }
>  
>  nsresult

May as well make this void, since we only ever return NS_OK.

::: layout/style/Loader.h
@@ +473,5 @@
>    // cache.
>    nsresult ObsoleteSheet(nsIURI* aURI);
>  
> +  nsresult UpdateLoadingData(URIPrincipalReferrerPolicyAndCORSModeHashKey* aKey,
> +                             SheetLoadData* aData);

Probably this shouldn't be public, since we only need to call it from SheetLoadData (a friend).

::: layout/style/ServoStyleSheet.cpp
@@ +208,5 @@
>  {
>    MOZ_ASSERT(!mMedia || mMedia->IsServo());
>    RefPtr<URLExtraData> extraData =
>      new URLExtraData(aBaseURI, aSheetURI, aSheetPrincipal);
> +  extraData->SetReferrerPolicy(mInner->mReferrerPolicy);

Let's just add the referrer policy as an extra URLExtraData constructor argument.

Comment 5

[Thomas Nguyen (:tnguyen)]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #3)
> (In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #2)
> > The test does not cover the child sheet loading and @font-face and we may
> > have to complement that. Before doing that, I may need to ask a feedback
> > from css expert to see if I am missing anything.
> 
> The only other thing I can think of is SVG resource documents referenced by
> CSS properties, e.g. filter:url(...), marker-start:url(...), etc.  These
> loads get initiated nsExternalResourceMap::PendingLoad::StartLoad, though
> that's also used for non-CSS SVG resource document references too, like
> <linearGradient xlink:href="...">.  It's probably worth checking what
> referrer we're actually sending here.
> 
> There's also -moz-binding but I guess we don't have to worry about that.

Great, thanks Cameron. I am looking at the SVG loading, it seems we only use default policy in this case.

Comment 6

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Apply referrer policy to request resources referenced from stylesheet

Review commit: https://reviewboard.mozilla.org/r/191476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191476/

Comment 7

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Add more referrer policy css tests.

Review commit: https://reviewboard.mozilla.org/r/194798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194798/

Comment 8

[Thomas Nguyen (:tnguyen)]

Hi David, sorry for tagging you here, could you please take a look at this patch?
- The basic requirements in this bug are all resources fetching from stylesheet should follow referrer policy in 
https://w3c.github.io/webappsec-referrer-policy/#integration-with-css
- The basic idea of wpt test is : the stylesheet request will store the request headers in server side then we later send another checking request again via XHR (adding ?report-header). The server will reply back a json of the headers that were used to request in the first place via CSS.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Due to MozReview's inability to inspect old patch state in a usable way
(some combination of bugs like bug 1234279, bug 1249468, bug 1296135),
I'm no longer accepting review requests in MozReview.   If you'd like me
to review patches, please submit those review requests as attached patch
files.

Comment 10

[Thomas Nguyen (:tnguyen)]

Attachment: Add more referrer policy css tests.

MozReview-Commit-ID: GZjLrlmoK4C

Comment 11

[Thomas Nguyen (:tnguyen)]

Attachment: Apply referrer policy to request resources referenced from stylesheet

MozReview-Commit-ID: 84AoFANESJe

Comment 12

[Thomas Nguyen (:tnguyen)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #9)
> Due to MozReview's inability to inspect old patch state in a usable way
> (some combination of bugs like bug 1234279, bug 1249468, bug 1296135),
> I'm no longer accepting review requests in MozReview.   If you'd like me
> to review patches, please submit those review requests as attached patch
> files.

I see, attached the patches as you suggested. Thanks.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8923688 [details] [diff] [review]
Apply referrer policy to request resources referenced from stylesheet

This isn't quite a complete review, but I think there are enough issues here to justify asking for a new patch for further review.  (I'll be unavailable next week, so it might be better to ask somebody else (probably heycam or bzbarsky).


css::URLValue should probably have the referrer policy as an added
parameter to the (now) 4-argument constructor and the (now) 5-argument
constructor.  This will simplify the code change in
CSSParserImpl::SetValueToURL, and require that you also change
nsGenericHTMLElement::ParseBackgroundAttribute.  (Presumably there
should be an appropriate referrer policy set there too?)  It seems
better to make it a parameter to the constructor since it seems like it
should be required.

I'd note that there are also some cases in SVG where we send an
incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
this in my tree, the current state of which is:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/53f130d100e7/unapplied.pass-cssvalue-url-through
Implementing Referrer-Policy seems like it might make this more
important to fix.  (I probably don't have time.)




The method name on nsContentUtils, GetReferrerPolicyFromResponse, seems odd,
given that it's getting it from a channel.  Why not call it
GetReferrerPolicyFromChannel?

In nsContentUtils:
>+  rv = httpChannel->GetRequestSucceeded(&requestSucceeded);

What in the spec justifies ignoring Referrer-Policy when displaying error
responses?  If something, it should probably be cited here; if nothing,
this code seems wrong.

>+  if (NS_FAILED(rv) || !requestSucceeded) {
>+    return NS_ERROR_FAILURE;
>+  }

In the NS_FAILED(rv) case, perhaps this should propagate rv?

In the !requestSucceeded case, it seems like this should just be NS_OK; getting
an HTTP 4xx response isn't an error in our code.

That said, I don't see any justification for returning nsresult from this function
at all.  It should just return mozilla::net::ReferrerPolicy like
GetReferrerPolicyFromHeader does.

>+  NS_ConvertUTF8toUTF16 headerValue(tRPHeaderCValue);
>+  aPolicy = GetReferrerPolicyFromHeader(headerValue);
I'd suggest just writing:

  aPolicy =
    GetReferrerPolicyFromHeader(NS_ConvertUTF8toUTF16(tRPHeaderCValue));

to avoid having to name an extra variable.

Also, the name tRPHeaderCValue seems very strange.  Why not just call
that headerValue?




In nsExternalResourceMap::PendingLoad::StartLoad, it's not clear to me
why the correct referrer is the document.  Isn't this used for various
references between SVG files?  Shouldn't the referrer thus be the
referencing file (and the policy used be its referrer-policy)?

It seems like the spec is also short here; the integration with SVG
should be explained in the spec, given the complexity of referencing in
SVG.

What do other browsers do here?

Also, other code seems to warn if SetReferrerWithPolicy fails, such as:
https://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/layout/style/FontFaceSet.cpp#660-662



In FontFaceSet::FindOrCreateUserFontEntryFromFontFace, the code appears
to fall back to the document's referrer policy if the style sheet's
referrer policy is unset.  The spec doesn't say to do this, so you shouldn't.
(Do other implementations?  Or is there a reason you think this is desirable?
If so, maybe a spec issue should be raised.)

The same is true of ImageLoader::LoadImage.


Why does SheetLoadData::GetReferrerPolicy do this same sort of
inheritance, but both to parent sheets and to documents?  I don't see
the spec saying to do this.  (See my comments on FontFaceSet.)


SheetLoadData::SetReferrerPolicyFromHeader should just return void, not
nsresult.  (It might also be good if bz looked at this part of the
patch.  That said, I'm not even sure how much this code is used
post-Stylo.)

Loader::UpdateLoadingData should probably rename:
  aKey -> aOldKey
  key -> newKey
since that's clearer.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Note that all of these behavior differences I mention should be testable -- so if there aren't such tests already, you should be adding tests to web-platform-tests that fail with the current patch but pass with the behavior fixed.)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8923687 [details] [diff] [review]
Add more referrer policy css tests.

I'm not sure what the policies on renaming files within wpt (in order to restructure tests) are.  James?

Comment 16

[Thomas Nguyen (:tnguyen)]

(In reply to David Baron :dbaron: ⌚️UTC-8 (busy week of November 6-10) from comment #13)
> Comment on attachment 8923688 [details] [diff] [review]
> Apply referrer policy to request resources referenced from stylesheet
> 
> This isn't quite a complete review, but I think there are enough issues here
> to justify asking for a new patch for further review.  (I'll be unavailable
> next week, so it might be better to ask somebody else (probably heycam or
> bzbarsky).

Thanks for sharing your time, that is a very insightful review.


> 
> The method name on nsContentUtils, GetReferrerPolicyFromResponse, seems odd,
> given that it's getting it from a channel.  Why not call it
> GetReferrerPolicyFromChannel?
> 
> In nsContentUtils:
> >+  rv = httpChannel->GetRequestSucceeded(&requestSucceeded);
> 
> What in the spec justifies ignoring Referrer-Policy when displaying error
> responses?  If something, it should probably be cited here; if nothing,
> this code seems wrong.
> 
> >+  if (NS_FAILED(rv) || !requestSucceeded) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> 
> In the NS_FAILED(rv) case, perhaps this should propagate rv?
> 
> In the !requestSucceeded case, it seems like this should just be NS_OK;
> getting
> an HTTP 4xx response isn't an error in our code.

You are right, I moved this from style/Loader.cpp to more common place nsContentUtils and did not removing !requestSucceeded.
We can bail out !requestSucceeded in Css loader but it seems wrong in common case
> That said, I don't see any justification for returning nsresult from this
> function
> at all.  It should just return mozilla::net::ReferrerPolicy like
> GetReferrerPolicyFromHeader does.
> 
> >+  NS_ConvertUTF8toUTF16 headerValue(tRPHeaderCValue);
> >+  aPolicy = GetReferrerPolicyFromHeader(headerValue);
> I'd suggest just writing:
> 
>   aPolicy =
>     GetReferrerPolicyFromHeader(NS_ConvertUTF8toUTF16(tRPHeaderCValue));
> 
> to avoid having to name an extra variable.
> 
> Also, the name tRPHeaderCValue seems very strange.  Why not just call
> that headerValue?
> 
Thank you, I will refactor that.
> 
> In nsExternalResourceMap::PendingLoad::StartLoad, it's not clear to me
> why the correct referrer is the document.  Isn't this used for various
> references between SVG files?  Shouldn't the referrer thus be the
> referencing file (and the policy used be its referrer-policy)?
> 
> It seems like the spec is also short here; the integration with SVG
> should be explained in the spec, given the complexity of referencing in
> SVG.
> 
> What do other browsers do here?

I see Chrome has landed Referrer Policy in CSS
https://codereview.chromium.org/2780533002 and it seems they did not care about SVG case. Referrer policy integration is not explicitly shown in the specs so I think we should at least use the referrer and referrer policy from document. For further details, I filed an open bug and we could discuss how to integrate referrer policy in SVG exactly.
We could fix that in a follow up bug, shouldn't we?
> I'd note that there are also some cases in SVG where we send an
> incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
> this in my tree, the current state of which is:
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> 53f130d100e7/unapplied.pass-cssvalue-url-through
> Implementing Referrer-Policy seems like it might make this more
> important to fix.  (I probably don't have time.)
> 
Thanks for the patch. I think we should fix that in a follow up bug, wait for a while until we could determine how to do with SVG

> Also, other code seems to warn if SetReferrerWithPolicy fails, such as:
> https://searchfox.org/mozilla-central/rev/
> af86a58b157fbed26b0e86fcd81f1b421e80e60a/layout/style/FontFaceSet.cpp#660-662
> 
> 
> 
> In FontFaceSet::FindOrCreateUserFontEntryFromFontFace, the code appears
> to fall back to the document's referrer policy if the style sheet's
> referrer policy is unset.  The spec doesn't say to do this, so you shouldn't.
> (Do other implementations?  Or is there a reason you think this is desirable?
> If so, maybe a spec issue should be raised.)
> 
> The same is true of ImageLoader::LoadImage.
> 
> 
> Why does SheetLoadData::GetReferrerPolicy do this same sort of
> inheritance, but both to parent sheets and to documents?  I don't see
> the spec saying to do this.  (See my comments on FontFaceSet.)

You are right, spec does not mention explicitly about that, but I added this behaviour intentionally. Spec says Referrer policy can be defined "Implicitly, via inheritance.". So in the case unset, Referrer policy will fall back to parent's policy or document's policy.  Does it look more reasonable?

Comment 17

[Thomas Nguyen (:tnguyen)]

A couple of open bugs in spec
https://github.com/w3c/webappsec-referrer-policy/issues/109#issuecomment-342202657
That clarifies the behavior of referencing resource from child. I will update the patch based on that
SVG:
https://github.com/w3c/webappsec-referrer-policy/issues/108#issuecomment-342135981
That's still uncertain, I will file a follow-up bug so we can keep eye on that.

Comment 18

[Thomas Nguyen (:tnguyen)]

(In reply to David Baron :dbaron: ⌚️UTC-8 (busy week of November 6-10) from 
> 
> I'd note that there are also some cases in SVG where we send an
> incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
> this in my tree, the current state of which is:
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> 53f130d100e7/unapplied.pass-cssvalue-url-through
> Implementing Referrer-Policy seems like it might make this more
> important to fix.  (I probably don't have time.)
> 

May I get more details of failure cases so we could test that correctly? Given that the spec of referencing in SVG is still uncertain, do you agree that I move svg things to a separate bug (bug 1415044)?

Comment 19

[James Graham [:jgraham]]

There's no general imposition against renaming files, so this is fine. For Gecko in particular you might need to move any corresponding expectation files under testing/web-platform/meta/ and we are not currently good about doing that automatically on import, but that will improve in the future.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #18)
> (In reply to David Baron :dbaron: ⌚️UTC-8 (busy week of November 6-10) from 
> > 
> > I'd note that there are also some cases in SVG where we send an
> > incorrect referrer -- at least pre-Stylo.  I have part of a patch to fix
> > this in my tree, the current state of which is:
> > https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> > 53f130d100e7/unapplied.pass-cssvalue-url-through
> > Implementing Referrer-Policy seems like it might make this more
> > important to fix.  (I probably don't have time.)
> > 
> 
> May I get more details of failure cases so we could test that correctly?
> Given that the spec of referencing in SVG is still uncertain, do you agree
> that I move svg things to a separate bug (bug 1415044)?

I don't think it's that distinct.  The issue I'm pointing out here is references *from CSS* (but for the CSS properties originally introduced by SVG), not references from the SVG to other SVGs.  I'm OK with punting the issue of what to do about references from one SVG to another, but I think it would be best to fix references from CSS properly in this bug, given that the referrer policy spec makes it more important that we get the referrer right.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8923687 [details] [diff] [review]
Add more referrer policy css tests.

I should probably cancel this, since I'm waiting for revision of the main patch before reviewing the tests.

Comment 22

[Thomas Nguyen (:tnguyen)]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #21)
> Comment on attachment 8923687 [details] [diff] [review]
> Add more referrer policy css tests.
> 
> I should probably cancel this, since I'm waiting for revision of the main
> patch before reviewing the tests.

Sorry for my delayed working on this bug. I will continue working on this bug this week based on your information.
Thanks you

Comment 23

[Thomas Nguyen (:tnguyen)]

Attachment: Parser referrer policy from header and propagate to stylesheet

MozReview-Commit-ID: 30XiSIUEuOv

Comment 24

[Thomas Nguyen (:tnguyen)]

Attachment: Propagate referrer policy from doc/sheet to URLValueData

Then use the referrer policy from URLValueData to fetch resource in
Loader and ImageLoader

MozReview-Commit-ID: JVnJWFTOPaG

Comment 25

[Thomas Nguyen (:tnguyen)]

Attachment: Use correct referrer policy for fontface loader

MozReview-Commit-ID: AP6LFKLrIt6

Comment 26

[Thomas Nguyen (:tnguyen)]

Attachment: WIP - Passing URLValue to SVG CSS property

MozReview-Commit-ID: 4UcUlCgWsxY

Comment 27

[Thomas Nguyen (:tnguyen)]

Attachment: Passing URLValue object to svg style system

We pass mozilla::css::URLValue objects instead of nsIURI. URLxtraData in
that object brings referrer and referrer policy so we can send correct Referer
headers.

MozReview-Commit-ID: 8GyUgLyvGDI

Comment 28

[Thomas Nguyen (:tnguyen)]

Attachment: Send correct Referrer header when referencing SVG from CSS

MozReview-Commit-ID: 35sQ5ZOMRyd

Comment 29

[Thomas Nguyen (:tnguyen)]

Attachment: Add referencing svg from css wpt test

MozReview-Commit-ID: CUZ0JFzMkR6

Comment 30

[Thomas Nguyen (:tnguyen)]

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #29)
> Created attachment 8942871 [details] [diff] [review]
> Add referencing svg from css wpt test
> 
> MozReview-Commit-ID: CUZ0JFzMkR6

I added some wpt tests contain: filter, clip-path, marker(start, mid, end), mask(-image), fill and stroke. The test context is in internal, inline and external styling, PI and presentation attributes. Hi Cameron, am I missing anything in the fixing svg patch and test cases?

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8942861 [details] [diff] [review]
Passing URLValue object to svg style system

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

f+ but there a couple of problems with the patch.

I think you need to do something for <mpath xlink:href=""> and <feImage xlink:href=""> too.  (Well, like <textPath xlink:href=""> it is not a CSS feature, so you may or may not want to do that in a separate bug.)

::: layout/painting/nsImageRenderer.cpp
@@ +184,4 @@
>        nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(targetURI), elementId,
> +                                                content->GetUncomposedDoc(), base);
> +
> +      RefPtr<mozilla::css::URLValue> url =

It's probably worth a comment here saying that -moz-element() only ever allows local document references, so we don't really need to provide accurate referrer policy etc. data, but it's required for the nsSVGPaintingProperty API we (now) have.

::: layout/svg/SVGObserverUtils.cpp
@@ -306,2 @@
>        ? SVGObserverUtils::GetFilterURI(aFilteredFrame, i)
> -      : aFilters[i].GetURL()->ResolveLocalRef(aFilteredElement);

We can't drop these ResolveLocalRef calls, since they are what let us handle url(#bare_fragment) values in SVG specially (where they resolve to elements in the document, not resolved against the URL of the style sheet).

@@ +400,4 @@
>      bool hasRef = false;
>      if (maskUri) {
> +      nsCOMPtr<nsIURI> uri = maskUri->GetURI();
> +      uri->GetHasRef(&hasRef);

You can just call HasRef() on the URLValue directly.

@@ +1027,1 @@
>  SVGObserverUtils::GetMarkerURI(nsIFrame* aFrame,

For all of these GetXXXURI methods, since we'll need to maintain something like the ResolveURLUsingLocalRef call, we might need to construct a new URLValue object (if it was indeed a local reference URL).  So I think it will make sense to have their return types be already_AddRefed<css::URLValue>.

@@ +1075,5 @@
>    const nsStyleSVGReset* svgReset = aFrame->StyleSVGReset();
>    MOZ_ASSERT(svgReset->mMask.mLayers.Length() > aIndex);
>  
> +  return static_cast<mozilla::css::URLValue*>(svgReset->mMask.mLayers[aIndex]
> +                                                             .mImage.GetURLValue());

I don't think this cast is correct, since GetURLValue() might return a css::URLValue object or a css::ImageValue object, depending on the nsStyleImage::mType.

::: layout/svg/SVGObserverUtils.h
@@ +461,5 @@
>  class SVGObserverUtils
>  {
>  public:
>    typedef mozilla::dom::Element Element;
> +  typedef nsInterfaceHashtable<nsPtrHashKey<mozilla::css::URLValue>,

I think this should be nsRefPtrHashKey, otherwise we will hold on to a raw URLValue pointer and the URLValue might be destroyed somewhere.

Comment 32

[Thomas Nguyen (:tnguyen)]

Attachment: Part 1: Parse referrer policy from header and propagate to stylesheet

Every child sheet loading request will use the correct referrer policy
which is parsed from Referrer-Policy header

MozReview-Commit-ID: 30XiSIUEuOv

Comment 33

[Thomas Nguyen (:tnguyen)]

Attachment: Part 2: Propagate referrer policy from doc/sheet to URLValueData

MozReview-Commit-ID: LXzTPpS90vg

Comment 34

[Thomas Nguyen (:tnguyen)]

Attachment: Part 3: Use correct referrer policy for fontface loader

MozReview-Commit-ID: IMzqO2tCDgh

Comment 35

[Thomas Nguyen (:tnguyen)]

Attachment: Part 4: Use correct referrer policy for image loader

MozReview-Commit-ID: 6wgN3ySaG3i

Comment 36

[Thomas Nguyen (:tnguyen)]

Attachment: Part 5: Passing URLValue object to svg style system

We pass mozilla::css::URLValue objects instead of nsIURI. URLxtraData in
that object brings referrer and referrer policy so we can send correct Referer
headers.

MozReview-Commit-ID: 8GyUgLyvGDI

Comment 37

[Thomas Nguyen (:tnguyen)]

Attachment: Part 6: Send correct Referrer header when referencing SVG from CSS

MozReview-Commit-ID: 35sQ5ZOMRyd

Comment 38

[Thomas Nguyen (:tnguyen)]

Attachment: Part 7: Restruct, moving wpt tests to new folder

MozReview-Commit-ID: 8qfszTSzbzN

Comment 39

[Thomas Nguyen (:tnguyen)]

Attachment: Part 8: Add referencing svg from css wpt test

MozReview-Commit-ID: CUZ0JFzMkR6

Comment 40

[Thomas Nguyen (:tnguyen)]

Attachment: Part 9: Add loading child stylesheet wpt tests

MozReview-Commit-ID: DB15kNiDdQP

Comment 41

[Thomas Nguyen (:tnguyen)]

Attachment: Part 10: Add loading font source wpt test

MozReview-Commit-ID: HFIyaL1WAbb

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8942862 [details] [diff] [review]
Send correct Referrer header when referencing SVG from CSS

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

::: dom/svg/SVGAnimationElement.cpp
@@ +452,5 @@
>    nsCOMPtr<nsIURI> targetURI;
>    nsCOMPtr<nsIURI> baseURI = GetBaseURI();
>    nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(targetURI),
>                                              aHrefStr, OwnerDoc(), baseURI);
> +  mHrefTarget.Reset(aNodeForContext,

Can SVG SMIL animations target elements in other documents?  I can't remember.  If not, then you could add a similar comment here about needing to pass in referrer information even though we know mHrefTarget won't end up loading an external document.

::: layout/svg/SVGObserverUtils.cpp
@@ +194,5 @@
> +    referrerPolicy = aURI->mExtraData->GetReferrerPolicy();
> +  }
> +
> +  mObservedElementTracker.Reset(aObservingContent, uri, referrer,
> +                                referrerPolicy, true, aReferenceImage);

(I guess another option would be to make IDTracker take a css::URLValue too, although maybe that seems a bit odd as IDTracker is also used for non-CSS things.)

Comment 43

[Thomas Nguyen (:tnguyen)]

(In reply to Cameron McCormack (:heycam) from comment #31)
> Comment on attachment 8942861 [details] [diff] [review]
> Passing URLValue object to svg style system
> 
> Review of attachment 8942861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ but there a couple of problems with the patch.
> 
> I think you need to do something for <mpath xlink:href=""> and <feImage
> xlink:href=""> too.  (Well, like <textPath xlink:href=""> it is not a CSS
> feature, so you may or may not want to do that in a separate bug.)

Yes, fixing the IDTracker.Reset would fix some of the referencing from svg to svg, but not all.
I filed bug 1415044 for that and we can fix the rest in that bug.

> (I guess another option would be to make IDTracker take a css::URLValue too,
> although maybe that seems a bit odd as IDTracker is also used for non-CSS
> things.)

I see that seems odd too. I will keep the current idea and will change if you think adding a new function in IDTracker is a better idea.

Thanks for your feedback.

Comment 44

[Thomas Nguyen (:tnguyen)]

I retake it after 6 months idle. It seems that the patch does not work anymore, because at the moment URLValue is expected to be created from Rust with rust string argument. I talked a bit with Cameron, well... while we could add the Gecko string to the URLValue, but it's a little awkward and there're several things there we will not use.
Since we need to pass referrer and referrer policy through svg style system, I think we could use URLExtraData instead ( or may create a new struct with only referrer + referrer policy)

Comment 45

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 1: Parse referrer policy from header and propagate to stylesheet

This also fixes loading child stylesheet case, using correct referrer policy stored
in parent sheet.

MozReview-Commit-ID: ARXQyleD9Wq

Comment 46

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 3: Propagate referrer policy from doc/sheet to URLExtraData

Referrer policy argurment is passed from sheet/doc to URLExtraData, default
value is RP_Unset. We use default value in some cases, particularly when there's
no certain spec talks about that (svg for example)

MozReview-Commit-ID: 5VAX1ZUXD3i

Comment 47

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 4: Use correct referrer policy for fontface loader

MozReview-Commit-ID: DKdVZMOjAQh

Comment 48

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 5: Use correct referrer policy for image loader

MozReview-Commit-ID: JA7gkRH9cDJ

Comment 49

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 6: Passing referrer information to svg style system

We create new object URLExtraRefferINfo and pass it to svg system instead of
nsIURI. The object brings referrer and referrer policy so we can send correct
Referer headers.

MozReview-Commit-ID: 2gLnQPEE9t5

Comment 50

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 7: Send correct Referrer header when referencing SVG from CSS

MozReview-Commit-ID: DAkZ1wjuEe0

Comment 51

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 8: Restruct, moving wpt tests to new folder

MozReview-Commit-ID: JHNu8tqGTdi

Comment 52

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 9: Add referencing svg from css wpt test

MozReview-Commit-ID: Fl643GY6Jhc

Comment 53

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 10: Add loading child stylesheet wpt tests

MozReview-Commit-ID: EoAXTANJH1T

Comment 54

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 11: Add loading font source wpt test

MozReview-Commit-ID: 2SLZ1zqY2Jf

Comment 55

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 11: Update web-platform tests meta using ---manifest-update

MozReview-Commit-ID: H66Qst9skKu

Comment 56

[Cameron McCormack (:heycam)]

(In reply to Thomas Nguyen[:tnguyen] ni? plz from comment #45)
> This also fixes loading child stylesheet case, using correct referrer policy
> stored in parent sheet.

Can you explain how this works?

Comment 57

[Thomas Nguyen (:tnguyen)]

Thanks you.
Ah, correcting referrer policy from the referrer-policy header could simply fix the case request resource is child sheet loading 
https://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#1527)
But that's not all for other loadings (font face and image), we need further modifications

Comment 58

[Cameron McCormack (:heycam)]

I see what you mean, thank you!

Comment 59

[Phabricator Automation]

Comment on attachment 8989405 [details]
Bug 1330487 - Part 4: Use correct referrer policy for fontface loader

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1923

Comment 60

[Cameron McCormack (:heycam)]

Can you check with your patches whether we are correctly setting the referrer policy for images loaded from CSS generated content?  For example

  <style>
  p::before { content: url(image.png); }
  </style>
  <p></p>

and, now that bug 215083 has landed:

  <style>
  p { content: url(image.png); width: 100px; height: 100px; }
  </style>
  <p></p>

since I'm not sure if they go through the same codepath for image loading as e.g. background-image.

Comment 61

[Phabricator Automation]

Comment on attachment 8989410 [details]
Bug 1330487 - Part 5: Use correct referrer policy for image loader

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1924

Comment 62

[Phabricator Automation]

Comment on attachment 8989412 [details]
Bug 1330487 - Part 7: Send correct Referrer header when referencing SVG from CSS

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1926

Comment 63

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 2 : Refactor, new constructor of URIPrincipalReferrerPolicyAndCorsModeHashKey from SheetLoadData

MozReview-Commit-ID: LC3jzw8mli7

Comment 64

[Thomas Nguyen (:tnguyen)]

(In reply to Cameron McCormack (:heycam) from comment #60)
> Can you check with your patches whether we are correctly setting the
> referrer policy for images loaded from CSS generated content?  For example
> 
>   <style>
>   p::before { content: url(image.png); }
>   </style>
>   <p></p>
> 
> and, now that bug 215083 has landed:
> 
>   <style>
>   p { content: url(image.png); width: 100px; height: 100px; }
>   </style>
>   <p></p>
> 
> since I'm not sure if they go through the same codepath for image loading as
> e.g. background-image.
There's a test for inline css div.styled {content: url(...). And I added a debug message in https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/layout/style/ImageLoader.cpp#355, it should be the samepath.

Comment 65

[Phabricator Automation]

Comment on attachment 8989403 [details]
Bug 1330487 - Part 1: Parse referrer policy from header and propagate to stylesheet

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1921

Comment 66

[Phabricator Automation]

Comment on attachment 8994121 [details]
Bug 1330487 - Part 2 : Refactor, new constructor of URIPrincipalReferrerPolicyAndCorsModeHashKey from SheetLoadData

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D2290

Comment 67

[Phabricator Automation]

Comment on attachment 8989411 [details]
Bug 1330487 - Part 6: Passing referrer information to svg style system

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1925

Comment 68

[Phabricator Automation]

Comment on attachment 8989404 [details]
Bug 1330487 - Part 3: Propagate referrer policy from doc/sheet to URLExtraData

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1922

Comment 69

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1330487 - Part 12: Update web-platform tests meta using ---manifest-update

Comment 70

[Phabricator Automation]

Comment on attachment 8989762 [details]
Bug 1330487 - Part 8: Restruct, moving wpt tests to new folder

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1942

Comment 71

[Phabricator Automation]

Comment on attachment 8989763 [details]
Bug 1330487 - Part 9: Add referencing svg from css wpt test

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1943

Comment 72

[Phabricator Automation]

Comment on attachment 8989764 [details]
Bug 1330487 - Part 10: Add loading child stylesheet wpt tests

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1944

Comment 73

[Phabricator Automation]

Comment on attachment 8989766 [details]
Bug 1330487 - Part 11: Add loading font source wpt test

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D1945

Comment 74

[Phabricator Automation]

Comment on attachment 8995874 [details]
Bug 1330487 - Part 12: Update web-platform tests meta using ---manifest-update

Cameron McCormack (:heycam) has approved the revision.

https://phabricator.services.mozilla.com/D2497

Comment 75

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29b451758da8a39079cb6713020e800ea9531776

Comment 76

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&tochange=fa218e33eefc623e73a0abf3986361c34726f6fc&fromchange=a65ee6ac6f57633123a661affd82d87bd06723ef&selectedJob=198362349

Comment 77

[Thomas Nguyen (:tnguyen)]

This bug could be landed after 1486198 (because some tests here easily trigger crash in Moz2DImageRenderer)

Comment 78

[Thomas Nguyen (:tnguyen)]

The crashes have gone after landing 1486198, but try failed at TVW3
https://treeherder.mozilla.org/#/jobs?repo=try&revision=666f850709769b9315dc549621d6cecf5491e188&selectedJob=199447301

The reason is that svg test runs slow (if the test is 0.8*TIME_OUT, the test will be marked as flaky failed)

Comment 79

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=137d7dfa3a4cdd0e1e926f69dddfb3e1a8ebd37a&selectedJob=199460347

Comment 80

[Thomas Nguyen (:tnguyen)]

try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=137d7dfa3a4cdd0e1e926f69dddfb3e1a8ebd37a&selectedJob=199460347

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7af45723ee38
Part 1: Parse referrer policy from header and propagate to stylesheet r=heycam

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0e98df4e42f
Part 2 : Refactor, new constructor of URIPrincipalReferrerPolicyAndCorsModeHashKey from SheetLoadData r=heycam
https://hg.mozilla.org/integration/autoland/rev/8ea1aad78f1f
Part 3: Propagate referrer policy from doc/sheet to URLExtraData r=heycam
https://hg.mozilla.org/integration/autoland/rev/f5e373fc941f
Part 4: Use correct referrer policy for fontface loader r=heycam
https://hg.mozilla.org/integration/autoland/rev/a68d1fcf5b77
Part 5: Use correct referrer policy for image loader r=heycam
https://hg.mozilla.org/integration/autoland/rev/a46f689bce75
Part 6: Passing referrer information to svg style system r=heycam
https://hg.mozilla.org/integration/autoland/rev/6bab7a8f441f
Part 7: Send correct Referrer header when referencing SVG from CSS r=heycam
https://hg.mozilla.org/integration/autoland/rev/0f19433e45b7
Part 8: Restruct, moving wpt tests to new folder r=heycam
https://hg.mozilla.org/integration/autoland/rev/b20b9b86b510
Part 9: Add referencing svg from css wpt test r=heycam
https://hg.mozilla.org/integration/autoland/rev/2180d6ea9c6f
Part 10: Add loading child stylesheet wpt tests r=heycam
https://hg.mozilla.org/integration/autoland/rev/221f5b9e37ed
Part 11: Add loading font source wpt test r=heycam
https://hg.mozilla.org/integration/autoland/rev/9332515c425d
Part 12: Update web-platform tests meta using ---manifest-update r=heycam

Comment 83

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13029 for changes under testing/web-platform/tests

Comment 84

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13029
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/429550021?utm_source=github_status&utm_medium=notification)

Comment 85

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7af45723ee38
https://hg.mozilla.org/mozilla-central/rev/f0e98df4e42f
https://hg.mozilla.org/mozilla-central/rev/8ea1aad78f1f
https://hg.mozilla.org/mozilla-central/rev/f5e373fc941f
https://hg.mozilla.org/mozilla-central/rev/a68d1fcf5b77
https://hg.mozilla.org/mozilla-central/rev/a46f689bce75
https://hg.mozilla.org/mozilla-central/rev/6bab7a8f441f
https://hg.mozilla.org/mozilla-central/rev/0f19433e45b7
https://hg.mozilla.org/mozilla-central/rev/b20b9b86b510
https://hg.mozilla.org/mozilla-central/rev/2180d6ea9c6f
https://hg.mozilla.org/mozilla-central/rev/221f5b9e37ed
https://hg.mozilla.org/mozilla-central/rev/9332515c425d

Comment 86

[James Graham [:jgraham]]

It seems like some tests failed the upstream stability checks (see [1]).

Do we know if those are existing failures or newly introduced?

[1] https://travis-ci.org/web-platform-tests/wpt/builds/429550021?utm_source=github_status&utm_medium=notification

Comment 87

[Ethan Tseng [:ethan]]

(In reply to James Graham [:jgraham] from comment #86)
> It seems like some tests failed the upstream stability checks (see [1]).
> Do we know if those are existing failures or newly introduced?

Thomas or Chris, could you check this?

Comment 88

[Thomas Nguyen (:tnguyen)]

One of them, the test is taking too long > 10000:

Hi James, do you know any try jobs or any equivalent test jobs so that I could reproduce the failures?

Comment 89

[James Graham [:jgraham]]

The "too long" failures are all in Chrome; there isn't a job on CI that will run the tests in Chrome (although you can try ./mach wpt --product=chrome --binary=/path/to/chrome -- path/to/test). I think the fact that the time is longer than the supposed timeout is an indication of a bug in that check, so I'm not *super* worried about that.

In Firefox Travis is showing unstable tests, which is more concerning. If these are only existing tests that moved I can force-merge upstream; otherwise we should figure out how to fix them.

Comment 90

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9685fc7d4b76a05f5d644b8badc2a056d0737815&selectedJob=200356861
Running wpt tests 20 times

Comment 91

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9685fc7d4b76a05f5d644b8badc2a056d0737815&selectedJob=200356861
Another running 20 times wpt tests but I don't see any failures.

Comment 92

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2c3ff1c31a79c1b2b34779ff8a25795e406c0b1&selectedJob=200392618
2

Comment 93

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f74bf015805bb691ffd9effc3c04ea452516f63&selectedJob=200449907

Comment 94

[Thomas Nguyen (:tnguyen)]

I tried to run test verification ( I guess that will be similar to travis ci). That test will be run 10 times, then the same test will be run another 5 times, each time in a new browser instance. Once this is done, the whole sequence will be repeated in the test chaos mode 
Running wpt tests again 20 times, and also verification test 20 times and I don't see any failure. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f74bf015805bb691ffd9effc3c04ea452516f63&selectedJob=200449907

Comment 95

[Thomas Nguyen (:tnguyen)]

(In reply to James Graham [:jgraham] from comment #89)
> The "too long" failures are all in Chrome; there isn't a job on CI that will
> run the tests in Chrome (although you can try ./mach wpt --product=chrome
> --binary=/path/to/chrome -- path/to/test). I think the fact that the time is
> longer than the supposed timeout is an indication of a bug in that check, so
> I'm not *super* worried about that.
> 
> In Firefox Travis is showing unstable tests, which is more concerning. If
> these are only existing tests that moved I can force-merge upstream;
> otherwise we should figure out how to fix them.

Thank James for looking at this.
Travis fails in every test we moved. I tried a lot of tests on nightly (you could see try jobs in previous comments) and I don't see any failure. I have no idea why we had such failures
I created a dummy PR with the same commit
https://travis-ci.org/web-platform-tests/wpt/builds/432030764?utm_source=github_status&utm_medium=notification
and it passed. Do you think it is ok if we merge the tests?

The only failure now is in Chrome, ./mach wpt --product=chrome --binary=/path/to/chrome -- path/to/test.
It probably out of topic here but when I try to run the command I get an error:

"Missing hosts file configuration. Run

./wpt make-hosts-file | sudo tee -a /etc/hosts" Do you know about that? It seems be related to https://github.com/web-platform-tests/wpt/issues/13112

Comment 96

[James Graham [:jgraham]]

Thanks for the detailed investigation. I'm confident we're not adding new problems, so merging.

Comment 97

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 98

[Florian Scholz (Open Web Docs)]

I've documented this here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy#Integration_with_CSS

Does it look good to you?

Comment 99

[Thomas Nguyen (:tnguyen)]

lgtm. Thanks

Comment 100

[Chris Mills [:cmills]]

I added a release note covering this:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#Security

Is this OK, or is it behind a pref at the moment?

Comment 101

[Jonathan Watt [:jwatt]]

(In reply to Pulsebot from comment #82)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a46f689bce75
Part 6: Passing referrer information to svg style system r=heycam

This caused bug 1550531.

The change from using nsURIHashKey to using nsRefPtrHashKey here:

https://hg.mozilla.org/mozilla-central/rev/a46f689bce75692d05843dd390018bae6e57860f#l4.163

meant that when checking whether a given URLAndReferrerInfo was already in a hash table we would compare the URLAndReferrerInfo's address instead of checking for URL/value equality here:

https://hg.mozilla.org/mozilla-central/file/a46f689bce75692d05843dd390018bae6e57860f/layout/svg/SVGObserverUtils.cpp#l602

Since consumers of GetPaintingPropertyForURI generate a new URLAndReferrerInfo to pass each time, we would then never match and would allocate a new nsSVGPaintingProperty and add it to the table each time. This memory leak appears to have gone unnoticed until the frontend code started using -moz-element with the landing of:

https://bugzilla.mozilla.org/show_bug.cgi?id=1540984#c31

I think this only happens (the -moz-element gets used) when the user has a theme with a large image banner installed. E.g. https://addons.mozilla.org/en-US/firefox/addon/firefox-quantum-nightly/

Thomas, you probably want to audit the other patches for unintentional hashing changes that may have resulted in similar issues.

Comment 102

[Thomas Nguyen (:tnguyen)]

Hashing change is only used in the patch you pointed out (and you fixed it). Thanks for the fix.