1319088 - Bad filename in an xhr request

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kanor]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

Example code:
Html
<input type="file" name="file">

JS
var request = new XMLHttpRequest();
var url = "action_upload";
var formData = new FormData()
formData.append("file_path_select", "/");
formData.append("file", document.getElementsByName("file")[0].files[0]);
request.open("POST", url);
request.send(formData);



Actual results:

I get a query with filename "/mydoc.txt"

The browser use the new attribute webkitrelativepath ?
https://developer.mozilla.org/fr/Firefox/Versions/50#Files_and_directories


Expected results:

I should have had "mydoc.txt"

Comment 1

[Fabian]

I can confirm this problem. I tested it with 45.3.0 --> no slash prefix. 50.0 --> slash prefix.

Comment 2

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

Hi Kanor. I have created a test case for the steps mentioned, in comment 0, here; https://jsfiddle.net/kg6a2452/ . When I click on 'Browse..." button, I'm redirected to "/root/Desktop" path. After that I have selected a random *.txt doc to be uploaded and clicked on "Open" button. The name of the file is displayed near the 'Browse..." button. Tested with latest Nightly (Build ID: 20161124030208) and latest Firefox release v50.0.

Can you please elaborate a bit more the steps and the expected? Maybe I am missing something.

Comment 3

[Fabian]

One way to reproduce this issue is to upload a file on https://imgur.com/ (no login required to do so).

I uploaded an image with Chrome (Version 54.0.2840.98 (64-bit)), see screenshot http://prnt.sc/db9f3j. The same image uploaded wit Firefox (50.0) leads to the following screenshot http://prnt.sc/db9eqr.

The red rectangle shows the difference.

Comment 4

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

I have tested your issue on latest Firefox release v50.0 and latest Nightly build (Buid ID: 20161124030208) and managed to reproduce it. I have followed the steps form comment 3.

I did a regression range with mozregression and obtained the following pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=974cfc29c1e30561d40882c051f07724eff99491&tochange=5785e6e80c6696b6e941edcbc06583864d95ae95

The following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1288683

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

This looks rather bad one. Baku, you were looking at the submission part.

Comment 6

[Andrea Marchesini [:baku]]

I cannot reproduce this issue in nightly. Vlad, can you? It seems to me that the issue happens just in beta.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This was filed using FF50. Isn't that the release version now.
Do we need to get some fix to branches too?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

[Tracking Requested - why for this release]:
This is really bad regression and there is also another dup, bug 1320897

Comment 10

[Loic]

Testcase added.
Open the Netmonitor tab (F12), select a file, click on "save" then check the parameters of the request fake.php:

FF49:
Content-Disposition: form-data; name="photo-path"; filename="image.png"

FF50:
Content-Disposition: form-data; name="photo-path"; filename="/image.png"

Comment 11

[Andrea Marchesini [:baku]]

I can reproduce it. /me working on it.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: foo.patch

The path must be set only when we expose a directory.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8815792 [details] [diff] [review]
foo.patch

So this is backing out part of bug 1258490.

Please ensure this fixes also bug 1320897.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

baku, could you also test bug 1318097 and bug 1320897.

(I'll try to verify that too)

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Has this been broken since 49 release? I agree this isn't great, but I think the fix can wait 2 weeks for 50.1.0.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Felipe, Is this fix something that can be pushed via a system add-on update? I didn't think it would be. But doesn't hurt to ask. :)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Has this been broken since 49 release?
No. The problematic code landed in bug 1258490, but that was enabled in bug 1288683.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

(In reply to Ritu Kothari (:ritu) from comment #17)
> Hi Felipe, Is this fix something that can be pushed via a system add-on
> update? I didn't think it would be. But doesn't hurt to ask. :)

Not the proper fix, but we could disable the feature via the prefs from bug 1288683.

Note that we still have the problem of system add-ons only going out to 55% of people though.

Comment 21

[Gerry Chang [:gchang]]

Track 51+/52+/53+ as regression.

Comment 22

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/uploading-file-using-xhr-prepends-slash-to-filename/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/958963bca768
FormData should not add extra '/' in the Blob path, r=smaug

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8815792 [details] [diff] [review]
foo.patch

Approval Request Comment
[Feature/Bug causing the regression]: Entries API (bug 1288683)
[User impact if declined]: FromData could contain an extra '/' in file path, causing web incompatibility.
[Is this code covered by automated tests?]: yes. mochitest included
[Has the fix been verified in Nightly?]: It's just landed today.
[Needs manual test from QE? If yes, steps to reproduce]: We have a STR in this bug and in many of the duplicated bugs. It's easy to reproduce.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: no.
[Why is the change risky/not risky?]: We have many tests for Entries API, but this corner-case was not tested at all. The change removes a few lines for settings an extra path where it was not needed.
[String changes made/needed]: none

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8815792 [details] [diff] [review]
foo.patch

let's take this in aurora52

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/776b92e3ec43

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8815792 [details] [diff] [review]
foo.patch

Fix for path/file uploading issue, includes new tests, let's uplift for today's beta 6 build.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b86ace0637a

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/958963bca768

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8815792 [details] [diff] [review]
foo.patch

Site-compat issue, let's include it in 50.1.0

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/bc4ddcda09a3