Redact request password from logs.

[xaviershay]

Fixes #349 and OSVDB-117461.

[xaviershay]

Disclaimer: I have zero familiarity with this code.

[phlipper]

👍

[derekprior]

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.

[xaviershay]

so generic filter everywhere seems a good feature, but I don't think classifies as an OSVDB level thing. I mean, I can do $stdout.puts "somepassword" anywhere I like... This PR should just cover filtering things we know for sure are secret. So perhaps api key is in relevant, if someone can give me a list I'll patch them.

I have no particular interest in fixing this beyond getting it out of bundle audit.

(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.)

[ktheory]

@xaviershay Might be nice to extract sanitized_url as a method to encourage reuse. But NBD.

[ktheory]

👍 LGTM (having little familiarity w/ this code).

And I share @xaviershay's motivations/confusion about OSVDB.

[derekprior]

this PR should just cover filtering things we know for sure are secret. So perhaps api key is in relevant, if someone can give me a list I'll patch them.

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 password and api_key only this solution still does nothing about filtering from headers, post and put bodies or responses.

I have no particular interest in fixing this beyond getting it out of bundle audit

You can tell bundle audit to skip this vulnerability with the --ignore flag. This is what I have done after confirming I am not affected.

[nealharris]

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.

[hotgazpacho]

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.

[ab]

Thanks for the PR, @xaviershay. This functionality looks much better.

And thank you, everyone, for bearing with a fairly dubious OSVDB report.