Read-Only Files Changed to Read-Write

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: David E. Ross, Assigned: Simon Bünzli)

Tracking

Trunk
mozilla1.9alpha8
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070222 SeaMonkey/1.1.1 Mnenhy/0.7.5.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2) Gecko/20070222 SeaMonkey/1.1.1

I intentionally set my cookies.txt file to read-only.  Sites that set cookies cause the browser to change these to read-write.  

Reproducible: Always

Steps to Reproduce:
1.  With the browser closed, locate cookies.txt in a profile.  
2.  Right-click on the file and select Properties in the pull-down context menu.  
3.  On the Properties window, select the Read-only checkbox and then select the OK button.  
4.  Start the browser.  
5.  Start the Cookie Manager.  Count the number of cookies.  
6.  Go to a Web site that is known to set cookies (e.g., the Bugzilla site cited above).  
7.  Do a query to locate a bug report.  
8.  Terminate the browser.  
9.  On the cookies.txt file, right-click and and select Properties in the pull-down context menu.  
10. Restart the browser.  
11. Start the Cookie Manager.  Count the number of cookies.  
Actual Results:  
The read-only checkbox is cleared.  Cookies have been written to the file.  

Expected Results:  
The read-only checkbox should remain checked.  No new cookies should have been written to the file.  

See bug #257288 regarding why some of us want to lock our cookies.txt to read-only.  See especially the following comments in that bug report:  12, 24 (last paragraph), and 27.  

In any case, changing a user's read-only file to read-write without any warning (let alone asking permission) is quite wrong.  

What is happening to me is that I am losing my preferred settings (contained in cookies) whenever I request a non-preferred operation on certain Web sites, including the Bugzilla site.  I have preferred columns and sorting for query reports and other preferred settings.  If I change any of them for a single instance, I must then manually restore them the next time I visit Bugzilla.  This is a loss of data -- cookie data that control settings -- and is thus a critical bug.
why is this not a duplicate of bug 257288?
Component: Networking: File → Networking: Cookies
QA Contact: networking.file → networking.cookies
It is.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 257288
(Reporter)

Comment 3

11 years ago
This is NOT the same as bug 257288.  This bug reports an improper change in the properties of read-only files, to read-write.  This is something new within recent Gecko or UI versions and was not happening as recently as 11 November 2006 (the time of the last comment on this issue in bug 257288).  

Bug 257288 reports that, when a file is read-only, the Cookie Manager writes a new file with the old file's name and a numeric suffix.  However, in that bug (dating back almost three years), the properties of the read-write file were not changed. 

Note that bug 375778 hides the effect of bug 257288 by changing read-only files to read-write.  Since read-only files are not found, the erroneous behavior reported in bug 257288 does not occur.  This must NOT be considered a fix for bug 257288 since bug 375778 makes things worse instead of better.  

Also note that, if no software changes were made and bug 375778 was left as a duplicate of bug 257288, then bug 257288 could be closed.  After all, the error reported in bug 375778 now eliminates the symptoms described in bug 257288 (as explained in the paragraph above).  

Reopening this bug report.  
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Reporter)

Comment 4

11 years ago
From <https://bugzilla.mozilla.org/show_bug.cgi?id=257288#c38>, it appears that this problem does not affect Mac versions.  

Comment 5

11 years ago
Agreed, cookies.txt read-only permissions are disrespected by Firefox 2 on my two Windows XP installations, and it is this bug (<a href="show_bug.cgi?id=375778" title="REOPENED - Read-Only Files Changed to Read-Write">375778</a>) which prevents the conditions that would otherwise trigger the 10,000-cookiefiles bug <a href="show_bug.cgi?id=257288" title="NEW - read-only profile files (cookies, bookmarks, etc) not respected, results in data loss">257288</a>.  Meanwhile, this permission-disrespect bug does NOT occur on my Mac and Windows 2000 installations, thereby allowing the 10,000-cookiefiles bug to occur.

Comment 6

11 years ago
Ugh, let me try that again...

Agreed, cookies.txt read-only permissions are disrespected by Firefox 2 on my two Windows XP installations, and it is this (bug 375778) which prevents the conditions that would otherwise trigger the 10,000-cookiefiles bug 257288.  Meanwhile, this permission-disrespect bug does NOT occur on my Mac and Windows 2000 installations, thereby allowing the 10,000-cookiefiles bug to occur.

Comment 7

11 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) 
Gecko/20070509 Netscape/7.2 (ax) Firefox/2.0.0.4 SeaMonkey/1.1.2

Two problems: cookies.txt stops being Read-only and cookies are not removed at end-of-session.

I allow cookies based on privacy settings and accept for current 
session only, which should delete the cookies when I exit my browser.

network.cookie.cookieBehavior is set to 3.
network.cookie.lifetimePolicy is set to 2.

Additionally, I have my cookies.txt set to Read-only.

Yet, after going to a site in SeaMonkey that sets cookies, I find that my cookies.txt is no longer Read-only and it has accepted cookies and retained them. They are not deleted on exit.

This was not a problem in Mozilla Suite 1.7.13.

Comment 8

11 years ago
-> 1.8 branch since we now use storage on trunk (1.9) and thus have different file writing code.
Version: unspecified → 1.8 Branch
(Assignee)

Comment 9

11 years ago
Created attachment 269507 [details] [diff] [review]
don't proceed with overwriting a file which can't be opened for writing

This bug still affects all consumers of nsISafeOutputStream.
Attachment #269507 - Flags: review?(dveditz)
(Assignee)

Updated

