search

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] Agent cookies are not correctly attached to right domains/paths on redirect #1807

Open robaca opened 2 weeks ago

robaca commented 2 weeks ago

Describe the bug

Node.js version: 20.11.0

OS version: MacOS Sonoma 14.5

Description:

We are using superagent agent to perform an OpenID authorization code flow before interface tests, crossing domain boundaries on multiple redirects. After upgrading from superagent 8.0.8 to 9.0.2, this fails because of missing cookies in the latter redirects.

After adding some console.logs to the agent lib, it looks like in 8.0.8 the _saveCookies() method is not storing the right url.hostname/path but null/null, which means that all cookies are attached later irrespective of the domain, while in 9.0.2 it's passing wrong domains and paths into this.jar.setCookies(), so that they are not attached later.

Actual behavior

Example: agent starts in domain www.service.org which sets a session Cookie, then redirects to oidc.service.org for authentication, which itself sets cookies. Afterwards the same agent is used to post login credentials on oidc.service.org, which causes a redirect back to www.service.org. Now the session cookie is not passed to www.service.org, as it has been saved in the cookie jar with the redirection host/path for oidc.service.org.

Expected behavior

Cookies set in the response of any redirecting request that have no domain part by themselves should be saved with the domain of that exact request, not with that of the followup request.

Checklist

robaca commented 2 weeks ago

btw we cannot workaround setting a domain as our service is multi-tenant and the express-session middleware does not support this. Created https://github.com/expressjs/session/issues/988 in addition.

robaca commented 2 weeks ago

I created a failing test here: https://github.com/ladjs/superagent/commit/22f6632d8df6ef08de23ceb19d4a21a5099a1d64

robaca commented 4 days ago

I tried a fix here: https://github.com/ladjs/superagent/compare/master...robaca:superagent:master

For some reason the multipart tests are failing for me locally (maybe because of Node version), otherwise looks promising. I didn't check the PR rules beforehands, so it's most probably not yet ready for a PR.