-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: skip safe methods #167
base: master
Are you sure you want to change the base?
Conversation
cc17dba
to
b5ec359
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike this approach. You basically add the check that the method is in the array on every request. But this is not necessary to be done on every request. You could hook onRoute and if the route is a protected route you add csrfProtection as preParsing hook to the routeObject of the onRoute hook.
@Uzlopak I'm not very familiar with the 'onRoute' hook yet (and I hadn't thought about it :p) Is the hook triggered after plugins registration ? |
It would be a large text to describe what i mean. See following code pieces:
fastify.addHook('onRoute', onRoute) Now we need the onRoute Code which we for simplicity put into the plugin function to access the pluginOPtions opts function onRoute (routeOpts) {
const { hook = 'preValidation' } = opts // <-- pluginOptions
if (routeOpts.csrfProtect || (opts.autoProtect && opts.protectedMethods.includes(routeOpts.method)) {
routeOpts[hook] = routeOpts[hook] || []
routeOpts[hook].push(csrfProtection)
}
} If you know your csrf token is in body, you need to parse it, so you would set the hook to And of course validate if hook is a valid lifecycle hook and keep into account that onRequest and preValidation have the arity of 3 but preParsing has the arity of 4. So you could do it like here This should help you to get atleast a mvp. there are probably some improvements possible to improve the DX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good idea as @Uzlopak suggested. This could probably be a line in the readme stating that you don't need this for safe methods, and to use route encapsulation so it doesn't apply to safe methods on routes.
You can close this PR, i don't use this lib on my side, it was just to help but right now I don't have a lot of time :) |
Checklist
npm run test
and the Code of conduct
Fix issue: #166
This PR introduce the ability to pass the protected methods as module options