deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

support parsing "set-cookie" header in http/cookie.ts getCookies method

Open andykais opened this issue 4 years ago • 10 comments

Is your feature request related to a problem? Please describe. I need to parse the "set-cookie" response header of a request for a web scraper.

Describe the solution you'd like getCookie could support parsing "set-cookie" response headers, similar to how it parses "cookie" request headers

Describe alternatives you've considered implementing my own cookie parser logic, since https://deno.land/std/http/cookie.ts doesnt look for the "set-cookie" header

andykais avatar Mar 02 '22 03:03 andykais

Deno Cookie implementation is based on https://datatracker.ietf.org/doc/html/rfc6265 and doesn't include parsing "set-cookie" response headers.

getspooky avatar Mar 03 '22 10:03 getspooky

alright. I ended up just reimplementing the getCookie logic myself, which is pretty straight forward. If supporting set-cookie is out of the scope of std, then thats fine since I have my workaround. Ill let a @getspooky or another contributor close this issue if that is the case.

(side note) this seems kind of redundant

    for (const kv of c) {
      const [cookieKey, ...cookieVal] = kv.split("=");
      assert(cookieKey != null);
      const key = cookieKey.trim();
      out[key] = cookieVal.join("=");
    }

why not just write

    for (const kv of c) {
      const [cookieKey, cookieVal] = kv.split("=", 2);
      assert(cookieKey != null);
      const key = cookieKey.trim();
      out[key] = cookieVal;
    }

andykais avatar Mar 04 '22 15:03 andykais

@andykais thanks for feedback, i will check that.

getspooky avatar Mar 04 '22 18:03 getspooky

why not just write

    for (const kv of c) {
      const [cookieKey, cookieVal] = kv.split("=", 2);
      assert(cookieKey != null);
      const key = cookieKey.trim();
      out[key] = cookieVal;
    }

@andykais Setting the limit argument to 2 would exclude all of the parts of the string after a potential second occurrence of "=".

Looking at the syntax in the spec, it's not evident to me that "=" is an invalid token in a cookie value. A safer approach which also avoids creating an intermediate array would be:

for (const kv of c) {
  const sliceIndex = kv.indexOf("=");
  assert(sliceIndex > 0); // key and value exist, and key length is > 0
  const key = kv.slice(0, sliceIndex).trim();
  out[key] = kv.slice(sliceIndex + 1); // should this be trimmed, too?
}

TS Playground

jsejcksn avatar Mar 05 '22 06:03 jsejcksn

@jsejcksn oh you are right! I think I have been assuming the limit arg works differently for a while now :sweat_smile: thanks for the tip

andykais avatar Mar 05 '22 18:03 andykais

@andykais What's your method of parsing the set-cookie header?

iuioiua avatar May 24 '22 02:05 iuioiua

function parse_resonse_cookies(response: Response) {
  const cookies: {[key:string]: string} = {}
  for (const [key, value] of response.headers.entries()) {
    if (key === 'set-cookie') {
      const kv_pairs = value
        .split(/;[ ]*/)
        .map(cookie_str => {
          const index = cookie_str.indexOf('=')
          if (index === -1) throw new Error(`invalid cookie '${cookie_str}'`)
          return [
            cookie_str.slice(0, index),
            cookie_str.slice(index + 1)
          ].map(s => s.trim())
        })
      Object.assign(cookies, Object.fromEntries(kv_pairs))
    }
  }
  return cookies
}

andykais avatar May 24 '22 02:05 andykais

The code in https://github.com/denoland/deno_std/issues/1977#issuecomment-1135331882 won't handle attributes which aren't in the key-value format (e.g. HttpOnly and Secure — see Set-Cookie on MDN for more info): it will throw an exception when encountering those (and they're very common). @iuioiua

I agree that having something in std to handle parsing these headers would be nice, however, it's not a trivial task.

I'd like to hear from someone on the Deno core team about whether there's interest in this. If so, I can create a draft PR with an initial implementation to begin the feedback cycle.

jsejcksn avatar May 25 '22 00:05 jsejcksn

Thank you @andykais, but I did encounter the errors that @jsejcksn mentioned. I decided to go with something far simpler, which suits my use case:

function getSetCookie(headers: Headers): string {
  return headers.get("set-cookie")!
    .split(", ")
    .map((cookie) => cookie.split("; ")[0])
    .join("; ");
}

iuioiua avatar May 29 '22 22:05 iuioiua

The code in #1977 (comment) won't handle attributes which aren't in the key-value format (e.g. HttpOnly and Secure — see Set-Cookie on MDN for more info): it will throw an exception when encountering those (and they're very common). @iuioiua

I agree that having something in std to handle parsing these headers would be nice, however, it's not a trivial task.

I'd like to hear from someone on the Deno core team about whether there's interest in this. If so, I can create a draft PR with an initial implementation to begin the feedback cycle.

@bartlomieju

iuioiua avatar May 29 '22 22:05 iuioiua

@kt3k, this was implemented in #2475.

iuioiua avatar Aug 14 '22 09:08 iuioiua