Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create explainer.md #16

Merged
merged 10 commits into from
Jul 27, 2016
Merged

Create explainer.md #16

merged 10 commits into from
Jul 27, 2016

Conversation

bsittler
Copy link
Contributor

Long overdue, this is an attempt to describe the API and its reasons for existing

Long overdue, this is an attempt to describe the API and its reasons for existing
@bsittler
Copy link
Contributor Author

What do you think, @esprehn @domenic @mkruisselbrink @dmurph @annevk

@domenic
Copy link

domenic commented Jul 15, 2016

This looks pretty good to me. My notes while reading:

  • All the opinions seem really nice.
  • I wonder what cookieList is. Hopefully just an array.
  • I'm not sure string-or-regexp matching is a good thing to build in. I'm not sure why we would anticipate the browser being better at determining a subset of cookies the developer is interested in than the developer is.
  • put vs. set, hmm. It would be good to explain why "put" instead of the more common "set".
  • I wonder if we want to restrict this API to secure origins? Then secure is not necessary
  • Why "expiration" instead of "expires"?
  • I assume that expiration's type is string or number, with dates being converted to numbers. Accepting only dates would be a bad API (e.g. for people using moment.js or Date.now()).
  • It's interesting that you can set httpOnly cookies from JS but not read them. Not a problem, just interesting.
  • Why not build a convenience method for deleting cookies? Setting it to a zero-date expiration time is very obtuse.
  • The observe API seems borked. .get() returns a promise, which does not have a .observe() method. I think you want cookieStore.observe('COOKIENAME', cookie => ...)? This totally breaks the "More generally: any ... operation can be used with .observe in place of .then".
  • What does "coalesce changes" mean. User-agent-defined intervals? Microtask timing?
  • Where did this totally separate service worker API at the bottom come from?? Why are we now using document in a service worker?? Why do you have to register interest in cookies? (I have a guess on that last one but it's something an explainer should explain...)

@bsittler
Copy link
Contributor Author

bsittler commented Jul 15, 2016

Thanks for taking a look at this, @domenic! Responses inline below:

This looks pretty good to me. My notes while reading:

  • All the opinions seem really nice.
  • I wonder what cookieList is. Hopefully just an array.

Yes, an array of cookie objects like {name: '...', value: '...'}, reflecting only those cookie fields visible in Cookie: headers or String(document.cookie).

  • I'm not sure string-or-regexp matching is a good thing to build in. I'm not sure why we would anticipate the browser being better at determining a subset of cookies the developer is interested in than the developer is.

The idea here is mostly to avoid firing up a service worker or JS handler when a cookie it doesn't care about changes.

  • put vs. set, hmm. It would be good to explain why "put" instead of the more common "set".

I'm not completely decided on this myself. On the one hand, the HTTP header is Set-Cookie; on the other hand, the effective keys for this operation are odd, including domain, name, and path, of which only name will be supplied in many cases, and the "get" operation only reveals the name and value, not the rest of the key, and I'm concerned that calling it "set" will mislead developers into thinking that a cookie name/value they "get" can then be updated via "set" with the same name and a different value, whereas in fact they also need to correctly determine the hidden parts of the key too.

  • I wonder if we want to restrict this API to secure origins? Then secure is not necessary

I don't see this as a powerful feature outside of ServiceWorker.

I believe there are separate efforts underway to protect secure origins from cookie attacks carried out on unsecured origins in the same domain, including @mikewest's https://tools.ietf.org/html/draft-ietf-httpbis-cookie-prefixes-00 and https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-00

I think you still need to be able to read and write non-"Secure"-flagged cookies (e.g. domain-wide UI preference cookies) from a ServiceWorker to interoperate with the existing web, and I also think this API is mainly improving convenience without introducing any security-relevant new powers (other than JS cookie access from SW, which is already secure-origin-only), and so the benefit from allowing all web content content (regardless of origin) to migrate to the new API likely outweighs any downside.

  • Why "expiration" instead of "expires"?

Not entirely sure about this one either, but I think "expires" (despite being the parameter name used in Set-Cookie) wrongly suggests that its value is boolean, whereas in fact it is a nullable timestamp with behavior side-effects (Session vs. non-Session lifetime). I was even thinking of allowing the special value 'session' here to indicate the non-timestamp behavior. What do you think?

  • I assume that expiration's type is string or number, with dates being converted to numbers. Accepting only dates would be a bad API (e.g. for people using moment.js or Date.now()).

Agreed. I'll add clarifying text and/or examples to make this more obvious.

  • It's interesting that you can set httpOnly cookies from JS but not read them. Not a problem, just interesting.

Agreed. I think it's useful primarily when the value encodes a capability which is not usable directly from script, e.g. an OAuth 2 authorization code - in that case because the script lacks the client certificate/client secret needed to redeem the code for a refresh token, access token and/or other bearer tokens due to the script running outside the server-side application's trust perimeter.

  • Why not build a convenience method for deleting cookies? Setting it to a zero-date expiration time is very obtuse.

I was somewhat reluctant to do this due to the same multipart-key concerns that led to my slightly preferring "put" to "set". However, I'm not deeply attached to this and can add sugar for this if you think it will make a noticeable usability improvement.

  • The observe API seems borked. .get() returns a promise, which does not have a .observe() method. I think you want cookieStore.observe('COOKIENAME', cookie => ...)? This totally breaks the "More generally: any ... operation can be used with .observe in place of .then".

Agreed, this is broken. After some offline discussion with @dmurph I'm leaning toward ripping out and redoing the observe API (and adding unobserve) based on newer versions of the IndexedDB observer API.

  • What does "coalesce changes" mean. User-agent-defined intervals? Microtask timing?

Good point. I think I meant both:

  • You won't see more than one state for the same domain/path/name triple in the same callback

and

  • Multiple consecutive changes which would all trigger the same callback may instead trigger the callback only once, with the timing of the callback being user agent-defined and with no guarantees that there won't be a delay between the time the cookie change occurs and the time the callback is invoked.

This is intended to allow e.g. background tab power-saving modes where cookie change notifications occur only periodically (e.g. every ten minutes) or are temporarily suspended (e.g. due to power-saving modes) and only later resumed.

This is also intended to offer no better guarantees about change notification than you would get today using String(document.cookie) inside setInterval.

  • Where did this totally separate service worker API at the bottom come from?? Why are we now using document in a service worker?? Why do you have to register interest in cookies? (I have a guess on that last one but it's something an explainer should explain...)

The "document." is a typo, I'll delete that. Thanks for spotting it! I'll fix that and also add an explanation. Summary: registering interest is needed so that .observe(...) will be side-effect-free and so that events that will cause a stopped ServiceWorker to be started are all very explicitly under the control of the ServiceWorker APIs without strange side effect-wakeups due to use of unrelated APIs.

Edit: clicked Comment too soon! Finished the dangling partial sentences

@domenic
Copy link

domenic commented Jul 15, 2016

The idea here is mostly to avoid firing up a service worker or JS handler when a cookie it doesn't care about changes.

This probably ties in to my other question about the service worker API. I would restrict this kind of matching to the service worker specific "interest registration" API, and not have coookieStore.match/matchAll.

I don't see this as a powerful feature outside of ServiceWorker.

My guess is that you'd find plenty of people on various browser security teams who disagree with you; in many senses, cookies are the original powerful feature. It seems very clear that we would be restricting cookies to secure origins if they were created today. Perpetuating that mistake in new APIs seems bad. I think it would be good to consult with Chrome Security (e.g. @mikewest) on this.

Not entirely sure about this one either, but I think "expires" (despite being the parameter name used in Set-Cookie) wrongly suggests that its value is boolean, whereas in fact it is a nullable timestamp with behavior side-effects (Session vs. non-Session lifetime). I was even thinking of allowing the special value 'session' here to indicate the non-timestamp behavior. What do you think?

I don't feel strongly; I just thought it was weird to depart from well-known cookie terminology. expires doesn't sound boolean to me. Maybe we should see how others feel.

This is also intended to offer no better guarantees about change notification than you would get today using String(document.cookie) inside setInterval.

Sure, but setInterval of how many milliseconds? :)

It sounds like the intent here is to not offer any guarantees. I'm a bit worried that will lead people to polling in order to get better guarantees. But maybe it's fine. In any case, it'd be good to be more explicit in the explainer, like you were in your second bullet.

Summary: registering interest is needed so that .observe(...) will be side-effect-free and so that events that will cause a stopped ServiceWorker to be started are all very explicitly under the control of the ServiceWorker APIs without strange side effect-wakeups due to use of unrelated APIs.

The weird part about this API is that it causes an one API (cookieStore.observe, or whatever it ends up being) to be totally broken until some "unrelated" API (event.registerCookieInterest) is called.

I'm not sure there is a good fix; you either have to have this action at a distance, or you have to give the ability to wake up a service worker to an API that is not service worker specific. I tried to look into how the push API does this, and came away a bit confused; it doesn't seem to use InstallEvent, but it is of course very tied to service workers. In that case I guess assigning self.onpush = is what causes a service worker to wake up periodically---more event listeners that cause side effects, ugh.

@inexorabletash
Copy link
Member

A few notes:

  • Matching on RegExp is going to require detailed spec and implementation work.
    • I assume RegExp.$1 (etc) would not be modified, nor would lastIndex of the passed regex instance,
    • The matching would be done by a separate regex engine that would need to match ECMAScript semantics (does V8 expose it? probably not, I assume they compile the regex onto their heap...)
    • Are there any other platform APIs that take regexps?
  • Having match()/matchAll() do prefix-match on strings default is unexpected. Maybe we need Prefix in the names?
  • You may want to convert API examples from using .then(...) to await
  • Agreed that the API should not accept Date instances; all DOM APIs have moved to DOMTimeStamp, i.e. a number like a UNIX epoch.
  • Requiring deletion by setting expiration: 0 seems terrible in a new API. Even if it's defined under the hood that way, we should not make developers understand that.
  • "Other write failures ... will fail silently: the promise will resolve but the cookie will not be set." - this seems... surprising. Is there a reason we can't report failures for all of the listed cases? (size limits, out-of-range date, different eTLD+1) - since the failure is async, why would we not reject on these?
  • "change" as the event name (for ephemeral contexts) seems a bit generic.

@bsittler
Copy link
Contributor Author

Thanks for taking a look at this, @inexorabletash - responses inline below:

A few notes:

  • Matching on RegExp is going to require detailed spec and implementation work.
    • I assume RegExp.$1 (etc) would not be modified, nor would lastIndex of the passed regex instance,

Agreed. Probably this should either be modified to take a RegExp pattern as a string, or to get rid of regexp matching entirely.

  • The matching would be done by a separate regex engine that would need to match ECMAScript semantics (does V8 expose it? probably not, I assume they compile the regex onto their heap...)

Yes, I don't actually like the regular expressions here at all.

  • Are there any other platform APIs that take regexps?

Not that I'm aware of. Fetch interception is prefix-based, postMessage is '*' (by itself) or exact match, most others are exact-match.

  • Having match()/matchAll() do prefix-match on strings default is unexpected. Maybe we need Prefix in the names?

Agreed. Indeed, cookie names with well-known prefixes is really the only special case I care about here. Maybe we can start with (String or Array). At that point I'm inclined to remove match/matchAll entirely and switch to using "get"/"getAll" with a new optional "matchPrefix" boolean in the optional options bag. What do you think about that?

  • You may want to convert API examples from using .then(...) to await

I like it :)

  • Agreed that the API should not accept Date instances; all DOM APIs have moved to DOMTimeStamp, i.e. a number like a UNIX epoch.

