NO-ISSUE: Remove credentials and tokens from debug logs - #563
Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
Summary by CodeRabbit
WalkthroughCentralizes error logging for token form requests in TokenSource by removing a local wrapped logger and calling s.logger.ErrorContext directly; sendTokenForm now logs failures as “Failed to send token form”. Individual OAuth flows (code, device, credentials, password) drop redundant per-flow error logs and standardize debug-field names (prefixing request/URI fields with Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/oauth/oauth_token_source.go`:
- Around line 805-810: The centralized token-form failure log call using
s.logger.ErrorContext currently omits the error variable; update the
ErrorContext invocation (the call that logs "Failed to send token form") to
include the actual error (err) as a field (e.g., slog.Any or slog.Any("err",
err) or slog.Error) alongside the existing "endpoint" and "!request" fields so
the log contains the root-cause information for send/token form failures; modify
the ErrorContext call in oauth_token_source.go where s.logger.ErrorContext is
invoked to add the err argument.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: bd550b31-7b20-41c8-90e5-241ee3b6f965
📒 Files selected for processing (5)
internal/oauth/oauth_code_flow.gointernal/oauth/oauth_credentials_flow.gointernal/oauth/oauth_device_flow.gointernal/oauth/oauth_password_flow.gointernal/oauth/oauth_token_source.go
💤 Files with no reviewable changes (2)
- internal/oauth/oauth_credentials_flow.go
- internal/oauth/oauth_password_flow.go
|
/retest |
The OAuth flow implementations were logging token requests and responses at debug and error levels, which could expose sensitive data such as client secrets, passwords, and access tokens. This removes those log statements from the individual flow files and from the `sendForm` method, where the form data was attached to a local logger context. Error logging for failed token requests is now centralized in `sendTokenForm`, with the request marked as sensitive using the `!` field name prefix. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
2efb506 to
6101c0f
Compare
|
/retest |
|
@jhernand: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
sensitive data (client secrets, passwords, access tokens, refresh tokens) to
debug and error logs.
sendFormlogger context, where it wasattached to a local logger and could surface credentials in every subsequent
log line.
sendTokenFormwith the requestmarked using the
!field name prefix so it is redacted by default.!prefix to the redirectqueryfield in the authorization codeflow and to
verification_uri/verification_uri_completein the deviceflow, since these can embed authorization codes or user codes.
Test plan
ginkgo run internal/oauthpasses (91 specs).