Closed Bug 280419 Opened 15 years ago Closed 12 years ago

When a temporary file is created from the download manager it should be readonly.

Categories

(Core Graveyard :: File Handling, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bryan, Assigned: mkmelin)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040910

When a download is requested and the user clicks 'Open' the file is saved in the
temporary folder. For word documents this allows users to edit and save the file
back to disk. When mozilla closes it will delete the temporary file, loosing the
changes.

The file should be marked readonly so the user can't change the file.

This is a particular problem with webmail.

Reproducible: Always

Steps to Reproduce:
1.Open a word document from the web by clicking the 'Open' button on the
download prompt.
2.Edit the document and select 'Save' from the 'File' menu in word.
3.Exit word.
4.Exit mozilla.

Actual Results:  
The file was deleted including the changes.

Expected Results:  
The temporary file should have been saved as readonly - so the user could not
modify the file. Ideally this would be done by removing the write permissions
for the file.
The webmail package I tried this with was 'horde'. The bug was produced by
clicking on the link on the left - not the disk icon - when trying to save an
attachment.
*** Bug 280420 has been marked as a duplicate of this bug. ***
Assignee: download-manager → file-handling
Component: Download Manager → File Handling
Product: Mozilla Application Suite → Core
QA Contact: ian
Version: unspecified → Trunk
This is easy to do if we decide it's desirable, fwiw; just change all
occurrences of 0600 to 0400 in
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp
bz, that won't work on windows, will it?
Not sure, actually.  I thought we set some sort of bits on at least WinXP...

But yes, in general we don't really have a way to mark files readonly on Windows
that I know of.
related problem for attachments: Bug 220808
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/
This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → EXPIRED
This is a real issue.
Status: RESOLVED → UNCONFIRMED
Resolution: EXPIRED → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: helpwanted
bug 107088 fixed this on non-windows platforms
OS: All → Windows 95
Hardware: All → PC
Not really.  The files are still 0600; this bug is asking for them to be 0400.
OS: Windows 95 → All
Hardware: PC → All
I can't see any harm in doing this, as long as it won't muck with delete-on-exit on poorly-behaved filesystems (its been a while, but I remember oddness with deleting readonly things on older Windows, of course none that we're supporting on trunk!)

Duplicate of this bug: 314601
Blocks: 220808
For what it's worth, I just tried to implement BZ's suggestion from comment #3. It did not work on Windows XP, files were still read/write.
I'm pretty sure the nsIFile file permissions have never worked on Windows.  Someone would need to make them work...
Depends on: 369108
Attached patch proposed fix (obsolete) — Splinter Review
This should fix it for others besides windows, I tested it under linux. Filed bug 369108 about the remaining windows issue.
Assignee: file-handling → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #253852 - Flags: review?(cbiesinger)
Comment on attachment 253852 [details] [diff] [review]
proposed fix

doesn't this also mark normal downloads as readonly?

(also, you can't assert that SetPermissions succeeded. if it fails, that's not a programming error.)
Attached patch proposed fix, v2 (obsolete) — Splinter Review
No, save to disk is in another if-clause (where FixFilePermissions is also called).

A minor caveat is that this patch doesn't affect files handled by plugins (e.g. in one profile I had mozplugger set to handle .doc).
Attachment #253852 - Attachment is obsolete: true
Attachment #254311 - Flags: review?(cbiesinger)
Attachment #253852 - Flags: review?(cbiesinger)
Attached patch proposed fix, v3 (obsolete) — Splinter Review
no need for the "rv = " ...
Attachment #254311 - Attachment is obsolete: true
Attachment #254316 - Flags: review?(cbiesinger)
Attachment #254311 - Flags: review?(cbiesinger)
Christian: ping on the review request...
Keywords: helpwanted
please use something like -pu9 as diff options next time

I'd rather you didn't rely on FixFilePermissions to fix up the permissions... it's only implemented on unix, while this code is cross-platform. It seems like this would actually make a difference on Mac and BeOS.
also, sorry for the delay. I was quite busy with a lot of stuff lately.
Hmm, so maybe we should fix FixFilePermissions? The mac implementation would be copy-paste from unix, no?
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/unix/nsOSHelperAppService.cpp#1677

Have no idea about BeOS though.
Duplicate of this bug: 376351
Attached patch proposed fix, v4Splinter Review
Ok, maybe we're better off just setting readonly before opening.
Attachment #254316 - Attachment is obsolete: true
Attachment #267284 - Flags: review?(cbiesinger)
Attachment #254316 - Flags: review?(cbiesinger)
Comment on attachment 267284 [details] [diff] [review]
proposed fix, v4

So, how about this? At least it's simple:)
Attachment #267284 - Flags: review?(cbiesinger) → review+
Attachment #267284 - Flags: superreview?(dmose)
Comment on attachment 267284 [details] [diff] [review]
proposed fix, v4


>+    // make the tmp file readonly so users won't edit it and loose the changes
>+    mFinalFileDestination->SetPermissions(0400);
>+

Spelling nit: s/loose/lose/

sr=dmose with that fixed.
Attachment #267284 - Flags: superreview?(dmose) → superreview+
Attachment #267284 - Flags: approval1.9?
Attachment #267284 - Flags: approval1.9? → approval1.9+
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.344; previous revision: 1.343
done

I can only confirm this fixes it for linux. Can someone report back for other platforms? Given the approach taken, bug 369108 is not so relevant anymore...

->FIXED, if it doesn't work on your platform, SetPermissions for that platform needs to get fixed to take readonly into account
Status: ASSIGNED → RESOLVED
Closed: 14 years ago12 years ago
No longer depends on: 369108
Resolution: --- → FIXED
Or... i guess bug 369108 could still be relevant. Depends on how widely you interpret it.
Depends on: 401316
Blocks: 410794
Comment on attachment 267284 [details] [diff] [review]
proposed fix, v4

dmose: is this safe for the 1.8 branch?
Attachment #267284 - Flags: approval1.8.1.13?
If it lands on 1.8 bug 410794 should also land there. (And probably bug 401316, but that hasn't been approved for trunk, yet.)
Comment on attachment 267284 [details] [diff] [review]
proposed fix, v4

clearing branch approval request until the other bugs mentioned above are resolved one way or another.
Attachment #267284 - Flags: approval1.8.1.13?
Duplicate of this bug: 431021
Depends on: 439938
Duplicate of this bug: 76743
This used to work on Windows but now (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0) the files seem not to be set as read only before opening.

Can anyone confirm the regression ?
Flags: needinfo?(mkmelin+mozilla)
Sadly yes, filed bug 1062524.
Flags: needinfo?(mkmelin+mozilla)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.