I agree with this. Should we also disallow HTTP-compatible date strings, though?

  • Requiring deletion by setting expiration: 0 seems terrible in a new API. Even if it's defined under the hood that way, we should not make developers understand that.

Agreed, I will add sugar for this.

  • "Other write failures ... will fail silently: the promise will resolve but the cookie will not be set." - this seems... surprising. Is there a reason we can't report failures for all of the listed cases? (size limits, out-of-range date, different eTLD+1) - since the failure is async, why would we not reject on these?

I'm hoping that (at least in document contexts) this API can be polyfilled. My hope is to not force rejection on eTLD+1 because it's not something that is easily implemented in a polyfill.

  • "change" as the event name (for ephemeral contexts) seems a bit generic.

Would moving this to a CookieObserver instance make it any more palatable?

@esprehn
Copy link

esprehn commented Jul 15, 2016

cookieStore.get('COOKIENAME').observe(cookie => console.log(
  cookie ?
    ('New value: ' + cookie.value) :
    'No longer set or no longer visible to script'))

This is pretty strange, it's using a subclass of Promise here so you have then() and observe().

I think we should follow the observer pattern that the rest of the platform is using (ResizeObserver, IntersectionObserver, MutationObserver, IDBObserver)

so you'd do:

var observer = new CookieObserver(function(changeRecords) {
  // changeRecords has information about what cookies changed.
});
observer.observe(...patterns here...);

