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

Properly report ring request parameters #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jeremyvdw
Copy link
Contributor

@jeremyvdw jeremyvdw commented Nov 21, 2017

As of version 1.0.131, the parameters of the request is not reported to Rollbar.

From ring wiki, base request parameters look like this:

{:http-method :get
 :uri "/search"
 :query-string "q=clojure"}

It would then make sense that those are to be reported to rollcage client like:

{:url "/search"
 :params {:http-method :get
          :query-string "q=clojure"}

Also ring's wrap-parameters middleware adds three keys to the map: :query-params, :form-params and :params. As a large number of people are using this middleware, it sounds reasonable to add those as arguments sent to Rollbar.

@gordonsyme
Copy link
Member

gordonsyme commented Nov 22, 2017

Thank you for the PR!

I do have some concerns about sending query string/parameters and form parameters in the general case, it's fairly common for those to contain secrets such as API tokens or other credentials.

@jeremyvdw
Copy link
Contributor Author

@gordonsyme I do agree, that's a perfectly valid concern.

Official rollbar-gem tackle this issue with a configurable scrubber: https://github.com/rollbar/rollbar-gem/blob/efe6468af071f0a37751033dc9e35dbec224be6c/lib/rollbar/configuration.rb#L103

To keep changes minimal, I can propose to update :forms-params and :params to obfuscate values of a defined list of keys (i.e: :password, :password_confirmation, :secret, etc..).

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.

2 participants