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

Issue 844: Redact api token in chat message #1102

Closed
wants to merge 1 commit into from

Conversation

briemarie
Copy link

@briemarie briemarie commented Aug 24, 2022

Debug logs are printing api tokens as part of the request message.
Instead of printing the token, the token is transformed into a redacted
string if it is not empty. If the token is empty, which is a useful
piece of information that should be surfaced in the request, the empty
string is transformed into a nil string to make it more obvious.

Pull Request Guidelines

These are recommendations for pull requests.
They are strictly guidelines to help manage expectations.

PR preparation

[PASSED] Run make pr-prep from the root of the repository to run formatting, linting and tests.

Should this be an issue instead - YES IT IS #844
  • is it a convenience method? (no new functionality, streamlines some use case)
  • exposes a previously private type, const, method, etc.
  • is it application specific (caching, retry logic, rate limiting, etc)
  • is it performance related.
API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

  • no tests, if you're adding to the API include at least a single test of the happy case.
  • If you can accomplish your goal without changing the API, then do so.
  • dependency changes. updates are okay. adding/removing need justification.
Examples of API changes that do not meet guidelines:
  • in library cache for users. caches are use case specific.
  • Convenience methods for Sending Messages, update, post, ephemeral, etc. consider opening an issue instead.

Debug logs are printing api tokens as part of the request message.
Instead of printing the token, the token is tarnsformed into a redacted
string if it is not empty. If the token is empty, which is a useful
piece of infomation that should be surfaced in the request, the empty
string is transformed into a nil string to make it more obvious.
Copy link

@christophercutajar christophercutajar left a comment

Choose a reason for hiding this comment

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

LGTM

@briemarie
Copy link
Author

@christophercutajar I see you approved this. Will it be getting merged or was there another release that fixed the issue of the token being exposed in logs?

@christophercutajar
Copy link

@kanata2 when you have time can you please review it yourself and merge it!

Comment on lines +250 to +255
switch token {
case "":
token = "nil"
default:
token = "<REDACTED>"
}
Copy link
Member

Choose a reason for hiding this comment

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

If the token is rewritten, will it not be able to authenticate correctly?

Copy link
Member

Choose a reason for hiding this comment

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

This changes affect other than debug logs.

@kanata2 kanata2 added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Oct 31, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale label Apr 5, 2023
@briemarie
Copy link
Author

kanata2 I realize that my proposed solution wasn't the proper one, but has this been solved internally by your team in another PR?

@briemarie briemarie closed this Apr 5, 2023
@briemarie briemarie deleted the briemarie-issue-844 branch April 5, 2023 22:55
@kanata2
Copy link
Member

kanata2 commented Apr 5, 2023

No, we have not worked on it yet.
If you think the labeling by GitHub Actions is inappropriate, please re-open it and reply to my comment.

@briemarie
Copy link
Author

@kanata2 Its ok that this PR is closed since it wasn't the right solution. Looks like the original issue #844 is still open so hopefully that gets addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback given When a review has been conducted and awaiting the response from the comitter(s) stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants