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!

[claude]

Using .casecmp(x).zero? is a Ruby antipattern because String#casecmp returns nil (not 0) when the two strings have incompatible encodings, causing a NoMethodError at runtime. Prefer casecmp? which returns a boolean:

Suggested change

	SAFE_HEADERS.any? { |h| h.casecmp(match[1]).zero? } ? line : "#{match[1]}: [REDACTED]\n"
	SAFE_HEADERS.any? { |h| h.casecmp?(match[1]) } ? line : "#{match[1]}: [REDACTED]\n"

[claude]

Net::HTTP sends raw wire bytes through the debug output, so header lines use CRLF terminators (\r\n). When msg.lines splits on \n, each line retains a trailing \r — meaning (.+) in the regex captures "Bearer token\r", not "Bearer token".

The redacted replacement uses \n only, so safe headers (returned unchanged) get \r\n while redacted headers get \n, producing mixed line endings in the output. This is harmless for display purposes but worth being aware of. If you want to preserve the original terminator:

def redact_line(line)
  match = line.match(HEADER_REGEX)
  return line.gsub(/[\w\-._]{50,}/, "[REDACTED]") unless match

  terminator = line.end_with?("\r\n") ? "\r\n" : "\n"
  SAFE_HEADERS.any? { |h| h.casecmp?(match[1]) } ? line : "#{match[1]}: [REDACTED]#{terminator}"
end

[claude]

The body-redaction regex [\w\-._]{50,} catches JWT and cpln profile token output (which use base64url encoding with A-Za-z0-9-_), but misses tokens using standard base64 alphabet (+, /, =). For example, an AWS-style secret key or a base64-encoded blob in a response body would slip through.

If any response bodies might contain standard-base64 secrets, consider extending the character class:

line.gsub(/[A-Za-z0-9\-._+\/=]{50,}/, "[REDACTED]")

This is a design trade-off — the wider class increases false-positive redactions in body JSON (e.g. long base64-encoded file content). Worth documenting the reasoning for the chosen character class either way.

[claude]

RedactedDebugOutput is a top-level class defined in controlplane_api_direct.rb. Since it is only used to wrap the debug output of ControlplaneApiDirect, nesting it as a private inner class (ControlplaneApiDirect::RedactedDebugOutput) or moving it to its own file would make the dependency explicit and avoid polluting the top-level namespace.

[claude]

Review: Redact sensitive data from trace output

The whitelist approach is the right call here — defaulting to redacting all headers except known-safe ones is much harder to get wrong than a blacklist. The overall direction is good. A few issues worth addressing before merge:

Missing tests (most important)

RedactedDebugOutput is a security-critical class — its entire purpose is to prevent credential leakage — but there are no tests for it. Without tests it's easy for a regression to silently re-expose sensitive data. The spec should cover at minimum:

• Authorization header is redacted
• Headers in SAFE_HEADERS (e.g. Content-Type) pass through unchanged
• Case-insensitive header matching (content-type vs Content-Type)
• Non-header lines with long token-like strings are redacted
• Non-header lines with short/normal strings pass through unchanged

casecmp(...).zero? → use casecmp?

String#casecmp returns nil when strings have incompatible encodings, and calling .zero? on nil raises NoMethodError. The idiomatic fix is casecmp? which returns true/false/nil and plays nicely with any?. See inline comment on line 21.

CRLF line endings in redacted output

Net::HTTP sends raw wire bytes through the debug callback, so header lines are CRLF-terminated (\r\n). When we split with msg.lines and then replace a header with "#{match[1]}: [REDACTED]\n", we silently drop the \r. Safe headers are returned unchanged (keeping \r\n) while redacted ones get \n only — mixed terminators in the output. Harmless for display, but easy to fix. See inline comment on line 21.

Body redaction character class

The regex [\w\-.\_]{50,} covers JWT / cpln base64url tokens (A-Za-z0-9-_.) but misses standard base64 characters (+, /, =). AWS-style secrets or base64-encoded response payloads would not be caught. If those credential formats are not a concern here, a comment explaining why the character class was chosen would help future maintainers.

Minor: top-level class in a non-matching file

RedactedDebugOutput lives at the top level in controlplane_api_direct.rb. Nesting it as ControlplaneApiDirect::RedactedDebugOutput or giving it its own file would make the coupling explicit and avoid adding an unexpected name to the global namespace.