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

refactor: replace csurf with csrf-csrf #864

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

pano9000
Copy link
Contributor

Hi,

this PR aims to replace the deprecated csurf with the alternative package csrf-csrf.

closes #858

@pano9000 pano9000 mentioned this pull request Dec 30, 2024
3 tasks
@eliandoran
Copy link
Contributor

@pano9000 , would there be anything else left to do here? I see it's in draft.

@pano9000
Copy link
Contributor Author

pano9000 commented Jan 4, 2025

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.
Apart from that I also had some TS type issues, but they might be gone, after I implement that point above

@pano9000
Copy link
Contributor Author

pano9000 commented Jan 5, 2025

@eliandoran I am now running into another issue that appears with the logout POST request, which causes the CSRF validation of csrf-csrf (it compares the value from the cookie "_csrf" AND the "x-csrf-token" header.) to fail.

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 csrfMiddleware and which are sent via Form Submssion – which currently really just seems to be the logout action, the other routes that use "simple" form submissions do not have csrf set as middleware.

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
@pano9000 pano9000 force-pushed the refactor_replace-csurf branch from cd1a47c to dea434c Compare January 5, 2025 18:42
@pano9000
Copy link
Contributor Author

pano9000 commented Jan 5, 2025

I found a way, which feels a tad bit "hack-ish" though, but works by reusing existing code mostly:

In entrypoint.ts changing the previous form based submit

    logoutCommand() {
        const $logoutForm = $('<form action="logout" method="POST">')
            .append($(`<input type='_hidden' name="_csrf" value="${glob.csrfToken}"/>`));

        $("body").append($logoutForm);
        $logoutForm.trigger('submit');
    }

to a JS triggered one like below:

    async logoutCommand() {
        // forced to use relative path here, because `server.post` is using "/api" as base path, 
        // but we want to navigate to "/logout"
        // this regenerates the session token and currently redirects to "/login",
        // causing the function call below to return the /login page's HTML, which is useless here though
        await server.post("../logout");

        // "manually redirect" to the login page
        window.location.replace("/login");
    }

by using server.post the x-csrf-token header AND the _csrf cookie value is sent to the server, and the csrf validation passes

@eliandoran
let me know if that would be an OK solution – currently I have this change above locally only.

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.

deps: csurf package is deprecated and should be replaced
2 participants