seriousme / fastify-openapi-glue

A plugin for the Fastify webserver to autogenerate a Fastify configuration based on a OpenApi(v2/v3) specification.
MIT License
196 stars 33 forks source link

Multiple Security Handlers Error Handling #557

Closed AlexBroadbent closed 5 months ago

AlexBroadbent commented 5 months ago

I'm having some trouble with excessive logging when I have a resource which has two security options. The expected case of if one passes then the request is allowed, but I have logging in both security handlers that will log an error. Because one security handler can't tell if the other one will/has pass, it's assumed to be a fail and so the error is logged.

A simplistic use case:

paths:
  "/resource":
    get:
      summary: Get Resource
      operationId: getResource
      security:
        - api_key:
            - resource:read
        - api_token:
            - resource:read

In this case, my api_key and api_token functions will be called and they are both a very simple implementation of:

export const api_key = (request, reply, params) => if (!isRequestValid(request, params)) throw new Error("Invalid request");
export const api_token = (request, reply, params) => if (!isRequestValid(request, params)) throw new Error("Invalid request");

Authenticating the request with a valid API key will throw an error from the api_token handler and having a vice versa with a valid API token.


Is there any way that you could pass through additional context into the security handlers to handle this, ie. if there are more than one handlers validating the request, only the last handler to be run would throw an error and the rest would just log that they don't match silently. @seriousme

seriousme commented 5 months ago

Hi Alex,

thanks for asking! Not sure what the exact question is.
The securityHandlers code only logs a securityhandler failure as debug:

https://github.com/seriousme/fastify-openapi-glue/blob/cde27cf37b4c9cc526453c3fb9cd50ad9d2f5cfd/lib/securityHandlers.js#L73

Do you want to be able to disable this debug log line? { apart from changing the debug level of fastify)

Kind regards, Hans

AlexBroadbent commented 5 months ago

Hi Hans,

Thanks for getting back to me so quickly on this.

The issue is more that the authentication method throws an exception which we then log ourselves because we don't know at the point of logging it if the request will be successful from the other authentication method on the request.

So the logging contains an error from one authentication method, but the actual request was successful and has the logs from the service completing the request. Which looks suspect when you're browsing the logs.

Does that make sense?

Kind regards, Alex

mhamann commented 5 months ago

I think it's ideal to keep the security handling functions relatively pure and avoid unnecessarily leaking context between them. It's also important that failing handlers throw errors in order to keep the contract consistent and prevent security vulnerabilities.

I understand you wanting to remove noise from your logging, but I think that falls outside the scope of this plugin.

What you could do instead is use the request object to collect context across handlers.

For example, when the request arrives, add an object to the request:

fastify.addHook('preHandler', async (req: FastifyRequest) => {
  req.securityStatus = {}
})

Now, in your security handler, set the result of each handler as it's processed:

const verifyApiKey = async (req: FastifyRequest) => {
  // It didn't work
  req.securityStatus['verifyApiKey'] = { status: 'failed', reason: 'api key not found' }
  throw new Error('API key validation failed')
})

const verifyApiToken = async (req: FastifyRequest) => {
  // It worked!
  req.securityStatus['verifyApiToken'] = { status: 'passed' }
})

Now you can use this information in your logger to decide what to log.

Thoughts?

AlexBroadbent commented 5 months ago

Thanks mhamann, I was thinking that would be the way to resolve this. Just figured if there's a use case for the library to handle it then it might be helpful.

I'll close this issue and this can be the workaround for anyone with the same use case. Thanks both.