Jonathan Kingston [:jkt] he/him Assignee
	Description • 7 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 2 • 7 years ago

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?

	:Gijs (he/him)
	Comment 5 • 7 years ago

(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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 6 • 7 years ago

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?

	:Gijs (he/him)
	Comment 7 • 7 years ago
mozreview-review

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...

	:Gijs (he/him)
	Comment 8 • 7 years ago

(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! :-)

	:Gijs (he/him)
	Comment 10 • 7 years ago
mozreview-review

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 11 • 7 years ago

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?

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 12 • 7 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 7 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 7 years ago
mozreview-review

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 16 • 7 years ago

> 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?

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 6 years ago

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

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 18 • 6 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 6 years ago

Sounds good.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 20 • 6 years ago
mozreview-review-reply

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.

	Mozilla Autoland
	Comment 21 • 6 years ago

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)

	Pulsebot
	Comment 23 • 6 years ago

Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf7974086669
Remove feed and pcast protocols. r=Gijs

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 24 • 6 years ago
bugherder

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

	Kohei Yoshino [:kohei]
	Comment 25 • 6 years ago

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

	Chris Mills [:cmills]
	Comment 26 • 6 years ago

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.

	Jonathan Kingston [:jkt] he/him Assignee
	Comment 27 • 6 years ago

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