feat(ajax.ts): send XSRF cookies in a custom header #5702
Conversation
There was a problem hiding this comment.
I know this is just a draft, and it looks okay. Nonetheless, I'm leaving comments.
As much as possible, I would look to other ajax libraries like axios for guidance here. We should probably set the same defaults with regards to this.
In particular, you'll notice that axios will check to see not only if withCredentials is true, but also to see if the URL is from the same origin.. I think that's expected for this sort of functionality.
They're appreciated 😄
Done. |
|
@benlesh I am working through unit tests, but if you get to this before they're up feel free to leave any comments. |
|
@benlesh a bit confused by the failing checks since they don't seem to have to do with this change? |
|
The API guardian tests are to show if there's been any changes to the public API. which there were in this. If you run |
Done. I am not sure why the expected |
|
@benlesh friendly ping |
|
Sorry, big conference day today. I will get back to this soon. |
src/internal/ajax/ajax.ts
Outdated
| try { | ||
| return document?.cookie.match(new RegExp(`(^|;\\s*)(${name})=([^;]*)`))?.pop() ?? ''; | ||
| } catch (err) { | ||
| if (err instanceof ReferenceError) { |
There was a problem hiding this comment.
Further explanation of the in-lined stuff above. The try/catch isn't necessary because it's really just here to catch the ReferenceError and then rethrow. If we check cookie inline like document?.cookie?.match, then there's no possibility of a ReferenceError and the try/catch has no purpose (because we're just rethrowing).
There was a problem hiding this comment.
Optional chaining defends against TypeErrors that get thrown due to accessing properties of already initialized variables whose value is null or undefined.
In Node.js, the document variable is not even initialized, so accessing the variable document itself (even without accessing properties on that object!) throws a ReferenceError that optional chaining will not prevent.
That being said, I have thought about this some and decided that I am probably being overly-defensive. I think that it is reasonable to leave it to the user to only use xsrf*Name options in the DOM, and to allow the ReferenceError to throw if they try to use it in Node. If someone really wants it in Node then the library can always change later.
There was a problem hiding this comment.
I see what you're getting at. Thank you for the explanation. I'll try to revisit this in another pass. I suspect there are some places where we need to just do a sanity check on references for environments like node and older runtimes. And I also suspect that there's a compact way to do this.
|
So so close. Just one last set of changes to help reduce the size impact of the feature. |
|
@benlesh I addressed your comments and left a bit of explanation for my reasoning for the code I had before, hope this is what you had in mind 😄 |
|
@benlesh friendly ping |
|
Merged! Thanks for all of your hard work, @DCtheTall! We'll have this in the next 7 beta release! 🎉 🎉 🎉 |
Description:
Add the ability to send XSRF cookies in a custom header. This implementation is based off of Axios' own implementation of the same feature.
Related issue (if exists):
This fixes #4003.