Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 this issue Jul 14, 2024 · 2 comments

Comments

@lyubomyr-shaydariv
Copy link

Prerequisites:

  • Mozilla Firefox (mine is 128.0b9 64-bit Linux)
  • MarkDownload 3.3.0
  • Make sure that the Disallowed Characters (to strip out of title/filename — in addition to /, ?, <, >, , :, *, |, ") option has no extra character, or at least a period (.) is not included to the option

Steps to reproduce:

  • Open a page where title contains a period (.) in its title (say, any .NET-related page)
  • Activate the markdown download button/shortcut thus all downloadable images must follow the pattern: <IMG_DIR_FOO>.<IMG_DIR_BAR>/<FILENAME>

Actual results in Firefox:

  • No images are downloaded
  • Nothing in logs

Actual results in Chrome:

  • All images downloaded obeying the <IMG_DIR_FOO>.<IMG_DIR_BAR>/<FILENAME> pattern

Current workaround:

  • Add period (.) to the Disallowed characters list.

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
Copy link
Author

lyubomyr-shaydariv commented Jul 14, 2024

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
Copy link
Author

Okay, more information how it fails:

  • Image filename prefix template: {date:YYYYMMDDTHHmmssZ} - {pageTitle}/ (note the trailing slash /)
  • If the page title contains a slash /, I guess Firefox may attempt to protect from file system path traversal.
  • If the page title contains a period ., I guess Firefox may consider trailing slashes as a part of file extension which is considered illegal.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant