[Feature Proposal] Configuration flag to conceal request body on parse errors

[aaronlahey]

I opened an issue here (ActionDispatch::Http::Parameters reveals sensitive information in exception messages and logs on parse error · Issue #41145 · rails/rails · GitHub) because we’re noticing that requests containing malformed JSON could potentially log sensitive information. One solution that came to mind is to introduce a flag like this…

Rails.application.configure do
  #
  # Defaults to false in test and development.
  # Defaults to true in any other environment.
  #
  config.action_dispatch.conceal_request_body_on_parse_errors = true
end

This would prevent log messages (even at the debug level) from containing information like this…

Error occurred while parsing request parameters.
Contents:

{"user":{"ssn":"secret!"}

…and ActionDispatch::Http::Parameters::ParseError messages from containing this (which could be logged in a piece of catch-all middleware)…

783: unexpected token at '{"user":{"ssn":"secret!"}'

I’m not familiar enough with Rails to know if that’s the best solution to the problem, but it felt like a new feature that could benefit from some feedback. Thanks!

[markvanlan (Mark VanLandingham)]

If the json being parsed in misformed, you won’t be able to sensibly parse it to hide sensitive key/value pairs, right? You could do something ugly like hiding the entire malformed param if it contains some string (“ssn” for example), but this sounds like a one-off issue not something that would be in core. It just doesn’t sound sensible to try to parse/hide malformed json.

[aaronlahey]

That makes sense! I agree that attempting to hide or show the malformed JSON by trying to do substring matching sounds complicated and imperfect. I feel like a better approach is to assume that malformed JSON could contain sensitive information in production environments and avoid logging it at all. That’s what I’ve tried to implement in this PR: https://github.com/rails/rails/pull/41149. What are your thoughts on that approach?

Here is my rationale for something like this being in core:

I think the default out-of-the-box behavior of Rails should be to be secure by default in production environments. If I avoid raising errors with messages containing sensitive information, avoid explicitly logging sensitive information, and lean on the provided tools for redacting sensitive information (filtered parameter logging), I should have a high degree of confidence that my logs will be free of sensitive information. In other words, I should be able to assume that no unforeseen edge cases such as malformed JSON will cause SSNs or passwords to leak into logs. More concretely, I don’t think developers should need to remember to special case ActionDispatch::Http::Parameters::ParseError to avoid having the security of their logs dependent on the correctness of the consumers of their API.

[markvanlan (Mark VanLandingham)]

I’m not someone who can actually make this kind of decision but…

It makes sense to have this as an option, but it should be false by default (not hiding the params). I agree with the sentiment that apps should be secure by default, but I don’t think this an example of insecurity in general. I imagine most rails applications do not have the issue with application logs revealing sensitive info. Let’s see what more important people think

[aaronlahey]

Thanks for the input, @markvanlan.