Skip to content

Conversation

@ecrupper
Copy link
Contributor

Addresses security concern that build author emails, which can be changed within GitHub enterprise to be a non-work email, were being logged. With this change, all http logs with the build as the body will now censor the email field.

Abstracted this routine into a sanitize function, which can be used in case there are future concerns with logging practices.

@ecrupper ecrupper self-assigned this Jun 29, 2022
@ecrupper ecrupper requested a review from a team as a code owner June 29, 2022 18:53
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #667 (b55ed1d) into master (7d6c7d5) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
+ Coverage   55.07%   55.10%   +0.02%     
==========================================
  Files         201      201              
  Lines       15952    15960       +8     
==========================================
+ Hits         8786     8794       +8     
  Misses       6788     6788              
  Partials      378      378              
Impacted Files Coverage Δ
router/middleware/logger.go 95.89% <100.00%> (+0.50%) ⬆️

plyr4
plyr4 previously approved these changes Aug 4, 2022
@ecrupper ecrupper changed the title chore(logger): censor build author email for security compliance feat(logger): censor build author email for security compliance Aug 9, 2022
wass3r
wass3r previously requested changes Aug 9, 2022
Copy link
Collaborator

@wass3r wass3r left a comment

Choose a reason for hiding this comment

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

the lack of coverage in the report threw me off for a min.

when you do sanitizeBuild := b and modify a field, it will modify for both b and santizeBuild, since it's a pointer. you can get past it by dereferencing like sanitizeBuild := *b but you will see the test fail then. turns out you never get past the if m, ok := body.(map[string]interface{}); ok check and always just return the original body.

if we can fix up the tests, this is good to go, i think.

@ecrupper
Copy link
Contributor Author

ecrupper commented Aug 9, 2022

the lack of coverage in the report threw me off for a min.

when you do sanitizeBuild := b and modify a field, it will modify for both b and santizeBuild, since it's a pointer. you can get past it by dereferencing like sanitizeBuild := *b but you will see the test fail then. turns out you never get past the if m, ok := body.(map[string]interface{}); ok check and always just return the original body.

if we can fix up the tests, this is good to go, i think.

Good catch!

@ecrupper ecrupper dismissed wass3r’s stale review August 15, 2022 15:51

Changes made

@ecrupper ecrupper merged commit 8672cbc into master Aug 18, 2022
@ecrupper ecrupper deleted the chore/secure-email-in-logger branch August 18, 2022 21:29
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.

4 participants