Closed Bug 1569797 Opened 6 months ago Closed 6 months ago

Allow to open RSS feed from local disk using file:// protocol handle

Categories

(MailNews Core :: Feed Reader, enhancement)

enhancement
Not set

Tracking

(thunderbird_esr6868+ fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird_esr68 68+ fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: sunrisechain, Assigned: alta88)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

While trying to create a reproducible scenario for another feature request, I wanted to do that using a rss.xml file on my local disc.
So I opened the subscribe dialog and entered the local path of the file as "file:///home/user/Downloads/rss.xml".

Actual results:

I get the error "The Feed URL is not a valid feed".

Expected results:

The feed should have been added and the items been shown as if I had entered a URL like "http://localhost/rss.xml"

Ben, you've been messing with feeds, do you know?

Flags: needinfo?(benc)

there isn't any reason (security or otherwise) to not allow file.

note that you can issue FeedUtils._validSchemes.push("file") in console to add it currently.

Assignee: nobody → alta88
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9081972 - Flags: review?(jorgk)
Comment on attachment 9081972 [details] [diff] [review]
add file to valid feed url schemes

Thanks, that was quick.
Attachment #9081972 - Flags: review?(jorgk)
Attachment #9081972 - Flags: review+
Attachment #9081972 - Flags: approval-comm-esr68?
Attachment #9081972 - Flags: approval-comm-beta?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/485dbfddfdfe
Allow opening RSS feed from file: URL. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

How did you create this patch, it didn't even have the file you're changing in it. I had to redo it.

Target Milestone: --- → Thunderbird 70.0

Thanks!

If all goes well, you'll get it next week in TB 69 beta 2 and the week after in TB 68.0 ESR. But sadly I don't control that alone.

Hmm, there was bug 254231 about this.
I'm not sure. When opening from file, that can refer to other files on your system, which is unwanted. But of course the feed would have to be on the file system to begin with.

Back in bug 254231 there was some hunk like this:

if (uri.schemeIs("file"))
{
  // Need to check for the file's existence explicitly due to Bug 875783.
  let file;
  try {
    file = Services.io.newURI(uri.spec, null, null).QueryInterface(Ci.nsIFileURL).file;
  }
  catch (ex) {}
  if (!file || !file.exists()) {
     // Simulate an invalid feed error.
    FeedUtils.log.info("Feed.download: file not found - " + uri.path);
    if (this.downloadCallback)
      this.downloadCallback.downloaded(this, FeedUtils.kNewsBlogRequestFailure);
    return;
  }
}

There referenced bug 875783 still exists. So is this still relevant?

Flags: needinfo?(alta88)

There referenced bug 875783 still exists. So is this still relevant?

Not as much as before, where a malformed file url would crash the UI. Now, it would be to better handle entering "file:", "file:/", file://", file:///" or a directory file user footgun with an error message. Users of feed testing are more sophisticated anyway, but if you want to add it, f+. And subscribing to nonexistent files or, say, a linux kernel, is handled. Obviously.

Flags: needinfo?(alta88)

Can you give me a line number where to insert this block? Here, right:
https://searchfox.org/comm-central/rev/61a2e430379fa81b1afdc4b88cfd9f97b55f955a/mailnews/extensions/newsblog/content/Feed.js#125

So the block above with s/uri/this.url/ ? What's the this.downloadCallback?

