ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.57k stars 1.33k forks source link

[fix] Following redirects discards set cookies without explicit domain #1760

Closed wilsonjackson closed 1 year ago

wilsonjackson commented 1 year ago

Describe the bug

Node.js version: 16.16.0

OS version: macOS monterey

Description: When redirects are followed (via .redirects()) cookies set by the redirecting server are not persisted correctly.

This bug was introduced with #1757, which assumes that there will always be a request object set on the response received by Agent.prototype._saveCookies. However, _saveCookies is invoked by two events: response and redirect, and that assumption is only true of the response event.

Actual behavior

If a cookie without an explicit domain has already been set in Superagent's cookie jar, that cookie cannot be overwritten using the same parameters during a followed redirect.

Expected behavior

Setting a cookie with the same parameters in a followed redirect should overwrite the previous cookie.

Code to reproduce

https://gist.github.com/wilsonjackson/746aee8f563e8486fa2f99c32baba235

Checklist

titanism commented 1 year ago

Thanks @wilsonjackson - PR welcome to fix?

wilsonjackson commented 1 year ago

@titanism Super happy to submit a fix, but I might need a little guidance.

_redirect emits the node response object: https://github.com/Leafly-com/superagent/blob/bc627eaea5de5d0fe630f6145183aa93c3d4df1a/src/node/index.js#L559

Should it call emitResponse instead? Or just do something similar to emitResponse?

btw failing test added here https://github.com/ladjs/superagent/pull/1761

titanism commented 1 year ago

I'm not sure - haven't looked at that internal code in a long time. Not too much time right now other than maintenance and PR reviews.

wilsonjackson commented 1 year ago

I'll take a stab and let you review, then.

titanism commented 1 year ago

v8.0.9 released to npm and GitHub releases

https://github.com/ladjs/superagent/releases/tag/v8.0.9