Removing CSRF token throughout the form #268
Conversation
EdwinGuzman
requested review from
mwbenowitz,
samanthaandrews and
charmingduchess
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| try { | ||
| const response = await callPatronAPI(data, createPatronUrl, appObj); | ||
| console.log("resonse", response); |
There was a problem hiding this comment.
nit: remove stray console.log()
| @@ -16,7 +16,7 @@ jest.mock("crypto", () => { | |||
| }; | |||
| }); | |||
|
|
|||
| describe("validateCsrfToken", () => { | |||
| describe.skip("validateCsrfToken", () => { | |||
There was a problem hiding this comment.
Yes, I was debating keeping it or not but I'll just remove.
There was a problem hiding this comment.
Just one question, but very possible I just missed part of this PR.
One thing that's been bothering me with this is: why CSRF here, it feels like a misaligned use of the standard. I think what we really need in the API is a CORS check? Perhaps that's already happening, but I'm just trying to wrap my head around what exactly we want to see here.
| @@ -60,7 +60,6 @@ const AddressContainer: React.FC = () => { | |||
| .post("/library-card/api/address", { | |||
| address: homeAddress, | |||
| isWorkAddress: false, | |||
| csrfToken, | |||
There was a problem hiding this comment.
I don't see this being checked/parsed anywhere. Does that happen automatically in the HTTP client?
Also, just to be sure, this argument/parameter isn't required by the endpoint? Don't want anything to crash because this suddenly isn't being passed
There was a problem hiding this comment.
It's parsed in a backend utility function.
This might be a moot point and I think I'll end up closing this PR to keep CSRF set. @nonword @charmingduchess can you give more details on why we should keep it in the app?
There was a problem hiding this comment.
I agree with Mike! If there is something obvious to replace it with, I think it would make sense to.
Description
Removes CSRF token initialization and verification for username, address, and the final form submission.
NOTE: Vercel never worked so that check can be ignored.
Motivation and Context
@charmingduchess noticed we weren't really using CSRF properly in this app.
Additionally, this issue is blocking testing the app on QA which is a problem because devops updated the RDS DB's postgres version. We cannot test and verify that their update is working if we cannot submit the form.
This works for me locally.
How Has This Been Tested?
Ran this app and the Card Creator API locally. The CC API points to the QA environment which hits the QA database.
Checklist: