Set csrf cookies on home page #263
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
charmingduchess
changed the title
| "Set-Cookie", | ||
| `nyplUserRegistered=false; Max-Age=-1; path=/; domain=${cookieDomain};` | ||
| ); | ||
| let newTokenCookie; |
There was a problem hiding this comment.
This is the only actual logical update, the rest of the PR is just a refactor with no real functional changes. I don't like how this looks, any suggestions on how to clean it up would be appreciated.
pages/new/index.tsx
Outdated
| generateNewCookieTokenAndHash, | ||
| generateNewToken, | ||
| parseTokenFromPostRequestCookies, | ||
| } from "../../src/utils/utils"; |
There was a problem hiding this comment.
this should be csrfUtils. That's why the vercel preview fails.
pages/new/index.tsx
Outdated
|
|
||
| import { serialize } from "cookie"; |
There was a problem hiding this comment.
nit: this is imported in CookieUtils as well. Just to keep things tidy and in one place, export serialize from CookieUtils so that you can import it from the file and then call cookie.serialize.
pages/new/index.tsx
Outdated
| if (!csrfToken) { | ||
| csrfToken = generateNewToken(); | ||
| const newTokenCookieString = generateNewCookieTokenAndHash(csrfToken); | ||
| newTokenCookie = serialize( |
There was a problem hiding this comment.
If you do remove this export from cookie directly from this file and move it to CookieUtils, you can create a new "serialize" function that only needs the newTokenCookieString argument since metadata is already in the same file.
There was a problem hiding this comment.
brilliant! thanks. i knew there was something fishy going on there
| return { | ||
| createHash: jest.fn().mockReturnValue({ | ||
| update: () => ({ | ||
| digest: () => "666", |
Description
Motivation and Context
The cookie setting in the getServerSideProps of the index page was overwriting the csrf token cookie headers. I refactored the csrf code to be more functional and to avoid unannounced side effects, in order for different flows to update the headers in different ways.
How Has This Been Tested?
All existing unit tests are passing, and I also updated the tests for the validateCsrf methods. I did not exhaustively unit test all of the more minor functions, mostly because they seem to be invoked in larger tests. I do think there is some gaps in the coverage in the validateAddress tests, which I will work on while this is being reviewed.
In order to properly run this code locally (and on live servers as well), you must start with deleting all cookies from the relevant domain. Part of why I missed this in the first place is I had been testing over and over again without deleting cookies from older sessions.
Checklist: