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

Remove contents of the Authorization header while dumping requests for broken pipe failure #2201

Closed
joycehuh opened this issue Jan 3, 2020 · 2 comments · Fixed by #2528
Closed

Comments

@joycehuh
Copy link

joycehuh commented Jan 3, 2020

  • With issues:

Description

#1836
This PR fix the issue partially, with brokenpipe case, the entire httpReques containing the user credentials is still being logged as error.

How to reproduce

Client cancel a downloading request will lead to broken pipe panic, and the RecoveryWithWriter method will log it as errors including the entire http request which contains sensitive user info like Authorization token etc.

Expectations

No logging of sensitive user data in the log.

Actual result

Logs contain sensitive user data when encounter broken pipe failure.

Environment

  • go version: go1.13.4
  • gin version (or commit ref): v.1.5.0
  • operating system: Linux
@zachnewburgh
Copy link
Contributor

zachnewburgh commented Oct 13, 2020

I also ran into this issue, which is caused by:

if brokenPipe {
  logger.Printf("%s\n%s%s", err, string(httpRequest), reset)

https://github.com/gin-gonic/gin/blob/master/recovery.go#L79

It seems like a potential fix could be:

if brokenPipe {
  logger.Printf("%s\n%s%s", err, strings.Join(headers, "\r\n"), reset)

Is there any update on progress to resolve this issue?

@ghoshabhi
Copy link

Thank you @zachnewburgh for fixing this! This was indeed an issue.

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 a pull request may close this issue.

3 participants