Repository navigation
feat(tools): add reusable sensitive JSON redaction helper - #457
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extracts and isolates a reusable JSON redaction utility, previously part of a larger change, into its own module. This refactoring aims to reduce code duplication and enhance maintainability by providing a centralized, tested mechanism for safely handling and logging sensitive parameters across various application flows, thereby improving overall security posture. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
Bot
added
size: L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-structured and reusable utility for redacting sensitive information from JSON values, utilizing tokenization to detect sensitive keys. However, the redaction logic in is_sensitive_key and tokenize_key_parts contains critical flaws that allow sensitive data to leak, as highlighted by relevant repository rules on robust keyword detection. For instance, lowercase alphanumeric keys like password123 bypass redaction, and common sensitive keys such as key, app_key, and token_key are not caught due to a restrictive context list and flawed keyword combination logic. Additionally, there is a suggestion to improve the readability and maintainability of the key sensitivity detection logic.
| fn is_sensitive_key(key: &str) -> bool { | ||
| let lower = key.to_ascii_lowercase(); | ||
| if SENSITIVE_EXACT.contains(&lower.as_str()) { | ||
| return true; | ||
| } | ||
|
|
||
| let parts = tokenize_key_parts(key); | ||
| if parts.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| if has_any(&parts, &["password", "passwd", "secret", "credential"]) { | ||
| return true; | ||
| } | ||
|
|
||
| if has_any(&parts, &["authorization", "cookie"]) { | ||
| return true; | ||
| } | ||
|
|
||
| let has_token = has_any(&parts, &["token", "jwt"]); | ||
| let has_key = has_any(&parts, &["key"]); | ||
| let has_context = has_any( | ||
| &parts, | ||
| &[ | ||
| "auth", | ||
| "oauth", | ||
| "authorization", | ||
| "api", | ||
| "access", | ||
| "refresh", | ||
| "session", | ||
| "bearer", | ||
| "private", | ||
| "client", | ||
| "id", | ||
| ], | ||
| ); | ||
|
|
||
| has_context && (has_token || has_key) | ||
| } |
There was a problem hiding this comment.
The is_sensitive_key function contains critical flaws that lead to incomplete redaction. The key key is not properly handled, and combinations like token_key are missed because the has_context list is restrictive and lacks common prefixes such as app, user, application, and account. To address these vulnerabilities and improve maintainability, the logic for checking sensitive key parts should be refactored. Consider combining the if blocks on lines 95-101 and extracting inline string slices into named constants (e.g., SENSITIVE_PARTS, TOKEN_PARTS, KEY_PARTS, CONTEXT_PARTS). This would centralize keyword lists, making the detection logic clearer and more robust against sensitive data leaks.
References
- When detecting commands or keywords in a string, use token-based or word-boundary checks instead of simple substring containment to avoid false positives. The current issue highlights a failure in robust keyword detection, leading to missed sensitive data, which is a similar problem to false positives from inadequate detection methods.
| if segment | ||
| .chars() | ||
| .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit()) | ||
| { | ||
| parts.push(segment.to_string()); | ||
| } else { | ||
| parts.extend(split_camel_case_key_parts(segment)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The tokenize_key_parts function fails to split keys that are entirely composed of lowercase letters and digits (e.g., password123, secret99). This allows these sensitive keys to bypass the redaction logic because they are treated as a single token that does not exactly match the sensitive keywords in is_sensitive_key. This is a significant security bypass as many sensitive keys follow this pattern.
References
- When detecting commands or keywords in a string, use token-based or word-boundary checks instead of simple substring containment to avoid false positives. The current tokenization failure for certain patterns leads to sensitive data bypass, underscoring the need for robust and effective token-based detection.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Looks good. Clean implementation, well-tested, no .unwrap()/.expect() in production code.
Not a duplicate of leak_detector.rs: The leak detector scans raw text for secret values (regex patterns like sk-proj-..., AKIA...). This module redacts JSON values by key name (e.g., anything under a key named "password" or "authorization"). They are complementary -- one catches leaked secret values at sandbox boundaries, the other sanitizes structured data for logging/display.
Key coverage looks thorough: password, token, secret, api_key, authorization, cookie, private_key, client_secret all covered. The camelCase tokenizer + contextual matching correctly handles compound keys like clientSecret, authToken, apiKey while the false-positive tests show it avoids author, token_count, tokenize.
One minor suggestion (non-blocking): redact_in_place(&mut Value) already exists internally -- consider also exposing it as a public function alongside redact_sensitive_json. Callers on hot paths with large JSON payloads could avoid the clone.
* feat(tools): add reusable sensitive JSON redaction helper * fix(tools): harden sensitive-key tokenization and context matching
* feat(tools): add reusable sensitive JSON redaction helper * fix(tools): harden sensitive-key tokenization and context matching
2 participants
Summary
This split PR extracts the reusable JSON redaction utility introduced in #361 into an isolated, reviewable unit.
Changes
src/tools/redaction.rswith recursive redaction for sensitive keyssrc/tools/mod.rsWhy this split
Multiple reasoning/streaming/web paths depend on safe parameter redaction. Isolating the helper first reduces duplication and keeps follow-up PRs narrower.
Testing
Ran locally on this branch:
cargo clippy --all --all-featurescargo test