1420622 - Remove Feed and Pcast related protocols

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, enhancement, P1)

Description

[Jonathan Kingston [:jkt] he/him]

pcast:// and feed:// protocols were never standardised and are very rarely used so won't impact a large number of users however have been a target in the past for security issues.

Bug 1345546 was to add telemetry for feed protocol usage which is likely only as high as it is due to the use of "feed:" prefixing from a few menu items in Firefox.

This work requires the testing of feed related "subscribe to this page" features to ensure that removing the protocol won't break those.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1420622 - Remove feed and pcast protocols.

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

Comment 2

[Jonathan Kingston [:jkt] he/him]

As you mentioned in Bug 1345546 the "subscribe to this page" feature prefixes urls with "feed:" this seems not to cause any issues but also I think it gives us reliable telemetry on the usage of this specific button:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-10-23&keys=__none__!__none__!__none__&max_channel_version=release%252F56&measure=FEED_PROTOCOL_USAGE&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-09-25&table=0&trim=1&use_submission_date=0

Given that the sum total of feed:// feed:http:// and feed:https:// is included in this count does it make sense to also remove the subscribe button?

Comment 3

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202980/diff/1-2/

Comment 4

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202980/diff/2-3/

Comment 5

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #2)
> As you mentioned in Bug 1345546 the "subscribe to this page" feature
> prefixes urls with "feed:" this seems not to cause any issues but also I
> think it gives us reliable telemetry on the usage of this specific button:
> 
> https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2017-10-23&keys=__none__!__none__!
> __none__&max_channel_version=release%252F56&measure=FEED_PROTOCOL_USAGE&min_c
> hannel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submis
> sions&start_date=2017-09-25&table=0&trim=1&use_submission_date=0
> 
> Given that the sum total of feed:// feed:http:// and feed:https:// is
> included in this count does it make sense to also remove the subscribe
> button?

I agree the usage is low enough that that would normally be appropriate.

However, I also agree with this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1350349#c3

that we don't have enough information about *use* of existing live bookmarks to justify removing the functionality right now, because once you're subscribed you wouldn't need to use the subscribe functionality again, but could still be relying on live bookmarks. Equally, it wouldn't make much sense to remove the subscribe functionality (crippling live bookmarks) without some replacement, and leaving in the actual live bookmarks. So I think we should check if people are still relying on live bookmarks, and if not we can rip the entire thing out. If people do still use their existing live bookmarks in substantial numbers, we'll probably need some kind of migration.

Comment 6

[Jonathan Kingston [:jkt] he/him]

Ok, cool that makes sense, the code for this removal is fine though right?

I couldn't seem to trigger a subscribe case that would need the use of those headers that allow the browser to route to the feed reader page for non readable content.

Do we even know what the reader does when it's not readable?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

https://reviewboard.mozilla.org/r/202980/#review208466

::: browser/base/content/browser-feeds.js
(Diff revision 3)
>      // preview UI
>      if (!href)
>        href = event.target.getAttribute("feed");
>      urlSecurityCheck(href, gBrowser.contentPrincipal,
>                       Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> -    let feedURI = makeURI(href, document.characterSet);

This used to cause an implicit security check, because the 'feed:' URI only allowed http/https, and would have thrown an exception otherwise.

Can we make that restriction more explicit here, and perhaps (if it's not there already) also add the urlSecurityCheck + the protocol restrictions in the places where we accumulate a list of 1 or more feeds the user can subscribe to (so we don't show the subscribe... items if the only feed(s) available are actually not subscribable due to these security restrictions)?

::: dom/bindings/test/test_exceptionSanitization.html:28
(Diff revision 3)
>      // NOTE: The idea here is to call something that will end up throwing an
>      // exception in a JS component and then propagate back to C++ code before
> -    // returning to us.  If opening a feed: URI stops doing that, we will need a
> +    // returning to us.  If opening a view-source: URI stops doing that, we will need a
>      // new guinea pig here.
> -    open('feed://java:script:codeshouldgohere','eWin');
> +    open('view-source://java:script:codeshouldgohere','eWin');

View-source isn't implemented in JS, so I don't believe this change is correct. You'll need another JS-implemented protocol handler...

Comment 8

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #6)
> Ok, cool that makes sense, the code for this removal is fine though right?

Yes, except for review comments. :-)

> I couldn't seem to trigger a subscribe case that would need the use of those
> headers that allow the browser to route to the feed reader page for non
> readable content.

I think it's for cases where the mime type is wrong. I don't know if in 2017 there are still a significant enough number of those cases around. Might help to dig through the (CVS-inclusive) archaeology to see when/how that stuff was added.

> Do we even know what the reader does when it's not readable?

I don't off-hand, but it should be easy to test. Would be a good idea to check! :-)

Comment 9

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202980/diff/3-4/

Comment 10

[:Gijs (he/him)]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

https://reviewboard.mozilla.org/r/202980/#review210530

Patch looks OK, but note the change to the dom bindings test is still not right. We need a JS-implemented handler, and "http" is definitely C++-implemented... :-)

::: browser/base/content/browser-feeds.js:277
(Diff revision 4)
> +    urlSecurityCheck(link.href, gBrowser.contentPrincipal,
> +                     Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);

