259708 - Trying to save file from data: protocol wipes every file in target directory not marked read-only

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Alex Vincent [:WeirdAl]]

Testcase coming up in a few minutes.

Mozilla/5.0 (Windows; U; Win98; rv:1.7.3) Gecko/20040913 Firefox/0.10

Basically, I had written an article for my blog (not yet posted) whereby a short
script would convert a textarea's contents into a data: URI for saving.

So with a content-type of application/octet-stream, Firefox would try to save
it.  Unfortunately, it would never go beyond the "Starting" phase of the "download".

When you hit cancel, bye-bye directory's contents, except for the first file.

Repeated testcase in Mozilla 1.8a3:  did NOT see the same results.  Example
worked perfectly.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: Testcase HTML page

Steps to reproduce:

(1) Tools, Options, Downloads.	Set the directory to a brand-new one.
(2) In said download directory, manually create three or four new files,
perhaps a directory or two.
(3) Open attachment, scroll to bottom and click "Save File" button.
(4) Mozilla Firefox asks you whether you want to open the file or save it...
choose "Save to disk".
(5) Download Manager appears with "Starting" message for Downloads directory
(this is probably NOT what should happen.  It should show an actual file being
downloaded.)
(6) Hit Cancel link in Download Manager.

Expected result:  Download cancels safely.
Actual result:	All but one file in your downloads directory are completely
gone, unrecoverable.

Reproducible:  Always

Comment 2

[Alex Vincent [:WeirdAl]]

blocking1.0? because of traumatic dataloss.  Yes, it's a rare scenario, but it's
very evil.

Comment 3

[Alex Vincent [:WeirdAl]]

Further testing shows the only reason any file in the target directory survived
-- for that matter, the target directory itself -- is because said file was
marked read-only.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sounds like some of the (forked) front-end code is making silly assumptions
about the ability to get a non-empty filename off a URI.

Since it's forked, I happen to not really care.

Comment 5

[Alex Vincent [:WeirdAl]]

I've tracked the problem back to
http://lxr.mozilla.org/aviarybranch/source/uriloader/exthandler/nsExternalHelperAppService.cpp
.  I think it unusual that we don't get a file picker to find out the preferred
filename for this file.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

whoa, what exactly in that file does this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm betting that the temp file is being set to the directory (by the Firefox
download manager breakage) and then that PR_TRUE in the delete that you fixed on
trunk recently kicks in.

Comment 8

[Alex Vincent [:WeirdAl]]

nsExternalAppHandler::InitializeDownload is suspicious, insomuch as it is the 
only code which refers to an aDownload object.

For that matter, I should clarify that it very well could be 
nsDownloadManager.cpp .  I thought nsExternalAppHandler was responsible because 
I didn't see it specify or request a filename right away.

Comment 9

[Darin Fisher]

(In reply to comment #7)
> I'm betting that the temp file is being set to the directory (by the Firefox
> download manager breakage) and then that PR_TRUE in the delete that you fixed on
> trunk recently kicks in.

we should make sure that the patch for bug 259890 lands on the aviary and 1.7
branches.

Comment 10

[Alex Vincent [:WeirdAl]]

