Cookies not imported from IE 7

VERIFIED FIXED in Firefox 3 beta1

Status

()

Firefox
Migration
VERIFIED FIXED
10 years ago
4 years ago

People

(Reporter: Kevin Brosnan, Assigned: jimm)

Tracking

2.0 Branch
Firefox 3 beta1
x86
Windows Vista
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
Go to a site where cookies are used to store data http://arstechnica.com is a good example, use the buttons to change font size and the background to black. Start up Firefox for the first time, go through the migration wizard select IE, browse to arstechnica notice that none of the font or color settings were imported.
(Reporter)

Updated

10 years ago
OS: Windows NT → Windows Vista
confirmed on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/2007020823 Firefox/2.0.0.2 - i see the same as reported on comment #0

Updated

10 years ago
Flags: blocking1.8.1.3?
(Reporter)

Updated

10 years ago
Flags: blocking-firefox3?
Not blocking 1.8 branch, but probably so for FF3. When the fix is done for FF3 we can think about back-porting to the branch.

cc'ing mconnor to find a victim to work on IE7 migration issues
Flags: blocking1.8.1.4?

Updated

10 years ago
Blocks: 381459
Flags: blocking-firefox3? → blocking-firefox3+

Updated

10 years ago
Target Milestone: --- → Firefox 3 beta1

Updated

10 years ago
Target Milestone: Firefox 3 M7 → Firefox 3 M8
IE7 Migration -> jmathies
Assignee: nobody → jmathies
Target Milestone: Firefox 3 M8 → Firefox 3 M9
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

10 years ago
I'm not convinced this is a bug. The reason we don't see IE7's cookies is due to the fact that IE7 runs in protected mode, in which case cookies are stored in a different location from instances running at a medium integrity level. 

C:\Users\jim\AppData\Roaming\Microsoft\Windows\Cookies\ - medium
C:\Users\jim\AppData\Roaming\Microsoft\Windows\Cookies\Low - low

If Firefox was running at the same integrity level, the call in SpecialSystemDirectory.cpp - 

GetWindowsFolder(CSIDL_COOKIES, aFile);

would return the low integrity location. Loading low integrity cookies into a medium integrity app may constitute a security issue. I'd like to get some feedback from the security gurus on this before I add a "Low" import into things. 
(Assignee)

Comment 5

10 years ago
Created attachment 284183 [details] [diff] [review]
cookie import ie7 patch v.1
(Assignee)

Updated

10 years ago
Attachment #284183 - Flags: review?(gavin.sharp)
Comment on attachment 284183 [details] [diff] [review]
cookie import ie7 patch v.1

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

>   // find the cookies directory
>   NS_GetSpecialDirectory(NS_WIN_COOKIES_DIR, getter_AddRefs(cookiesDir));
>-  if (cookiesDir)
>+  if (cookiesDir) {
>+    // Check for Vista's UAC, if so, tack on a "Low" sub dir
>+    nsCOMPtr<nsIWindowsRegKey> regKey =
>+      do_CreateInstance("@mozilla.org/windows-registry-key;1");
>+    if (regKey) {
>+      nsAutoString registryPath;
>+      registryPath.AppendLiteral("Software\\Microsoft\\Windows\\CurrentVersion\\Policies\\System");

Use NS_NAMED_LITERAL_STRING?

>+      if (NS_SUCCEEDED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE,
>+                                    registryPath,
>+                                    nsIWindowsRegKey::ACCESS_QUERY_VALUE))) {
>+        PRUint32 value;
>+        if (NS_SUCCEEDED(regKey->ReadIntValue(NS_LITERAL_STRING("EnableLUA"),
>+          &value)) && value == 1) {

Wrapping is a bit strange... How about something like:
if (NS_SUCCEEDED(regKey->ReadIntValue(NS_LITERAL_STRING("EnableLUA"),
                                      &value)) &&
    value == 1) {

>+          if (path.Length() > 3) {
>+            dir.Append(Substring(path, path.Length()-3));
>+            if (dir.Equals(NS_LITERAL_STRING("Low")) == PR_FALSE) {
>+                cookiesDir->Append(NS_LITERAL_STRING("Low"));
>+            }

Why not just use leafName instead of GetPath+Substring?

Should we be trying to import from both Cookies\ and Cookies\Low? I suppose that'd only be useful if we're "privileged" while LUA is enabled...
(Assignee)

Comment 7

10 years ago
> Use NS_NAMED_LITERAL_STRING?
> Wrapping is a bit strange...
Will do.

> Why not just use leafName..
Ok, didn't realize that would return the ending dir.  

>Should we be trying to import from both Cookies\ and Cookies\Low? I suppose
>that'd only be useful if we're "privileged" while LUA is enabled...

There was a discussion about this. There are some merge issues and really, users aren't going to be switching back and forth between the two states. Most will have it on, some will have it off, so going with what is currently configured made better sense, while also keeping things simple.
(Assignee)

Comment 8

10 years ago
Created attachment 285008 [details] [diff] [review]
cookie import ie7 patch v.2
Attachment #284183 - Attachment is obsolete: true
Attachment #284183 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Attachment #285008 - Flags: review?(gavin.sharp)

Updated

10 years ago
Whiteboard: [has patch][need review gavin]
Comment on attachment 285008 [details] [diff] [review]
cookie import ie7 patch v.2

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

> nsIEProfileMigrator::CopyCookies(PRBool aReplace) 
> {
>   // IE cookies are stored in files named <username>@domain[n].txt
>   // (in <username>'s Cookies folder. isn't the naming redundant?)
>-  PRBool rv = NS_OK;
>+  nsresult rv = NS_OK;

I noticed this earlier, this code is definitely confused! :)

>+  if (cookiesDir) {
>+    // Check for Vista's UAC, if so, tack on a "Low" sub dir
>+    nsCOMPtr<nsIWindowsRegKey> regKey =
>+      do_CreateInstance("@mozilla.org/windows-registry-key;1");
>+    if (regKey) {
>+      nsAutoString registryPath;
>+      NS_NAMED_LITERAL_STRING(regPath,"Software\\Microsoft\\Windows\\CurrentVersion\\Policies\\System");
>+      registryPath.Append(regPath);

You can use regPath directly in the Open() call and remove registryPath entirely. 

>+      if (NS_SUCCEEDED(regKey->Open(nsIWindowsRegKey::ROOT_KEY_LOCAL_MACHINE,
>+                                    registryPath,
>+                                    nsIWindowsRegKey::ACCESS_QUERY_VALUE))) {
>+        nsresult rv;

Redeclaring rv... Intentionally? Makes the code hard to read. Given that the rest of this file doesn't obey the 80 character line limit, might as well just do NS_SUCCEEDED(...->ReadIntValue...) and not worry about line length, or use wrapping from my previous comment.

>+        PRUint32 value;
>+        rv = regKey->ReadIntValue(NS_LITERAL_STRING("EnableLUA"), &value);
>+        if (NS_SUCCEEDED(rv) && value == 1) {
>+          nsAutoString dir;
>+          // For cases where we are running under protected mode, check
>+          // cookiesDir for the Low sub directory. (Simpler than using
>+          // process token calls.)
>+          cookiesDir->GetLeafName(dir);
>+          if (dir.Equals(NS_LITERAL_STRING("Low")) == PR_FALSE) {

I'm not sure I understand... When would dir be equal to "Low"? I think you should clone cookiesDir, append "Low" unconditionally, then check Exists() to determine whether to use the clone or the original cookiesDir.

>+              cookiesDir->Append(NS_LITERAL_STRING("Low"));
>+          }

Nit: no brackets around single-line then-clause.
(Assignee)

Comment 10

10 years ago
>You can use regPath directly in the Open() call and remove registryPath
>entirely.

doh!

>Redeclaring rv... Intentionally?

I didn't want to mess with it's default value. I'll remove it.

>I'm not sure I understand... When would dir be equal to "Low"?

The point here is that cookiesDir may already point to the Low subdir, if and when Firefox, or some derivative is running at low integrity. The system will return the appropriate cookies dir depending on the integrity of the app that makes the call in the special system directories code. This is basically a safety catch, insuring that when and if we implement protected mode, the code doesn't break. If we just check the registry flag and tack on Low as a result, we would end up with .../Low/Low. Maybe there's a better way to do it but essentially this patch follows the logic:

check the protected mode flag
-> True (ie7 cookies are in Low)
   get the cookies dir we are given, ends with Low? 
   -> True (we are running at low integrity, path is good)
   -> False (tack on Low sub dir to get ie7's cookies)
(In reply to comment #10)
> The point here is that cookiesDir may already point to the Low subdir, if and
> when Firefox, or some derivative is running at low integrity.

Ah, right, of course. I wish there was a better way to detect that than to do a string compare on the directory name we get back...
(Assignee)

Comment 12

10 years ago
Created attachment 285128 [details] [diff] [review]
cookie import ie7 patch v.3

updated
Attachment #285008 - Attachment is obsolete: true
Attachment #285128 - Flags: review?(gavin.sharp)
Attachment #285008 - Flags: review?(gavin.sharp)
(Assignee)

Comment 13

10 years ago
>Ah, right, of course. I wish there was a better way to detect that than to do a
>string compare on the directory name we get back...

Well, there is, but it's complex process token type stuff. Probably 10-15 lines of code. afaict, just checking Low is going to work ok too. I'm not aware of a way to change the cookies location. There might be some obscure roaming profiles related thing that breaks this, but I would think we could address that down the road when and if it arises.
can wait till M10 to get this if it doesn't get in.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Comment on attachment 285128 [details] [diff] [review]
cookie import ie7 patch v.3

>Index: browser/components/migration/src/nsIEProfileMigrator.cpp

> nsIEProfileMigrator::CopyCookies(PRBool aReplace) 

>+            if (dir.Equals(NS_LITERAL_STRING("Low")) == PR_FALSE)

nit: if (!dir.EqualsLiteral("Low"))

>+  if (cookiesDir) {

...

>+    if (!cookieFiles)
>+      return NS_ERROR_FAILURE;
>+  }

Just noticed this: leaving this inside the "if (cookiesDir)" block will result in a null deref of cookieFiles later on in the function if getting cookiesDir somehow fails. Just leave it outside the block like it was before.

r=me with those two comments addressed.
Attachment #285128 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need review gavin] → [needs new patch]
(Assignee)

Comment 16

10 years ago
Created attachment 285271 [details] [diff] [review]
cookie import ie7 patch v.4

ok, nits addressed, recompiled and tested one more time to be sure everything's correct.
Attachment #285128 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs new patch]
Target Milestone: Firefox 3 M10 → Firefox 3 M9
Checking in browser/components/migration/src/nsIEProfileMigrator.cpp;
/cvsroot/mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp,v  <--  nsIEProfileMigrator.cpp
new revision: 1.66; previous revision: 1.65
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102407 Minefield/3.0a9pre. I tested by clearing my Firefox cookies and then building up a cookie history in IE, and then importing.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.