This won't be right for frames. Can you file a followup for that? Though to be fair, I can't think of a situation where that would be a serious problem if we're limiting to http/https anyway.

::: dom/bindings/test/test_exceptionSanitization.html:32
(Diff revision 4)
>    try{
>      // NOTE: The idea here is to call something that will end up throwing an
>      // exception in a JS component and then propagate back to C++ code before
> -    // returning to us.  If opening a feed: URI stops doing that, we will need a
> +    // returning to us.  If opening a http: URI stops doing that, we will need a
>      // new guinea pig here.
> -    open('feed://java:script:codeshouldgohere','eWin');
> +    open('http://java:script:codeshouldgohere','eWin');

I'm really confused - the http protocol handler is definitely not implemented in JS. :-)

If we don't have any remaining JS-implemented protocol handlers here, we could either add one specific to this test, or we could remove the test. Perhaps worth checking with bz, who I think added this test.

Comment 11

[Jonathan Kingston [:jkt] he/him]

Ah now I understand your JS protocol comment. Yes I think this is the last JS implemented protocol.

I'll fix the security check when I address the issue you mentioned about the unreadable feed. I think it would be good to fix the mime type issue you mentioned (which currently shows XML preview).

:bz if we are to remove these protocols is dom/bindings/test/test_exceptionSanitization.html relevant any more as we won't have any more JS protocols?

Comment 12

[Jonathan Kingston [:jkt] he/him]

> I don't off-hand, but it should be easy to test. Would be a good idea to check! :-)

I'm no longer concerned by this as feeds with the wrong content type just won't short circuit like they used to, they however so long as they the correct root level tag work fine. A document would have to have broken top level tags also to fall through to the normal content type viewing; this feels like a reasonable level of breakage and respectable fallback.

All test cases I tried locally break/work in both current and new code. For example sending the wrong content-type for RSS and changing the tag to be <riss> instead of <rss> causes an XML warning in both cases.

If we get reports of high breakage we could look into putting in a new loading flag to behave the same to mitigate the need to sniff the content.

> This won't be right for frames. Can you file a followup for that? Though to be fair, I can't think of a situation where that would be a serious problem if we're limiting to http/https anyway.

I couldn't even get iframe feeds to show up in this subscribe button anyway so maybe we ignore this comment.

Comment 13

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202980/diff/4-5/

Comment 14

[Boris Zbarsky [:bzbarsky]]

> is dom/bindings/test/test_exceptionSanitization.html relevant any more as we won't have any more JS protocols

It's relevant to the extent that we have any JS components reachable via a DOM API that can throw exceptions that then propagate up to that API.

The right fix is to probably just write an explicit test for this using some C++ that talks to dom/bindings/test/TestInterfaceJS.js instead of relying on finding an otherwise-web-exposed JS component.  I can do that if you'd like.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

https://reviewboard.mozilla.org/r/202980/#review210680

Just deleting the test is not the right thing here, imo.

Comment 16

[Jonathan Kingston [:jkt] he/him]

> The right fix is to probably just write an explicit test for this using some C++ that talks to dom/bindings/test/TestInterfaceJS.js instead of relying on finding an otherwise-web-exposed JS component.  I can do that if you'd like.

I would appreciate that if you have time, it's not an area of the code I really understand at all.

Would a follow up bug make sense for this or just a second patch?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here.  I put up a patch to fix the test in bug 1424362.

Comment 18

[Jonathan Kingston [:jkt] he/him]

Thank you :bz! I'm going to discard your r- as the feedback was what you fixed.
Will check it in today after checking that I met all Gijs's comments.

Thanks again!

Comment 19

[Boris Zbarsky [:bzbarsky]]

Sounds good.

Comment 20

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

https://reviewboard.mozilla.org/r/202980/#review210530

> This won't be right for frames. Can you file a followup for that? Though to be fair, I can't think of a situation where that would be a serious problem if we're limiting to http/https anyway.

Confirm this isn't for frames as we don't support showing feeds for iframes etc.

Comment 21

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b59fe5c046ab -d a5d1cd2a6eee: rebasing 439578:b59fe5c046ab "Bug 1420622 - Remove feed and pcast protocols. r=Gijs" (tip)
local [dest] changed dom/bindings/test/test_exceptionSanitization.html which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging browser/modules/test/unit/test_E10SUtils_nested_URIs.js
merging caps/tests/mochitest/browser_checkloaduri.js
merging dom/bindings/test/mochitest.ini
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging caps/tests/mochitest/browser_checkloaduri.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/bindings/test/mochitest.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 22

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8931881 [details]
Bug 1420622 - Remove feed and pcast protocols.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202980/diff/5-6/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf7974086669
Remove feed and pcast protocols. r=Gijs

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/cf7974086669

Comment 25

[Kohei Yoshino]

Have posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/feed-and-pcast-protocols-are-no-longer-supported/

Comment 26

[Chris Mills [:cmills]]

We never documented these protocols anywhere on MDN, so I've just added a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/59#Other_2

Hope that's OK.

Comment 27

[Jonathan Kingston [:jkt] he/him]

Haha course we didn't. That is great thank you.