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

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Bryan Ray]

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.

Comment 1

[Bryan Ray]

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.

Comment 2

[Frank Wein [:mcsmurf]]

*** Bug 280420 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

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

bz, that won't work on windows, will it?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[OstGote!]

related problem for attachments: Bug 220808

Comment 7

[Gervase Markham [:gerv]]

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/

Comment 8

[Gervase Markham [:gerv]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is a real issue.

Comment 10

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

bug 107088 fixed this on non-windows platforms

Comment 11

[Boris Zbarsky [:bzbarsky]]

Not really.  The files are still 0600; this bug is asking for them to be 0400.

Comment 12

[Mike Connor [:mconnor]]

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!)

Comment 14

[Jon Henry]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure the nsIFile file permissions have never worked on Windows.  Someone would need to make them work...

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This should fix it for others besides windows, I tested it under linux. Filed bug 369108 about the remaining windows issue.

Comment 17

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

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.)

Comment 18

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

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).

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

no need for the "rv = " ...

Comment 20

[Magnus Melin [:mkmelin]]

Christian: ping on the review request...

Comment 21

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

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.

Comment 22

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

also, sorry for the delay. I was quite busy with a lot of stuff lately.

Comment 23

[Magnus Melin [:mkmelin]]

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.

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4

Ok, maybe we're better off just setting readonly before opening.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 267284 [details] [diff] [review]
proposed fix, v4

So, how about this? At least it's simple:)

Comment 27

[Dan Mosedale (:dmosedale, :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.

Comment 28

[Magnus Melin [:mkmelin]]

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

Comment 29

[Magnus Melin [:mkmelin]]

Or... i guess bug 369108 could still be relevant. Depends on how widely you interpret it.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 267284 [details] [diff] [review]
proposed fix, v4

dmose: is this safe for the 1.8 branch?

Comment 31

[Magnus Melin [:mkmelin]]

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 32

[Daniel Veditz [:dveditz]]

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.

Comment 35

[cdurmont]

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 ?

Comment 36

[Magnus Melin [:mkmelin]]

Sadly yes, filed bug 1062524.