445823 - Feed protocol is not registered on Windows

Status: NEW

(Thunderbird :: OS Integration, defect)

Description

[Robert Strong (they/them - no direct email)]

This bug affects 2.0 and above. Filing under installer since there is no OS Integration component for Thunderbird and this does affect the installer though it is actually an OS Integration bug.

The code is present to register the feed protocol but it is never registered. On Vista the IsDefaultClientVista check will always return true even though feed is not set up in Thunderbird to use IApplicationAssociationRegistration
http://mxr.mozilla.org/mozilla/source/mail/components/shell/nsMailWinIntegration.cpp#653

I also checked on dveditz's XP system and it isn't being registered most likely due to the following call to OpenUserKeyForReading failing and not setting  isDefault to PR_FALSE. This would have succeeded if the feed protocol was added by the installer as are the other entries.
http://mxr.mozilla.org/mozilla/source/mail/components/shell/nsMailWinIntegration.cpp#591

I am fixing the call to OpenUserKeyForReading to set isDefault to PR_FALSE in Bug 404609 along with numerous other fixes so these line numbers should change dramatically if / when that bug is fixed.

Comment 1

[Robert Strong (they/them - no direct email)]

Since it was not functioning I disabled the setting as the default feed protocol in the patch in Bug 404609 for the time being. If it is added back keep in mind that the command line validator will need to validate thunderbird.url.feed
http://mxr.mozilla.org/mozilla/source/mail/components/nsMailDefaultHandler.js#347

Comment 2

[Rain [:rain1] (inactive)]

Does this block Thunderbird 3? Sounds like a problem if one of our three "set as default" options doesn't work.

Comment 3

[Magnus Melin [:mkmelin]]

Sounds like we should fix this. (Bug 348450 gave us the ability to handle feed: urls in the first place, without extra arguments.)

Comment 4

[Mark Banner (:standard8)]

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Since this is not a regression, and 3.1 is mostly about making a softer landing pad for Tb2 users at major update time, I'm removing the blocking- from this bug.  It'd be great to get it fixed, however, so adding the (unversioned) wanted-thunderbird+ flag, however.

Comment 6

[alta88]

Attachment: feedsOnWin.patch

robert, if you could provide some feedback on this patch it would be immensely appreciated.

Comment 7

[alta88]

Comment on attachment 8795548 [details] [diff] [review]
feedsOnWin.patch


This patch exactly follows the code for adding news. Although feed: is less used than mimetype sniffing, registration on win should still happen to indicate Tb is a first class feed reader.  Otherwise the disabled integration option should be removed from the dialog on win.

To test, change some url link in DOM inspector to prefix with feed: and create/set pref network.protocol-handler.external.feed to true in Fx, it should either launch Tb or attempt to subscribe (to an instance started without -no-remote).

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 8795548 [details] [diff] [review]
feedsOnWin.patch

Assuming you've tested this. Looks fine and the only thing that jumps out at me is that you should use -osint as the other handlers do.

Comment 9

[Kent James (:rkent)]

Comment on attachment 8795548 [details] [diff] [review]
feedsOnWin.patch

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

I have not finished my testing of this, but I have a serious issue. I think this needs a little more testing, and some documentation in the bug here of how it was tested, as I suspect that it was not really fully tested.

The feed as stated does not work. I took an existing feed, http://www.seattletimes.com/education/feed/ and as suggested changed the protocol to feed:// using DOM Inspector. When I then tried to use that to subscribe to a feed, it does not work (I get a message in TB saying "not a valid feed"). But this feed opens just fine using thunderbird.exe feed://www.seattletimes.com/education/feed/

I could make it work if I manually removed the feed: prefix in the shell commands in the registry. So what I believe is happening is that the %1 in the shell commands is the full url, and by prefixing the feed: you are passing:

feed:feed://www.seattletimes.com/education/feed/

which is invalid.

