1189070 - Migration to Firefox 42 wipes out some cookie permissions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150514102509

Steps to reproduce:

1. Create a profile containing various cookie exceptions.  I used ESR 38 and the list included some IP addresses and single-word hosts such as "localhost".
2. Open the profile in nightly (42 branch).
3. Examine the cookie exceptions list.



Actual results:

Most of the exceptions were still present, variously prefixed with (http:// or https:// (or both, is that such a good idea?).  Some were missing, IP address and localhost that I noticed.  Full path file:// exceptions were migrated as is.


Expected results:

All the hosts (possibly not <file>) should be migrated to the new list.

Additionally, certain hosts that have been accessed with a non-standard port should be specifically listed with that port.  This shouldn't be necessary since cookies are port-agnostic, but the new permission scheme thinks cookie permissions should be port-specific.

Comment 1

[(no longer active)]

(In reply to Ian Nartowicz from comment #0)
> Most of the exceptions were still present, variously prefixed with (http://
> or https:// (or both, is that such a good idea?).  Some were missing, IP
> address and localhost that I noticed.  Full path file:// exceptions were
> migrated as is.

Ian, can you please give a specific example of an entry that wasn't migrated correctly?  Also, would you be willing to share your permissions and places database (that contains your browsing history) with us off Bugzilla for debugging purposes?

> Additionally, certain hosts that have been accessed with a non-standard port
> should be specifically listed with that port.  This shouldn't be necessary
> since cookies are port-agnostic, but the new permission scheme thinks cookie
> permissions should be port-specific.

Is there something that breaks because of this?

Comment 2

[Ian Nartowicz]

Some hosts that were not migrated at all:
localhost
192.168.0.2
127.0.0.1

What breaks is anything that needs to set or read cookies for hosts that aren't migrated.  In my case that is an addon that communicates with an external application using XHR requests, also a web interface to the same application.

Comment 3

[Ian Nartowicz]

I'm not comfortable submitting my day-to-day profile, but I just tried this with a fresh profile.  No history, just two exceptions (127.0.0.1 and localhost, both allow), and neither was migrated.

No ports and no history involved here.  I originally thought the failure was to migrate and add the ports since these hosts are used with non-standard ports, but they aren't migrated at all.  The port migration isn't ideal though, because these hosts may not be in history if they are only accessed through javascript.  That's getting to be a bit of an edge case, although it is frustrating to users of my addon.

Comment 4

[(no longer active)]

Thanks for the info, Ian!  We realize that the port migration is not ideal, but we believe this is the best algorithm we could come up with, and we do realize that this will lose information for hosts with non-standard ports that do not appear in the user's history.  There is really not a better way to solve this though.

Michael, do you mind investigating next week, please?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

The problem appears, from my quick analysis, to be that we assume that the tldService won't fail in this call:

  rv = tldService->GetBaseDomainFromHost(aHost, 0, eTLD1);

Right now we NS_ENSURE_SUCCESS(rv, rv); right after that call, which is apparently wrong for URIs like localhost and IP addresses. We will have to change the migration code to correctly handle the case where this call fails, and then re-migrate users permissions if the fall fails.

Comment 6

[Ian Nartowicz]

getBaseDomain throws NS_ERROR_HOST_IS_IP_ADDRESS for IP addresses, and unqualified hostnames throw NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Don't discard localhost or IP address entries from the permissions database when migrating

This patch should re-migrate the localhost/IP address entries of people who already have migrated using the v7 migration. It should also fix the migration for everyone in the future.

I'm not sure if this is the most elegant or best way to handle it either.

Comment 8

[(no longer active)]

Comment on attachment 8641758 [details] [diff] [review]
Don't discard localhost or IP address entries from the permissions database when migrating

Review of attachment 8641758 [details] [diff] [review]:
-----------------------------------------------------------------

For the sake of completeness, please add some tests to cover IPv6 host names as well.

I'd appreciate if you would submit an interdiff for the next iteration.  Thanks!

::: extensions/cookie/nsPermissionManager.cpp
@@ +284,5 @@
>    nsPermissionManager::DBOperationType mOperation;
>    int64_t mID;
>  };
>  
> +class UpgradeIPHostToOriginDB final : public UpgradeHostToOriginHelper {

Please mark this and the other similar classes as MOZ_STACK_CLASS, otherwise the mID pointer for example is unsafe.

@@ +1233,5 @@
> +    // The version 7-8 migration is the re-migration of localhost and ip-address
> +    // entries due to errors in the previous version 7 migration which caused
> +    // localhost and ip-address entries to be incorrectly discarded.
> +    // The version 7 migration logic has been corrected, and thus this logic only
> +    // needs to execute if the user is currently on version 7.

Hmm, I'm not sure if I understand what guarantees that this will only execute for v7.  Can you elaborate, please?

@@ +1246,5 @@
> +        bool hostsIsBackupExists = false;
> +        mDBConn->TableExists(NS_LITERAL_CSTRING("moz_hosts_is_backup"),
> +                             &hostsIsBackupExists);
> +        if (dbSchemaVersion == 7 && hostsIsBackupExists) {
> +          bool migrationError;

You forgot to initialize this to false!

@@ +1272,5 @@
> +            if (newId > id) {
> +              id = newId;
> +            }
> +          }
> +          id++; // Move past the last id

Are you just trying to get the maximum id?  You can use this query instead: "SELECT MAX(id) FROM moz_hosts".

@@ +1310,5 @@
> +            rv = tldService->GetBaseDomainFromHost(host, 0, eTLD1);
> +            if (NS_SUCCEEDED(rv)) {
> +              // We only care about entries which the tldService can't handle
> +              continue;
> +            }

Can you move this up above right after getting the host?  That way, if this fails, we won't waste time reading out the right of the query fields.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I've done most of what you asked for, but there was a problem with ipv6 tests - it appears as though the origin code doesn't handle them correctly (see bug 1192666), so a fix on this is going to have to block until that is fixed.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Don't discard localhost or IP address entries from the permissions database when migrating

Updated version with new try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c1775495f3

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Interdiff

Comment 12

[(no longer active)]

Please nominate for Aurora 42 when this lands.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab28b01bb21

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8645504 [details] [diff] [review]
Don't discard localhost or IP address entries from the permissions database when migrating

Approval Request Comment
[Feature/regressing bug #]: bug 1186034
[User impact if declined]: Permissions for IP addresses or special hosts (like localhost) will not be migrated in the permissions manager.
[Describe test coverage new/current, TreeHerder]: There is a test present for the new migration (included in this patch), and the permissions manager component is relatively well tested.
[Risks and why]: This patch is not super small, and has not been on nightly for very long, so it could cause other breakage in the permission manager which we haven't forseen yet.
[String/UUID change made/needed]: None

NOTE: This depends on bug 1192666 which must be uplifted before this patch can be uplifted.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ab28b01bb21

Comment 16

[jen strickland :jenstrickland]

Are you certain this was introduced in 42? Since it's listed as a regression, would you verify and set the status flag for 42 accordingly, please?

This is a significant enough issue to expected user behavior, I tracked for 42 and 43.

Comment 17

[Nika Layzell [:nika] (ni? for response)]

(In reply to jen strickland :jenstrickland from comment #16)
> Are you certain this was introduced in 42? Since it's listed as a
> regression, would you verify and set the status flag for 42 accordingly,
> please?
> 
> This is a significant enough issue to expected user behavior, I tracked for
> 42 and 43.

I'm very confident that this was introduced by bug 1186034, which is in 42. The problems for ipv6 addresses (fixed in bug 1192666), specifically as they affect the permissions manager, were introduced in bug 1165263, when the permission manager started to use origins. This was also introduced in 42.

Comment 18

[(no longer active)]

Yes, this was introduced in 42.  It would be *really nice* to get this backported before the first Aurora builds go out.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

passing ni on to Sylvestre

Ehasn - We're scheduled to re-enabled Aurora updates today. Do you think this needs to be fixed before we re-enable updates? Will it be OK to fix this for tomorrow's Aurora update?

Comment 20

[Sylvestre Ledru [:Sylvestre]]

We are ready for aurora, we just had the sign off, we have a few blog post today, the patch is risky, I don't think we are going to delay it for this bug.
But taking it now.

Comment 21

[(no longer active)]

OK, I guess it's better late than never!  Thanks.

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Just to clarify, bug 1192666 must be uplifted before this can be uplifted.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

thanks!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/947b74a5bef4