deathau / markdownload

A Firefox and Google Chrome extension to clip websites and download them into a readable markdown file.
Apache License 2.0
2.91k stars 226 forks source link

Firefox fails silently to download all images when the page title contains a period (or probably many others as well) #332

Open lyubomyr-shaydariv opened 4 months ago

lyubomyr-shaydariv commented 4 months ago

Prerequisites:

Steps to reproduce:

Actual results in Firefox:

Actual results in Chrome:

Current workaround:

I drilled down to the browser.downloads.download at https://github.com/deathau/markdownload/blob/3.3.0/src/background/background.js#L460 before I figured it out and, as it is seen in the code, the download function is missing the catch. Adding the catch method to the promise returned with browser.downloads.download will produce the following error in the console:

Error: filename must not contain illegal characters

Have no idea why Firefox considers a period in the middle of the file path as an illegal character, and why it does not replace it with dummy character like _ (breaking the downloaded markdown file image references though). This issue seems to be even a "more edge case" than a case with filenames starting with a dot as described here. Interestingly, saving a page (Ctrl+S) with a period in the title works perfectly.

If I understand the Firefox correctly, the cause of the error might be caused with this check: https://searchfox.org/mozilla-central/rev/3d20a41f8c74116d79b863148741665e6f9567c1/toolkit/components/extensions/parent/ext-downloads.js#704 (whose error seems to be kind of misleading because the check seems to validate paths only, but reports an illegal filename) that indirectly relies on the sanitize method at https://searchfox.org/mozilla-central/rev/3d20a41f8c74116d79b863148741665e6f9567c1/uriloader/exthandler/nsExternalHelperAppService.cpp#3527 . Speaking frankly, no idea though if I'm on the right way.

Seems to need to get fixed in the upstream Firefox, and no a special fix in the extension should be provided. I guess the issue might remain open unless Firefox is fixed.

lyubomyr-shaydariv commented 4 months ago

Note: Adding the . to the exclusion list also removes the period between file basename and file extension.

This fact seems to be a need to fix the issue in the extension unless the Firefox is fixed.

lyubomyr-shaydariv commented 4 months ago

Okay, more information how it fails:

I'm not sure how generateValidFileName is supposed to work, but it seems it handles neither slash nor period properly. These two characters may need sanitizing for both path and filename but under different circumstances. Not sure how many path slashes are allowed in Firefox, and not sure if period is only allowed after the last slash.