11 years ago
Status: REOPENED → NEW
Component: Networking: Cookies → XPCOM
QA Contact: networking.cookies → xpcom
Version: 1.8 Branch → unspecified
(Assignee)

Updated

11 years ago
Assignee: nobody → zeniko
Severity: critical → major
(Assignee)

Updated

11 years ago
Attachment #269507 - Flags: review?(dveditz) → review?(benjamin)
(Assignee)

Updated

11 years ago
Flags: blocking1.9?

Comment 10

11 years ago
Comment on attachment 269507 [details] [diff] [review]
don't proceed with overwriting a file which can't be opened for writing

I'd like to get a second look at this from jmathies.
Attachment #269507 - Flags: review?(jmathies)
Attachment #269507 - Flags: review?(benjamin)
Attachment #269507 - Flags: review+

Comment 11

11 years ago
Comment on attachment 269507 [details] [diff] [review]
don't proceed with overwriting a file which can't be opened for writing

Certainly an acceptable way of testing write access. Looks like this will only execute when we're trying to move the file, and MoveFileW below it is going to fail anyway if somebody has the file open, so it doesn't look like it'll impact anything. I'm not as familiar with the backend xpcom io stuff as some other folks though, you might want to run this past somebody like Doug Turner.

Related - hasn't cookies.txt been replaced with an sqlite db?
Attachment #269507 - Flags: review?(jmathies) → review+

Comment 12

11 years ago
It has, recently - 

https://bugzilla.mozilla.org/show_bug.cgi?id=230933

So even with this patch, your cookie problem might not go away. We might want to revisit this and see if the database implementation has similar problems. e.g. If it aint broke, don't fix it.

Updated

11 years ago
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
(Assignee)

Comment 13

11 years ago
Thanks for the reviews.

(In reply to comment #12)
> So even with this patch, your cookie problem might not go away.

Indeed, although this is not exclusively about the cookie problem (that's rather bug 257288), but also applies e.g. to sessionstore.js (cf bug 351551) and other consumers of nsISafeOutputStream <http://lxr.mozilla.org/mozilla/search?string=nsISafeOutputStream>.
Keywords: checkin-needed

Comment 14

11 years ago
Comment on attachment 269507 [details] [diff] [review]
don't proceed with overwriting a file which can't be opened for writing

I'd still like Doug to take a look.
Attachment #269507 - Flags: review?(dougt)

Updated

11 years ago
Keywords: checkin-needed

Comment 15

11 years ago
Comment on attachment 269507 [details] [diff] [review]
don't proceed with overwriting a file which can't be opened for writing

maybe we should simply use MoveFileEx now that our windows system requirements have been lifted.  I believe the extra flags will allow us to remove most of this code.

can you also fix the spelling error in the comment block above this where it says "out of the say so".  s/say/way
(Assignee)

Comment 16

11 years ago
Created attachment 276092 [details] [diff] [review]
use MoveFileEx instead

Much simpler now.
Attachment #269507 - Attachment is obsolete: true
Attachment #276092 - Flags: superreview?(benjamin)
Attachment #276092 - Flags: review?(dougt)
Attachment #269507 - Flags: review?(dougt)

Comment 17

11 years ago
Comment on attachment 276092 [details] [diff] [review]
use MoveFileEx instead

I think we are also going to want to pass MOVEFILE_COPY_ALLOWED as an additional flag so that cross volume operations work.  And we may want to consider using MOVEFILE_WRITE_THROUGH so that the operation completes before returning.

In case, we are probably going to want to add unit tests at this point.  Benjamin can advise regarding this point.
Attachment #276092 - Flags: review?(dougt) → review-
(Assignee)

Comment 18

11 years ago
Created attachment 276130 [details] [diff] [review]
update to comment #17

(In reply to comment #17)
> consider using MOVEFILE_WRITE_THROUGH
Yep, especially since this code path is used for nsISafeOutputStream.

> In case, we are probably going to want to add unit tests at this point.
You'll have to look for a different volunteer for these.
Attachment #276092 - Attachment is obsolete: true
Attachment #276130 - Flags: superreview?(benjamin)
Attachment #276130 - Flags: review?(dougt)
Attachment #276092 - Flags: superreview?(benjamin)

Comment 19

11 years ago
Comment on attachment 276130 [details] [diff] [review]
update to comment #17

this looks fine.  I am concerned about regressions, so we should land early and test.

We should also add a few test cases to xpcom/tests/nsIFileTest.cpp for this case.  Sadly, copy and move is only tested on Unix.


nit:  i would have put |destPath.get(),| on its own line.
Attachment #276130 - Flags: review?(dougt) → review+

Updated

11 years ago
Attachment #276130 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed

Comment 20

10 years ago
Comment on attachment 276130 [details] [diff] [review]
update to comment #17

Needs approval before checking-in.
Attachment #276130 - Flags: approval1.9?

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [wanted-1.9] → [wanted-1.9] has patch
Target Milestone: --- → mozilla1.9 M8
Version: unspecified → Trunk
Comment on attachment 276130 [details] [diff] [review]
update to comment #17

Yummy code removal!
Attachment #276130 - Flags: approval1.9? → approval1.9+

Comment 22

10 years ago
dougt, would this be viable for 1.8.1 branch also, or do we still have old windows system requirements there?

Updated

10 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [wanted-1.9] has patch → [wanted-1.9]

Comment 23

10 years ago
Checking in xpcom/io/nsLocalFileWin.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v  <--  nsLocalFileWin.cpp
new revision: 1.174; previous revision: 1.173
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Blocks: 724256
You need to log in before you can comment on or make changes to this bug.