denoland / std

The Deno Standard Library
https://jsr.io/@std
MIT License
3.2k stars 621 forks source link

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

Closed andykais closed 2 years ago

andykais commented 2 years ago

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

getspooky commented 2 years ago

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

andykais commented 2 years ago

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;
    }
getspooky commented 2 years ago

@andykais thanks for feedback, i will check that.

jsejcksn commented 2 years ago

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

andykais commented 2 years ago

@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

iuioiua commented 2 years ago

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

andykais commented 2 years ago
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
}
jsejcksn commented 2 years ago

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.

iuioiua commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@kt3k, this was implemented in #2475.