Last Comment Bug 369910 - Cookies not imported from IE 7
: Cookies not imported from IE 7
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: 2.0 Branch
: x86 Windows Vista
: -- normal (vote)
: Firefox 3 beta1
Assigned To: Jim Mathies [:jimm]
:
Mentors:
Depends on:
Blocks: iemigratewin
  Show dependency treegraph
 
Reported: 2007-02-09 13:54 PST by Kevin Brosnan
Modified: 2013-07-24 03:39 PDT (History)
14 users (show)
mbeltzner: blocking‑firefox3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
cookie import ie7 patch v.1 (2.56 KB, patch)
2007-10-09 11:28 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Review
cookie import ie7 patch v.2 (2.86 KB, patch)
2007-10-15 15:04 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Review
cookie import ie7 patch v.3 (2.82 KB, patch)
2007-10-16 13:03 PDT, Jim Mathies [:jimm]
gavin.sharp: review+
Details | Diff | Review
cookie import ie7 patch v.4 (2.73 KB, patch)
2007-10-17 14:15 PDT, Jim Mathies [:jimm]
no flags Details | Diff | Review

Description Kevin Brosnan 2007-02-09 13:54:45 PST
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.
Comment 1 Carsten Book [:Tomcat] 2007-02-09 14:02:27 PST
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
Comment 2 Daniel Veditz [:dveditz] 2007-03-19 11:49:34 PDT
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
Comment 3 Mike Connor [:mconnor] 2007-08-11 23:37:48 PDT
IE7 Migration -> jmathies
Comment 4 Jim Mathies [:jimm] 2007-10-01 09:13:26 PDT
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. 
Comment 5 Jim Mathies [:jimm] 2007-10-09 11:28:10 PDT
Created attachment 284183 [details] [diff] [review]
cookie import ie7 patch v.1
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-15 11:04:17 PDT
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...
Comment 7 Jim Mathies [:jimm] 2007-10-15 11:32:40 PDT
> 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.
Comment 8 Jim Mathies [:jimm] 2007-10-15 15:04:32 PDT
Created attachment 285008 [details] [diff] [review]
cookie import ie7 patch v.2
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-16 12:03:48 PDT
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.
Comment 10 Jim Mathies [:jimm] 2007-10-16 12:22:23 PDT
>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)
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-16 13:01:00 PDT
(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...
Comment 12 Jim Mathies [:jimm] 2007-10-16 13:03:04 PDT
Created attachment 285128 [details] [diff] [review]
cookie import ie7 patch v.3

updated
Comment 13 Jim Mathies [:jimm] 2007-10-16 13:07:32 PDT
>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.
Comment 14 Mike Connor [:mconnor] 2007-10-16 22:32:51 PDT
can wait till M10 to get this if it doesn't get in.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-17 12:30:30 PDT
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.
Comment 16 Jim Mathies [:jimm] 2007-10-17 14:15:24 PDT
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.
Comment 17 Reed Loden [:reed] (use needinfo?) 2007-10-18 21:25:09 PDT
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
Comment 18 Marcia Knous [:marcia - use ni] 2007-10-24 17:59:33 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.