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

Open
3 tasks done
robaca opened this issue Jun 19, 2024 · 4 comments
Open
3 tasks done
Labels

Comments

@robaca
Copy link

robaca commented Jun 19, 2024

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

  • 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.
@robaca robaca added the Bug label Jun 19, 2024
@robaca
Copy link
Author

robaca commented Jun 19, 2024

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

robaca added a commit to robaca/superagent that referenced this issue Jun 20, 2024
@robaca
Copy link
Author

robaca commented Jun 20, 2024

I created a failing test here:
22f6632

@robaca
Copy link
Author

robaca commented Jul 2, 2024

I tried a fix here: 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.

@robaca
Copy link
Author

robaca commented Jul 8, 2024

btw. I get the same error in supertest. If I use it with my patched version and change one line in supertest/lib/agent.js to req.on('pre-redirect', this._saveCookies.bind(this)), it's working for me.

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

No branches or pull requests

1 participant