(In reply to Jorg K (GMT+2) from comment #12)

Can you give me a line number where to insert this block? Here, right:
https://searchfox.org/comm-central/rev/61a2e430379fa81b1afdc4b88cfd9f97b55f955a/mailnews/extensions/newsblog/content/Feed.js#125

yes.

So the block above with s/uri/this.url/ ? What's the this.downloadCallback?

this.url is a string, so the test would have to be startsWith("file:"). downloadCallback() cleans up and sets messages, and is different depending on whether the download is initiated from Subscribe dialog, which has its own handler.

Attached patch enableFile.patchSplinter Review

"Modernised" patch from bug 254231. Linter is happy.

I don't quite understand why the error handling is different from the stuff above, the invalid scheme, but you'll be the best judge of that. Would be good to test, too, where do I get a sample XML with a feed? There's talk about using local feeds in bug 1534163 comment #12, but I can't see a sample file anywhere. Maybe Ben has got one.

Flags: needinfo?(benc)
Comment on attachment 9082318 [details] [diff] [review]
enableFile.patch

why don't you just put any feed url into a browser, view source, and copy the raw xml into a local file.. or dig out a test file from the rdf bug. we don't actually need to test valid files and xml, just malformed file urls as i noted above.
Attachment #9082318 - Flags: feedback?(alta88) → feedback+
Attached file rss2_example.xml

Here's a example simple feed (from one of the unit test patches in bug 1534163).

I'm not totally sure what the special-case checking for file existence is for. The Feeds are fetched via XMLHTTPRequest. Isn't a missing file:// url handled exactly the same as a missing http:// url? And if not, why not?
ie the whole idea is that it doesn't matter which protocol you're using for the fetch...

Flags: needinfo?(benc)
Attached file arstechnica.xml

I used this one. Next problem: Clicking the "Validate" link takes us to the validator page and that complains. Somehow, it got http://file:///D%3A/Desktop/arstechnica.xml into the box. Hmm.

Thanks, Ben, your example works as well. Isn't bug 875783 your answer?

My reading of Bug 875783 is that XMLHTTPRequest will likely throw an exception if it's passed a malformed url (eg file:/tmp/foo.xml).
And that a well-formed file:// URL pointing to a missing file would be treated the same way as a missing http:// request (presumably with a locally-synthesized 404 error?)

So checking for badly-formed urls in order to give better feedback to the user might be reasonable, but checking for file existence should be left up to XMLHTTPRequest, right?

Just tried out a little test in Firefox (I'm assuming it behaves the same in TB):

<!DOCTYPE html>
<body>
  <script>
    var xhr = new XMLHttpRequest();
    var opened = false;
    try {
      // uncommenting to try the different cases:
      // xhr.open('GET', 'obviouslyBORKED:nfnjsdfsdfsd');
      // xhr.open('GET', 'http://example.com/a_url_that_does_not_exist.html');
      //xhr.open('GET', 'file:///tmp/non_existant.html'); // a file that doesn't exist
      xhr.open('GET', 'file:///tmp/fook.html'); // a file that exists
      xhr.onerror = function() {
        console.log("ONERROR: ", xhr)
      }
      xhr.onreadystatechange = function() {
        console.log("state: ", xhr.readyState)
      }
      opened = true;
    } catch (e) {
        console.log("Failed open", e);
    }
    if (opened) {
      try {
        xhr.send();
      } catch (e) {
        console.log("Failed send", e);
      }
    }
  </script>
</body>

In all four cases I tried above, no exceptions were ever throw.
The request would either succeed, or it'd call the onerror function.

Additionally, it looks reasonably tolerant of imperfect file:// urls. For example, file:/tmp/fook.html loaded just fine.

So we don't need/want the second part?

(In reply to alta88 from comment #11)

There referenced bug 875783 still exists. So is this still relevant?

Not as much as before, where a malformed file url would crash the UI. Now, it would be to better handle entering "file:", "file:/", file://", file:///" or a directory file user footgun with an error message. Users of feed testing are more sophisticated anyway, but if you want to add it, f+. And subscribing to nonexistent files or, say, a linux kernel, is handled. Obviously.

I'm not sure what's unclear about the error cases described above. They require testing file specially, in a way that doesn't apply to http. Try em. Well formed file urls that nevertheless don't exist are not at issue for xhr, correct.

But we're really getting into weeds here. It used to be that a file could be dnd'ed to folderpane or Subscribe dialog folder from an os file explorer, and subscribe would happen (for file enabled). That path couldn't error. So only users typing the url in Subscribe would potentially encounter this total edge case. And I only wrote that check since it did throw and crash the UI and need a restart. Now, you can just click the url field and try again. Frankly, you could also just test the urlfield input like startsWith("file") && !startWith("file:///") -> invalid url. But you still have to test for a directory.

(In reply to Jorg K (GMT+2) from comment #17)

Created attachment 9082467 [details]
arstechnica.xml

I used this one. Next problem: Clicking the "Validate" link takes us to the validator page and that complains. Somehow, it got http://file:///D%3A/Desktop/arstechnica.xml into the box. Hmm.

The validate link was added after the old file patch. It does not attempt to do anything other than query a web url at the validator site. The link should be disabled if the url is file. Anyone wanting to test should go to the site and paste their xml into the validator.

I don't think the second part is needed any more.
(I think Bug 875783 is out of date now, but was valid when Bug 254231 was being discussed).

Attachment #9081972 - Flags: approval-comm-esr68?
Attachment #9081972 - Flags: approval-comm-esr68+
Attachment #9081972 - Flags: approval-comm-beta?
Attachment #9081972 - Flags: approval-comm-beta+

I leave out the second part for now.

Component: Untriaged → Feed Reader
Product: Thunderbird → MailNews Core

Alta88, so we're not using the second part, is that right? I filed bug 1571561 to disable the validate button for file: URL feeds.

You need to log in before you can comment on or make changes to this bug.