WICG / cookie-store

Asynchronous access to cookies from JavaScript
https://wicg.github.io/cookie-store/
Apache License 2.0
143 stars 35 forks source link

Will returned cookies expose their metadata (expiry, path etc)? #11

Closed jakearchibald closed 6 years ago

jakearchibald commented 8 years ago

I realise that can't be polyfilled using document.cookie, but are we going to have the same restriction?

bsittler commented 8 years ago

FWIW Cookie: has the same restriction.

Path exposure can perhaps be polyfilled using hidden IFRAMES to multiple semi-cooperating super-pathed pages.

Unfortunately replaceState does not change the cookie jar view.

Domain exposure is trickier since document.domain is not taken into account when serializing document.cookie.

I actually think much more valuable information to expose would be Secure, which cannot be polyfilled AFAIK.

jakearchibald commented 8 years ago

FWIW Cookie: has the same restriction.

True, but is that reason enough to follow suit? Seems weird to have an API where set data becomes partially ungettable.

bsittler commented 8 years ago

Agreed, but if we fix it for JS we should at least consider extending that fix to HTTP with an additional request header

bsittler commented 8 years ago

This issue is still unresolved, but I think to be truly useful it needs to include a proposal for exposing the same information in HTTP requests. So far I haven't found any proposals for doing that, but it's likely I'm not looking hard enough.

FWIW the missing metadata could be represented very compactly:

Depending on the serialization format chosen, all the 0's above could perhaps be omitted entirely.

In addition to the already-serialized name, only Path and Domain are needed to delete an existing cookie - these are the parts most painfully absent in the HTTP Cookie: serialization and prevent straightforward delete-after-read. An app can actually brute-force deletion today by trying all combinations of in-scope domain and path for the request URL+cookie name. Perhaps the proposed API should be extended to include a deleteAll which does this?

pwnall commented 7 years ago

I think it'd make sense to expose metadata in JS, even if it's not exposed anywhere else (in HTTP). Asides from #12, exposing the metadata is necessary to make the API testing-friendly. (Apps should be able to write automated tests for their code that uses the API.)

FagnerMartinsBrack commented 7 years ago

It's still not clear what are the benefits of exposing cookie metadata to the user. @pwnall mentioned testing, do you have an example of which kind of testing would benefit from this?

Maybe since it's a browser API it's good to expose everything earlier because you can't extend later without causing breaking changes. In this case, would add a getter later really to be considered a breaking change?

pwnall commented 7 years ago

@FagnerMartinsBrack An example is an automated test where I read back a cookie that I've set, and I assert that the domain / path / expiration date match some expected values. (this is aside from the benefit of being able to pass cookie data to cookieStore.delete)

To your second point - Avoiding breaking changes would suggest not exposing the properties until later. Exposing the properties would be a non-breaking change, whereas removing them would be a breaking change.

That being said, I think the cookie metadata is useful and should be in the API, assuming that the security concerns will be mitigated.

FagnerMartinsBrack commented 7 years ago

An example is an automated test where I read back a cookie that I've set, and I assert that the domain / path / expiration date match some expected values. (this is aside from the benefit of being able to pass cookie data to cookieStore.delete)

What's the benefit from the POV of a developer of having to assert that a cookie has been set with domain/ path / expiration by the browser?

You'll be essentially testing the browser API which is assumed to already have been working and tested by the browser vendor. One can easily test that the standard cookie API has been called correctly by a domain-specific wrapper and then test the wrapper which is owned by the developer, not the platform (pseudo-code below):

MySystemClientStorageAPIDomainSpecificWrapper = ()=>{
  let storage;
  return {
    setStorage: (_storage) => { storage = _storage }
    set: (key, value) => { storage.set(key, value) }
  };
};

test(() =>{
  storageAPI = MySystemClientStorageAPIDomainSpecificWrapper();
  storageAPI.setStorage(cookieAPI);
  storageAPI.set('key', 'value')
  assertThat(cookieAPI).hasBeenCalledWith('key', 'value')
});

That would allow the developer to test that the cookie was set with the given expected value.

If the intent is to test the integration between the developer code and the platform (let's say, to make sure the application won't fail in production because of some browser bug), then that integration can be done by testing the behavior of the browser cookie API, otherwise there's no real coverage going on.

Why the behavior? Well, just because the browser cookie API is returning the same value that was set, that doesn't mean the cookie API is working. It just means the browser is returning that value: browserAPI = () => { set: (value) => { value =_value }, get: (value) => { return value } }

The test is the "first client" of an API. One would expect the API being used in the test (in this case, the getter) will also have a use case for the "second client" and onwards: the code outside the test, the "production code".

In this case, what's the use case for a developer to use the cookie getters in production code other than in testing?

To your second point - Avoiding breaking changes would suggest not exposing the properties until later. Exposing the properties would be a non-breaking change, whereas removing them would be a breaking change.

