feat: implement CookieStore

[KhafraDev]

Closes nodejs/node#42183

Implements:

• a mostly spec compliant CookieStore *
• CookieChangeEvent
• ability to create a CookieStore with 1 or more set-cookie headers while remaining spec compliant.
• rfc6265 compliant set-cookie header parsing **

* I follow Chrome's behavior where it mismatches from the spec. This can be seen when setting & getting a cookie name with bom characters in it. Chrome ignores these (?), so likewise the behavior is the same here.
** Without header parsing, CookieStore is entirely useless in node. With it, I can see it getting used a fair bit for parsing cookies server-side.

I haven't exposed it yet and it needs more tests, but nyc wasn't giving me good coverage reports anymore.

CookieStoreFrom also needs a way to set a hostname to check for valid/invalid cookies (would solve the issue of node not having an origin).

Things that need to be discussed:

• Evicting expired cookies (and most performant way of doing so)?

[mcollina]

how would this work in combination with fetch?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 99.31507% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.82%. Comparing base (6855006) to head (ce0abac).
Report is 1525 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/cookie-store/cookie-store.js	98.96%	1 Missing ⚠️
lib/cookie-store/util.js	98.76%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1441      +/-   ##
==========================================
+ Coverage   94.52%   94.82%   +0.30%     
==========================================
  Files          49       54       +5     
  Lines        4271     4563     +292     
==========================================
+ Hits         4037     4327     +290     
- Misses        234      236       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KhafraDev]

Something along the lines of whatwg/fetch#1384

I think it has more 'substantial' use server-side though.

import { createServer } from 'http'

const server = createServer(async (req, res) => {
   const cookieStore = req.headers['set-cookie'] ? CookieStoreFrom(req.headers['set-cookie']) : null

  if (cookieStore === null ) {
    res.statusCode = 404
    res.end('no cookies')
    return
  }

  const sessionCookie = await cookieStore.get('sessionId')

  if (sessionCookie && await matchesFromSomewhere(sessionCookie)) {
    res.statusCode = 200
    res.end('Welcome!')
    return
  }

  // ...
}).listen(0)

[mcollina]

good work!

[mcollina]

Could you add some test for fetch integration and docs?

[KhafraDev]

Fetch integration hasn't been done yet, I wasn't sure if it would be in scope for this PR.

edit: and having a cookie store is not in the spec, but a proposal @jimmywarting made. I think I'll make an issue under the wintercg fetch repo and see how set-cookie should be handled.

Actually, this seems to be done in wintercg/fetch#4 (or at least, could be solved by). A fetch implementation would be not combining set-cookie headers (until using .get()), and sending them to the CookieStore if one exists. We could also add .getAll('set-cookie'), similar to how cloudflare workers do it, and have the user construct their own CookieStore from the set-cookie headers returned.

[mcollina]

I would restrain from adding this without further fetch() integration.

[KhafraDev]

That's fair, I'll convert it to a draft PR until there's more discussion with integrating into fetch or there's been more discussion about use cases or value this has in undici/node core.

[blittle]

@KhafraDev I'm not sure if your example above would work with multiple cookies:

const server = createServer(async (req, res) => {
   // there might be multiple set-cookie headers, and req.headers['set-cookie'] currently does not properly return them
   const cookieStore = req.headers['set-cookie'] ? CookieStoreFrom(req.headers['set-cookie']) : null

  // instead what if `cookieStore` was just a property on the request?
  const cookieStoreReq = req.cookieStore;

  if (cookieStore === null ) {
    res.statusCode = 404
    res.end('no cookies')
    return
  }

  const sessionCookie = await cookieStore.get('sessionId')

  if (sessionCookie && await matchesFromSomewhere(sessionCookie)) {
    res.statusCode = 200
    res.end('Welcome!')
    return
  }

  // ...
}).listen(0)

[jimmywarting]

What's weird is that you create a server and listen for incomming set-cookies headers

const server = createServer(async (req, res) => {
   // there might be multiple set-cookie headers, and req.headers['set-cookie'] currently does not properly return them
   const cookieStore = req.headers['set-cookie'] ? CookieStoreFrom(req.headers['set-cookie']) : null

when clients makes request to a server then you are only sending cookies.
so it should rather be something like CookieStoreFrom(req.header.cookies)

  // instead what if `cookieStore` was just a property on the request?
  const cookieStoreReq = req.cookieStore;

i like that, and it was what i proposed in my original feature request. it would always exist on the request (even if no cookie are being sent) it should only populate the newly created cookiestore with new values. so you don't even have to check if req.cookieStore is null.

you would only have to do:

  const sessionCookie = await req.cookieStore.get('sessionId')
  if (!sessionCookie) return res.end('not logged in')

[KhafraDev]

@blittle with this minimal example, all cookies are received:

import { once } from 'events'
import { createServer } from 'http'
import { request } from 'undici'

const server = createServer((req, res) => {
	console.log(req.headers['set-cookie'])
        // [ 'a=b; Domain=example.com', 'c=d; Path=/account/' ]

	res.end('goodbye')
}).listen(0)

await once(server, 'listening')

await request(`http://localhost:${server.address().port}`, {
	headers: {
		'set-cookie': 'a=b; Domain=example.com',
		'SEt-CoOkIe': 'c=d; Path=/account/'
	}
})

[blittle]

@KhafraDev oh yes, you are right. It's the node request object, not undici's Request.