@inexorabletash
Copy link
Member

Should we also disallow HTTP-compatible date strings, though?

I think I lean towards allowing them, but we need to specify where the parsing occurs, and what about error cases. Part of that may involve comparing HTTP date parsing vs. ECMAScript date parsing (which may be impl dependent???) and see how well they align/diverge.

My hope is to not force rejection on eTLD+1 because it's not something that is easily implemented in a polyfill.

I don't think we should constrain ourselves to what's polyfillable here, if it improves the behavior of the API. Would developers be likely to rely on that behavior, i.e. is it useful/very common to probe that failure case? Or is it more commonly a bug?

Would moving this to a CookieObserver instance make it any more palatable?

Ah, yes it would. The explainer has document.cookieStore.addEventListener('change', ... and I missed that it wasn't simply on self. But also: shouldn't be document

@bsittler
Copy link
Contributor Author

Follow-up to @domenic's follow-up inlined:

The idea here is mostly to avoid firing up a service worker or JS handler when a cookie it doesn't care about changes.

This probably ties in to my other question about the service worker API. I would restrict this kind of matching to the service worker specific "interest registration" API, and not have coookieStore.match/matchAll.

I'm also hoping that a developer can avoid needing to handle changes to cookies they do not care about. Would it feel any better if match/matchAll were gone and there were a prefix-matching option and an array-of-names option for get/getAll instead?

I don't see this as a powerful feature outside of ServiceWorker.

My guess is that you'd find plenty of people on various browser security teams who disagree with you; in many senses, cookies are the original powerful feature. It seems very clear that we would be restricting cookies to secure origins if they were created today. Perpetuating that mistake in new APIs seems bad. I think it would be good to consult with Chrome Security (e.g. @mikewest) on this.

Is the idea that by making a cleaner cookie API available in unsecured contexts we would be inadvertently encouraging broader use of cookies in those contexts? In any case, I agree that this is worth discussing further.

Not entirely sure about this one either, but I think "expires" (despite being the parameter name used in Set-Cookie) wrongly suggests that its value is boolean, whereas in fact it is a nullable timestamp with behavior side-effects (Session vs. non-Session lifetime). I was even thinking of allowing the special value 'session' here to indicate the non-timestamp behavior. What do you think?

I don't feel strongly; I just thought it was weird to depart from well-known cookie terminology. expires doesn't sound boolean to me. Maybe we should see how others feel.

Ok, and this is certainly API feedback I'd like to have more of :)

This is also intended to offer no better guarantees about change notification than you would get today using String(document.cookie) inside setInterval.

Sure, but setInterval of how many milliseconds? :)

