enable per-site referrer-sending policies

NEW
Unassigned

Status

()

P5
enhancement
5 years ago
a year ago

People

(Reporter: geekboy, Unassigned)

Tracking

(Depends on: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
bug 822869 created the plumbing and prefs for global referrer trimming, spoofing and blocking policies.  We should make it so it can be specified per site (not just globally) so users/addons have more granular control but then we can use the same per-site logic when we implement the CSP referrer directive (bug 965727).

This bug is two parts: the permission manager storage of per-site settings and the code to hook the stored policies into the HTTPBaseChannel.
(Reporter)

Comment 1

5 years ago
ehsan: do you think nsIPermissionManager is the right place to store the site-specific settings?  I imagine there won't be very many of them, but this is more of a setting than a permission...
Flags: needinfo?(ehsan)
(Reporter)

Comment 2

5 years ago
to be explicit, there are four optional settings per site (same as global settings):
* referrer/level (send on user action like click only, or always)
* referrer/spoof (send real referrer or same-URI as requested content)
* referrer/trim (how much of the URI to send)
* referrer/xorigin (send always, send on etld+1 match or send on whole host match only)
(Reporter)

Comment 3

5 years ago
Created attachment 8368970 [details] [diff] [review]
storing per-site prefs in permission manager

This reads permissions out of nsIPermissionManager and if they exist for the site, applies them instead of the global settings.

There's no tests or good way to *set* these values for each site yet, but that'll be the next patch.

Jason: can you see if this looks like reasonable places to hit the permission manager and such?  I'm slightly nervous about perf, but four queries per channel shouldn't be super bad.
Attachment #8368970 - Flags: feedback?(jduell.mcbugs)

Comment 4

5 years ago
> four queries per channel shouldn't be super bad.

If that's an issue, you can always let the setter write a "hasRefererPrefs" = true when any of them are changed. If the read code finds this to be false, it doesn't need to check the others.
Just an idea...

Comment 5

5 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #1)
> ehsan: do you think nsIPermissionManager is the right place to store the
> site-specific settings?  I imagine there won't be very many of them, but
> this is more of a setting than a permission...

No, those are not permissions.  Please use the content prefs service instead.
Flags: needinfo?(ehsan)
(Reporter)

Comment 6

5 years ago
Comment on attachment 8368970 [details] [diff] [review]
storing per-site prefs in permission manager

Thanks Ehsan, will use content prefs instead.
Attachment #8368970 - Attachment description: bug966505-permmgr-storage → storing per-site prefs in permission manager
Attachment #8368970 - Attachment is obsolete: true
Attachment #8368970 - Flags: feedback?(jduell.mcbugs)
(Reporter)

Comment 7

5 years ago
Created attachment 8369574 [details] [diff] [review]
bug966505-contentpref-retrieval

Ok, this patch attempts to retrieve the referrer policy from nsIContentPrefService2, though seems to break the referrer tests due to wrong-threadyness.

Jason: Is this an appropriate place for the pref fetching (vs. nsHttpHandler), and if so, what is necessary to proxy this over to the main thread since nsIContentPrefService2 doesn't appear to be threadsafe?
Attachment #8369574 - Flags: feedback?(jduell.mcbugs)
(Reporter)

Comment 8

5 years ago
Comment on attachment 8369574 [details] [diff] [review]
bug966505-contentpref-retrieval

After a quick chat with mcmanus on IRC, this is the wrong place.  Gonna move it over to nsHttpChannel and try to do the content pref lookup in parallel with proxy lookups.
Attachment #8369574 - Flags: feedback?(jduell.mcbugs)
(Reporter)

Comment 9

5 years ago
Created attachment 8372574 [details] [diff] [review]
bug966505-contentpref-retrieval

mcmanus: I think this is more or less what we discussed, but I'm not sure I like it.  Basically, it creates a referrer data structure to store any site-specific (and eventually channel-specific) policy in HttpBaseChannel.  This structure is considered when SetReferrer() is called.  In nsHttpChannel, the setting fetcher is defined and it is launched from AsyncOpen().  When BeginConnect() is called, anything retrieved by the fetcher is copied into the HttpBaseChannel::mSiteReferrerSettings data structure and SetReferrer() is called to update the policy.

Reasons I'm unsure:

1. To test the per-site referrer policy I'll have to not only create a channel, but also open it to apply the referrer policy.  The previous referrer-policy test doesn't need to open the channel.  I am holding off writing a test until we decide where to put all the logic so that I can do an xpcshell test and not mochitest if possible. 

2.  SetReferrer() has to be called from inside BeginConnect() which feels a little shaky.  Perhaps I could break out the "strip referrer" logic into another routine called from BeginConnect() regardless of when SetReferrer() had been called in the past.

3.  The referrer-oriented logic is now spread across two classes (HttpBaseChannel and nsHttpChannel) instead of being gathered in one.

4. I think the pref fetcher is losing the race and not finished when BeginConnect() gets 
called:

Assertion failure: mRefPolicyFetcher->mCheckedPrefs (Should have checked referrer content prefs before BeginConnect()), at /home/sstamm/src/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4521

Anyway, mcmanus, what do you think about this general approach?
Attachment #8369574 - Attachment is obsolete: true
Attachment #8372574 - Flags: feedback?(mcmanus)
I don't mean to be negative here, but is this really worth the overhead it adds? I think we should try to implement <a referer>, CSP referrer, and change the default referrer policy and see if we can get away without doing this work.

Especially, is nsIContentPrefService2 going to be doing database queries to get this information? It seems like we should avoid doing that as much as possible.

Also, addons are able to implement things like this already, right? The UX team's 80-20-1 rule seems to say that we should leave this to addons.
(Reporter)

Comment 11

5 years ago
Brian: we're going to need much of this plumbing no matter which feature in the list is setting the referrer policy.  I was hoping this would be simpler than it has turned out to be, but alas.  

If I recall correctly, this per-site policy thing was on the list of things we decided to implement mainly because if we change the default global referrer policy, we'll want a way to turn referrers back on for individual sites in case of breakage.  So regardless of the other things, this will need to be done before we make the default referrer policy less permissive.
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #11)
> Brian: we're going to need much of this plumbing no matter which feature in
> the list is setting the referrer policy.  I was hoping this would be simpler
> than it has turned out to be, but alas.

I think we won't because, without this site-specific-preference stuff, it will be basically same-origin checks plus one flag per channel indicating how much of the referrer to send.

> If I recall correctly, this per-site policy thing was on the list of things
> we decided to implement mainly because if we change the default global
> referrer policy, we'll want a way to turn referrers back on for individual
> sites in case of breakage.  So regardless of the other things, this will
> need to be done before we make the default referrer policy less permissive.

I understand now. I don't think we'll need to implement something like that. This is a different level of risk than key pinning. If we break too many sites then we'll have to back off and reconsider what to do. But, for now I think YAGNI applies, especially since there is good reason to suspect we can shorten referrers without breaking compatibility in a significant way.

Comment 13

5 years ago
> is this really worth [it]

Yes. Referers are a privacy problem explicitly stated in the HTTP spec RFC 2616. I can disable them, and most of the web works just fine, with very few exceptions. Those are launchpad and MDC (since recently). I need to be able to keep referer off for the whole web, and enable it on these particular sites that break without it.
(Reporter)

Updated

5 years ago
No longer blocks: 965727

Updated

5 years ago
Depends on: 970092
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #9)
> Created attachment 8372574 [details] [diff] [review]
> bug966505-contentpref-retrieval
> 
> mcmanus: I think this is more or less what we discussed, but I'm not sure I
> like it.  Basically, it creates a referrer data structure to store any
> site-specific (and eventually channel-specific) policy in HttpBaseChannel. 
> This structure is considered when SetReferrer() is called.  In
> nsHttpChannel, the setting fetcher is defined and it is launched from
> AsyncOpen().  When BeginConnect() is called, anything retrieved by the
> fetcher is copied into the HttpBaseChannel::mSiteReferrerSettings data
> structure and SetReferrer() is called to update the policy.
> 
> Reasons I'm unsure:
> 
> 1. To test the per-site referrer policy I'll have to not only create a
> channel, but also open it to apply the referrer policy. 

fundamentally that's because its async.. you're going to need to do something like that, it might as well be asyncopen/onstartrequest. The proxy tests all had to change is a similar way when we made that asynchronous. It can certainly be done with xpcshell (and to make matters easier it should be fine if onstoprequest is called with an error - you don't have to open a channel to a real endpoint)
test if possible. 

> 
> 2.  SetReferrer() has to be called from inside BeginConnect() which feels a
> little shaky.  Perhaps I could break out the "strip referrer" logic into
> another routine called from BeginConnect() regardless of when SetReferrer()
> had been called in the past.
> 

I think you could change the BaseChannel implementation of SetReferrer() to be just mReferrer = param and then have a InsertReferrerHeader() function in nsHttpChannel called from BeginConnect() that writes the real header according to policy. (i.e. most of the logic that lives in BaseChannel right now).

> 3.  The referrer-oriented logic is now spread across two classes
> (HttpBaseChannel and nsHttpChannel) instead of being gathered in one.

see #2

> 
> 4. I think the pref fetcher is losing the race and not finished when
> BeginConnect() gets 
> called:

you're potentially running the proxy lookup and the referrer lookup in parallel. so beginconnect will need to be modified to do
"if (referrerlookupNotDone || mProxyRequest) return NS_OK".. the later one to finish will proceed.


hth
Attachment #8372574 - Flags: feedback?(mcmanus)

Updated

5 years ago
Depends on: 970136
Whiteboard: [necko-would-take]
I'll assume that Sid is no longer working on this.
Assignee: mozbugs → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.