254231 - Allow feeds from file://

Status: RESOLVED DUPLICATE of bug 1569797

(MailNews Core :: Feed Reader, enhancement)

Description

[Nickolay_Ponomarev]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040730 Firefox/0.9.1+ (best browser EVAR1!!)
Build Identifier: version 0.7+ (20040730)

Currently Thunderbird checks if the feed's URL is http:
>  if (!uri.schemeIs("http"))
>    return this.onParseError(this); // simulate an invalid feed error

It would be great if Thunderbird allowed me read feeds from my hard drive
without having to set up a local web server. Would be useful for testing TB's
feed parser as well as creating own feeds. Also this could be used to deliver
messages from local services to mail/rss client under Windows, which is
currently impossible (afaik).

If you think it's not needed, at least please isolate the check into a
replacable function, say Feed.checkURL(), so that those who need this may just
replace that function instead of changing TB code.

(The reason for checking for http in the URL is:
>This is just a sanity check so we don't try opening mailto urls, imap urls,
etc. that the user may have tried to subscribe to as an rss feed..)

Reproducible: Always
Steps to Reproduce:
1. Add http://www.squarefree.com/burningedge/index.rdf feed.
2. Delete the feed (del, OK, del from trash)
3. Add the feed - http://www.squarefree.com/burningedge/index.rdf - again.

Actual Results:  
The feed's items are no longer shown in the message view (at least most of them
- one item appeared sometimes).



Thanks in advance ;-)

Comment 1

[Nickolay_Ponomarev]

Damn, the original bug report is messed up because of Firefox bugs regarding
forms and Go back. Specifically it filled in blank textboxes with data from bug
254230.
Please ignore the text after "Reproducible: Always" (except for Thanks in
advance :-)

Comment 2

[Dan Cech]

This also prevents reading of private rss feeds over https, maybe the line
should read something like:

if (!uri.schemeIs("http") && !uri.schemeIs("https") && !uri.schemeIs("file"))
  return this.onParseError(this); // simulate an invalid feed error

Making the error a little more descriptive wouldn't hurt, but I don't know
enough about the code to suggest a mechanism for that.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: Allows file:// urls for feeds

This seems to do it. My first patch... :)

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: proposed fix. allows file and ftp feed urls

Updated the patch. Seems to work fine for ftp URLs too, so I made them allowed
also.

Comment 5

[Jyri Kytömäki]

So will the patch enable also support for HTTPS feeds?
Have somebody tested it?
I need HTTPS protocol and HTTP authentication support in order to reed our 
companys internal RSS feeds.

Comment 6

[Scott MacGregor]

Comment on attachment 190101 [details] [diff] [review]
proposed fix. allows file and ftp feed urls

I'm not sure I'm that keen about allowing ftp and file urls to run in this scenario. Maybe I'm just being paranoid from a security point of view and just need to think on this more.

Comment 7

[alta88]

can anyone think of a real reason this shouldn't be implemented?  (not sure how putting a file through a parser equals 'run'.. or how a remote file is in any way safer than a local file.)

i implemented this locally, trying .exe and .dll and other stuff and it just throws invalid feed like any other malformed xml.

if anything, it would make getting a testsuite infinitely easier, as mentioned above.

Comment 8

[Magnus Melin [:mkmelin]]

Same risks as with opening a web page locally. Scripts in the file can access other local files (at least below current path, can't recall what it's like nowadays) and send that data off to wherever it wants.

Comment 9

[Ludovic Hirlimann [:Usul]]

If we did this this would need to go thru a security review before landing

Comment 10

[Mark Banner (:standard8)]

I would suggest asking for feedback from mozilla.dev.platform/mozilla.dev.security.

Comment 12

[alta88]

it was always possible, but it's now simpler to allow feed file:// url handling.  the small function FileUtils.isValidScheme(aUri) can be overloaded to include "file".

note that due to bug 740865, an incorrect file will throw and require a restart.

Comment 13

[alta88]

Attachment: patch

[Security approval request comment]

Security approval requested to perform Thunderbird feed subscriptions from file:// urls.  This is an xhr get request on a file which should be a well formed feed (atom/rss syntax) xml file.  The resulting parsing of this file creates a feed message in the same way a remote http:// document is handled; the file would not be loaded in a docshell eg.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 752437 [details] [diff] [review]
patch

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

I'm fine with this

::: mailnews/extensions/newsblog/content/Feed.js
@@ +132,5 @@
> +      // Need to check for the file's existence explicitly due to Bug 740865.
> +      let file;
> +      try {
> +        file = Services.io.newURI(uri.spec, null, null).
> +                           QueryInterface(Ci.nsIFileURL).file;

in these kind of cases dots should go on the new line, and then align the dots

Comment 15

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

Comment on attachment 752437 [details] [diff] [review]
patch

Please don't set the sec-approval flag on patches that aren't security related. This isn't a security bug.

Comment 16

[alta88]

Attachment: updated

updated for comments, added bad parse log message.

Comment 17

[alta88]

(In reply to Al Billings [:abillings] from comment #15)
> Comment on attachment 752437 [details] [diff] [review]
> patch
> 
> Please don't set the sec-approval flag on patches that aren't security
> related. This isn't a security bug.

Then how is a security review initiated?  Or are you saying such a review is unnecessary here?

Comment 18

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

sec-approval isn't used for security review. Sec-approval is used when you want permission to check in a security bug. Please read the bug approval process and quit setting this flag on a non-security bug.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 752937 [details] [diff] [review]
updated

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

::: mailnews/extensions/newsblog/content/Feed.js
@@ +128,5 @@
>      }
>  
> +    if (uri.schemeIs("file"))
> +    {
> +      // Need to check for the file's existence explicitly due to Bug 740865.

Bug 740865 WFM so we can remove this.

Comment 20

[alta88]

Removing that file scheme check results in (win)

for file://foo:
Thu May 23 2013 12:15:21
Error: NS_ERROR_FAILURE: Failure
Source file: chrome://messenger-newsblog/content/Feed.js
Line: 177

for file:///foo:
Thu May 23 2013 12:16:59
Error: NS_ERROR_FILE_UNRECOGNIZED_PATH: File error: Unrecognized path
Source file: chrome://messenger-newsblog/content/Feed.js
Line: 177

Tb24.0a1 comm-central pulled 2 days ago and a mozilla-central pulled about a week ago.  As for Bug 740865, there was a discussion last fall about the Tb trains using gecko ESR, but it's not clear what the state is.

Comment 21

[Magnus Melin [:mkmelin]]

Maybe invalid path is the issue, like the message says. Tried file://C:/foobar ? (My succesful test was on linux.)

I think those discussions were related to a possible intermediate release, something like "17.1", but those plans are dropped AFAIK. Trunk need to follow mozilla-central, otherwise we'd break pretty quickly.

Comment 22

[alta88]

Right, and that does work, but the point of Bug 740865 is to not ever throw on send() regardless of what the url is.  In this case it's entered in a freeform text field..  Otherwise it has to be validated, like in the current patch.

I would have thought so, thus it seems Bug 740865 isn't really fixed, since file:/ or file:foo will throw on send() and crash the dialog.

Comment 23

[alta88]

Attachment: updated

Comment 24

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

(In reply to alta88 from comment #23)
> Created attachment 754984 [details] [diff] [review]
> updated

What info exactly are you needing from me?

Comment 25

[Ludovic Hirlimann [:Usul]]

Curtis does this need to go thru sec-review ?

Comment 26

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

I'll sec-review? and bring it up in triage to see what the team thinks

Comment 27

[alta88]

It would be extra extra great if we could finalize this in time for the Tb 24 major release, which branches on June 24.

Comment 29

[Wayne Mery (:wsmwk)]

Since this was closed WONTFIX, cancelling  sec-review

Comment 30

[Jorg K (CEST = GMT+2)]

Finally fixed in bug 1569797.