Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
wilsonjackson opened this issue Jan 24, 2023 · 5 comments · Fixed by #1761
Closed
3 tasks done

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

wilsonjackson opened this issue Jan 24, 2023 · 5 comments · Fixed by #1761
Labels

Comments

@wilsonjackson
Copy link
Contributor

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

  • I have searched through GitHub issues for similar issues.
  • I have completely read through the README and documentation.
  • I have tested my code with the latest version of Node.js and this package and confirmed it is still not working.
@titanism
Copy link
Collaborator

Thanks @wilsonjackson - PR welcome to fix?

wilsonjackson added a commit to Leafly-com/superagent that referenced this issue Jan 24, 2023
@wilsonjackson
Copy link
Contributor Author

@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 #1761

@titanism
Copy link
Collaborator

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
Copy link
Contributor Author

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

@titanism
Copy link
Collaborator

v8.0.9 released to npm and GitHub releases

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants