Closed Bug 1384493 Opened 7 years ago Closed 7 years ago

dom/base/test/test_link_stylesheet.html fails when we enable rcwn, due to speculative loading style

Categories

(Core :: Security, enhancement)

Product:
Core
Component:
Security
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: CuveeHsu, Assigned: tnguyen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

LoadStyleLink and LoadInlineStyle should use correct referrer policy. r=heycam
19.26 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
Speculative loading style should use correct referrer policy. r=wchen
8.15 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review 
RCWN would break the test by the following command

mach mochitest --setpref network.http.rcwn.enabled=true dom/base/test/test_link_stylesheet.html

bug 1358038 comment 0 has a brief of RCWN.
RCWN might:
 a) get some resource from net instead of cache
 b) remove If-Modified-Since/If-Match header

Hello Thomas,
Do you think the above behaviors would change the test?

If yes, we might disable RCWN in this test.
Otherwise, we should find the bug.
Flags: needinfo?(tnguyen)
I think the test should be passed as expected. The loading resource request should use the correct referrer/referrer policy.
I tried to run the test locally and it failed randomly (intermittent). Looks like the request use document's referrer policy instead of referrerPolicy attribute in the link element.
Plz see
http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/dom/base/nsStyleLinkElement.cpp#557
We got the correct policy here, but somewhere, we probably sent request with another policy. We should find that place and correct it by using GetLinkReferrerPolicy().
If fixing that is going to delay your work, please disable the test temporarily. We should use this bug to keep track of that issue.
Flags: needinfo?(tnguyen)
Thanks for your prompt response!

The following command might make failure more deterministic.
mach mochitest --disable-e10s  --setpref network.http.rcwn.enabled=true --setpref network.http.rcwn.max_wait_before_racing_ms=0 dom/base/test/test_link_stylesheet.html

assign myself to investigate
Assignee: nobody → juhsu
Whiteboard: [necko-active]
Well, we did a speculative loading that does not follow the referrerpolicy in attribute
The race condition in network.http.rcwn.enabled will decide if we need to use the second request network, then we failed randomly.
We have to fix that in speculative loading style.
Summary: dom/base/test/test_link_stylesheet.html fails when we enable rcwn → Speculative loading style should use correct referrer policy
Summary: Speculative loading style should use correct referrer policy → dom/base/test/test_link_stylesheet.html fails when we enable rcwn, due to speculative loading style
Component: Networking: Cache → Security
Assignee: juhsu → tnguyen
Thanks for taking this, Thomas!
Whiteboard: [necko-active]
To confirm, the patch works well to the test here :)
Attachment #8891208 - Flags: review?(wchen)
I uploaded a patch to correct referrer policy when loading style (preloading and some places which may parse referrerpolicy attribute)

Hi, wchen, heycam. Could you please take a look at the patch?
Thanks
Comment on attachment 8891209 [details]
Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy

https://reviewboard.mozilla.org/r/162416/#review167684

Hi Thomas, some questions.

