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

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Description

[u601362]

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"

Comment 1

[Jorg K (CEST = GMT+2)]

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

Comment 2

[alta88]

Attachment: add file to valid feed url schemes

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.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9081972 [details] [diff] [review]
add file to valid feed url schemes

Thanks, that was quick.

Comment 4

[Pulsebot]

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

Comment 5

[Jorg K (CEST = GMT+2)]

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

Comment 6

[u601362]

Thanks!

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Magnus Melin [:mkmelin]]

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.

Comment 10

[Jorg K (CEST = GMT+2)]

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?

Comment 11

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

Comment 12

[Jorg K (CEST = GMT+2)]

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?

Comment 13

[alta88]

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

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: enableFile.patch

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

Comment 15

[alta88]

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.

Comment 16

[Ben Campbell]

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

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Jorg K (CEST = GMT+2)]

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

Comment 19

[Ben Campbell]

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?

Comment 20

[Ben Campbell]

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.

Comment 21

[Jorg K (CEST = GMT+2)]

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

Comment 22

[alta88]

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

Comment 23

[Ben Campbell]

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

Comment 24

[Jorg K (CEST = GMT+2)]

I leave out the second part for now.

Comment 25

[Jorg K (CEST = GMT+2)]

TB 69 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/2281a77dfbf64434687ebd51c70f6793a773deac

Comment 26

[Jorg K (CEST = GMT+2)]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/7a5e0270f7b0c5b5f52c5aa4c4abf9d5f17899ac