jkhsjdhjs / node-fetch-cookies

node-fetch wrapper that adds support for cookie-jars
MIT License
28 stars 16 forks source link

Add support for 'localhost' urls #8

Closed thecyberwraith closed 4 years ago

thecyberwraith commented 4 years ago

Issue

The cookie jar can capture cookies set from a localhost server, but responses do not send the cookies to the same url.

Versions

/ The express server I was testing, using cookie-parser and express-session / const {getCleanServer} = require('./util/server.js');

describe('When cookies are set by a response', function () { function cookieCount(cookeJar) { let count = 0; for (const cookie of cookieJar.cookiesAll()) { count += 1; } return count; }

async function assertCookiesSetAndSent(url) { cookieJar = new CookieJar();

await fetch(cookieJar, url);

expect(cookieCount(cookieJar)).to.be.above(0);

let options = {};
await fetch(cookieJar, url, options);

expect(options.headers, 'Header is undefined')
  .to.not.be.undefined;
expect(options.headers.cookie, 'Header is defined, but cookie not defined')
  .to.not.be.undefined;

}

it('sends the cookies to normal urls', function () { return assertCookiesSetAndSent('http://www.pinterest.com/'); });

describe('When I setup a localhost server', function () { let server; beforeEach('setup the server', async function () { server = await getCleanServer(); });

afterEach('stop the server', function () {
  return server.close();
});

it('sends the cookies to localhost urls', function () {
  return assertCookiesSetAndSent('http://localhost:8080/signup');
});

}); });

## Test Output

When cookies are set by a response ✓ sends the cookies to normal urls (394ms) When I setup a localhost server 1) sends the cookies to localhost urls

1 passing (752ms) 1 failing

1) When cookies are set by a response When I setup a localhost server sends the cookies to localhost urls: AssertionError: Header is undefined: expected undefined not to be undefined at assertCookiesSetAndSent (test/test_cookie.js:29:17) at processTicksAndRejections (internal/process/task_queues.js:97:5) at async Context. (test/test_cookie.js:49:7)


# Possible Solution
I am not an expert in cookies. Testing through some of the code seems to indicate that in cookieJar.mjs on lines 78-82, the expression attempts to split subdomains from the hostname. However, it assumes there is at least one period in the hostname, and on line 82 it drops the last entry. However, when the hostname is simply "localhost", the only entry before line 82 is ['localhost'], so the only domain is dropped. I don't know enough about the project to know if modifying those lines directly would have unintentional consequences.
Flummi commented 4 years ago

I think that will also be the case with servers in the local network.

jkhsjdhjs commented 4 years ago

Yes, you're right. Lines 78-82 create ["sub.domain.com", "domain.com", "com"] from sub.domain.com, because cookies are stored in a map which maps domains to cookies. The slice(0, -1) at the end should just remove the TLD entry of the resulting array, because there won't ever be cookies stored for a TLD anyways. But I didn't have domains in mind, that just don't have any TLD, like localhost or similar. Thanks for investigating this issue and providing test code! I pushed https://github.com/jkhsjdhjs/node-fetch-cookies/commit/a9d9f6c6757f3c68594342813019525819544409, which should fix this issue. Can you test if it works for you? I would then release 1.4.1 with the fix.

thecyberwraith commented 4 years ago

After that commit was added to my project, both the sample test passed as well as the rest of my project's tests that relied on it. Looks resolved to me.

jkhsjdhjs commented 4 years ago

Thanks!