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

[Feature request] Add a CookieStore option to Request & Response #1384

Open
jimmywarting opened this issue Jan 23, 2022 · 5 comments
Open

[Feature request] Add a CookieStore option to Request & Response #1384

jimmywarting opened this issue Jan 23, 2022 · 5 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@jimmywarting
Copy link

jimmywarting commented Jan 23, 2022

I was looking into how some form of how a cookie jar could fit into NodeJS and Deno.
...it totally lacks any form of cookie handling. A default cookie jar would not be so perfect for backend either.

There exist some grate libraries like tough-cookie but isn't so web-ish when it comes to something as spec'ed as the CookieStore

I was thinking that it would be cool if you could construct CookieStore with new CookieStore() and being able to pass this to fetch/Request as an option. and so that we can agree upon a unified way of dealing with cookie jars in both Deno and NodeJS. while sending, reciving and encoding/decoding cookie strings

// this is just a ruff sketch up / proposal
// not sure if it would actually look anything like this in the end
// not even 100% sure if a CookieStore could handle all your scenario
// I don't know how CookieStore could behave with multiple domains, maybe it's doable with
// cookieStore.get({ name, url })    and not   cookieStore.get({ name })
// then servers can be free to read httpOnly cookies without any restrictions

const cookieStore = new CookieStore()
const request = new Request(url, { cookieStore })

fetch(url, { cookieStore }).then(response => {
  // Any response headers that had 'set-cookie' will now be in this CookieStore (incl. those in redirect)
  response.cookieStore === cookieStore // true
})

// unspecified cookieStore in browser would default to the default global cookie store
fetch(url).then(response => {
  response.cookieStore === window.cookieStore // default cookieStore
})

// Deno and NodejS could technically create a new CookieStore instance if no cookieStore is provided
const req = new Request(url)
req.cookieStore // new instances of CookieStore
fetch(req).then(response => {
  response.cookieStore === cookieStore // false
  response.cookieStore instanceof CookieStore // true
  // response.headers would not contain any `set-cookie` like the browser
  response.headers.has('set-cookie') // will always be false
})

// Service worker could modify a CookieStore
self.onfetch = evt => {
  evt.request.cookieStore.get('xyz').then(...)
}

// unrelated to fetch... but:
// http servers could maybe also utilize CookieStore in some way also and behave a bit like service workers?
import express from 'express'
const app = express()
app.use(async (request, response) => {
  // logged in user with a cookie 'xyz' makes a request to 'POST /blog'
  // So the incoming request contains a newly created cookieStore specific to this user only
  if (! (await request.cookieStore.get('xzy'))) {
    // the user don't seem to be logged in with the cookie xyz

    // response.cookieStore is a (completly new? or the same) CookieStore we could utilize 
    // for setting new cookies that would be sent back to the response/client
    await response.cookieStore.set('xyz', { ... })
  }
})

This would take care of setting request cookies that exist in the store and parsing set-cookies like during redirects as well. + it would also simplify getting/setting cookies, and the Header class wouldn't need any special handling like #1346, #973 (we could forbid the set-cookie header like the browser dose it and also remove any cookie header from the response.headers as well)

I was maybe thinking this could be useful for the web as well where you have the desire to handle multiple sessions/accounts simultaneously and dealing with cookies yourself in different tabs. This could also be a way of telling a single page application that cookies should never store a cookie on the clients machine (whetter it be session or persistent) maybe you could store a CookieStore in the IndexedDB? otherwise it would totally be discarded when you leave the page. This could be useful for banks so that you are logged out as soon as you close/refresh the page, and having to login again.

if you could pass a own instances of CookieStore to the request then it would also mean that any sub-sequent request to eg /static/css/main.css wouldn't send any cookie either via a <link> tag

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Jan 24, 2022
@jimmywarting jimmywarting changed the title [Feature request] Add a CookieStore option to Request [Feature request] Add a CookieStore option to Request & Response Jan 30, 2022
@justingrant
Copy link

This feature request could be generalized somewhat. The fundamental problem is that server-side HTTP clients need a way to get cookies from request A and send those cookies back to the server in request B. Adding a CookieStore option is one way to solve this (perhaps the best way?) but if Node offers a different (but still ergonomic and secure) way to propagate cookies, that may be OK.

The rest of this comment provides more explanation and justification for this feature requests.

Server-Side Cookies Use Cases

Copying from nodejs/node#41749 (comment):

Use cases for simulating browsers exist on a continuum. On one end is automated testing of browser apps and other cases where you want the client to work exactly like a real browser. For these cases, heavyweight tools like puppeteer are great because they run JS and really act like a browser.

