Skip to content
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

Add option for omitting request data from Faraday exceptions #1526

Merged

Conversation

ClaytonPassmore
Copy link
Contributor

Description

Adds an option to the :raise_error middleware that prevents request data from being included in the generated Faraday exception.

Request data can include sensitive headers (e.g. Authorization) or other request parameters. Uncaught exceptions tend to make their way into bug trackers, which is not a place where you want sensitive information to go!

In order to prevent request data from being included in Faraday exceptions, you can now configure the :raise_error middleware so that it is omitted:

Faraday.new do |faraday|
  faraday.raise_error, include_request: false
end

Todos

  • Documentation

Additional Notes

Request data started being bundled in Faraday exceptions in #1181.

To prevent this from becoming a breaking change, I preserved the existing behaviour - request data is included by default.

That said, I would love to see request data omitted by default in a future major version. As a user of this gem, I don't expect "response" data to include "request" data.

@iMacTia
Copy link
Member

iMacTia commented Sep 12, 2023

Thank you for the addition @ClaytonPassmore, this makes sense to me.

I'd be happy to merge this in and cut a new release, but first I'd like to add some documentation to the middleware page 🙏

I'll need to think a bit more about the "make this a default in the next major version", as I think it's a hard balance to strike. It's true that the request headers/body might contain sensitive data, but this is not logged automatically and if you send data to a 3rd party service (e.g. error tracker) then I'd expect you to trust that as well, or at least the data to have a controlled retention period.
Making this default to false in a future major release could do more harm than good, I'll need to ponder it a bit more.

Just an observation: rather than removing the whole request from the exception, do you think we could do something smarter and make it so that you can filter just the data you need to?
For example, the logger middleware allows you to define filters to strip things like passwords from logs. That's an easier case since we're dealing with strings, but maybe we can apply the same concept here in a similar way as well?

@ClaytonPassmore
Copy link
Contributor Author

Thanks for the reply @iMacTia! I added some documentation to the middleware page as requested 👍

Making this default to false in a future major release could do more harm than good, I'll need to ponder it a bit more.

That's totally fair. Having the ability to configure the behaviour is enough to satisfy my needs 👌

Just an observation: rather than removing the whole request from the exception, do you think we could do something smarter and make it so that you can filter just the data you need to?
For example, the logger middleware allows you to define filters to strip things like passwords from logs. That's an easier case since we're dealing with strings, but maybe we can apply the same concept here in a similar way as well?

I had a very similar thought when I first approached this problem. One of the challenges I encountered is that there are so many different parts of a request that are formatted differently - headers, body params, and query params. I wasn't sure if it made sense to have separate filter options for each type of field - e.g.

Faraday.new do |f|
  f.raise_error,
    header_filters: ["Authorization", "X-Private-Key"],
    query_param_filters: ["api_key", /.*_?email/],
    body_filters: [
      /"api_key":("[^"]+")/,  # E.g. JSON
      /<email>([^<]+)</email>/  # E.g. XML
    ]
end

The filtering for body parameters felt extremely cumbersome to specify - I think it's a little too easy to specify a pattern that ends up breaking the content type or doesn't entirely replace the sensitive content.

As a result, I went with the "big hammer" approach rather than the "finesse" approach 😄 Definitely open to your thoughts on this though!

@iMacTia
Copy link
Member

iMacTia commented Sep 12, 2023

Fair enough, I think if we want to add a more "granular" approach later on it won't clash with the one proposed in this PR, so happy to get this in for the time being 👍

@iMacTia iMacTia merged commit 5ccddaa into lostisland:main Sep 12, 2023
8 checks passed
@ClaytonPassmore ClaytonPassmore deleted the update/raise-error-request-option branch September 12, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants