Closed
Bug 254231
Opened 20 years ago
Closed 10 years ago
Allow feeds from file://
Categories
(MailNews Core :: Feed Reader, enhancement)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1569797
People
(Reporter: asqueella, Unassigned)
References
Details
Attachments
(1 file, 4 obsolete files)
2.39 KB,
patch
|
alta88
:
review+
|
Details | Diff | Splinter Review |
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 ;-)
Reporter | ||
Comment 1•20 years ago
|
||
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 :-)
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•20 years ago
|
||
This seems to do it. My first patch... :)
Attachment #174873 -
Flags: review?(mscott)
Updated•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•19 years ago
|
||
Updated the patch. Seems to work fine for ftp URLs too, so I made them allowed also.
Attachment #174873 -
Attachment is obsolete: true
Attachment #190101 -
Flags: review?(mscott)
Updated•19 years ago
|
Attachment #174873 -
Flags: review?(mscott)
Comment 5•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #190101 -
Flags: review?(mscott)
Updated•18 years ago
|
OS: Windows XP → All
Updated•18 years ago
|
QA Contact: rss
Updated•16 years ago
|
Assignee: mscott → nobody
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•13 years ago
|
||
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 10•13 years ago
|
||
I would suggest asking for feedback from mozilla.dev.platform/mozilla.dev.security.
Comment 12•13 years ago
|
||
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•12 years ago
|
||
[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.
Attachment #190101 -
Attachment is obsolete: true
Attachment #752437 -
Flags: sec-approval?
Attachment #752437 -
Flags: review?(mkmelin+mozilla)
Comment 14•12 years ago
|
||
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
Attachment #752437 -
Flags: review?(mkmelin+mozilla) → review+
Comment 15•12 years ago
|
||
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.
Attachment #752437 -
Flags: sec-approval?
Comment 16•12 years ago
|
||
updated for comments, added bad parse log message.
Assignee: nobody → alta88
Attachment #752437 -
Attachment is obsolete: true
Attachment #752937 -
Flags: sec-approval?
Attachment #752937 -
Flags: review+
Comment 17•12 years ago
|
||
(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?
Flags: needinfo?(abillings)
Comment 18•12 years ago
|
||
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
Flags: needinfo?(abillings)
Updated•12 years ago
|
Attachment #752937 -
Flags: sec-approval?
Keywords: checkin-needed
Updated•11 years ago
|
Keywords: checkin-needed
Comment 19•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Attachment #752937 -
Attachment is obsolete: true
Attachment #754984 -
Flags: review+
Flags: needinfo?(curtisk)
(In reply to alta88 from comment #23) > Created attachment 754984 [details] [diff] [review] > updated What info exactly are you needing from me?
Flags: needinfo?(curtisk)
I'll sec-review? and bring it up in triage to see what the team thinks
Flags: needinfo?(curtisk) → sec-review?
![]() |
||
Updated•11 years ago
|
Flags: sec-review? → sec-review?(dveditz)
Updated•11 years ago
|
Flags: sec-review?(dveditz) → sec-review?
Updated•11 years ago
|
Flags: sec-review? → sec-review?(dveditz)
Updated•11 years ago
|
Flags: sec-review?(dveditz)
Flags: sec-review?
Flags: needinfo?(dveditz)
Updated•11 years ago
|
Flags: sec-review?(dveditz)
Flags: sec-review?
Flags: needinfo?(dveditz)
Comment 27•11 years ago
|
||
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•9 years ago
|
||
Since this was closed WONTFIX, cancelling sec-review
Flags: sec-review?(dveditz)
You need to log in
before you can comment on or make changes to this bug.
Description
•