Avoid leaking exception messages in response body

[davidor]

Fixes #153

[unleashed]

Rack and/or Sinatra should already be doing this. Have you tried running with APP_ENV=production and RACK_ENV=production in the environment?

[davidor]

@unleashed Yes, I've tried with those.

Our code simply raises the error, puma catches it, and when configured with RACK_ENV=production, it replaces the exception message with a generic one to avoid leaking potentially sensitive information: https://github.com/puma/puma/blob/81c2ccb6d4c040de03119109d8784ef28f04035e/lib/puma/server.rb#L770

What I'm proposing in this PR is to do that work in our application. The reason is that Falcon, unlike Puma, does not replace the error message with a generic one.

[unleashed]

What I'm proposing in this PR is to do that work in our application. The reason is that Falcon, unlike Puma, does not replace the error message with a generic one.

I know what you are trying to do. What I am saying is that Sinatra and Rack should be able to handle this for us, but we might have the wrong configuration (and handling). I think the code we have for dealing with exceptions can be added to a Sinatra/Rack error handling mechanism rather than in custom middlewares. For example in: https://github.com/3scale/apisonator/blob/v2.99.0/lib/3scale/backend/listener.rb#L9

...we could do this:

configure :production do
  disable :raise_errors
end

...and figure out what is breaking and fix it. We could add this to a new issue to tackle later if you want. See :raise_errors in http://sinatrarb.com/configuration.html.

As for this workaround, why can't this be added just for Falcon?

[davidor]

We can't just disable :raise_errors with our codebase. Currently, we rely on raising those errors, so the ExceptionCatcher middleware can catch the Backend::Error ones (ApplicationKeyInvalid, UserKeyInvalid, etc.) and set the appropriate status code and message.

After that step, the logging middleware writes the error and later, the external logging middleware sends it to bugsnag/airbrake.

With that structure, I think that the easiest solution is the one I proposed. Add another middleware at the end of the chain that checks if something raised, and if so, sets a generic response message instead of bubbling the exception up to Puma/Falcon and relying on them to set the error.

[unleashed]

That's why I said to open a new issue and analyze how to best use the facilities instead of adding more middlewares. Btw, Falcon should have such a provision, like Puma and Thin have, so maybe open an issue there as well?

Wouldn't the easiest thing be to replace https://github.com/3scale/apisonator/blob/v2.99.0/lib/3scale/backend/logging/middleware.rb#L35 with:

rescue Exception => e
  raise e if !env.production?
  [500, "internal server error"]
end

Alternatively, when setting up each server, we could provide server specific middlewares. In any case, up to you now.

[davidor]

@unleashed I opened a PR a while ago in Falcon, but it has not been merged.

I don't like the idea of dealing with this in the logging middleware as you propose. First, it should be in the external middleware logging rather than in the logging middleware. Also, I don't think it should be the responsibility of either of those 2 to change the response body. I'd rather make this explicit even if it means adding an extra trivial middleware. I think that ideally this should be in the exception catcher one, but that requires more work and potentially other problems. I don't think reorganizing all of that is worth it at this point just to add such a small feature.

[unleashed]

@davidor you made the point of the middleware being the easiest, not what is or not something to like - that is just doing an ugly hack, just like the way we are handling these now, which is why I proposed to open an issue to investigate options. :)