Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Sanitize Headers on Errors #507

Merged
merged 2 commits into from
Jul 21, 2020
Merged

Sanitize Headers on Errors #507

merged 2 commits into from
Jul 21, 2020

Conversation

jimmyjames
Copy link
Contributor

Changes

Sanitize the authorization header on errors, as we do with passwords and secrets.

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage

Checklist

!error.response ||
!error.response.request ||
(!error.response.request._data && !error.response.request._header)
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a huge fan of this formatting, but the pre-commit hook did this, so 🤷

@jimmyjames jimmyjames marked this pull request as ready for review July 20, 2020 19:12
@jimmyjames jimmyjames requested review from a team and davidpatrick and removed request for a team July 20, 2020 19:12
@jimmyjames jimmyjames added medium Medium review CH: Security enhancement An enhancement or improvement to the SDK that could not be otherwise categorized as a new feature labels Jul 20, 2020
@jimmyjames jimmyjames changed the title sanitize auth headers on errors Sanitize Headers on Errors Jul 20, 2020
@davidpatrick davidpatrick added this to the v2Next milestone Jul 21, 2020
@davidpatrick davidpatrick merged commit e23cd86 into master Jul 21, 2020
@lbalmaceda lbalmaceda deleted the sanitize-bearer branch July 22, 2020 14:41
@jimmyjames jimmyjames modified the milestones: v2Next, v2.27.1 Jul 23, 2020
@jimmyjames jimmyjames mentioned this pull request Jul 23, 2020
3 tasks
}

Object.keys(collection).forEach(function(key) {
if (key.toLowerCase().match('password|secret|authorization')) {
Copy link

Choose a reason for hiding this comment

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

I'm not a big fan of such block list approach. This would miss keys such as token, bearer, auth, key and so on.

Copy link

Choose a reason for hiding this comment

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

@davidpatrick any thoughts?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CH: Security enhancement An enhancement or improvement to the SDK that could not be otherwise categorized as a new feature medium Medium review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants