Closed Bug 1815613 Opened 1 year ago Closed 19 days ago

Filename name suggested for downloaded file accepted as it is.

Categories

(Fenix :: Downloads, defect, P3)

Firefox 109
All
Android

Tracking

(firefox128 fixed)

RESOLVED FIXED
128 Branch
Tracking Status
firefox128 --- fixed

People

(Reporter: gabri.ns, Assigned: npoon2003, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxdroid] [group4] )

Attachments

(4 files)

Steps to reproduce:

  1. Open https://en.savefrom.net/
  2. Input https://youtube.com/watch?v=eMn_77h2_Kk
  3. Continue from browser
  4. Click download button

Actual results:

Download confirmation show up with name containing unsafe character as shown in attachment. After download complete, it doesn't show up in download manager and the file cannot be opened nor be found anywhere as it is containing invalid file name.

Expected results:

Download confirmation already change the filename to safe name and after download completion file exist in filesystem.

The severity field is not set for this bug.
:cpeterson, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(cpeterson)
Flags: needinfo?(cpeterson)
Severity: -- → S3

Response's header
Content-Disposition: attachment; filename="HONDA ODYSSEY 2000 A/T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.mp4"

the unsafe char is / (which indicates a folder in Linux based OS)

The file download successfully in Download folder , However chars before / are ignored in the filename resulting T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.mp4

I thinks it is websites' duty to make filename not contains unsafe chars.

In desktop firefox, the "A/T" is translated to "A_T" and "|" is striped , I think Fenix should do this too.

In Edge (Android) , "/" and "|" are translated to "_".

See Also: → 1816330
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true

Attempt to create a downloadable file that can reproduce this bug

Whiteboard: [fxdroid] [group4]

Hey gabri.ns,
Can you still reproduce the bug? 

It seems like the website provided in the STR (https://en.savefrom.net/) has changed the way it names the downloadable files, I could only have videoplayback.webm for example.

I tried with the file I attached on this bug, hosted from my computer, and it was correctly renamed as described on Comment 2.

QA, can you give it a try to confirm we can close this bug?

Flags: qe-verify+
Flags: needinfo?(gabri.ns)

Actually my test might not have been enough, as I only downloaded the file with a name containing |, but maybe we need to try specifically with what jackyzy823 suggested:

Response's header
Content-Disposition: attachment; filename="HONDA ODYSSEY 2000 A/T | REVIEW & TEST DRIVE | UDAH MURAH SEHARGA GENIO.mp4"

This issue is still reproducible as described.

  • The unsafe characters are still displayed as shown in the original attachment.
  • After download complete, it doesn't show up in download manager and the file cannot be opened nor be found anywhere as it is containing invalid file name is also still the case.

Tested on the latest Nightly build (128.0a1 from 2024-05-23).
Tested device: Samsung Galaxy S23 Ultra (Android 14).

Thanks for testing :Lorand Janos!

Can you please provide a description of how you tested, so that I can try to reproduce and fix it?

Flags: needinfo?(ljanos)

Hi, sure,

  1. I accessed the mentioned page to download videos: https://en.savefrom.net/
  2. I used the URL of the video given in the second step which is still active: https://youtube.com/watch?v=eMn_77h2_Kk
  3. After the video was loaded, I tapped on the Download button below which opened the download popup (with the mentioned "unsafe characters") .
  4. After tapping on download, while it shows that the download is completed, tapping on "Open" gets a "Failed to play video" error message.
  5. Searching for the downloaded video in the downloads returns no result.
Flags: needinfo?(ljanos)
Flags: qe-verify+
Assignee: nobody → npoon2003

Notes after pairing with Titouan

  • After following the instructions to reproduce the bug, we realize that the video gets saved to the downloads directory as "T _ REVIEW & TEST DRIVE _ UDAH MURAH SEHARGA GENIO.mp4". That is, besides the name getting concatenated, the "|" is replaced with "_". Upon opening the video, it plays successfully. However, the file does not appear in the fenix downloads directory. Furthermore, when fenix prompts to open the file, it results in an error.
  • After further investigation, we realized that the illegal characters for filenames on Android are at least these characters: /, <, >, *, ", :, ?, , |. If we edit String.replaceEscapedCharacters in String.kt (link) to also replace "|" with some legal character, such as "", the feature works mostly as expected. That is, the file name replaces the "|" with "" and the file appears in fenix's download directory where the video can be played. Also, after the download completes, fenix opens a prompt to open (and play) the file which succeeds, and the file appears in the downloads directory where it can be played successfully. The one expected behaviour that is missing is that the name remains the same but the illegal characters are substituted with legal characters.
  • We believe that the reason for the concatenation behaviour in the name lies in extractFileNameFromUrl (link) and String.sanitizeFileName (link).

Follow Up Actions

  • Update String.replaceEscapedCharacters to also replace / substitute other illegal characters in Android filenames
  • Update the tests in StringTest.kt (link) to take the illegal characters into account

(In reply to Nicholas Poon [:Nick] from comment #10)

Notes after pairing with Titouan

  • After following the instructions to reproduce the bug, we realize that the video gets saved to the downloads directory as "T _ REVIEW & TEST DRIVE _ UDAH MURAH SEHARGA GENIO.mp4". That is, besides the name getting concatenated, the "|" is replaced with "_". Upon opening the video, it plays successfully. However, the file does not appear in the fenix downloads directory. Furthermore, when fenix prompts to open the file, it results in an error.
  • After further investigation, we realized that the illegal characters for filenames on Android are at least these characters: /, <, >, *, ", :, ?, \, |. If we edit String.replaceEscapedCharacters in String.kt (link) to also replace "|" with some legal character, such as "", the feature works mostly as expected. That is, the file name replaces the "|" with "" and the file appears in fenix's download directory where the video can be played. Also, after the download completes, fenix opens a prompt to open (and play) the file which succeeds, and the file appears in the downloads directory where it can be played successfully. The one expected behaviour that is missing is that the name remains the same but the illegal characters are substituted with legal characters.
  • We believe that the reason for the concatenation behaviour in the name lies in extractFileNameFromUrl (link) and String.sanitizeFileName (link).

Follow Up Actions

  • Update String.replaceEscapedCharacters to also replace / substitute other illegal characters in Android filenames
  • Update the tests in StringTest.kt (link) to take the illegal characters into account
Attachment #9404200 - Attachment description: WIP: Bug 1815613 - When downlaoding, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _ → WIP: Bug 1815613 - When downloading, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _
Attachment #9404200 - Attachment description: WIP: Bug 1815613 - When downloading, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _ → Bug 1815613 - When downloading, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _
Pushed by tthibaud@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3be3c6c0ef5e
When downloading, replace illegal filename characters (<, >, *, ", :, ?, \, |) with _ r=android-reviewers,tthibaud
Status: NEW → RESOLVED
Closed: 19 days ago
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: