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

Cookie host-only-flag is not taken into account during storage #266

Open
max-stytch opened this issue Mar 17, 2023 · 6 comments
Open

Cookie host-only-flag is not taken into account during storage #266

max-stytch opened this issue Mar 17, 2023 · 6 comments

Comments

@max-stytch
Copy link

TL;DR

Cookies with different subdomain characteristics are clobbered during writes.
When setting two cookies with the same name, but one has domain specified and the other does not, I'd expect both to remain in the store. Instead, the second will always overwrite the first.

Expected Behavior

I would expect this library to match the behavior found in the browser. Tested on https://example.com using

  • Chrome 111.0.5563.64
  • Firefox 107.0
  • Safari 16.3 (18614.4.6.1.6)

image

Actual Behavior

> var tough = require("tough-cookie");
> var cookiejar = new tough.CookieJar();
> cookiejar.setCookieSync('foo=a', 'https://example.com');
Cookie="foo=a; Path=/; hostOnly=true; aAge=1ms; cAge=1ms"
> cookiejar.setCookieSync('foo=b;domain=example.com', 'https://example.com');
Cookie="foo=b; Domain=example.com; Path=/; hostOnly=false; aAge=27ms; cAge=22437ms"
> cookiejar.getCookieStringSync('https://example.com')
'foo=b'

RFC 6265bis reads

If the cookie store contains a cookie with the same name, domain, host-only-flag, and path as the newly-created cookie:

  1. Let old-cookie be the existing cookie with the same name, domain, host-only-flag, and path as the newly-created cookie...

This is different from the original RFC 6265, which reads

  1. If the cookie store contains a cookie with the same name,
    domain, and path as the newly created cookie:
@wjhsf
Copy link
Contributor

wjhsf commented Mar 17, 2023

Thanks for pointing this out! It's been a while since we've done a thorough review of the code to ensure spec compliance, and it's on our roadmap.

@max-stytch
Copy link
Author

Awesome! I'd be happy to try to contribute a fix, but I'm a little unsure how to best contribute with the typescript rewrite in progress. Any advice on that front?

@awaterma
Copy link
Member

We should be following the RFC 6265 definition. In 5.3.11:

11.  If the cookie store contains a cookie with the same name,
        domain, and path as the newly created cookie:

        1.  Let old-cookie be the existing cookie with the same name,
            domain, and path as the newly created cookie.  (Notice that
            this algorithm maintains the invariant that there is at most
            one such cookie.)

        2.  If the newly created cookie was received from a "non-HTTP"
            API and the old-cookie's http-only-flag is set, abort these
            steps and ignore the newly created cookie entirely.

        3.  Update the creation-time of the newly created cookie to
            match the creation-time of the old-cookie.

        4.  Remove the old-cookie from the cookie store.

We were just discussing this in a test for the typescript implementation; we should validate that we have the correct behavior (@colincasey); but I think we do.

It looks like RFC6265bis hasn't been approved yet and will expire at the end of May: This Internet-Draft will expire on 11 May 2023. From the mailing list, it looks like the draft will progress to the next step, so we need to start working on those changes: https://lists.w3.org/Archives/Public/ietf-http-wg/2023JanMar/0261.html

I'll create a Board to help us track proposed changes; would you want to work with us on that @max-stytch? We could use some help on stories to track there, as well as implementations on the test side are always a good way to start!

@awaterma
Copy link
Member

I've created the following classic project for RFC 6265-bis:

https://github.com/salesforce/tough-cookie/projects/7

I've just created the project, but I'll start adding stories as I get time. :)

@max-stytch
Copy link
Author

Summary: This document is ready for publication as a Proposed Standard

Hey! That's great - BIS has been in pending for 5 years!

Check out this diff to see all the changes between the original RFC and the update.

@awaterma
Copy link
Member

Thank you for the diff link. It will help us, but we really need to analyze the differences and create stories in order to get a tracking version.

If you'd like to help, that would be a great spot to start!

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

No branches or pull requests

3 participants