:( Note bug 259890 comment 7.  The patch for Mozilla trunk will not apply to
Firefox 1.0 branch.

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Ported patch

Untested, cargo-cult.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160030 [details] [diff] [review]
Ported patch

r+sr=bzbarsky

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 160030 [details] [diff] [review]
Ported patch

Actually, clearing reviews.  This patch is on the wrong bug.  It may well fix
this bug (in that the directory will no longer be wiped), but even with it in
you won't be able to save...

Comment 14

[Alex Vincent [:WeirdAl]]

Comment on attachment 160030 [details] [diff] [review]
Ported patch

obsoleting, as this is being covered by bug 259890

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I just committed the patch for 259890, which might also fix this -- can someone
confirm that it does with tomorrow's nightly, and resolve this if so?

Comment 16

[Alex Vincent [:WeirdAl]]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040925 Firefox/0.10
is still showing this bug!

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/sweetlou-0.9/Firefox-win32.zip
dated 9/25/04  	2:35:00 PM

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Oh, I wasn't doing the right steps to reproduce the bug.  I can reproduce it
with my PR1.0 build, but my aviary-1.0 nightly with my patch isn't failing to
download - it downloads successfully and it's so fast I can't hit cancel.

Comment 18

[Alex Vincent [:WeirdAl]]

confirmed, this bug is still present in 09/25 builds.  :-(

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Asa suggested this become a security-sensitive bug due to possible side-effects.

Comment 20

[Asa Dotzler [:asa]]

Tracy, can you try to figure out when this was first introduced and which
products/releases it impacts?

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch

patch

Comment 22

[Tracy Walker [:tracy]]

This bug affects releases at least back to Firefox 0.8
I can't reproduce this on Mozilla builds.  Tested 1.8a4 and 1.7

On Firefox 0.8 and 0.9  I didn't have to cancel the download.  It was too fast
to cancel anyhow.  But the folder I selected to download to was overwritten by
the downloaded testfile (it kept the Folder name).

Mozilla builds installed the file into the selected Folder without data loss.

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 160553 [details] [diff] [review]
patch

r=shaver.

Comment 24

[Alex Vincent [:WeirdAl]]

For the record, though I may be too late, I was able to reproduce this bug also
on Linux with a debug-built Firefox.

Comment 25

[Alex Vincent [:WeirdAl]]

The patch does fix this bug, but I get an exception running my testcase:

*** exception in validateLeafName: [Exception... "Component returned failure
code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.append]" 
nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)"  location: "JS frame ::
file:///home/ajvincent/mozilla/dist/bin/components/nsHelperAppDlg.js ::
anonymous :: line 211"  data: no]

Comment 26

[Alex Vincent [:WeirdAl]]

fixed Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040929 Firefox/0.10
as of a sweetlou build timestamped 2004092917

We pass the testcase, and I can file a separate (non-blocking-aviary) bug for
fixing the download manager (or our help dialog) to get a filepicker so we save
the file properly.

Question, though:  are there any other protocols which can force a pseudo-download?

I'm leaving this bug as ASSIGNED, in case there are such protocols or resolving
as FIXED would open this bug to the public.  Until I am assured of such, I think
I should not file the spinoff bug for getting the filepicker.

Comment 27

[Tracy Walker [:tracy]]

Verified fixed on Windows branch build 2004-09-30-08-0.9


fyi: I also tried some various scenarios with sending, saving and opening the
file on the Thunderbird branch and had no data loss.

Comment 28

[Alex Vincent [:WeirdAl]]

Given that this bug is fixed, as tested by myself and Tracy Walker, could we
please re-expose this bug to the public?  I'd like to be able to talk about it
again.

Comment 29

[Asa Dotzler [:asa]]

Alex, we're hustling to get a Firefox update in place, both an xpi patch and a
full 0.10.1 build. If all goes well, we'll be shipping that tomorrow. I'd like
to keep this closed until then. Is that OK with you?

Comment 30

[Alex Vincent [:WeirdAl]]

Fine.  :) I'm just writing out some analysis of the bug's process now for
posting to my blog.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 160553 [details] [diff] [review]
patch

ok, so this patch cathes an exception and modifies the filename if it finds
one. (I do think "unnamed" should be localizable, btw).

but wouldn't a better fix be _not_ to recursively delete something that is
supposed to be a single file? it seems to me like there must be a remove(true)
somewhere that would better be a remove(false).

Comment 32

[Daniel Veditz [:dveditz]]

Isn't the recursive delete what you fixed in bug 259890? Or did that turn out to
be unrelated?

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

dveditz: comment 18 said that the patch there did not fix this bug; so it seems
unrelated

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Isn't the cause of the problem more likely to be the Remove(PR_TRUE) here?
http://lxr.mozilla.org/aviarybranch/source/toolkit/components/downloads/src/nsDownloadManager.cpp#566
That checks if the file exists, and if it does, recursively deletes it.

Comment 35

[Daniel Veditz [:dveditz]]

We would like to leave this bug security-sensitive for a couple of days after
the patch release to give people a chance to protect themselve before leaking
exploit details.

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #31)
> but wouldn't a better fix be _not_ to recursively delete something that is
> supposed to be a single file? it seems to me like there must be a remove(true)
> somewhere that would better be a remove(false).

ok, fixed by
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/mozapps/downloads/content&command=DIFF_FRAMESET&file=downloads.js&rev1=1.28&rev2=1.29&root=/cvsroot
(checked in as part of bug 262478)

Comment 37

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Opening bug at reporter's request.

Comment 38

[Alex Vincent [:WeirdAl]]

I filed bug 262949 per comment 31 of this bug, to see if we can get a better
filename than "unnamed".