Closed
Bug 259708
Opened 20 years ago
Closed 20 years ago
Trying to save file from data: protocol wipes every file in target directory not marked read-only
Categories
(Toolkit :: Downloads API, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: WeirdAl, Assigned: bugs)
References
Details
(Keywords: dataloss, fixed-aviary1.0, testcase, Whiteboard: [sg:fix])
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•20 years ago
|
||
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
Reporter | ||
Comment 2•20 years ago
|
||
blocking1.0? because of traumatic dataloss. Yes, it's a rare scenario, but it's very evil.
Flags: blocking-aviary1.0?
Updated•20 years ago
|
Flags: blocking-aviary1.0? → blocking-aviary1.0+
Reporter | ||
Comment 3•20 years ago
|
||
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.
Summary: Trying to save file from data: protocol wipes every file in target directory except first one → Trying to save file from data: protocol wipes every file in target directory not marked read-only
Comment 4•20 years ago
|
||
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.
Reporter | ||
Comment 5•20 years ago
|
||
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•20 years ago
|
||
whoa, what exactly in that file does this?
Comment 7•20 years ago
|
||
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.
Reporter | ||
Comment 8•20 years ago
|
||
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•20 years ago
|
||
(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.
Reporter | ||
Comment 10•20 years ago
|
||
:( Note bug 259890 comment 7. The patch for Mozilla trunk will not apply to Firefox 1.0 branch.
Untested, cargo-cult.
Comment 12•20 years ago
|
||
Comment on attachment 160030 [details] [diff] [review] Ported patch r+sr=bzbarsky
Attachment #160030 -
Flags: superreview+
Attachment #160030 -
Flags: review+
Assignee: bugs → cst
Reporter | ||
Updated•20 years ago
|
Attachment #160030 -
Flags: approval-aviary?
Depends on: 259890
Comment 13•20 years ago
|
||
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...
Attachment #160030 -
Flags: superreview+
Attachment #160030 -
Flags: review+
Attachment #160030 -
Flags: approval-aviary?
Reporter | ||
Comment 14•20 years ago
|
||
Comment on attachment 160030 [details] [diff] [review] Ported patch obsoleting, as this is being covered by bug 259890
Attachment #160030 -
Attachment is obsolete: true
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?
Reporter | ||
Comment 16•20 years ago
|
||
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
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.
Reporter | ||
Comment 18•20 years ago
|
||
confirmed, this bug is still present in 09/25 builds. :-(
Assignee: cst → bugs
Assignee | ||
Comment 19•20 years ago
|
||
Asa suggested this become a security-sensitive bug due to possible side-effects.
Group: security
Status: NEW → ASSIGNED
Comment 20•20 years ago
|
||
Tracy, can you try to figure out when this was first introduced and which products/releases it impacts?
Assignee | ||
Comment 21•20 years ago
|
||
patch
Comment 22•20 years ago
|
||
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 on attachment 160553 [details] [diff] [review] patch r=shaver.
Attachment #160553 -
Flags: review+
Reporter | ||
Comment 24•20 years ago
|
||
For the record, though I may be too late, I was able to reproduce this bug also on Linux with a debug-built Firefox.
OS: Windows 98 → All
Reporter | ||
Comment 25•20 years ago
|
||
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]
Reporter | ||
Comment 26•20 years ago
|
||
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•20 years ago
|
||
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.
Reporter | ||
Updated•20 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 28•20 years ago
|
||
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•20 years ago
|
||
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?
Reporter | ||
Comment 30•20 years ago
|
||
Fine. :) I'm just writing out some analysis of the bug's process now for posting to my blog.
Comment 31•20 years ago
|
||
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•20 years ago
|
||
Isn't the recursive delete what you fixed in bug 259890? Or did that turn out to be unrelated?
Comment 33•20 years ago
|
||
dveditz: comment 18 said that the patch there did not fix this bug; so it seems unrelated
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•20 years ago
|
||
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.
Whiteboard: [sg:fix] please don't remove security flag before Monday Oct 4
Comment 36•20 years ago
|
||
(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)
Updated•20 years ago
|
Keywords: fixed-aviary1.0
Opening bug at reporter's request.
Group: security
Reporter | ||
Comment 38•20 years ago
|
||
I filed bug 262949 per comment 31 of this bug, to see if we can get a better filename than "unnamed".
Updated•20 years ago
|
Whiteboard: [sg:fix] please don't remove security flag before Monday Oct 4 → [sg:fix]
Updated•20 years ago
|
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•