[HOLD for payment 2023-02-10] File name with "/" removes the extension and breaks the file when downloaded

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open Expensify
2. Open any report
3. Attach any file with "/" as the filename and valid file extension.
4. Download the file.

Expected Result:

Valid file downloaded

Actual Result:

Malformed file downloaded due to removed extension

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.53-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

video.mov

Recording.54.mp4

Expensify/Expensify Issue URL:
Issue reported by: @oesayan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673509312939899

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b3bd35954214a174
• Upwork Job ID: 1614013175418511360
• Last Price Increase: 2023-01-13

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01b3bd35954214a174

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mananjadhav (Internal)

[melvin-bot]

Triggered auto assignment to @dangrous (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[NicMendonca]

In the thread @mountiny mentioned this being internal, @dangrous please let me know otherwise!

[mountiny]

@dangrous i think it could have solution in both, but ideally we would make sure this is processed fine in the backend (havnet looked into where is the root cause of this)

[NicMendonca]

bump @dangrous 👋

[dangrous]

Hi sorry - was OOO yesterday! Will look into this today.

[dangrous]

Okay so figured out part of it -

When a user uploads an attachment, we sanitize the filename. This would turn, for example, /.png into just .png - we use this:
preg_replace('/[^a-zA-Z0-9\-\._]/', '', $filename);

The / actually gets converted into a : at some point in the process, so the regex also filters out that.

However, after that point I've confirmed that the filetype is correct, so I'm not sure why it's downloading as png.txt.

But either way, if we allow colons in the filename, the behavior is resolved. It downloads as _.png but you are able to open it fine. @mountiny what do you think about this? Is there an insecurity in allowing colons, or can you think of other behavior that might get messed up?

[mananjadhav]

@dangrous I think one reason for not using : colons is Windows doesn't support : in the file names. So when saving the files the users would probably get an error. Are you saying that : gets converted into _ underscore when the file is downloaded? Then that should be just fine.

[mountiny]

I cant really confidently answer if this opens door for some other issues.

I would say we could also not allow the / in file name, I dont think its very common to have filenames with this character.

But as Manan mentioned, I think converting it to underscore and running with that should be fine if we want just backend solution

[dangrous]

So at some point in the flow (I'm not sure where yet) the / gets converted into a : - this happens on the App side somewhere.

Then, the : is stripped out on the Web-E side.

After changing the regex in testing to allow :, somewhere between storing the file and downloading it, that : gets converted into a _.

So maybe if I can find where /.png becomes :.png on the App side, we could make it switch it into _.png and then we'd be all set. That could also be external, too.

[mananjadhav]

There's a lot of somewheres. So are we saying this could purely be external? with no change on the backend?

[mountiny]

I think we should do this in both, change the / to _ in frontend and backend as well so we not only rely on the frontend and make sure we dont leave room for vulnerabilities.

[dangrous]

That's fair. So I can put up a PR that adjusts https://github.com/Expensify/Web-Expensify/blob/62f13038a4fe5e3f9991f1d2b23e7a00474a1827/lib/ReportUtils.php#L2221-L2224 to also replace any /s and :s with _s, and then we can open this externally to do the same on the App side?

For reference, I know it's getting switched to a : on the app side because when you attempt to upload a /.png the header says:

Before you hit the server at all.

[mananjadhav]

I think the / to : separator is done by the MacOS (*nix).

and then when we get the file from the backend, we don't get any / or :. The originalMessage content received from the backend.

<img src="https://www.expensify.com/chat-attachments/4313048699244575240/w_3249cb0371a0cac30c660b74ee79935ee9d0c9b0.png.1024.jpg" data-expensify-source="https://www.expensify.com/chat-attachments/4313048699244575240/w_3249cb0371a0cac30c660b74ee79935ee9d0c9b0.png" data-name=".png" data-expensify-width="225" data-expensify-height="224" />

Notice the data-name which only contains .png and hence we save the file it shows just png.

[dangrous]

Ah okay. So we'd need to add something there to switch it to a _ but that should be fine. The data-name is only showing png because it goes through that regex above.

When I replace the regex on the backend to swap out : and /, it succesfully downloads from the front end as [an underscore for each slash].png so looks like it's working there, even without a frontend change.

[oesayan]

@mananjadhav is right - that's an old tale in MacOS world since before some old version MacOS used : instead of slashes and some layers of MacOS still use :, some layers use slash - Finder allows slashes to be used in filenames, while some other layers do not use slashes and replace them with : .
That's why this behaviour is the same on iOS and MacOS, however on Android the OS replaces the slashes with _.

[akshayasalvi]

Proposal

If we want to replace the file name at the client, we should replace the name during the upload. I think we should replace both the / and : during the upload.

In AttachmentPicker/index.js

App/src/components/AttachmentPicker/index.js

Lines 33 to 40 in 5370c36

	onChange={(e) => {
	const file = e.target.files[0];
	if (file) {
	file.uri = URL.createObjectURL(file);
	this.onPicked(file);
	}

onChange={(e) => {
                        const file = e.target.files[0];
                        if (file) {
                            file.name = file.name.replace(/[\/:]/g,'_').replace(/_+/g, '_');
                            // ... rest of the code
                        }

We need to use regex because if we have multiple consecutive colon or slash, we should replace with single underscore.

I think we should be doing this only with index.js, but if it needs to be done for the native too, I can do that.

Update for the native is: In AttachmentPicker/index.native.js on L154,

App/src/components/AttachmentPicker/index.native.js

Lines 150 to 155 in 5370c36

	if (fileData.width === -1 || fileData.height === -1) {
	this.showImageCorruptionAlert();
	return;
	}
	return getDataForUpload(fileData).then((result) => {

fileData.fileName = fileData.fileName.replace(/[\/:]/g,'_').replace(/_+/g, '_');

[Pujan92]

@dangrous @mananjadhav @akshayasalvi

Associated PR seems to raise a regression where we won't able to reach out to Avatar Crop part and the infinite loader will remain on the screen after cleaning the filename.
Reason is uri will be undefined as the file object is overwritten here.

App/src/components/AttachmentPicker/index.js

Lines 39 to 42 in 51f1a77

	file.uri = URL.createObjectURL(file);
	if (file.name !== cleanName) {
	file = new File([file], cleanName);
	}

Screen.Recording.2023-02-03.at.6.39.27.PM.mp4

To solve we need to make an order change

diff --git a/src/components/AttachmentPicker/index.js b/src/components/AttachmentPicker/index.js
index dddceb7cf7..dc6573e02c 100644
--- a/src/components/AttachmentPicker/index.js
+++ b/src/components/AttachmentPicker/index.js
@@ -36,10 +36,10 @@ class AttachmentPicker extends React.Component {
 
                         if (file) {
                             const cleanName = FileUtils.cleanFileName(file.name);
-                            file.uri = URL.createObjectURL(file);
                             if (file.name !== cleanName) {
                                 file = new File([file], cleanName);
                             }
+                            file.uri = URL.createObjectURL(file);
                             this.onPicked(file);

Screen.Recording.2023-02-03.at.6.39.48.PM.mov

Context for me - #14757

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[dangrous]

Re regression - yes, it looks like this did cause a regression, based on @Pujan92's analysis. I also tried reverting the change locally and confirmed that behavior was correct prior to the change.

I'm not sure if a checklist change is needed; I think the thing that was missed here was honestly just proofreading - I should have noticed that the file would no longer have the uri if the name was changed. I guess technically we could have noticed the bug by testing all places where attachments were uploaded, rather than just as attachments - but I'm not sure that's the primary solution here.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.64-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Replace file names with underscore when they have slash / #14664

If no regressions arise, payment will be issued on 2023-02-10. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mananjadhav / @dangrous] The PR that introduced the bug has been identified. Link to the PR: n/a - edge case that's existed since the renaming was created in 2020.
• [@mananjadhav / @dangrous] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: n/a - edge case that's existed since the renaming was created in 2020.
• [@mananjadhav / @dangrous] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: n/a - edge case that's existed since the renaming was created in 2020, no bug to be caught - regression test should help us stay stable in the future.
• [@akshayasalvi] Propose regression test steps to ensure the same bug will not reach production again. - updated version here
• [@NicMendonca] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[akshayasalvi]

@dangrous I just went through the comments. I didn't think upload Avatar will be any different as it is the same picker. I can see another PR is merged, is there any action pending here for me?

[dangrous]

Hey @akshayasalvi so to clarify (this is confusing!) the step on the checklist above is actually to propose a regression test for the original bug - that is, that /.png etc. won't be able to be downloaded. I think it would be something along the lines of "Upload a file named ///.png and make sure you can download it." but formatted appropriately according to the link.

Does that make sense?

[dangrous]

Separately, @mananjadhav, I think this is likely just an edge case we didn't consider before, rather than a regression. The initial renaming looks like it was added 7/24/20 into the backend so it was quite some time ago (possibly before the App existed?). Therefore no PR to comment on, and I don't think we need the discussion, as long as we get the new regression test. Does that sound right to you?

[dangrous]

Bump @akshayasalvi and @mananjadhav on the above - thanks!

[akshayasalvi]

Regression Test Proposal

Bug: File name with special characters like "/" breaks the file download

Proposed Test Steps:

1. Go to URL https://staging.new.expensify.com/
2. Log in with any account & navigate to any conversation
3. Have an image/file with the name as : or / with the extension, for example, ///.png.
4. In the selected conversation, upload the above file
5. Verify that the preview should show if it is an image and all occurrences of the special character '/' is replaced with _
6. Click on upload and ensure the file is successfully uploaded
7. Open the file in the attachment preview once uploaded
8. Ensure you're able to preview if it is an image
9. Click on download for the image/file and once the file is downloaded, it should open successfully.

@dangrous @mananjadhav Is this okay for the test proposal?

[mananjadhav]

I was OOO and just getting back. @akshayasalvi this looks fine, sorry I just saw your comment.

I don't think we need the discussion, as long as we get the new regression test. Does that sound right to you?

Yes @dangrous. Let's confirm the test proposal and then we should be good.

[dangrous]

Hi @akshayasalvi this looks great! A couple edits - primarily I think we should just be very specific (you're right that it doesn't actually matter the kind of file, etc. but it'll be easier to test if it's particular).

1. Go to URL staging.new.expensify.com.
2. Log in with any account & navigate to any conversation.
3. Have an image with the name ///.png.
4. Upload that file to the conversation as an attachment.
5. Verify that the upload preview shows the image correctly, and the name of the file is shown as ___.png (all occurrences of the special character / are replaced with _).
6. Click on upload and ensure the file is successfully uploaded.
7. Open the file in the attachment preview.
8. Ensure you're able to preview the image.
9. Download the image and once the file is downloaded (as ___.png), ensure it opens successfully.

How does that sound?

[NicMendonca]

@mananjadhav @akshayasalvi @oesayan - can you three please apply to the job so I can issue payment? https://www.upwork.com/jobs/~01b3bd35954214a174

[akshayasalvi]

@NicMendonca applied, thank you.

@dangrous Yes this sounds good. It's my first time writing the regression test proposal. thank you for the help.

[mananjadhav]

@NicMendonca I've applied too.

[oesayan]

@NicMendonca and I have applied too 🤠

[NicMendonca]

@mananjadhav @akshayasalvi - paid, thank you!

@oesayan - I'll issue payment once you accept the offer, thanks!

[NicMendonca]

everyone had been paid! thank you!!