fix: more than one cookie header field

[archvlad]

I will explain situation:

1. POST request to login. In request I have these cookies.
   

2. 302 redirect with Set-Cookie header
   

3. GET request to redirected location
   

As you can see there are duplicates of cookies (which leads to unsuccessful authentication), becuase of 58 line in index.js options.headers.append("cookie", cookies.slice(0, -2));
After these line of code we get this in options.headers:

Headers {
  [Symbol(map)]: [Object: null prototype] {
    'content-type': [ 'application/x-www-form-urlencoded' ],
    cookie: [
      'pmaCookieVer=5; phpMyAdmin=t3fjcmoinh1e7oiqc80fmhm7p9; pma_lang=en; pma_collation_connection=utf8mb4_unicode_ci',
      'pmaCookieVer=5; phpMyAdmin=csifugk2m203tp765a6ruc0vhk; pma_lang=en; pma_collation_connection=utf8mb4_unicode_ci; pmaUser-1=%7B%22iv%22%3A%22KNZROnB7GeBDEs8iKmP2zQ%3D%3D%22%2C%22mac%22%3A%2281aced86a47270c06327f582a87e23af754c8d11%22%2C%22payload%22%3A%225Pj4V05G7DRxriW%5C%2Fa%5C%2Fa8Wg%3D%3D%22%7D; pmaAuth-1=%7B%22iv%22%3A%22Zrk7%5C%2FwTGcvIOOuIdM07AwA%3D%3D%22%2C%22mac%22%3A%2233872c92af625065c65fc67b11ecd4fc71189079%22%2C%22payload%22%3A%22M05zWTlI778DW8%5C%2FGSHps9qZIACXeIMAs%2BIJ9fq%2BTYtN5lpGvSHMvDYYonHyOxqWW%22%7D'
    ]
  }
}

5.4. The Cookie Header

When the user agent generates an HTTP request, the user agent MUST NOT attach more than one Cookie header field.

In the http request I still got one cookie header but i has duplicates.

When I change append() to set() I have successfull authention and don't have duplicates of cookies.

Headers {
  [Symbol(map)]: [Object: null prototype] {
    'content-type': [ 'application/x-www-form-urlencoded' ],
    cookie: [
      'pmaCookieVer=5; phpMyAdmin=slbcfis01f8ccur6e84hhgr6g9; pma_lang=en; pma_collation_connection=utf8mb4_unicode_ci; pmaUser-1=%7B%22iv%22%3A%222NyH%5C%2FYh5SQFDA0SHLnqKAw%3D%3D%22%2C%22mac%22%3A%22fe9d9cdda8a2f055e40f7fbf5347c87b973a5379%22%2C%22payload%22%3A%22hPimz9ssKT1s8yEB77uZ2w%3D%3D%22%7D; pmaAuth-1=%7B%22iv%22%3A%22pLJDdMMWG8WWtABhE30r3w%3D%3D%22%2C%22mac%22%3A%22a9023ed717298a4062a776d5a8e2394a0dc343d2%22%2C%22payload%22%3A%22NxgFR4E7FLeOlPgXeKZo30oZbpXKrMTWd0GpnUzOlHwXNLPv0OUciA7ze2NRmZNs%22%7D'
    ]
  }
}

[jkhsjdhjs]

Hmm, this should only be a problem if the user also manually passes a Headers object that already contains a cookies header. Are you doing something like that or is there another bug here?

[archvlad]

I don't set cookie header, I just create cookieJar const cookieJar = new CookieJar(); then I send GET request and in response I receive cookies, which populate cookieJar. Then I send POST and receive 302 redirect which also includes some cookies. And becuse I use redirect: 'follow', I automaticaly send GET to redirected location. But In this request in cookies i have duplicates probably because of this line options.headers.append("cookie", cookies.slice(0, -2));.

[jkhsjdhjs]

Ah yes, now I see what you mean. The issue seems to be that the previous cookie header isn't removed and passed to the next request in case of a redirect. The next request however, again adds all valid cookies from the cookie jar, thus leading to the duplicate cookie header.

I thought a bit about the current situation and I think, that the current behavior isn't exactly great. Should the user (intentionally or by mistake) supply a cookie header field, the current behavior would be to append the cookie jar cookies anyway, leading to duplicate header fields. Your fix, however, would just silently overwrite the users cookies.
I think we should be more explicit in this case and instead of duplicating header fields or silently overwriting them simply throw an exception, so the user knows that something isn't right.
To avoid passing the cookies header to the next fetch call when following redirects, the cookie header field could simply be deleted before the recursive call.

What's your opinion on this?

[archvlad]

I looked at how this situation is handled in superagent.
I come up with these lines of code:

