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

PP-13360 csrf express middleware #1314

Merged
merged 2 commits into from
Dec 17, 2024
Merged

PP-13360 csrf express middleware #1314

merged 2 commits into from
Dec 17, 2024

Conversation

nlsteers
Copy link
Contributor

WHAT

  • express middleware for handling csrf

@nlsteers nlsteers force-pushed the pp-13360/csrf_module branch from 33dbf7e to 74ecebb Compare December 16, 2024 16:47
Copy link

@DomBelcher DomBelcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, just a few thoughts

src/utils/middleware/csrf.middleware.js Outdated Show resolved Hide resolved
src/utils/middleware/csrf.middleware.js Outdated Show resolved Hide resolved
src/utils/middleware/csrf.middleware.js Outdated Show resolved Hide resolved
*/
const checkToken = (req, res, next) => {
const csrfSecret = req[sessionKey][secretKey]
const csrfToken = (req.body && req.body[tokenKey]) || (req.query && req.query[tokenKey])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be possible to pass the csrf token in a query paramter? since the validation is only enforced on PUT or POST operations I think it should only ever be expected in the body, unless there's a case where we are passing in a query parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glad you asked :D, in the case of multipart/form-data encoded POSTs we don't have (easy) access to the body when validating the token so the token is passed as a query parameter. Basically anywhere we use a file input type

src/utils/middleware/csrf.middleware.js Outdated Show resolved Hide resolved
@nlsteers nlsteers force-pushed the pp-13360/csrf_module branch from 9f1595d to 9a153c6 Compare December 16, 2024 19:00
Copy link

@DomBelcher DomBelcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

@nlsteers nlsteers merged commit e91528f into master Dec 17, 2024
4 checks passed
@nlsteers nlsteers deleted the pp-13360/csrf_module branch December 17, 2024 10:18
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.

2 participants