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

Don't call .toString on ResponseLogger input #3488

Merged
merged 2 commits into from Jun 23, 2020

Conversation

rossabaker
Copy link
Member

@rossabaker rossabaker commented Jun 11, 2020

We should not call toString on the input to ResponseLogger, because it will usually be a request, and we don't have the means to redact non-default sensitive information.

Fixes #3482

@rossabaker rossabaker added the bug Determined to be a bug in http4s label Jun 11, 2020
@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jun 11, 2020

Correct me if I'm wrong, but we're already not logging anything from the request on the happy path, so are we making things worse not logging the request on the unhappy path?

@kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Jun 14, 2020

Correct me if I'm wrong, but we're already not logging anything from the request on the happy path

Can you please say more on "not logging anything from the request on the happy path," @rossabaker? Are you referring to https://github.com/http4s/http4s/pull/3488/files#diff-8d48b95cb07b5444f253909f24b34efcR71?

@@ -67,7 +67,7 @@ object ResponseLogger {
}
.guaranteeCase {
case ExitCase.Error(t) => fk(log(s"service raised an error: ${t.getClass}"))
case ExitCase.Canceled => fk(log(s"service cancelled response for request [$req]"))
case ExitCase.Canceled => fk(log(s"service canceled response for request"))
Copy link
Contributor

@kevinmeredith kevinmeredith Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion, per #3482 (comment), log "service canceled response for request: ${req.method} ${req.pathInfo}." rather than throw away all details of the request via "service canceled response for request".

Regarding:

But we'd also be fighting against people who put a request ID in their headers and are using that to correlate the logs.

Or, if you modify the toString of Request, perhaps you could include headers' values that are definitely safe to log? Does such a list of universally non-sensitive HTTP Request Headers exist?

Copy link
Member Author

@rossabaker rossabaker Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that it's not a Request, it's an A. We can't refer to method and pathInfo here without pattern matching. If we pattern match, we need to handle ContextRequest. If we handle ContextRequest, it raises questions about other request wrappers. The only failsafe tools we have are to:

  • Not log anything about A
  • Log A's type. Not useful.
  • Log A's .toString, and change Request's .toString definition. Nobody had an appetite for this.

The default Request#toString does redact a few well-known headers, but many people, including the issue reporter, have additional sensitive headers. We can't get them all without some global system property, which, no.

Copy link
Contributor

@kevinmeredith kevinmeredith Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @rossabaker. Given the risk of #3482, is it better to use this PR's updated String instead of possibly leaking sensitive headers' values?

In other words, it seems to me that merging this PR is worth it to avoid logging sensitive headers' values.

Thoughts?

Copy link
Member Author

@rossabaker rossabaker Jun 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with either this approach or changing Request#toString and picked the former just to get a fix out there.

I'm wary of pattern matching, which only makes the leak more rare without actually fixing it.

satorg
satorg approved these changes Jun 23, 2020
Copy link
Contributor

@satorg satorg left a comment

I'd approve it since it definitely won't make things worse.

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jun 23, 2020

Yep, can always adopt the toString approach later if people prefer it. Let's get the bug fixed.

@rossabaker rossabaker merged commit 08a1979 into http4s:series/0.21 Jun 23, 2020
9 of 10 checks passed
@kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Jun 23, 2020

Nice change, Ross.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Determined to be a bug in http4s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants