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

fix(e2core): Remove logging of sensitive data about a request #424

Merged
merged 1 commit into from
May 3, 2023

Conversation

javorszky
Copy link
Contributor

No issue

Found the place where the logging of the entire request happened which included tokens and request body and removed it.

Copy link

@callahad callahad left a comment

Choose a reason for hiding this comment

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

Looks like we still potentially log the unredacted msg.Data() if JSON unmarshalling fails.

But more broadly... this seems kind of blunt. I assume we still want to log the request, but just omit the Authorization header and the message body.

(I'm also not sure why we're calling fmt.Println() instead of using a method on the ll struct.)

@javorszky
Copy link
Contributor Author

If unmarshaling from json fails, it's probably because whatever is in msg.Data is not a json, and it would be super useful to know what was in there that caused the json unmarshaler to fail.

I think that fmt.Println was a remnant of a debug session I was doing and I forgot to take it out. With this changeset it is no longer there.

@callahad
Copy link

callahad commented May 3, 2023

I'm still a bit torn (at least we should elide the authorization header), but this PR is strictly better than the status quo, so let's roll with it

@javorszky javorszky merged commit d13b16e into main May 3, 2023
@javorszky javorszky deleted the gabor/remove-tokens-from-logs branch May 3, 2023 18:49
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.

3 participants