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

Add x-oauth-basic nosec annotation & address gosec unhandled errors #719

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

NickMeves
Copy link
Member

@NickMeves NickMeves commented Aug 10, 2020

Add a #nosec annotation to hardcoded x-oauth-basic password & handle finicky unhandled errors that weren't caught in a previous merge

Description

Gosec cleanup

Motivation and Context

Other PRs are getting intermittent Gosec issues.

How Has This Been Tested?

Unit Tests

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@NickMeves NickMeves requested a review from a team as a code owner August 10, 2020 22:14
@NickMeves
Copy link
Member Author

@gargath Do you mind taking a look at the logger.go changes? It looks like in the merge we ended up with a double \n in the standard logger (one to the buffer, one to the writer handler directly).

Gosec stopped me from just fixing up the annotation cause of the unhandled errors in logger (hence the little bit of scope creep). I'm disappointed its more fickle than we had hoped 😢

@wonderhoss
Copy link
Collaborator

Yeah, that makes sense. My bad for not spotting that. Everything in formatLogMessage() should go to the buffer, obviously.

@NickMeves
Copy link
Member Author

Yeah, that makes sense. My bad for not spotting that. Everything in formatLogMessage() should go to the buffer, obviously.

No worries! I think the Gosec changes that touched the logger a bit heavily happened right before your logging change merged.

Thanks for giving this a peek 👍

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

@NickMeves I thought we had added gosec to the linter? Should this not have blocked the previous merge?

@NickMeves
Copy link
Member Author

@JoelSpeed I'm more confused that people are getting blocked by Gosec findings that didn't pop up for me when I ran manually (I didn't get the hardcoded password issue for x-ouath-basic when running locally).

I'll keep an eye on this to make sure it isn't too much of a burden if it isn't consistent.

@NickMeves NickMeves merged commit 35ed7a3 into oauth2-proxy:master Aug 11, 2020
@NickMeves NickMeves deleted the gosec-x-oauth-basic-skip branch August 11, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants