-
Notifications
You must be signed in to change notification settings - Fork 931
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
Redact request password from logs. #352
Conversation
Fixes rest-client#349 and OSVDB-117461.
Disclaimer: I have zero familiarity with this code. |
👍 |
Seems like a good start but incomplete. Password in the uri is just one way to disclose one secret. There's also api_key, api_token or whatever. These could appear as headers, cookies, or in post bodies. I too am not familiar with this gem beyond it being used as a dependency of a dependency in a project, so I'm not sure if headers or cookies are in its scope. But it seems like post bodies are definitely in scope. Additionally tokens (session token perhaps) could come back in a response which is also logged. I think what's needed is something akin to rails filter params which would be defaulted to password, api_key or other common values and be scrubbed from all logs. |
so generic filter everywhere seems a good feature, but I don't think classifies as an OSVDB level thing. I mean, I can do I have no particular interest in fixing this beyond getting it out of (Frankly, I'm disappointed an OSVDB was filed without any sort of repro or discussion of potential impact [or at least nothing actionable to those getting warnings about it], but I don't know where to direct that complaint.) |
@@ -536,7 +536,18 @@ def log_request | |||
return unless RestClient.log | |||
|
|||
out = [] | |||
out << "RestClient.#{method} #{url.inspect}" | |||
sanitized_url = begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xaviershay Might be nice to extract sanitized_url
as a method to encourage reuse. But NBD.
👍 LGTM (having little familiarity w/ this code). And I share @xaviershay's motivations/confusion about OSVDB. |
I have no dog in this fight - I'm not impacted by the vulnerability, I understand why it was opened, and also understand the frustration in how poorly it was specified. That said, this solution is incomplete. Even if you limit this to
You can tell bundle audit to skip this vulnerability with the |
Saying the solution is incomplete presumes the problem @xaviershay is trying to fix is: 'sometimes people send secrets in URIs'. While it's true that this is a problem, it's not terribly clear how to solve it at a technical level. One could imagine starting a blacklist of param names; that will always be incomplete. Also, as for your concern about headers, post bodies, and responses, it's pretty non-standard to wantonly log those. At the very least, it's less standard to log those than it is to log URIs. In contrast, this change is incredibly high yield. The credentials portion of a URI is essentially guaranteed to be sensitive, and therefore inappropriate for logging. Not logging those seems like an obvious win, and we shouldn't stop this from going out because it doesn't solve every evil about over-logging. |
Perhaps a configuration option that allows specifying the headers that should be sanitized (in addition to the password portion of the URI) might be a good option. |
Thanks for the PR, @xaviershay. This functionality looks much better. And thank you, everyone, for bearing with a fairly dubious OSVDB report. |
Version 1.8.0 * tag 'v1.8.0': Version 1.8.0 Add history notes for 1.8.0. Update version spec test. Version 1.8.0.rc1 Fix up cookie redirect functionality and tests. Add cookie jar for handling HTTP redirection. Add dependency on http-cookie gem. Tag version 1.7.3 Import history from 2.0.x branch. Fix tests for 1.7.x backport of rest-client#352. Redact request password from logs. Drop MIME::Types monkey patch. Rename Platform.mac? to .mac_mri? for clarity. Fix RestClient::Request docs when used with yard
Version 1.8.0 * tag 'v1.8.0': Version 1.8.0 Add history notes for 1.8.0. Update version spec test. Version 1.8.0.rc1 Fix up cookie redirect functionality and tests. Add cookie jar for handling HTTP redirection. Add dependency on http-cookie gem. Tag version 1.7.3 Import history from 2.0.x branch. Fix tests for 1.7.x backport of rest-client#352. Redact request password from logs. Drop MIME::Types monkey patch. Rename Platform.mac? to .mac_mri? for clarity. Fix RestClient::Request docs when used with yard
Version 1.8.0 * tag 'v1.8.0': Version 1.8.0 Add history notes for 1.8.0. Update version spec test. Version 1.8.0.rc1 Fix up cookie redirect functionality and tests. Add cookie jar for handling HTTP redirection. Add dependency on http-cookie gem. Tag version 1.7.3 Import history from 2.0.x branch. Fix tests for 1.7.x backport of rest-client#352. Redact request password from logs. Drop MIME::Types monkey patch. Rename Platform.mac? to .mac_mri? for clarity. Fix RestClient::Request docs when used with yard
Fixes #349 and OSVDB-117461.