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(lib/adapters/xhr.js): CVE-2023-45857 #6002

Closed
wants to merge 1 commit into from

Conversation

valentin-panov
Copy link
Contributor

@valentin-panov valentin-panov commented Oct 15, 2023

CVE-2023-45857
#6006

Hi team, @jasonsaayman and @DigitalBrainJS

It's crucial to ensure the protection of CSRF tokens. These tokens should be treated as confidential information and managed securely at all times.
You may check it here:
https://portswigger.net/web-security/csrf/preventing
https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html

I believe that lib/adapters/xhr.js:191 must check withCredentials && isURLSameOrigin to populate X-XSRF-TOKEN header.

Kind regards, Valentin Panov

@valentin-panov
Copy link
Contributor Author

the issue possibly won't be solved with this commit.

@sbley
Copy link

sbley commented Oct 20, 2023

Why wouldn't the commit solve the issue? The CSRF token would then not be sent to other domains anymore.

@valentin-panov
Copy link
Contributor Author

Why wouldn't the commit solve the issue? The CSRF token would then not be sent to other domains anymore.

Thank you for your reply.
First things first. The same origin request isn't affected by the withCredentials option.
Second. I believe that we must have a bit more sophisticated behaviour. I would like to have an option, where to send that header. I am perfectly aware of the possibility of manual header creation, interceptors etc. However, if we already have that automation, let's tune it. I hope it helps to understand, why I closed that PR without merging.

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

Successfully merging this pull request may close these issues.

None yet

2 participants