Could you comment more on how this was tested? Unfortunately it is really hard to test, as the registry entries only seem to be written on the first install of the program. I suspect that might be what went wrong here.

Overall it is an impressive piece of work, so let's try to get this finished.

Comment 10

[alta88]

@rkent - you're right, sorry, I resuscitated some things I tried many years ago when I still used windows and had a build environment there, etc. and which worked in pieces.. 

I removed -osint from the shell command in order to bypass nsMailDefaultHandler.validate, which checks for flags and in this case there is no flag, just a url starting with feed://.  It might mean hacking the cmdLine.findFlag parser to put back something for validation, which seems rather unworth it and probably in toolkit/ anyway.

I think the only change needed is to remove |feed:| from the commandline strings. It's clearly getting to Tb if you got the 'not valid' message, ie just about working.

So 'impressive' leaves me looking at the floor. But it would be a very large effort to get a win build environment up for this one thing. So I'll swap your help here on win for some bug that's annoying you..

Comment 11

[Robert Strong (they/them - no direct email)]

I think an app should be able to have its own validator
https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#618

The validator along with the startup code mitigate some exploits which were fixed many years ago. I'll leave it at that.

Comment 12

[alta88]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #11)
> I think an app should be able to have its own validator
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserContentHandler.js#618
> 

Tb does, as you know. It validates -mail and -compose flag args, as does the firefox one for -url. Feeds aren't in this category of commandline arg.

> The validator along with the startup code mitigate some exploits which were
> fixed many years ago. I'll leave it at that.

The -osint is used only for flag argument validation.  Any other string/url (flagless) is processed in well worn territory in handle(), and feeds fall into this category.

Comment 13

[alta88]

Attachment: feedsOnWin.patch

Updated to register feed as using just the unprefixed, unflagged url argument passed in the commandline.

Comment 14

[Kent James (:rkent)]

What I am having a hard time with is understanding why the issues that are solved by -osint are not needed here. Alta88 you seem to believe you have an understanding of that in comment 12, could you try again to explain why it is not an issue? I'm not really sure what you mean by "aren't in this category" and "The -osint is used only for flag argument validation.  Any other string/url (flagless) is processed in well worn territory in handle(), and feeds fall into this category."

Comment 15

[alta88]

If you look here:
https://dxr.mozilla.org/comm-central/source/mail/components/nsMailDefaultHandler.js#427
you notice that -osint is only used to check -mail and -compose args; if those aren't present it returns. Neither of those are used for a feed uri arg, thus 'not in this category'.

Meaning, the supported syntax for feeds is
thunderbrid.exe feed://www.seattletimes.com/education/feed/ or
thunderbrid.exe feed:http://www.seattletimes.com/education/feed/
and not
thunderbrid.exe -mail feed://www.seattletimes.com/education/feed/

Now, whether such uris should be arguments to flags isn't the issue; feed: and mailto: and such protocol uris were implemented long ago without flags.

Comment 16

[Kent James (:rkent)]

Hi Robert,

You were involved in the original Bug 384384 plus you raised the issue here. I can't seem to get the test cases in that bug to work (like I can't get -chrome to load javascript even from the command line), so I can't convince myself that I understand the issues well enough to validate alta88's claim that -osint validation is not needed. Are you able to look at this and agree that it is not needed?

Comment 17

[Robert Strong (they/them - no direct email)]

I really can't. The mitigation that bug 384384 provides handles several unusual cases and there is no way I can tell if a similar mitigation might be needed at some point for this.

Comment 18

[alta88]

Attachment: feedsOnWin.patch

Ok, this version uses -osint and invokes -mail with the url. rkent, could you please push a win try to produce a .exe for testing.

Comment 19

[Kent James (:rkent)]

(In reply to alta88 from comment #18)
> Created attachment 8805782 [details] [diff] [review]
> feedsOnWin.patch
> 
> 
> Ok, this version uses -osint and invokes -mail with the url. rkent, could
> you please push a win try to produce a .exe for testing.

I've spent quite a bit of time trying to understand the issues here, but enough to convince me that the original approach (without -osint -mail) was probably a bad idea, at least if we are talking IE6. Yet I am not sure how much the issues of bug 384384 are relevant today, I had to do an install of XP and IE6 to reproduce, and IE6 is really unusable. But creating a protocol with a simple call string of "%s" seems to me to make that old problem worse, as that gives you the ability IIUC to open thunderbird with virtually any command string given the IE6/7 bug. So I think it is safer to use the -osint and -mail in the newer patch.

Unfortunately try is down now, we'll have to wait until it gets back up to continue.

There is still a lot in the patch that I do not understand, and this is an area that could have security implications so we have to be careful. I already have way more time in this bug (like probably over 12 hours) than I can afford, and I need to turn to other pressing issues at the moment. I'm not ready to approve this, and it could be many weeks before I can get to it again. If someone else with more knowledge can take the time to understand and explain that, great, but otherwise it is going to be awhile before I can get back to this, even after try functionality returns.

Comment 20

[alta88]

It's difficult to understand[1] why you would attempt to test this on IE6, which was EOLed 2 1/2 years ago.  Are you claiming that Tb will support IE6 generally? XP and Vista support by Mozilla is ending with 52ESR, fyi.

In Bug 389257, you will notice the flag within an argument string execution vulnerability was not reproducible in IE7. Further, the worst of the -chrome problems, executing privileged javascript: commands, was long ago (pre hg 'epoch') impossible with that flag due to URI_INHERITS_SECURITY_CONTEXT testing. Even for IE6. See Bug 1175653 (which should likely be ported to Tb) for more.

But fine, given that I don't wish to waste *my* time either, the feed: protocol was included in the harmless even if no longer useful mitigations of -osint in the last patch, meaning it is treated *exactly* the same as mailto: and news: et al. by the command line validator.

Do you have anything more concrete than 'unknown unknowns' here?  Because blocking this exact same handling of feed: as mailto: et al. based on not even a theoretical reason, much less actual example, is completely unreasonable.

[1] I'm going to refrain from 'exquilla driving tb' for now.

Comment 21

[Kent James (:rkent)]

(In reply to alta88 from comment #20)
> It's difficult to understand[1] why you would attempt to test this on IE6,
> which was EOLed 2 1/2 years ago.  Are you claiming that Tb will support IE6
> generally? XP and Vista support by Mozilla is ending with 52ESR, fyi.
> 
> In Bug 389257, you will notice the flag within an argument string execution
> vulnerability was not reproducible in IE7. Further, the worst of the -chrome
> problems, executing privileged javascript: commands, was long ago (pre hg
> 'epoch') impossible with that flag due to URI_INHERITS_SECURITY_CONTEXT
> testing. Even for IE6. See Bug 1175653 (which should likely be ported to Tb)
> for more.
> 
> But fine, given that I don't wish to waste *my* time either, the feed:
> protocol was included in the harmless even if no longer useful mitigations
> of -osint in the last patch, meaning it is treated *exactly* the same as
> mailto: and news: et al. by the command line validator.
> 
> Do you have anything more concrete than 'unknown unknowns' here?  Because
> blocking this exact same handling of feed: as mailto: et al. based on not
> even a theoretical reason, much less actual example, is completely
> unreasonable.
> 
> [1] I'm going to refrain from 'exquilla driving tb' for now.

I attempted on IE6/FF2.0/XP because I wanted to understand the original bug. Robert Strong keeps making the point that we need to understand the issues. I don't understand 'exquilla driving tb' - did you mean IE6? There has been no mention of exquilla in this bug.

What I said was that your new approach seems to me to be less risk, which is good, and we could probably move forward on that basis. But I am under extreme pressure in other areas at the moment, so I warned you that might be slow to respond, and left open for others to complete reviews. But there is no reason we cannot get this into TB 52, there is plenty of time for that.