-
Notifications
You must be signed in to change notification settings - Fork 77
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
refactor: replace csurf with csrf-csrf #864
base: develop
Are you sure you want to change the base?
Conversation
@pano9000 , would there be anything else left to do here? I see it's in draft. |
yeah, I was waiting for some feedback from the original developer of the csrf-csrf module regarding the secret usage – I did get a reply a couple of days ago, so I will be continuing on this soon again. |
@eliandoran I am now running into another issue that appears with the logout POST request, which causes the CSRF validation of Unfortunately the logout action uses standard form submission via POST, which is not capable of including custom HTTP headers, like the required "x-csrf-token" – this then causes a "Invalid CSRF Token" 403 error, because the values of cookie and custom Header are of course not identical when compared with each other (since the x-csrf-token value is undefined). This affects all routes, that use the I need to do some additional checking here on how to handle this the best way |
I've kept the identical same settings as before – however they are not *ideal* from what I read. More secure settings will need to be tested a bit more thoroughly first and will be a separate PR.
since `cookie-parser` is not configured with a secret, req.secret is not set and hence is `undefined`, which then is used as literal 'undefined' in the hashing function – making it less secure. Instead we can use the existing sessionSecret: the `csrf-csrf` developer confirmed in their Discord chat, that it would be ok to use the same secret here.
`req.csrfToken` might be undefined according to `csrf-csrf` provided types, so use type narrowing to make sure it exists, before calling it
cd1a47c
to
dea434c
Compare
I found a way, which feels a tad bit "hack-ish" though, but works by reusing existing code mostly: In
to a JS triggered one like below:
by using @eliandoran |
Hi,
this PR aims to replace the deprecated
csurf
with the alternative packagecsrf-csrf
.closes #858