I think the requested interval does not matter when the timers are sometimes suspended indefinitely (so long as they don't get fresh focus) in background tabs in existing browsers. My intent is to not require the browser to spend more CPU time on those tabs than it already does, and ideally much less (by eliminating the attempted polling which is harmful in foreground tabs but may already be ineffective already in tabs without focus or perhaps in out-of-view IFRAMEs.)

It sounds like the intent here is to not offer any guarantees. I'm a bit worried that will lead people to polling in order to get better guarantees. But maybe it's fine. In any case, it'd be good to be more explicit in the explainer, like you were in your second bullet.

I agree, and I'll try to be more explicit about it. I think the guarantee should be something like "no worse than a 5-second polling check using setInterval in a top-level tab with UI focus or its IFRAMEs that are currently visible or absolutely positioned offscreen (i.e., hidden service IFRAMEs which may still be ) or a ServiceWorker processing requests on behalf of such a context, ditto but 10 minutes for other contexts". What do you think? Is that too explicit? (I suspect so...) In any case I think those are the sorts of polling loops I'm aiming to replace, except that often they are not sophisticated enough to throttle themselves.

Summary: registering interest is needed so that .observe(...) will be side-effect-free and so that events that will cause a stopped ServiceWorker to be started are all very explicitly under the control of the ServiceWorker APIs without strange side effect-wakeups due to use of unrelated APIs.

The weird part about this API is that it causes an one API (cookieStore.observe, or whatever it ends up being) to be totally broken until some "unrelated" API (event.registerCookieInterest) is called.

It's not entirely broken: it works fine for the remaining duration of the script execution context (and stops working after that, just like every other side-effect-free event handler registration.)

I'm not sure there is a good fix; you either have to have this action at a distance, or you have to give the ability to wake up a service worker to an API that is not service worker specific. I tried to look into how the push API does this, and came away a bit confused; it doesn't seem to use InstallEvent, but it is of course very tied to service workers. In that case I guess assigning self.onpush = is what causes a service worker to wake up periodically---more event listeners that cause side effects, ugh.

Right, that was precisely the sort of weirdness I was hoping to avoid a repetition of. If you have any other suggestions on how to accomplish this, I'm definitely interested!

@bsittler
Copy link
Contributor Author

Follow-up to @inexorabletash's follow-up inlined:

Should we also disallow HTTP-compatible date strings, though?

I think I lean towards allowing them, but we need to specify where the parsing occurs, and what about error cases. Part of that may involve comparing HTTP date parsing vs. ECMAScript date parsing (which may be impl dependent???) and see how well they align/diverge.

On further thought I think we could implement this sanely. So rejecting in at least most such cases should not be a problem.

My hope is to not force rejection on eTLD+1 because it's not something that is easily implemented in a polyfill.

I don't think we should constrain ourselves to what's polyfillable here, if it improves the behavior of the API. Would developers be likely to rely on that behavior, i.e. is it useful/very common to probe that failure case? Or is it more commonly a bug?

I suspect successful code rarely or never hits this case. I'm not sure there's anything particularly useful a developer can do in this case. However, I think we already indirectly expose the eTLD+1 restrictions through the unrelated document.domain feature, so I retract what I said earlier about this being difficult for a polyfill. I'll remove the caveat.

Would moving this to a CookieObserver instance make it any more palatable?

Ah, yes it would. The explainer has document.cookieStore.addEventListener('change', ... and I missed that it wasn't simply on self. But also: shouldn't be document

Agreed, that is a typo.

@bsittler
Copy link
Contributor Author

Thanks for taking a look at this, @esprehn! I'm inlining my responses below:

cookieStore.get('COOKIENAME').observe(cookie => console.log(
  cookie ?
    ('New value: ' + cookie.value) :
    'No longer set or no longer visible to script'))

This is pretty strange, it's using a subclass of Promise here so you have then() and observe().

I think we should follow the observer pattern that the rest of the platform is using (ResizeObserver, IntersectionObserver, MutationObserver, IDBObserver)

Agreed, I'll be changing to that (and likely also eliminating match/matchAll entirely.)

so you'd do:

var observer = new CookieObserver(function(changeRecords) {
  // changeRecords has information about what cookies changed.
});
observer.observe(...patterns here...);

Yes, exactly like that :)

@domenic
Copy link

domenic commented Jul 18, 2016

I'm also hoping that a developer can avoid needing to handle changes to cookies they do not care about. Would it feel any better if match/matchAll were gone and there were a prefix-matching option and an array-of-names option for get/getAll instead?

Not really. I prefer a compositional API that encourages you to use JavaScript's existing ability to use boolean logic to avoid taking actions (e.g., if statements or Array.prototype.filter). In the case of registering for notifications in a service worker, having to declaratively state the filter in a form the engine can use makes sense, but in the rest of the API, it's just awkward and unnecessary.

I think the guarantee should be something like "no worse than a 5-second polling check using setInterval in a top-level tab with UI focus or its IFRAMEs that are currently visible or absolutely positioned offscreen (i.e., hidden service IFRAMEs which may still be ) or a ServiceWorker processing requests on behalf of such a context, ditto but 10 minutes for other contexts". What do you think? Is that too explicit? (I suspect so...)

It's hard for me to say. I guess this might be a place for implementers to weigh in more... Getting the spec-ese right here is going to be tricky, but first we'd want to see what people are OK with implementing.

- cookieList is indeed an array. Examples have been expanded to show this

- regexp is gone in favor of strict name matching, name prefix matching, and "give me everything" where the developer-supplied script sorts it out; tried to strike a balance between not waking up handlers when the change is to an unrelated cookie that happens to be in-scope and the matching is definitely practical to implement in the browser (exact match or exact prefix match - both are used in ServiceWorker already) on the one hand and allowing JS to decide whether a change is interesting on the other hand (omit interest list and you get called back every time)

- switched to `get`

- added `delete`

- proposed restricting write operations to secure contexts

- `expires` rather than `expiration`, added explicit examples of numeric expiration timestamps

- added some initial discussion of security implications

- overhauled change monitoring APIs

- attempted to explain change coalescing

- attempted to explain ServiceWorker API and rationale

- `match`/`matchAll` are gone

- `await`

- silent failures are gone

- changed event name
- minor reformatting

- note limitation of ServiceWorker cookie monitoring
@bsittler
Copy link
Contributor Author

I've attempted to incorporate your feedback, though I may have overlooked or misunderstood some of it. Please take a look and let me know what you think, @domenic @inexorabletash @esprehn

clarify that the only ephemeral script contexts supported is ServiceWorker
Clarifications
@bsittler bsittler self-assigned this Jul 22, 2016
@bsittler
Copy link
Contributor Author

bsittler commented Jul 22, 2016

BTW, typical cookie scanners described on the web seem to use setInterval and do not even bother throttling when the window/tab is blurred and poll at rates in the 1-10 Hz range:

http://stackoverflow.com/questions/14344319/can-i-be-notified-of-cookie-changes-in-client-side-javascript

https://www.experts-exchange.com/questions/22641766/Javascript-Call-function-as-soon-as-cookie-value-changes-Set-listener-for-changed-cookie-value.html

I have also seen implementations that automatically adapt the polling interval based on focus/blur and requested-vs.-actual time of invocation, which combines to give crude load-reduction and likely improves power consumption somewhat, but still is far more expensive than not polling.

The first linked page above also mentions the cookie API for Chrome extensions (https://developer.chrome.com/extensions/cookies#event-onChanged) which has efficient change detection and may be an improvement over the change monitoring API currently outlined in this explainer. It does, however, assume a different security model than the one applicable to document.cookie and this API, including revealing step-by-step mutations to cookie jar state and (assuming I understand it correctly) HttpOnly cookie data.

@domenic
Copy link

domenic commented Jul 26, 2016

This is starting to look pretty solid. It might be time to merge and move the discussion to separate issues, but for now I'll just continue leaving comments...

In a ServiceWorker you can read a cookie from the point of view of a particular in-scope URL, which may be useful when handling regular (same-origin, in-scope) fetch events or foreign fetch events.

Why restrict to in-scope? As noted, the security boundary here should be the origin... And why restrict this to service workers?

I might name the parameter asURL, but url is fine too. WDYT?

Sometimes an expected cookie is known by a prefix rather than by an exact name:

I still think the correct way to do this is to have people do filtering in JavaScript with .startsWith. I think at the very least the v1 of the API should do that, and consider adding additional sugar later.

cookieChanges.updates

Why a separate updates property, instead of just an array? Just an array would match MutationObserver better.

cookieChange.urls

The semantics of this are not very clear to me, but I imagine a more formal spec will help.

Successive attempts to observe on the same CookieObserver with effectively identical or overlapping interests are ignored to allow straightforward idempotent setup code.

Does this match the other observers on the platform? I am pretty sure it does not match MutationObserver...

@bsittler
Copy link
Contributor Author

Thanks for the additional feedback, @domenic ! Feedback inlined below:

This is starting to look pretty solid. It might be time to merge and move the discussion to separate issues, but for now I'll just continue leaving comments...

Thanks! I'll go ahead and do that.

In a ServiceWorker you can read a cookie from the point of view of a particular in-scope URL, which may be useful when handling regular (same-origin, in-scope) fetch events or foreign fetch events.

Why restrict to in-scope? As noted, the security boundary here should be the origin... And why restrict this to service workers?

For URL scope my intent was to allow access to all URLs for which the ServiceWorker could control a page and inject script to achieve the same effect using existing APIs - in other words not an expansion of ServiceWorker power, just an improvement in efficiency and ease of use. I realize that by successfully guessing/constructing a 404 page URL which allows IFRAME-ing and then running script inside it the same technique could expand to the whole origin, but a carefully constructed site (one where no pages are IFRAME-able) can actually deny this capability to a path-scoped ServiceWorker today and I was reluctant to remove that restriction without further discussion of the implications.

I'd especially like to understand why ServiceWorker fetch interception and foreign fetch interception should be path-scoped when cookie access is not. Would it be OK to move that discussion to a separate issue?

I might name the parameter asURL, but url is fine too. WDYT?

I chose url because I believe that's what it's already called when intercepting a fetch, which is the place I expect this to be used. Really, though, I don't feel strongly about it.

Sometimes an expected cookie is known by a prefix rather than by an exact name:

I still think the correct way to do this is to have people do filtering in JavaScript with .startsWith. I think at the very least the v1 of the API should do that, and consider adding additional sugar later.

I have seen quite a few cookie change monitors that are prefix-based (even though it's frequently implemented using a regular expression match, the intent is clearly prefix matching in most cases), and I would like to give them a working implementation without each one having to reinvent the filtering (perhaps incorrectly - some of the regular expression matching accidentally does substring matching when prefix is the clear intent from context.) This also allows an implementation to avoid even running JavaScript when the change is to an irrelevant cookie.

However, I'd still like to understand the rationale here more completely. Can we move this discussion to a separate issue, perhaps?

cookieChanges.updates

Why a separate updates property, instead of just an array? Just an array would match MutationObserver better.

Good point. I'll change to that, and I'll also update it to make it clear that the observer instance is passed in as the second parameter to the callback, and add a disconnect method to stop callbacks until a subsequent observe.

cookieChange.urls

The semantics of this are not very clear to me, but I imagine a more formal spec will help.

The intent of cookieChange.urls is to enumerate all the observed URLs (asURL-s?) to which the update element is applicable.

Successive attempts to observe on the same CookieObserver with effectively identical or overlapping interests are ignored to allow straightforward idempotent setup code.

Does this match the other observers on the platform? I am pretty sure it does not match MutationObserver...

I intended to follow MutationObserver's behavior for multiple overlapping observations, but perhaps I got it wrong. I realize it's not the spec, but I read this explanation at https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver :

Adding an observer to an element is just like addEventListener, if you observe the element multiple times it does not make a difference. Meaning if you observe element twice, the observe callback does not fire twice, nor will you have to run disconnect() twice. In other words, once an element is observed, observing it again with the same observer instance will do nothing. However if the callback object is different it will of course add another observer to it.

The living standard https://dom.spec.whatwg.org/#dom-mutationobserver-observe also suggests that additional calls to observe silently replace existing ones:

For each registered observer registered in target’s list of registered observers whose observer is the context object:

  1. Remove all transient registered observers whose source is registered.
  2. Replace registered’s options with options.

 - Mention cookie-aversion and other contexts with disallowed cookie access, and our chosen approach: reject operations, allow monitoring (but don't call back unless/until reads are possible)
- Explain why we path-scope a ServiceWorker/disallow reading for out-of-scope URLs
- Describe how fragile path-scoping is for this API
- Mention possibly-surprising port number handling
- Mention possibly-surprising different-path cookie-writing
- Removed unobserve, replaced it with less-flexible disconnect
- Changed description of observe to clarify additive behavior and lack of duplicated change reports
- Changed (optional) matchType to accept one of two values: 'equals' and 'startsWith'; clarified that 'equals' is the default
- CookieStore and URL are now one-per-item in CookieChange entries
- The first parameter for CookieObserver callbacks and event.data for the CookieChangeEvent are now simply arrays of cookie changes
- The second paramter for CookieObserver callbacks is now the CookieObserver
- The url is now per-CookieInterest item in calls to observe and InstallEvent's registerCookieChangeInterest
@bsittler
Copy link
Contributor Author

@domenic : I have attempted to clean up observe and InstallEvent's registerCookieChangeInterest and also the shape of the parameters passed in event.data for CookieChangeEvent and as the first parameter in the CookieObserver callback. I think I will go ahead and merge, and further issues can be filed and tracked independently.

@bsittler bsittler merged commit 833f42a into master Jul 27, 2016
@bsittler
Copy link
Contributor Author

I neglected to address this part of @domenic 's feedback earlier pertaining to reading cookies and monitoring for cookie changes on non-default URLs:

... why restrict this to service workers?

This is for the same reason that I propose to restrict the read operations and monitoring to in-scope URLs for ServiceWorkers: I believe a carefully-designed site can probably deny this capability to pages it serves today (though few do), at least in the absence of user interaction (due to popup-blocking), and I don't intend to give websites any major new cookie-related powers they don't already have through document.cookie and <meta http-equiv="set-cookie" ... >.

Should we later decide that it is useful and reasonable to extend this capability to document contexts and/or out-of-scope URLs, it should be very easy to do so without breaking by-then-extant uses of the API. On the other hand, once granted such additional capabilities would be very difficult to retroactively take away without breakage.

bsittler added a commit that referenced this pull request Jul 27, 2016
Changes I inadvertently neglected in #16 :
 - Fix bit-rot in countMatchingSimpleOriginCookies example
 - Add missing url+getAll examples (countMatchingCookiesForRequestUrl and countAllCookiesForRequestUrl)
@bsittler bsittler mentioned this pull request Jul 27, 2016
@bsittler
Copy link
Contributor Author

Missed a couple minor changes here which I'm making now in #17 - please take a look, and also let me know there if I missed anything else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants