Conversation
Signed-off-by: Frazer Smith <frazer.dev@icloud.com>
preHandler hookpreHandler hook
| try { | ||
| if (options.addHook === true) { | ||
| fastify.addHook('onRequest', verifyBearerAuthFactory(options)) | ||
| fastify.addHook('preHandler', verifyBearerAuthFactory(options)) |
There was a problem hiding this comment.
Why not preValidation or preParsing for instance? We shouldn't validate anything if the auth isn't there in my opinion
There was a problem hiding this comment.
That also works, was just updating the handler to correctly match the documentation. Can be changed!
There was a problem hiding this comment.
Yeah sure, I was just pointing it out :)
In any case, this would be a major right?
gurgunday
left a comment
There was a problem hiding this comment.
Can we change it to preParsing if we're going to release a major?
I think that would be best. I've just rejigged |
climba03003
left a comment
There was a problem hiding this comment.
I would update the document and make the hook configurable.
Eomm
left a comment
There was a problem hiding this comment.
While I do agree that this would be a major, I think the onRequest hook seems right to me.
Whenever we get a request, we want to ensure that it is a valid client to call us.
Example: I have an endpoint with auth configured, if I get a malicious payload of 1GB, with this change we will parse it!
So, reading the issue:
I noticed this as I was registering @fastify/bearer-auth before @fastify/cors (which is an onRequest hook) and pre-flight requests were being caught by this plugin, and auth shouldn't be applied to them.
Isn't switching the cors/auth plugins the solution?
Or at most: fix the documentation
I'm leaning more to making it configurable as climba suggested. Both |
|
Seems fair. Make it configurable but default to |
The documentation states that this plugin adds a
preHandlerhook but the code says otherwise.I noticed this as I was registering
@fastify/bearer-authbefore@fastify/cors(which is anonRequesthook) and pre-flight requests were being caught by this plugin, and auth shouldn't be applied to them.Checklist
npm run testandnpm run benchmarkand the Code of conduct