Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Create a pref to control the default referrer

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: francois, Assigned: tnguyen)

Tracking

({dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [domsecurity-backlog])

Attachments

(3 attachments, 11 obsolete attachments)

15.69 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
12.96 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
51.26 KB, patch
tnguyen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 months ago
The default referrer is defined to be no-referrer-when-downgrade. It would be good to give users the ability to change this via an about:config pref. Currently users can _override_ various aspects of the referrer, but they can't change the default in a way that still respects the wishes of sites that provide a different policy.

I'm thinking of network.http.referer.defaultPolicy which would take these values:

0 - no-referrer
1 - same-origin
2 - strict-origin-when-cross-origin
3 - no-referrer-when-downgrade (default)

That pref would default to 3 and we should write tests to ensure that nothing actually changes as a result of adding this pref.

The default for the header is set here: https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/netwerk/protocol/http/HttpBaseChannel.cpp#1284

and we probably also need to change the meta referrer code too.

Once we have that pref, we can experiment with it in Shield.
Whiteboard: [domsecurity-backlog]
(Assignee)

Updated

10 months ago
See Also: → bug 587523
(Assignee)

Comment 1

10 months ago
Reference
https://www.w3.org/TR/referrer-policy/#user-controls

Nothing in this specification should be interpreted as preventing user agents from offering options to users which would change the information sent out via a `Referer` header. For instance, user agents MAY allow users to suppress the referrer header entirely, regardless of the active referrer policy on a page.
(Assignee)

Updated

10 months ago
Assignee: nobody → tnguyen
(Assignee)

Updated

9 months ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla52
(Assignee)

Comment 2

9 months ago
As discussed, we are handling the case that a site does not specify referrer policy and causes a fallback to a referrer policy defined as default preference.
After checking our code, the required change is going too far as I thought. We are using 3 kinds of aliases to define this default value (and the value of them is 0u):

- nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE (as search in [1])
- nsIHttpChannel.REFERRER_POLICY_DEFAULT (search result in [2])
- mozilla::net::RP_Default (search in [3])

I am looking back the specs and we now have "Empty string state" [4]:
The empty string corresponds to no referrer policy, causing a fallback to a referrer policy defined elsewhere, or in the case where no such higher-level policy is available, defaulting to "no-referrer-when-downgrade". We defined a REFERRER_POLICY_UNSET which corresponds this state.

The idea is that I am going to move all the 3 aliases above to set the default to REFERRER_POLICY_UNSET, then the policy will be replaced as default value in channel before sending out [5].

First approach (patch attached), does anybody have any thoughts about that?
[1] https://dxr.mozilla.org/mozilla-central/search?q=no_referrer_when_downgrade&redirect=false
[2] https://dxr.mozilla.org/mozilla-central/search?q=REFERRER_POLICY_DEFAULT&redirect=false
[3] https://dxr.mozilla.org/mozilla-central/search?q=RP_Default
[4] https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-empty-string
[5] https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/netwerk/protocol/http/HttpBaseChannel.cpp#1273
(Assignee)

Comment 3

9 months ago
Created attachment 8804619 [details] [diff] [review]
Create a pref to control the default referrer

MozReview-Commit-ID: 7KBEIrrfOZw
(Assignee)

Updated

9 months ago
Flags: needinfo?(josh)
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(francois)
(Assignee)

Updated

9 months ago
Depends on: 1264792
I didn't check all the textual replacements in your patch. But the idea and actual pref handling code look sensible to me. Though, we should probably put the pref under network.http.referer as Francois suggested.
Flags: needinfo?(franziskuskiefer)
(Assignee)

Comment 5

9 months ago
Comment on attachment 8804619 [details] [diff] [review]
Create a pref to control the default referrer

Thanks Franziskus.
I just tried to run some tests and this bug is blocked by bug 1264792.
I would like to set the milestone of this bug to mozilla53 because it requires longer time than I thought.
Attachment #8804619 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Target Milestone: mozilla52 → mozilla53
(Assignee)

Updated

9 months ago
Flags: needinfo?(josh)
Flags: needinfo?(francois)
(Assignee)

Updated

8 months ago
Depends on: 1318376
(Assignee)

Comment 6

7 months ago
Created attachment 8818198 [details] [diff] [review]
Create a pref to control the default referrer

MozReview-Commit-ID: JK1IeQ99nqX
(Assignee)

Updated

7 months ago
Attachment #8818198 - Attachment is obsolete: true
(Assignee)

Comment 7

7 months ago
Created attachment 8820163 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: Exju6vN8dRG
(Assignee)

Comment 8

7 months ago
Created attachment 8820164 [details] [diff] [review]
Create a pref to control the default referrer - part 2

MozReview-Commit-ID: 2hZb5l87XJu
(Assignee)

Comment 9

7 months ago
Created attachment 8820166 [details] [diff] [review]
Create a pref to control the default referrer - part 3

MozReview-Commit-ID: 7nf9NmmvLwQ
(Assignee)

Comment 10

7 months ago
Created attachment 8820179 [details] [diff] [review]
Create a pref to control the default referrer - part 3

MozReview-Commit-ID: 7nf9NmmvLwQ
(Assignee)

Updated

7 months ago
Attachment #8820166 - Attachment is obsolete: true
(Assignee)

Comment 11

7 months ago
dxr search
https://dxr.mozilla.org/mozilla-central/search?q=regexp%3Ano_referrer_when|rp_default|er_policy_default

I am changing all 3 defaults in several places
- nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE
- nsIHttpChannel.REFERRER_POLICY_DEFAULT
- mozilla::net::RP_Default

to nsIHttpChannel.REFERRER_POLICY_UNSET ( and mozilla::net::RP_Unset)
- Changing in necko: before sending out a request, the referrer policy then will be checked if it equals Unset and will cause a fallback to the user control default policy defined by Preference.
- Change in Fetch: If request’s referrer policy is the empty string, then set request’s referrer policy to "no-referrer-when-downgrade". Should change to the user control value here.
- Others: mostly change the above 3 defaults to Unset value
Hi Patrick, bkelly, may I get your review in these patches?
Thanks
(Assignee)

Updated

7 months ago
Attachment #8820163 - Flags: review?(bkelly)
(Assignee)

Updated

7 months ago
Attachment #8820164 - Flags: review?(mcmanus)
(Assignee)

Updated

7 months ago
Attachment #8820179 - Flags: review?(bkelly)
Attachment #8820164 - Flags: review?(mcmanus) → review+
Comment on attachment 8820163 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

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

Looks reasonable, but I think we should have a single source of truth on the pref.  Please expose the default policy from necko using a nsNetUtil function.

::: dom/fetch/FetchDriver.cpp
@@ +107,5 @@
>  // Necko HTTP implementation.
>  nsresult
>  FetchDriver::HttpFetch()
>  {
> +  workers::AssertIsOnMainThread();

nit: Can you just use MOZ_ASSERT(NS_IsMainThread()) here?  We typically don't use the worker AssertIsOnMainThread() outside of worker-specific code.

@@ +286,5 @@
> +      // 2 - strict-origin-when-cross-origin
> +      // 3 - no-referrer-when-downgrade (default)
> +
> +      uint8_t rpUserControl = Preferences::GetInt("network.http.referer.userControlPolicy", 3);
> +      net::ReferrerPolicy referrerPolicy = net::RP_No_Referrer_When_Downgrade;

Please create a nsNetUtil function that returns the default value here.  It can then internally use gHttpHandler and reuse the same code the channel code uses.

::: dom/workers/ScriptLoader.cpp
@@ +205,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel)) {
>      mozilla::net::ReferrerPolicy referrerPolicy = parentDoc ?
> +      parentDoc->GetReferrerPolicy() : mozilla::net::RP_Unset;

Are you removing the RP_Default value completely?  Seems like we are effectively saying it should not be used, right?
Attachment #8820163 - Flags: review?(bkelly) → review-
Comment on attachment 8820179 [details] [diff] [review]
Create a pref to control the default referrer - part 3

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

r=me with comments addressed

::: dom/base/nsContentUtils.cpp
@@ +8521,5 @@
>    net::ReferrerPolicy referrerPolicy = mozilla::net::RP_Unset;
>    while (tokenizer.hasMoreTokens()) {
>      token = tokenizer.nextToken();
> +    // If token is a referrer policy and token is not the empty string,
> +    // then set policy to token

I think it would be clearer to just do:

if (token.IsEmpty()) {
  continue;
}

And then remove the comment and the extra conditional below.

::: image/imgLoader.cpp
@@ +2009,5 @@
>      nsresult rv = LoadImage(aURI,
>                              aInitialDocumentURI,
>                              aReferrerURI,
>                              refpol == mozilla::net::RP_Unset ?
> +                              mozilla::net::RP_Unset : refpol,

I think you can replace the ternary with just refpol.
Attachment #8820179 - Flags: review?(bkelly) → review+
(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #11)
> I am changing all 3 defaults in several places
> - nsIHttpChannel.REFERRER_POLICY_NO_REFERRER_WHEN_DOWNGRADE
> - nsIHttpChannel.REFERRER_POLICY_DEFAULT
> - mozilla::net::RP_Default

I think we should probably also remove the DEFAULT constants as well to avoid people accidentally using them in new places.
(Assignee)

Comment 15

7 months ago
Created attachment 8821083 [details] [diff] [review]
Create a pref to control the default referrer policy - part 3

MozReview-Commit-ID: LgYN693WNbD
(Assignee)

Updated

7 months ago
Attachment #8820179 - Attachment is obsolete: true
(Assignee)

Comment 16

7 months ago
Created attachment 8821084 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

MozReview-Commit-ID: 18CX3J3jYho
(Assignee)

Updated

7 months ago
Attachment #8820164 - Attachment is obsolete: true
(Assignee)

Comment 17

7 months ago
Created attachment 8821085 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: GyFKuzXnVp5
(Assignee)

Updated

7 months ago
Attachment #8820163 - Attachment is obsolete: true
(Assignee)

Comment 18

7 months ago
You are right, should remove the unnecessary constants to avoid people from mistakenly using them later.
Thanks for your review.
Updated the patch based on the comment 10 (adding netUtil function and removing default constants)
Could you please take a look again?
(Assignee)

Updated

7 months ago
Attachment #8821084 - Flags: review?(mcmanus)
(Assignee)

Updated

7 months ago
Attachment #8821083 - Flags: review+
(Assignee)

Updated

7 months ago
Attachment #8821085 - Flags: review?(bkelly)
(Assignee)

Updated

7 months ago
Attachment #8821084 - Flags: review?(bkelly)
Comment on attachment 8821084 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

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

::: netwerk/base/nsNetUtil.cpp
@@ +2378,5 @@
> +NS_GetDefaultReferrerPolicy(uint32_t& aDefaultPolicy)
> +{
> +  aDefaultPolicy = RP_No_Referrer_When_Downgrade;
> +
> +  if (!gHttpHandler) {

Oh, hmm.  I didn't consider that you might not have a gHttpHandler, but I guess thats the case in the content process.

Rather than spin up the full nsHttpHandler in the child process its probably better just to create a static variable here that contains the pref value.  gNetDefaultReferrer or something.  You can then use the preference APIs to keep it updated.
Attachment #8821084 - Flags: review?(bkelly) → review-
Comment on attachment 8821085 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

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

r=me with comments addressed

::: dom/fetch/FetchDriver.cpp
@@ +279,5 @@
>      // If request’s referrer policy is the empty string,
>      // then set request’s referrer policy to "no-referrer-when-downgrade".
>      if (mRequest->ReferrerPolicy_() == ReferrerPolicy::_empty) {
> +      // Bug 1304623 : default referrer policy could be controlled by user
> +      // pref network.http.referer.userControlPolicy

I don't think we need this comment here any more.  Its clear we are pulling the default referrer from somewhere else.

@@ +281,5 @@
>      if (mRequest->ReferrerPolicy_() == ReferrerPolicy::_empty) {
> +      // Bug 1304623 : default referrer policy could be controlled by user
> +      // pref network.http.referer.userControlPolicy
> +      uint32_t referrerPolicy;
> +      Unused << NS_GetDefaultReferrerPolicy(referrerPolicy);

It would be nice if this method just returned the referrerPolicy.  Return our old default if it would have failed.
Attachment #8821085 - Flags: review?(bkelly) → review+
Comment on attachment 8821084 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

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

::: netwerk/base/nsNetUtil.cpp
@@ +2378,5 @@
> +NS_GetDefaultReferrerPolicy(uint32_t& aDefaultPolicy)
> +{
> +  aDefaultPolicy = RP_No_Referrer_When_Downgrade;
> +
> +  if (!gHttpHandler) {

+1 to bkelly.. you can use that same logic in parent and child
Attachment #8821084 - Flags: review?(mcmanus)
(Assignee)

Comment 22

7 months ago
I took a look at this a gain. If we have a httpchannel, I think HttpBaseChannel keeps reference of nsHttpHandler in both parent and child case
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#104
(and nsHttpConnection.cpp also do) 
Basically we will not care about the referrer/referrer policy with out having a http channel/connection, so probably, may I just assert sth like?
    MOZ_ASSERT(gHttpHandler);
    if (!gHttpHandler) return default;

Thanks Patrick, Ben for your help. Happy holidays to you.
Flags: needinfo?(bkelly)
(Assignee)

Comment 23

7 months ago
Hmm, likely I am missing something. I still do not cover all the cases where content process calls xpcom registration, nsHttpHandler.Init
(Assignee)

Comment 24

7 months ago
Created attachment 8822372 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

MozReview-Commit-ID: I3ZYh9rrvGx
(Assignee)

Updated

7 months ago
Attachment #8821084 - Attachment is obsolete: true
(Assignee)

Comment 25

7 months ago
Created attachment 8822373 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: GZcwncNCKgl
(Assignee)

Updated

7 months ago
Attachment #8821085 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8822373 - Flags: review+
(Assignee)

Updated

7 months ago
Flags: needinfo?(bkelly)
(Assignee)

Comment 26

7 months ago
Updated patch based on comment 20
(Assignee)

Updated

7 months ago
Attachment #8822372 - Flags: review?(mcmanus)
Attachment #8822372 - Flags: review?(bkelly)
Comment on attachment 8822372 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

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

lgtm - thanks
Attachment #8822372 - Flags: review?(mcmanus) → review+
Attachment #8822372 - Flags: review?(bkelly) → review+
(Assignee)

Comment 28

7 months ago
Thanks you all for your review.
(Assignee)

Comment 29

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11fb2105bf03
(Assignee)

Comment 30

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08445c744e75
(Assignee)

Comment 31

7 months ago
Created attachment 8823926 [details] [diff] [review]
Create a pref to control the default referrer policy - part 1

MozReview-Commit-ID: 6R7kLB6jvhP
(Assignee)

Updated

7 months ago
Attachment #8822373 - Attachment is obsolete: true
(Assignee)

Comment 32

7 months ago
Created attachment 8823928 [details] [diff] [review]
Create a pref to control the default referrer policy- part 2

MozReview-Commit-ID: GEUDZ8UZAT5
(Assignee)

Updated

7 months ago
Attachment #8822372 - Attachment is obsolete: true
(Assignee)

Comment 33

7 months ago
Created attachment 8823929 [details] [diff] [review]
Create a pref to control the default referrer policy - part 3

MozReview-Commit-ID: 1A6IHPeNYBQ
(Assignee)

Updated

7 months ago
Attachment #8821083 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8823926 - Flags: review+
(Assignee)

Updated

7 months ago
Attachment #8823928 - Flags: review+
(Assignee)

Comment 34

7 months ago
Comment on attachment 8823929 [details] [diff] [review]
Create a pref to control the default referrer policy - part 3

Rebased carry + and running try
Attachment #8823929 - Flags: review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 35

7 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a042126872f4
Create a pref to control the default referrer policy - part 1. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d0bfecc06b
Create a pref to control the default referrer policy- part 2. r=mcmanus, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f60ec0d460a
Create a pref to control the default referrer policy - part 3. r=bkelly
Keywords: checkin-needed
(Assignee)

Comment 36

7 months ago
This seems have to be backed out. I ve just found that it may leak sensitive data in the case unset

Comment 37

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a042126872f4
https://hg.mozilla.org/mozilla-central/rev/73d0bfecc06b
https://hg.mozilla.org/mozilla-central/rev/6f60ec0d460a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 38

7 months ago
Leave open until bug 1329065 is fixed
Status: RESOLVED → REOPENED
Depends on: 1329065
Resolution: FIXED → ---
(Assignee)

Updated

6 months ago
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago6 months ago
Resolution: --- → FIXED

Comment 39

6 months ago
Thomas, thanks for getting this done!  Good job!
Thanks to all the reviewers as well. :)
Keywords: dev-doc-needed
(Reporter)

Updated

5 months ago
Depends on: 1340827
I've added a note to the Referrer-Policy page about this:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy#Browser_compatibility

And a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#HTTP

Let me know if there are any more changes you think I should make. cheers!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.