On the other end of the continuum are simpler cases where you don't care if the client works exactly like a browser, you just need it to work enough like a browser that the server won't reject requests. These cases are common when extracting data from a website that doesn't offer an API, aka screen scraping. You can also use heavyweight tools for this case too, but in my experience plain HTTP requests are preferred because they're much faster, use much less RAM, place fewer demands on remote servers (e.g. won't download scripts and CSS), and have fewer dependencies & moving parts that can fail.

Screen-scraping seems like it should be unusual, but in my experience there are quite a few production apps (and many more internal apps or side projects) which sadly need to interact with remote servers that only have a browser interface, not an API. Sometimes it's because the remote server is a legacy system that can't easily be updated, which is common in government, banking, telecom, or honestly almost anything in non-tech companies with limited IT resources. Sometimes it's because you're trying to automate access to a remote system whose owners don't want you to automate it, e.g. competitors. Sometimes it's because getting API access requires jumping through so many hoops that it's not worth the hassle. But it's frustratingly common, and it'd be great to be able to handle these cases without having to reach for a scraping library when a cookie-jar-enabled HTTP client could be all that's needed.

Here's another discussion of use cases from nodejs/node#41749 (comment):

It’s a common use case to want to request data from an API that requires authentication, where that authentication check takes the form of checking for a cookie. It’s not only done when you’re trying to simulate a browser.

Workarounds

Below is what server apps must do to enable cookie propagation between requests. There may be more steps, but the two below are the ones I know about. Note that redirect requests require different handling to support cases where the cookie is set by a redirect response.

  1. Read and parse the Cookie header after each response, and then manually set the Cookie header of subsequent requests. Libraries like tough-cookie can help with the parsing and emitting.
  2. Ensure that the redirect option is set to "manual" (not the default "follow"). Then manually handle all redirection in userland code, including setting the Cookie header as described in (1). I'm not sure if there's an existing library that will handle this redirect logic in the same way that tough-cookie will handle (1). Note that, unlike in the browser, responses using "manual" will provide callers access to the headers, per lib: add fetch nodejs/node#41749 (comment). (Manual redirect responses in the browser are opaque and don't provide readable headers to userland code, as discussed in lib: add fetch nodejs/node#41749 (comment))

Justification

Both (1) and (2) above are non-trivially complex. Both cookies and redirection are fraught with potential security issues and the manual steps described above are quite un-ergonomic.

So, realistically, I'd expect that most cookie-needing Node apps will rely on an (as yet unwritten?) library that wraps fetch and handles cookies and redirects. Hopefully this library will not have security or perf problems!

More Discussion

I don't know enough about the security implications to have an opinion about whether cookie control as recommended in the OP is needed for fetch in browsers. It's possible that it may be a really bad idea in the browser for security reasons. But on the server there does need to be a way to propagate cookies from one request to later requests, because a single global cookie jar is not acceptable for server-side use.

@jimmywarting
Copy link
Author

couldn't have said it any better.
there is one fetch wrapper that adds a cookiejar for you called fetch-cookie but it don't handle cookies in redirect. maybe there are something better that handles manual redirect - haven't looked for anything.

@valeriangalliat
Copy link

Hey there! 👋 fetch-cookie maintainer here :)

We do support redirects (although I'm currently working on improving that part as it's not perfectly compliant), but only for the node-fetch specific wrapper require('fetch-cookie/node-fetch'). As far as I can remember, when the redirect logic was contributed years ago, node-fetch allowed redirect: 'manual' but I don't think such a mechanism was part of the official spec yet, and I didn't want to tie the library specifically to node-fetch.

I realized recently that redirect: 'manual' is now officially supported so I'm gonna make handling cookies in redirects the default in the next major version.

That being said, fetch-cookie cannot currently work with undici because the set-cookie header is not exposed by the fetch API (as per spec) so in reality fetch-cookie can only be used alongside node-fetch for now.

If the Node.js implementation ever allows userland code to read the set-cookie header then it should work seamlessly with future fetch-cookie versions including redirects.

That being said I do support the feature request in the original message. I wrote fetch-cookie 7 years ago as what I thought would be a "temporary workaround" and now hundreds of people depend on it. Anything that makes it obsolete would make me more than happy, and that CookieStore proposition would definitely help.

So, realistically, I'd expect that most cookie-needing Node apps will rely on an (as yet unwritten?) library that wraps fetch and handles cookies and redirects. Hopefully this library will not have security or perf problems!

The main issue I have with a fetch wrapper is that the redirect logic must be re-implemented. node-fetch does a fantastic job at handling redirects and I'm pretty sure undici does too, so it makes me sad to have to bypass that upstream redirect implementation in order to re-implement it in the wrapper. This is error prone and requires duplicated efforts in maintenance.

One thing that I've been dreaming about recently that would help with that is a way to properly wrap fetch. Typically fetch implementations are recursive when following redirects (e.g. in node-fetch) and if we had a way to recurse through the wrapper, we would be able to have cookies in redirects without requiring the wrapper to set redirect: 'manual' and re-implementing the whole redirect logic. I'm not sure if that's something desirable for the fetch spec itself but that would certainly help making a reliable wrapper where the redirect logic would be guaranteed to be consistent with the upstream implementation.

@justingrant
Copy link

That being said, fetch-cookie cannot currently work with undici because the set-cookie header is not exposed by the fetch API (as per spec) so in reality fetch-cookie can only be used alongside node-fetch for now.

@valeriangalliat, this statement seems to contradict what @devsnek says in nodejs/node#41749 (comment):

you can still use cookies with the current api, you are able to read set-cookie headers in responses and set cookie headers in requests.

I confirmed on Node 17.5 and I am able to read the Set-Cookie header:

(await fetch('https://wikipedia.com/')).headers.get('set-cookie')
// => 'WMF-Last-Access=18-Feb-2022;Path=/;HttpOnly;secure;Expires=Tue, 22 Mar 2022 00:00:00 GMT, WMF-Last-Access-Global=18-Feb-2022;Path=/;Domain=.wikipedia.org;HttpOnly;secure;Expires=Tue, 22 Mar 2022 00:00:00 GMT, GeoIP=(REDACTED!); Path=/; secure; Domain=.wikipedia.org'

The main issue I have with a fetch wrapper is that the redirect logic must be re-implemented. node-fetch does a fantastic job at handling redirects and I'm pretty sure undici does too, so it makes me sad to have to bypass that upstream redirect implementation in order to re-implement it in the wrapper. This is error prone and requires duplicated efforts in maintenance.

I strongly agree with this point.

@meyfa
Copy link

meyfa commented Sep 8, 2022

you can still use cookies with the current api, you are able to read set-cookie headers in responses and set cookie headers in requests

This is true, though the fact that multiple Set-Cookie headers get joined together into a single string makes it unusable. The joined string cannot be unambiguously parsed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

5 participants