Redact sensitive data from trace output

[dzirtusss]

Summary

• Add RedactedDebugOutput class to filter sensitive data from --trace output
• Whitelist safe headers (Content-Type, Host, etc.) and redact all others
• Redact long token-like strings (50+ chars) in request/response bodies
• Update trace warning message to reflect that sensitive data is now redacted

Before

Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6...

After

Authorization: [REDACTED]

[coderabbitai]

Warning

Rate limit exceeded

@dzirtusss has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 23 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 9b4c8ea and 862c328.

📒 Files selected for processing (3)

• CHANGELOG.md
• lib/core/config.rb
• lib/core/controlplane_api_direct.rb

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch sergey/remove-sensitive-from-trace

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #261: Redact sensitive data from trace output

Overall Assessment

This is a solid security improvement that addresses a real risk of accidentally exposing sensitive data in trace output. However, there are critical issues that need to be addressed.

Critical Issues

1. Missing Return Value in << Method (lib/core/controlplane_api_direct.rb:8)

The RedactedDebugOutput#<< method writes to stdout but doesn't return self. This breaks the expected behavior of the << operator. Net::HTTP expects set_debug_output to receive an object where << returns self for method chaining.

Fix: Add 'self' as return value in the << method.

2. Overly Broad Redaction Pattern (lib/core/controlplane_api_direct.rb:19)

The regex [\w-._]{50,} will redact ANY sequence of 50+ characters including legitimate non-sensitive data like long JSON values, URLs, concatenated UUIDs, etc.

Recommendation: Use more targeted patterns for known token formats (JWT, API keys) or add character class validation.

3. Missing Test Coverage - CRITICAL

There are NO tests for the RedactedDebugOutput class. This security-critical feature absolutely needs test coverage for:

• Redacting sensitive headers (Authorization, etc.)
• Preserving safe headers (Content-Type, Host, etc.)
• Redacting long token-like strings in bodies
• Preserving short strings
• Case-insensitive header handling
• Return value of << method

Potential Issues

4. Header Matching Efficiency (lib/core/controlplane_api_direct.rb:21)
Using casecmp with any? works but is less efficient than normalizing to lowercase and using include?.

5. Limited Safe Headers List
Consider adding: User-Agent, Accept-Encoding, Transfer-Encoding, Vary, ETag, Last-Modified, Expires, Server

6. Regex Escaping (lib/core/controlplane_api_direct.rb:5)
The hyphen in [A-Za-z-] doesn't need escaping when at the end of a character class.

Security Considerations

Positive:

• Addresses main security concern
• Whitelist approach is correct
• Updated warning message is accurate

Concerns:

• 50-char threshold is arbitrary and could miss shorter tokens (32-40 chars)
• Structured JSON/XML data with sensitive field names won't be caught

Code Quality & Style

Positive:

• Follows Ruby style guide
• Proper use of freeze for constants
• Clear naming
• Good separation of concerns

Suggestions:

• Add class-level comment explaining purpose
• Consider extracting to separate file
• Add inline comments for regex patterns

Action Items

Must Fix Before Merge:

1. Add self return value to << method (Critical bug)
2. Add comprehensive test coverage for RedactedDebugOutput
3. Review and refine the 50-character regex pattern

Should Fix:
4. Optimize header matching with downcase + include
5. Expand safe headers list

Nice to Have:
6. Extract to separate file
7. Add structured data redaction for JSON bodies
8. Make redaction rules configurable

Conclusion

This PR solves an important security problem but needs test coverage and has a critical bug. The << method must return self and the redaction pattern needs refinement. With these fixes, this will be a solid security improvement.

Recommendation: Request changes for items 1-3, then approve after addressed.

[claude]

Code Review

Thanks for this security improvement! This PR addresses an important concern by redacting sensitive data from trace output. Here is my feedback:

Strengths

1. Security improvement: Properly addresses the security concern of sensitive data leaking in trace output
2. Clean implementation: The RedactedDebugOutput class is well-structured and focused
3. Good use of constants: SAFE_HEADERS and HEADER_REGEX are appropriately defined
4. Updated warning message: The new message in lib/core/config.rb:28 accurately reflects the redaction behavior
5. Follows project style: Uses double quotes per .rubocop.yml and frozen string literal

Potential Issues

1. Security: Incomplete redaction coverage (Critical)

The current implementation has a significant gap at lib/core/controlplane_api_direct.rb:19:

The code only redacts long strings (50+ chars) in non-header lines, but misses shorter sensitive tokens. Examples:

• API keys can be shorter than 50 characters
• Query parameters with tokens
• JSON bodies with short credentials like secret or password fields

Recommendation: Consider also redacting common sensitive JSON keys like token, secret, password, apiKey, etc., regardless of length.

2. Security: Header case-sensitivity issue

Line lib/core/controlplane_api_direct.rb:21 has a subtle bug with casecmp. While casecmp handles case-insensitivity correctly, there is a potential nil return issue. If match[1] contains non-ASCII characters, casecmp could return nil, causing zero? to raise an error.

Recommendation: Use casecmp? (with ?) instead as it is more idiomatic and returns a boolean directly.

3. Missing safe headers

Common safe headers that might be useful for debugging are missing from the whitelist:

• User-Agent
• Accept-Encoding
• Accept-Language

These do not contain sensitive data and could help with debugging.

Test Coverage (Critical Gap)

The PR adds a new class but no tests for it. This is a security-critical feature that should be thoroughly tested.

Recommendation: Add a spec file spec/core/redacted_debug_output_spec.rb with tests for:

1. Redacting non-whitelisted headers
2. Preserving whitelisted headers
3. Redacting long token-like strings in request bodies
4. Edge cases: Empty lines, lines without colons, Unicode in headers, multiple colons in header values

Code Quality Suggestions

1. Consider extracting to separate file: RedactedDebugOutput could live in its own file like lib/core/redacted_debug_output.rb for better separation of concerns.

2. Add inline documentation: Given this is a security feature, a brief class-level comment explaining the redaction rules would be helpful.

3. Regex naming: The regex at line 19 could benefit from a constant name like LONG_TOKEN_REGEX for clarity.

Security Assessment

Overall: This is a good security improvement, but the current implementation may provide a false sense of security due to gaps in redaction coverage. The 50-character threshold misses many real-world secrets.

Recommendation: Consider a more comprehensive approach by redacting based on common secret patterns and values for known sensitive JSON keys.

Summary

This PR moves in the right direction but needs:

1. Test coverage (critical)
2. Broader redaction patterns to catch shorter secrets (critical)
3. Minor fix for casecmp? usage (low priority)

Please add tests before merging. Great work on improving security!