1277698 - Consider making feed: DANGEROUS_TO_LOAD

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1277697 +++

It's not clear to me that the web ever needs to link to feed:<anything>. We could just break it completely, and that would make sense and be safer. It would avoid having to fix/investigate bug 1277697.

(this can be opened up once bug 1277583 lands everywhere, I think.)

Comment 1

[Phil Ringnalda (:philor)]

Linking to it was the entire point of feed: - it was invented as a way to let a blog link to its feed and have that link wind up in the user's feed reader subscription UI whether or not the author could control the mimetype the server sent, and no matter what the user's browser did with random XML documents. Denying linking to it makes it utterly pointless (which would be good for us, getting to drop our whole troublesome and half-assed implementation, but would be Bad For Our Users).

Comment 2

[Daniel Veditz [:dveditz]]

Bad in theory? It's 2016, not 2006 -- does anyone still rely on feed: links? Is this something we could Google or ask Google folks about, or do we have to implement Telemetry for it? Or do we just break it in Beta and see if anyone screams?

Comment 3

[Phil Ringnalda (:philor)]

Actually, you could probably ask your local copy of Chrome what it does with feed: links, and call that good enough.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

I'd be happy turning this off in Beta and seeing if anyone notices. At least making it an about:config pref and pref'ing it off by default.

Comment 5

[:Gijs (he/him)]