::: dom/base/nsContentSink.cpp:656
(Diff revision 1)
> +            // https://html.spec.whatwg.org/multipage/urls-and-fetching.html#referrer-policy-attribute
> +            // Specs says referrer policy attribute is an enumerated attribute,
> +            // case insensitive and includes the empty string
> +            if (referrerPolicy.IsEmpty()) {
> +              referrerPolicy = value;
> +              referrerPolicy.StripWhitespace();

Is StripWhitespace() really the right thing to call?  That will turn e.g. "  or   i  gin  " into "origin", not just strip it from the start/end.  But in any case, HTML enumerated attributes don't allow white space in their values at all.  So I think we should just avoid stripping and allow AttributeReferrerPolicyFromString to reject strings that have white space in them.

The other uses of StripWhitespace() for the other Link header attributes above look suspicious, too!  Can you please file a bug for that?  (Perhaps there is some legacy need to allow random white space in these attributes but I don't know.)

::: dom/base/nsStyleLinkElement.cpp:526
(Diff revision 1)
> +  // if referrer attributes are enabled in preferences, load the link's referrer
> +  // attribute. If the link does not provide a referrer attribute, ignore this
> +  // and use the document's referrer policy

This comment seems wrong / out of date.  Can you update it to talk about referrerpolicy="" instead?

::: dom/base/nsStyleLinkElement.cpp:530
(Diff revision 1)
> +  net::ReferrerPolicy referrerPolicy = GetLinkReferrerPolicy();
> +  if (referrerPolicy == net::RP_Unset) {
> +    referrerPolicy = doc->GetReferrerPolicy();
> +  }

It seems like we do this "fall back to the document's referrer policy if not specified on the element" thing twice: once here, and then inside Loader::LoadInlineStyle.  Is that really needed?

::: dom/xml/nsXMLContentSink.cpp:1283
(Diff revision 1)
>  
>    nsContentUtils::GetPseudoAttributeValue(aData, nsGkAtoms::title, aTitle);
>  
>    nsContentUtils::GetPseudoAttributeValue(aData, nsGkAtoms::media, aMedia);
>  
> +  nsContentUtils::GetPseudoAttributeValue(aData, nsGkAtoms::referrer, aMedia);

Is there a spec that says <?xml-stylesheet?> processing instructions should support a referrer="" attribute?  And if so, why is it referrer="" and not referrerpolicy="" like on <link> etc.?
Comment on attachment 8891209 [details]
Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy

https://reviewboard.mozilla.org/r/162416/#review167684

> Is StripWhitespace() really the right thing to call?  That will turn e.g. "  or   i  gin  " into "origin", not just strip it from the start/end.  But in any case, HTML enumerated attributes don't allow white space in their values at all.  So I think we should just avoid stripping and allow AttributeReferrerPolicyFromString to reject strings that have white space in them.
> 
> The other uses of StripWhitespace() for the other Link header attributes above look suspicious, too!  Can you please file a bug for that?  (Perhaps there is some legacy need to allow random white space in these attributes but I don't know.)

You are right, https://html.spec.whatwg.org/multipage/semantics.html#the-link-element
crossorigin should not be use StripWhitespace too.

> It seems like we do this "fall back to the document's referrer policy if not specified on the element" thing twice: once here, and then inside Loader::LoadInlineStyle.  Is that really needed?

You are right. Because we added aReferrerPolicy parameter, will remove the check condition in LoadInlineStyle

> Is there a spec that says <?xml-stylesheet?> processing instructions should support a referrer="" attribute?  And if so, why is it referrer="" and not referrerpolicy="" like on <link> etc.?

Thank you for pointing out that , no explicit specs talks about that. I think should use EmptyString() and use document'referrer policy instead.
Thanks
Comment on attachment 8891209 [details]
Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy

https://reviewboard.mozilla.org/r/162416/#review167684

> You are right, https://html.spec.whatwg.org/multipage/semantics.html#the-link-element
> crossorigin should not be use StripWhitespace too.

the spec http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#strip-leading-and-trailing-whitespace seems out of date.
I will not use CompressWhitespace and keep all white spaces
Comment on attachment 8891209 [details]
Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy

https://reviewboard.mozilla.org/r/162416/#review167710

r=me with these comments addressed.

::: dom/base/nsContentSink.cpp:651
(Diff revision 2)
> +            // https://html.spec.whatwg.org/multipage/urls-and-fetching.html#referrer-policy-attribute
> +            // Specs says referrer policy attribute is an enumerated attribute,
> +            // case insensitive and includes the empty string

Maybe note in the comment here that we will parse this value with AttributeReferrerPolicyFromString later, which will handle parsing it as an enumerated attribute.

::: dom/base/nsStyleLinkElement.cpp:526
(Diff revision 2)
> +  // if referrer attributes are enabled in preferences, load the link's
> +  // referrerpolicy attribute. If the link does not provide a referrerpolicy
> +  // attribute, ignore this and use the document's referrer policy

"if referrer attributes are enabled in preferences" -- are there still actually such a preference?  Looking through GetLinkReferrerPolicy, I don't see it doing any pref checks.  If that part of the sentence is out of date too, please remove it.

Also, consider duplicating the comment in nsContentSink::ProcessLink.

::: dom/xml/nsXMLContentSink.cpp:1256
(Diff revision 2)
>        type.IsEmpty()                          ||
>        type.LowerCaseEqualsLiteral("text/css")) {
>      return DidProcessATokenImpl();
>    }
>  
> -  nsAutoString href, title, media;
> +  nsAutoString href, title, media, referrerPolicy;

Nit: remove referrerPolicy.

::: dom/xml/nsXMLContentSink.cpp:1264
(Diff revision 2)
>    // If there was no href, we can't do anything with this PI
> -  if (!ParsePIData(data, href, title, media, isAlternate)) {
> +  if (!ParsePIData(data, href, title, media,isAlternate)) {
>        return DidProcessATokenImpl();
>    }
>  
> -  rv = ProcessStyleLink(node, href, isAlternate, title, type, media);
> +  rv = ProcessStyleLink(node, href, isAlternate, title, type, media, EmptyString());

Maybe comment here that <?xml-stylesheet?> processing instructions don't have a referrerpolicy pseudo-attribute, so we pass in an empty string for it.

::: layout/style/Loader.h:231
(Diff revision 2)
>     * @param aMedia the media string for the sheet.
>     * @param aObserver the observer to notify when the load completes.
>     *        May be null.

Can you add a comment in here documenting the argument?
Attachment #8891209 - Flags: review?(cam) → review+
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #12)
> the spec
> http://www.w3.org/html/wg/drafts/html/CR/infrastructure.html#strip-leading-
> and-trailing-whitespace seems out of date.
> I will not use CompressWhitespace and keep all white spaces

Please use the WHATWG HTML spec as a reference instead. :-)
Cool, thanks Cameron
Comment on attachment 8891209 [details]
Bug 1384493 - LoadStyleLink and LoadInlineStyle should use correct referrer policy

https://reviewboard.mozilla.org/r/162416/#review167714

Updated the patch
Comment on attachment 8891208 [details]
Bug 1384493 - Speculative loading style should use correct referrer policy

https://reviewboard.mozilla.org/r/162414/#review168452
Attachment #8891208 - Flags: review?(william) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
The first patch doesn't meet the review requirements for Autoland to push it.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
I see "A suitable reviewer has not given a "Ship It!" in the patch even html/parser peer gave a r+
Hi William, could you please take a look again to see if you could change the flag? Or there is a potential bug in review board system.  
Thanks for your review.
Flags: needinfo?(william)
When preloading style link in <head>, we used speculative referrer policy which is
delievered from meta tag and ignored the referrerpolicy attribute.
We should use referrerpolicy attribute with the higher priority

MozReview-Commit-ID: 1rQmBV01jvV
If the link element has referrerpolicy attribute, we should use policy in the
attribute with higher priority

MozReview-Commit-ID: GZZeRaoxPUw
Attachment #8891208 - Attachment is obsolete: true
Attachment #8891209 - Attachment is obsolete: true
Comment on attachment 8897314 [details] [diff] [review]
Speculative loading style should use correct referrer policy

Change to use raw patch due to potential bug in reviewboard
Attachment #8897314 - Flags: review+
If the link element has referrerpolicy attribute, we should use policy in the
attribute with higher priority

MozReview-Commit-ID: GZZeRaoxPUw
When preloading style link in <head>, we used speculative referrer policy which is
delievered from meta tag and ignored the referrerpolicy attribute.
We should use referrerpolicy attribute with the higher priority

MozReview-Commit-ID: 1rQmBV01jvV
Attachment #8897314 - Attachment is obsolete: true
Attachment #8897315 - Attachment is obsolete: true
Attachment #8897318 - Flags: review+
Flags: needinfo?(william)
Attachment #8897319 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a77f844150
Speculative loading style should use correct referrer policy. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e3c926ff03
LoadStyleLink and LoadInlineStyle should use correct referrer policy. r=heycam
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/19a77f844150
https://hg.mozilla.org/mozilla-central/rev/53e3c926ff03
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
See Also: → CVE-2017-7842
See Also: → 1399780
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: