-
Notifications
You must be signed in to change notification settings - Fork 4
Redact more sensitive http headers. #599
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
Conversation
WalkthroughThis PR refactors the sensitive header detection mechanism in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/uhttp/wrapper.go (1)
497-501: Clean implementation of the substring-based detection.The loop correctly checks if any header key contains sensitive substrings. The approach is straightforward and maintainable.
Optional: Consider using a map for micro-optimization
If the list of sensitive strings grows significantly in the future, you could convert the slice to a map for O(1) lookup, though with only 7 items the current approach is perfectly fine:
var sensitiveStrings = map[string]bool{ "api-key": true, "auth": true, // ... } // Then in the loop: for sensitiveString := range sensitiveStrings { if strings.Contains(headerKey, sensitiveString) { sensitive = true break } }This is purely optional and not necessary at the current scale.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/uhttp/wrapper.go(2 hunks)pkg/uhttp/wrapper_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (2)
pkg/uhttp/wrapper.go (1)
479-487: Excellent refactoring with improved security coverage.The centralized
sensitiveStringslist is much more maintainable than scattered explicit checks. The addition ofx-forwarded-forandx-forwarded-protois particularly good for privacy, as these can leak client IP addresses and routing information.Note that using substring matching (via
Containsbelow) is intentionally broader than prefix matching—for example, headers like "Authentication-Info" or "Custom-Api-Key" will now be redacted, which aligns well with the PR's goal of redacting more sensitive headers.pkg/uhttp/wrapper_test.go (1)
532-532: Good test coverage for the new substring matching behavior.Adding "Custom-Api-Key" to the test suite effectively validates that the substring-based detection works for headers that aren't explicitly listed but contain sensitive patterns. This demonstrates the improved flexibility of the new approach.
Also applies to: 544-544
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.