async function fetch(cookieJars, url, options) {
    let cookies = "";

    // If user pass cookies with options, add them to separate cookieJar.
    if (options?.headers?.cookie) {
        const newCookieJar = new CookieJar();

        // Parse options.headers.cookie to get array of 'name=value'
        let cookiesFromOptions = options.headers.cookie
            .split(/;\s*/)
            .map(c => c.trim())
            .filter(c => c != "");
        
        // Add each cookies to cookieJar
        cookiesFromOptions.forEach(c => newCookieJar.addCookie(c, url));
        
        // Not to change original cookieJars create shalow copy of cookieJars plus new separate cookieJar
        cookieJars = Array.isArray(cookieJars)
            ? [...cookieJars, newCookieJar]
            : [cookieJars, newCookieJar];
    }

[archvlad]

I have some errors in above code, but i won't delete it just to keep track of our thoughts. Shortly, I create cookieJar from cookie header from options and then use this cookieJar. I fixed these code:

async function fetch(cookieJars, url, options) {
    let cookies = "";

    let cookieJarBasedOnOptions = null;

    // If user pass cookies with options, add them to separate cookieJar.
    if (options?.headers?.cookie) {
        cookieJarBasedOnOptions = new CookieJar();

        // Parse options.headers.cookie to get array of 'name=value'
        let cookiesFromOptions = options.headers.cookie
            .split(/;\s*/)
            .map(c => c.trim())
            .filter(c => c != "");
        
        // Add each cookies to cookieJar
        cookiesFromOptions.forEach(c =>
            cookieJarBasedOnOptions.addCookie(c, url)
        );
    }

    const addValidFromJars = jars => {
        // since multiple cookie jars can be passed, filter duplicates by using a set of cookie names
        const set = new Set();
        jars.flatMap(jar => [...jar.cookiesValidForRequest(url)]).forEach(
            cookie => {
                if (set.has(cookie.name)) return;
                set.add(cookie.name);
                cookies += cookie.serialize() + "; ";
            }
        );
    };

    const convertToArray = variable => {
        if (Array.isArray(variable)) {
            return variable;
        } else if (variable !== null) {
            return [variable];
        } else {
            return [];
        }
    };

    if (cookieJars || cookieJarBasedOnOptions) {
        cookieJars = convertToArray(cookieJars).concat(
            convertToArray(cookieJarBasedOnOptions)
        );
        if (
            Array.isArray(cookieJars) &&
            cookieJars.every(c => c instanceof CookieJar)
        )
            addValidFromJars(cookieJars.filter(jar => jar.flags.includes("r")));
        else if (cookieJars instanceof CookieJar)
            if (cookieJars.flags.includes("r")) addValidFromJars([cookieJars]);
            else
                throw paramError("First", "cookieJars", "fetch", [
                    "CookieJar",
                    "[CookieJar]"
                ]);
    }

[jkhsjdhjs]

Ah I see, so basically you're creating a temporary cookie jar for the user-supplied cookies via the cookie header. I think that's not really necessary here: if the user wants to provide more cookies they should just create another CookieJar themselves or add cookies to the already-existing cookie jar. I think that adding support for parsing user-supplied cookie headers would just make the codebase more complicated without yielding any real benefit.

I think a patch like the following should fully suffice to resolve this issue:

diff --git a/src/index.mjs b/src/index.mjs
index 098449c..10a912b 100644
--- a/src/index.mjs
+++ b/src/index.mjs
@@ -55,6 +55,8 @@ async function fetch(cookieJars, url, options) {
     // or, if headers is an object, construct a Headers object from it
     options.headers = new Headers(options.headers);
     if (cookies) {
+        if (options.headers.has("cookie"))
+            throw new Error("options.headers already contains a cookie header!");
         options.headers.append("cookie", cookies.slice(0, -2));
     }
     if (wantFollow) options.redirect = "manual";
@@ -93,6 +95,8 @@ async function fetch(cookieJars, url, options) {
         }
         const location = result.headers.get("location");
         options.redirect = "follow";
+        // delete cookie header from previous request to avoid a duplicate cookie header
+        options.headers.delete("cookie");
         return fetch(cookieJars, location, options);
     }
     return result;

[archvlad]

In terms of resolving initial issue, it will resolve it. But we will lose the ability to pass cookie header to options, which I think not so good because you library should extends node-fetch, not limit it. As I said earlier, in superagent I can transfer cookie header which will be sending next to other cookies from jar. Your thoughts?

[jkhsjdhjs]

But we will lose the ability to pass cookie header to options, which I think not so good because you library should extends node-fetch, not limit it.

True, but I don't think there would be a substantial loss of functionality here. The only purpose of this library is to handle the cookie and set-cookie headers, thus I think if someone uses this library, they wouldn't want to manually interact with the cookie header in the first place. In my opinion, node-fetch-cookies should have full control of the cookie header for this purpose, since the user interacting with this header or being able to manually supply cookie headers may have possible side-effects that could possibly lead to loss of functionality, for instance if a user passes a malformed cookie header, that causes all following cookies appending by node-fetch-cookies to be rejected as well.

Besides providing malformed cookies, I just don't see any benefit of allowing user-supplied cookie headers, since all well-formed cookies can also be provided via Cookie instances contained in a CookieJar. Can you give me a reasonable usecase for this?

[archvlad]

node-fetch-cookies should have full control of the cookie header

I thought about it and now I agree with you. node-fetch-cookies abstract user from working with headers.cookie by using CookieJar. So even if a user wants to supply his own cookies he should do it via CookieJar.

[jkhsjdhjs]

Nice, thanks for thinking about it and having this discussion with me. If we both agree that it's the right decision, it's less likely to be a stupid decision :D
Do you want to change your PR to implement the changes or should I just implement them and close this PR?

[archvlad]

You can do it.