That's what I thought. My only concern is that the addition of new features for web APIs are known to have caused problems in the wild even when authors believe they would not have caused a breaking change. That's one of the reasons why no property in Object.prototype can be added (people override it), why Chrome is stuck with GZIP (server's code did not consider other types of compression) and why IE could never change the cookie limitation (big sites were relying on the cookie limitation to work).

If the methods are enumerable, for example, would it be reasonable to think the developer's code ("the client") can use the length of the methods in a way that changing them (adding the getters later) would cause websites to break?

We've seen similar things like this happening before.

FagnerMartinsBrack commented 7 years ago

The whole point I'm trying to make is:

Despite anything, there probably should be some consultation with early document.cookie RFC editors to make sure the lack of getters was a conscious decision and why

pwnall commented 6 years ago

@FagnerMartinsBrack Sorry, I had trouble parsing the question / points out of your earlier comments. Here are responses based on a brief chat with @bsittler

Cookie testing changes

First, I agree that you can mock / stub APIs, but that can both testing and production code more complex.

Second, there is a bit of value in being able to use the real Cookies API rather than a mock / stub. Writing to / reading from the real cookie store gives you some assurance that you used the correct APIs and that the values you passed were accepted by the browser.

Breaking the Web with API changes

This API is in incubation, so it shouldn't be used anywhere in the wild. So I don't think that the concern of breaking things applies here.

Justification

Asides from testing, #12 explains that the current API proposal has a rough edge where passing the result of a get or an item in a getAll result to delete is not guaranteed to delete the cookie that the result points to. I don't think this rough edge should exist. I may be biased by the fact that Chrome's implementation for exposing all the cookie attributes is very straightforward.

FagnerMartinsBrack commented 6 years ago

First, I agree that you can mock / stub APIs, but that can both testing and production code more complex.

It doesn't make it more complex, it forces your internal API to be more decoupled as long as you can keep cohesion. That's the whole purpose of designing to improve testability, which a great think for maintainability purposes.

With that in mind, you can avoid mocks if you want to:

test(() =>{
  storageAPI = MySystemClientStorageAPIDomainSpecificWrapper();
  storageAPI.setStorage(inMemoryAPI);
  storageAPI.set('key', 'value')
  assertThat(inMemoryAPI.get('key')).to.equal('value')
});

[...] there is a bit of value in being able to use the real Cookies API rather than a mock / stub. Writing to / reading from the real cookie store gives you some assurance that you used the correct APIs and that the values you passed were accepted by the browser.

@pwnall In the 7 years of jquery-cookie/js-cookie existence, there was never a bug or use case where having the getter APIs in the browser would make things easier.

It does give you some assurance that the browser is working fine, but testing the browser is a theoretical benefit unless it's measurable. It may not be worth the tradeoff of cluttering the browser API unless one can come up with code examples where that would be useful.

[...] the current API proposal has a rough edge where passing the result of a get or an item in a getAll result to delete is not guaranteed to delete the cookie that the result points to. I don't think this rough edge should exist. I may be biased by the fact that Chrome's implementation for exposing all the cookie attributes is very straightforward.

I'm not on top of that issue. If the idea is to allow the dev to remove cookies despite the subdomain they are, it might be worth investigating the potential security implications.

pwnall commented 6 years ago

@FagnerMartinsBrack Thank you for your feedback!

I don't currently see any security issues with giving site authors the information that they've set in cookies, and I see some upside as mentioned above. I currently plan to expose cookie metadata (as shown in the current explainer), but will definitely keep looking at developer feedback as the API goes through Chrome's origin trials.

FagnerMartinsBrack commented 6 years ago

I don't currently see any security issues with giving site authors the information that they've set in cookies

We don't know what we don't know.

The problem is not legit site authors, but reading the cookies from cross-origin. We're all aware that cookies are used to store sensitive information like the session, so just need to be extra careful this is not opening a whole new can of worms.

Just a head's up anyway.

pwnall commented 6 years ago

I agree that it's important to not regress the security story here.

For the kind of vulnerabilities that you've described, I think the principles at https://wicg.github.io/cookie-store/explainer.html#the-scope-path--domain are a helpful way to frame the problem. If what you're concerned with can be done with document.cookie, it's not a new attack vector. If there is a situation in which document.cookie doesn't expose the cookie but this API does, that's definitely worth bringing up and most likely fixing.

muodov commented 4 years ago

@pwnall @FagnerMartinsBrack I hope it's not too late to jump in with a developer perspective on these points:

It's still not clear what are the benefits of exposing cookie metadata to the user.

will definitely keep looking at developer feedback as the API goes through Chrome's origin trials.

We are developing a sandboxing proxy at Surfly. One of the required tasks is quite literally "transfer existing cookies from one domain to another". When our solution kicks in, the integrating application has already set some cookies, and we need to carefully transfer those values to a sandboxed scope.

Currently we have a quite a complicated implementation involving all those hacks mentioned above and in #10 by @jakearchibald. Non-httpOnly cookies are transferred using our JS API library, and for httpOnly cookies we ask our clients to implement a server-side integration.

The main challenge in this is to determine and transfer the complete scope of a cookie (path, domain, and flags), otherwise there will be inconsistencies in sandbox behaviour. With the current document.domain and Cookie header format it is of course tricky. In the first version we did actually implement all we could to determine the scope automatically: bruteforcing the paths in JS, and even considered bruteforcing over HTTP for httpOnly cookies. However, it quickly became an over-engineered mess of hacks, and in combination with 3rd-party cookie workarounds it was completely unmaintainable. So we gave up on that and in the current version we just require our clients to explicitly pass all cookie scopes to us. Now we have to spend time to educate clients on cookie scopes, but at least it is relatively straightforward. It is a shame though because we know that technically we could do it ourselves, but it's just way too complex.

From the security point of view, I suppose the question is whether cookie metadata is considered sensitive, because as far as I understand, this debate is only about those cookies whose names/values are already readable. Although I agree with @FagnerMartinsBrack that we'd better ask a security person, which I am not anymore.