1182347 - Eliminate nsIPrincipal::cookieJar

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

Jonas and I had a call about this today. The conclusion was that the current design of .cookieJar is too complicated, and we should get rid of it. Rationale is as follows:

The idea behind .cookieJar was to have every piece of private data in the system be associated with a key, such that we can efficiently clear all private data associated with that key. However, it has two downsides:

(1) It requires extra complication in the database schema of every consumer that stores private data. In order for this to be efficient, the database needs an extra column to store the cookieJar value.

(2) It is very inflexible:
* We can only wipe data for things that are encoded in .cookieJar. New OriginAttributes don't have this capability by default.
* To add this capability for a new OriginAttribute, we'd need to change the format of .cookieJar, which potentially involves adding migrations for _every single_ consumer that uses .cookieJar as a key.
* It is difficult to clear data based on a subset of the attributes encoded in .cookieJar. In the current world, clearing all data associated with a given app involves sending two notifications: one with inBrowser=true and the other with inBrowser=false. This kinda-sorta works, but only because there are only two possible values for inBrowser. For any other data types that we add (strings, ints, etc), this doens't work.

The idea behind the new scheme is to introduce a new dictionary class called OriginAttributesPattern, which has the same members as OriginAttributes, except that each entry is optional. To clear private data, callers send a JSON-ified object containing the attributes that they want to match - each attribute makes the operation more restrictive. Sending {appId: 42} clears all data associated with app 42, whereas sending {appId: 42, inBrowser: true} clears all data that is associated with app 42 _and also_ is in a mozBrowser.

This requires consumers to parse the originAttributes for each origin in the database, which could be slow - but we think this operation is infrequent enough that we should be optimizing for simplicity and flexibility, not speed. For the cases where speed matters, the consumer can add extra columns to the database for different originAttributes, and detect when all the attributes in the pattern are available as columns - in that case, it can use a fast-path that matches the results in a single query.

I'm attaching the patches that I have - please let me know what you think. Yoshi, I'm really sorry for switching up the design on you like this, but hopefully this will make it easier and simpler going forward.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Implement OriginAttributesPattern. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Migrate existing code away from .cookieJar. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove nsIPrincipal::cookieJar. v1

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d08b038cf5

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8631905 [details] [diff] [review]
Part 1 - Implement OriginAttributesPattern. v1

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

::: dom/webidl/ChromeUtils.webidl
@@ +50,5 @@
> + * (2) Update the methods on mozilla::OriginAttributes, including equality,
> + *     serialization, and deserialization.
> + * (3) Update the methods on mozilla::OriginAttributesPattern, including matching.
> + * (4) Bump the CIDs (_not_ IIDs) of all the principal implementations that
> + *     use OriginAttributes in their nsISerializable implementations.

I'm curious to know why updating the CID is important. But that's not new so not affecting this review.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm curious to know why updating the CID is important.

Because otherwise nsISerializable might create an object that doesn't match the stored data and try writing said data into it.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8631906 [details] [diff] [review]
Part 2 - Migrate existing code away from .cookieJar. v1

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

looks good otherwise, but I think it'd be nice to not have to run a full JSON parser to handle this notification?

::: dom/apps/Webapps.jsm
@@ +4862,2 @@
>      }
> +    this._notifyCategoryAndObservers(null, "clear-origin-data", JSON.stringify(attributes));

So this still will require a bunch of C++ callsites to do JSON parsing of the attributes, no?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> So this still will require a bunch of C++ callsites to do JSON parsing of
> the attributes, no?

As noted in the comments, it's implicit in the bindings machinery. The callers just do:

OriginAttributesPattern pattern;
if (!pattern.Init(aJSONString)) {
  ... // handle failure.
}

So the API is super convenient and safe, and it's just a question of overhead, which doesn't really worry me at all. The alternative would be to make some sort of nsIOriginAttributesPatternWrapper, which could hold onto the underlying object and expose the ability to access it as a jsval or access it as a C++ object. What we have here seems like a much simpler and cleaner solution.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8631906 [details] [diff] [review]
Part 2 - Migrate existing code away from .cookieJar. v1

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

Ah, I missed that comment.

Could you move the comment to be directly above the class and also include the code example. Since the .Init function lives entirely in generated code, it's quite hard to figure out how to use it, or that it exists.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8631905 [details] [diff] [review]
Part 1 - Implement OriginAttributesPattern. v1

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

::: dom/webidl/ChromeUtils.webidl
@@ +28,5 @@
>    originAttributesToSuffix(optional OriginAttributesDictionary originAttrs);
> +
> +  /**
> +   * Returns true if the members of |originAttrs| match the provided members
> +   * members of |pattern|.

There are two occurrences of 'members' here.

Comment 11

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11a6611adb36

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/696c6303dcd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea814efe34e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2995b5458fa

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/696c6303dcd6
https://hg.mozilla.org/mozilla-central/rev/ea814efe34e9
https://hg.mozilla.org/mozilla-central/rev/a2995b5458fa