(In reply to Phil Ringnalda (:philor) from comment #3)
> Actually, you could probably ask your local copy of Chrome what it does with
> feed: links, and call that good enough.

http://jsbin.com/kawidahabo/edit?html,output

All of these trigger a popup saying it wants to open the protocol with the default application as supplied by the OS (Thunderbird on this particular machine). It clearly doesn't know how to handle this itself.

Comment 6

[Jay Gilbert]

(In reply to Phil Ringnalda (:philor) from comment #3)
> Actually, you could probably ask your local copy of Chrome what it does with
> feed: links, and call that good enough.

I would doubt that since when one uses the feed: protocol firefox sends a special request header to the web server as follows:

X-Moz-Is-Feed: 1

So apparently Mozilla took the lead on this one and others followed(directing use of feed: to Mozilla apps) and everyone forgot about it.

Comment 7

[:Gijs (he/him)]

(In reply to Jay Gilbert from comment #6)
> (In reply to Phil Ringnalda (:philor) from comment #3)
> > Actually, you could probably ask your local copy of Chrome what it does with
> > feed: links, and call that good enough.
> 
> I would doubt that since when one uses the feed: protocol firefox sends a
> special request header to the web server as follows:
> 
> X-Moz-Is-Feed: 1
> 
> So apparently Mozilla took the lead on this one and others
> followed(directing use of feed: to Mozilla apps) and everyone forgot about
> it.

I don't think that's relevant to "what does the web do with these URIs" - the Chrome behaviour shows that it will just treat feed: URIs like any other unknown scheme. It doesn't send any headers at all.

The issue is that we use that header ourselves, it seems - internally it's used to force the contents being interpreted as a feed so we can show the feed subscriber UI. See https://dxr.mozilla.org/mozilla-central/search?q=x-moz-is-feed&=mozilla-central&redirect=true

If we're intentionally showing the subscriber UI we could just parametrize the about:feed URI the way we've done with about:reader, and make the page unlinkable from the web (already the case on 48+).

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> The issue is that we use that header ourselves, it seems - internally it's
> used to force the contents being interpreted as a feed so we can show the
> feed subscriber UI. See
> https://dxr.mozilla.org/mozilla-central/search?q=x-moz-is-feed&=mozilla-
> central&redirect=true
> 
> If we're intentionally showing the subscriber UI we could just parametrize
> the about:feed URI the way we've done with about:reader, and make the page
> unlinkable from the web (already the case on 48+).

Actually, that doesn't really make a difference here - we could still make the URI dangerous to load, which would have the same effect.

Comment 9

[Jay Gilbert]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > The issue is that we use that header ourselves, it seems - internally it's
> > used to force the contents being interpreted as a feed so we can show the
> > feed subscriber UI. See
> > https://dxr.mozilla.org/mozilla-central/search?q=x-moz-is-feed&=mozilla-
> > central&redirect=true
> > 
> > If we're intentionally showing the subscriber UI we could just parametrize
> > the about:feed URI the way we've done with about:reader, and make the page
> > unlinkable from the web (already the case on 48+).
> 
> Actually, that doesn't really make a difference here - we could still make
> the URI dangerous to load, which would have the same effect.

Thanks, that's the next thing I was going to look into so you just saved me some time.  I thought that may be the case.  My point originally was that a plan existed at some time and none of us actually know what it was entirely.

Replying to you quoting your own reply gets me double XP, I'm leveling up fast ;-)

Comment 10

[Justin Dolske [:Dolske]]

(In reply to Al Billings [:abillings] from comment #4)
> I'd be happy turning this off in Beta and seeing if anyone notices. At least
> making it an about:config pref and pref'ing it off by default.

We could just add a simple telemetry probe, to see how often it's actually used in the wild. Although interpreting the data is a bit tricky in cases like this... I suspect it's not something that's going to be used very frequently, even for "active" RSS users. At least, my impression -- as someone who still uses a feed reader daily -- is that adding new feeds is something that only happens on timescales of weeks or months. But that infrequency is itself an argument for removing it. :)

So I think I'd be comfortable just removing it by fiat, even though RSS is a touchy enough feature that having some supporting data would be nice.

[And on that point, the bug removing it should clearly state that this isn't removing support for RSS / feeds in general -- just the special handling Firefox does for this protocol. We don't need a misinformed mob screaming at us...]

Comment 11

[Daniel Veditz [:dveditz]]

FWIW in Safari feed: links trigger an "Add '[XXX]' to Shared Links?" confirmation dialog. For the feed://domain/path form [XXX] is the domain, and for the http/https versions you get "Add '(null)' to Shared Links?". A feed MIME type also triggers the same prompt, again with just the domain showing, no URL or any metadata taken from the feed itself.

It's not a very usable feature. Makes me think it's some ancient left-over they just haven't killed yet.

Making the protocol DANGEROUS_TO_LOAD will be different than the Chrome behavior: Chrome doesn't handle feed: but does give the user an avenue to use an external protocol handler. DANGEROUS_TO_LOAD links will be simply broken in Firefox. We may want to make this a pref so that by default they are DANGEROUS, but broken users can fiddle with about:config (or get an add-on) to restore the functionality. This would also give us a recovery path via hotfix add-on should it turn out to break more than we expect when it hits release.

A slower approach--much slower--would be to add Telemetry and wait a release or two to make sure there aren't many of these hit in the wild. Personally I'd rather guess there aren't (no support in the most popular browser) and go with the faster "pref it off" approach.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

I think we should just remove the protocol handling. We've removed others over the last few years. 

(I use a feed reader every day too but I just cut and paste URLs of feeds into it when I add new ones.)

Comment 13

[Jay Gilbert]

(In reply to Al Billings [:abillings] from comment #12)
> I think we should just remove the protocol handling. We've removed others
> over the last few years. 
> 
> (I use a feed reader every day too but I just cut and paste URLs of feeds
> into it when I add new ones.)

This is probably the best idea with having all things relating to feeds and about:feeds only accessible by chrome.

Comment 14

[:Gijs (he/him)]

(In reply to Al Billings [:abillings] from comment #12)
> I think we should just remove the protocol handling. We've removed others
> over the last few years. 

The problem is that we rely on the protocol handling and the hacks involved in it internally. If the site defines RSS feeds as links, we show them in the 'subscribe to' list, and if you click an item in there, we'll open feed:<url> (if the URL itself starts with http: or https: instead of feed: - http://searchfox.org/mozilla-central/source/browser/base/content/browser-feeds.js#89-90 ) and that forces the content to be rendered as a feed.

We can rework all of that but it's a lot more effort and higher risk, as a first step, than making the protocol unlinkable. 

Additionally, if we remove the protocol handling and rework the <link> handling to e.g. use about:feeds?feed=<encodedURL>, we'd still break web content using "feed:" links in <link> themselves, which I don't *think* we'll break by making the protocol dangerous_to_load, because the web pages aren't the ones loading the URL - we (chrome privileged browser code) are. Should probably test that, and/or get telemetry on whether feed: actually gets used on the web before switching to about:feeds?feed=<encodedURL> over feed:url.

Comment 15

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Al Billings [:abillings] from comment #12)
> > I think we should just remove the protocol handling. We've removed others
> > over the last few years. 
> 
> The problem is that we rely on the protocol handling and the hacks involved
> in it internally. If the site defines RSS feeds as links, we show them in
> the 'subscribe to' list, and if you click an item in there, we'll open
> feed:<url> (if the URL itself starts with http: or https: instead of feed: -
> http://searchfox.org/mozilla-central/source/browser/base/content/browser-
> feeds.js#89-90 ) and that forces the content to be rendered as a feed.

In fact, thinking about this, I wonder what happens if a webpage gives a <link> to a feed that uses feed:chrome://whatever/ . Of course, then you'd still need to convince the user to click on 'subscribe to this feed'...

Comment 16

[Jay Gilbert]

Dan, Gijs thanks you've just given me an idea about something I absolutely have to test.

Dan, you're probably right and that's fine with me.  I have no idea how much of the web actually uses feed: but I may be underestimating it.  We can proceed as Dan suggests while working to lock everything down more with the most minor changes possible.

So after we see how much end users rely on feed: we should only then have our changes hit release if the usage of it is low enough.  This way we should be able to implement changes should we need to much faster once we have the information we need.

I'm still wondering what the overall plan originally was here.  It seems like a mess.

Comment 17

[:Gijs (he/him)]

Attachment: Make feed: unlinkable

I think the security checks that happen for feeds added as <link> elements and whether or not they show in the UI should be sufficient to avoid feed:chrome:// from being currently possible. I verified that this patch doesn't break our subscribing button from loading the thing pointed at by a <link rel="feed"> element. I added a pref so we can hotfix/late-minute-disable this if it actually breaks Real Websites somehow, but it seems unlikely.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 8760930 [details] [diff] [review]
Make feed: unlinkable

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

::: browser/app/profile/firefox.js
@@ +690,5 @@
>  
>  pref("browser.feeds.handler", "ask");
>  pref("browser.videoFeeds.handler", "ask");
>  pref("browser.audioFeeds.handler", "ask");
> +pref("browser.feeds.internal", true);

I don't think we should add the pref to firefox.js. In fact, I think the pref check should only land for Beta... If we ship successfully without having to hotfix it, then we shouldn't have a potentially dangerous footgun pref sitting around for the indefinite future.

::: browser/components/feeds/FeedConverter.js
@@ +493,5 @@
>    get protocolFlags() {
> +    if (Services.prefs.getBoolPref("browser.feeds.internal")) {
> +      let {URI_DANGEROUS_TO_LOAD, ALLOWS_PROXY_HTTP, ALLOWS_PROXY} =
> +        Ci.nsIProtocolHandler;
> +      return URI_DANGEROUS_TO_LOAD | ALLOWS_PROXY | ALLOWS_PROXY_HTTP;

Why not "return (this._http.flags | URI_DANGEROUS_TO_LOAD);"?

I don't really understand the impact of setting (or not) all these flags, so I'm not sure I'm the best reviewer for this.

Comment 19

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Al Billings [:abillings] from comment #12)
> > I think we should just remove the protocol handling. We've removed others
> > over the last few years. 
> 
> The problem is that we rely on the protocol handling and the hacks involved
> in it internally. If the site defines RSS feeds as links, we show them in
> the 'subscribe to' list, and if you click an item in there, we'll open
> feed:<url> (if the URL itself starts with http: or https: instead of feed: -
> http://searchfox.org/mozilla-central/source/browser/base/content/browser-
> feeds.js#89-90 ) and that forces the content to be rendered as a feed.

Is there really a lot of reworking to change it to just open the URL directly, instead of dealing with the "feed:" prefixing? EG, the relevant <link rel> on https://dolske.wordpress.com (great blog, everyone should read it) points to https://dolske.wordpress.com/feed/. I get the same behavior pasting that URL into the URL bar as opening it through the Bookmarks -> Subscribe to Page menu.

If it is, then I think the next step would be taking a hard think about if we want to retain any of the built-in support for previewing feeds. Great or Dead.

> We can rework all of that but it's a lot more effort and higher risk, as a
> first step, than making the protocol unlinkable.

(Agreed, we should probably move to a followup bug to talk about longer range feed reader mitigations.)

> Additionally, if we remove the protocol handling and rework the <link>
> handling to e.g. use about:feeds?feed=<encodedURL>, we'd still break web
> content using "feed:" links in <link> themselves

I think the point is it's not clear that we need to do anything special. Especially given that Chrome doesn't seem to (and for that matter, opening https://dolske.wordpress.com/feed/ in Chrome just shows me a dump of XML).

Comment 20

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #18)
> Comment on attachment 8760930 [details] [diff] [review]
> Make feed: unlinkable
> 
> Review of attachment 8760930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/profile/firefox.js
> @@ +690,5 @@
> >  
> >  pref("browser.feeds.handler", "ask");
> >  pref("browser.videoFeeds.handler", "ask");
> >  pref("browser.audioFeeds.handler", "ask");
> > +pref("browser.feeds.internal", true);
> 
> I don't think we should add the pref to firefox.js. In fact, I think the
> pref check should only land for Beta... If we ship successfully without
> having to hotfix it, then we shouldn't have a potentially dangerous footgun
> pref sitting around for the indefinite future.

OK.

> ::: browser/components/feeds/FeedConverter.js
> @@ +493,5 @@
> >    get protocolFlags() {
> > +    if (Services.prefs.getBoolPref("browser.feeds.internal")) {
> > +      let {URI_DANGEROUS_TO_LOAD, ALLOWS_PROXY_HTTP, ALLOWS_PROXY} =
> > +        Ci.nsIProtocolHandler;
> > +      return URI_DANGEROUS_TO_LOAD | ALLOWS_PROXY | ALLOWS_PROXY_HTTP;
> 
> Why not "return (this._http.flags | URI_DANGEROUS_TO_LOAD);"?

Because this._http.flags includes URI_LOADABLE_BY_ANYONE. Behaviour when both that and URI_DANGEROUS_TO_LOAD (both governing 'who can/should be loading this') are specified I think is undefined, though I believe CAPS currently does the right thing.

> I don't really understand the impact of setting (or not) all these flags, so
> I'm not sure I'm the best reviewer for this.

OK.

Comment 21

[:Gijs (he/him)]

Attachment: Patch without pref

Christoph, do you feel comfortable reviewing this? If not, can you recommend someone else? Thanks!

Comment 22

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #14)
> > (In reply to Al Billings [:abillings] from comment #12)
> > > I think we should just remove the protocol handling. We've removed others
> > > over the last few years. 
> > 
> > The problem is that we rely on the protocol handling and the hacks involved
> > in it internally. If the site defines RSS feeds as links, we show them in
> > the 'subscribe to' list, and if you click an item in there, we'll open
> > feed:<url> (if the URL itself starts with http: or https: instead of feed: -
> > http://searchfox.org/mozilla-central/source/browser/base/content/browser-
> > feeds.js#89-90 ) and that forces the content to be rendered as a feed.
> 
> Is there really a lot of reworking to change it to just open the URL
> directly, instead of dealing with the "feed:" prefixing? EG, the relevant
> <link rel> on https://dolske.wordpress.com (great blog, everyone should read
> it) points to https://dolske.wordpress.com/feed/. I get the same behavior
> pasting that URL into the URL bar as opening it through the Bookmarks ->
> Subscribe to Page menu.

From a quick look at devtools, your server is well-behaved and sends "application/rss+xml; charset=UTF-8" as the content type. comment #1 indicated the reason we have this whole malarkey is to deal with cases where the author does not control the content headers sent, but controls the content... whether that is at all worth bothering with now that geocities is dead and we live in 2016, I don't know. I also haven't done the archaeology to see if there were other reasons for this decision. Probably makes sense to take follow-up investigation to a separate bug.

Note that the comparison to Chrome is actually somewhat counter to the point of removing linkability here. If a website has:

<a href="feed:http://example.com/feed">Click to subscribe to my feed</a>

that currently works in both Chrome and Firefox. The patch will break it, on the hypothesis that nobody actually does that. Our mileage may vary as to whether that is true.

Comment 23

[:Gijs (he/him)]

Attachment: Patch with pref for beta

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #21)
> Created attachment 8761086 [details] [diff] [review]
> Patch without pref
> 
> Christoph, do you feel comfortable reviewing this? If not, can you recommend
> someone else? Thanks!

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8761086 [details] [diff] [review]
Patch without pref

(In reply to :Gijs Kruitbosch from comment #21)
> Created attachment 8761086 [details] [diff] [review]
> Patch without pref
> 
> Christoph, do you feel comfortable reviewing this? If not, can you recommend
> someone else? Thanks!

Even though I would suppose that's the right fix I would feel more comfortable if smaug can review that changeset.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8761099 [details] [diff] [review]
Patch with pref for beta

Shouldn't you compare the return value of getPrefType to something?

r=me, I guess, as long as our feed UI keeps working....

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8761086 [details] [diff] [review]
Patch without pref

r=me

Comment 28

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #26)
> Comment on attachment 8761099 [details] [diff] [review]
> Patch with pref for beta
> 
> Shouldn't you compare the return value of getPrefType to something?

It returns 0 for unknown prefs ( http://searchfox.org/mozilla-central/source/modules/libpref/nsIPrefBranch.idl#35 ) and so we can treat it as a boolean, and JS will do the rest. Admittedly not super-clear if you've not seen the pattern before (there are a number of other occurrences in the codebase). Considering we're basically never going to change the values of those constants, and any additional values will probably just end up using higher int values, I think this is OK.

Comment 29

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/f48fbe411650

Comment 30

[:Gijs (he/him)]

Comment on attachment 8761086 [details] [diff] [review]
Patch without pref

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: web can link to feed: URIs which it shouldn't be able to do.
[Describe test coverage new/current, TreeHerder]: limited test coverage, need to update tests from bug 1277583 for this change and then get sec-approval to land those.
[Risks and why]: could potentially break websites that depend on linking to this using normal links (rather than <link rel=""> in their <head> ). That's a web compat risk, but a very minor one considering feed support in other browsers.
[String/UUID change made/needed]: no.

Comment 31

[:Gijs (he/him)]

Comment on attachment 8761099 [details] [diff] [review]
Patch with pref for beta

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: web can link to feed: URIs which it shouldn't be able to do.
[Describe test coverage new/current, TreeHerder]: limited test coverage, need to update tests from bug 1277583 for this change and then get sec-approval to land those.
[Risks and why]: could potentially break websites that depend on linking to this using normal links (rather than <link rel=""> in their <head> ). That's a web compat risk, but a very minor one considering feed support in other browsers. This risk is further minimized by the fact that this patch adds support for a pref which we could use to restore the old behaviour either late-ish in beta or with a hotfix if need be.
[String/UUID change made/needed]: no.

Comment 32

[Carsten Book [:Tomcat]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1277698

Comment 33

[Carsten Book [:Tomcat]]

(In reply to :Gijs Kruitbosch from comment #29)
> remote:   https://hg.mozilla.org/integration/fx-team/rev/f48fbe411650

https://hg.mozilla.org/mozilla-central/rev/f48fbe411650 of course

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c7667ce0a8d9c37318c19cbfdfaa34db9d409cc

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da4a9eed33bd