fix(security): add missing provider token patterns to secret redaction#1970
fix(security): add missing provider token patterns to secret redaction#1970ColinM-sys wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
The secret redaction system (secret-patterns.ts + debug.sh) only covered NVIDIA (nvapi-, nvcf-) and GitHub (ghp_, github_pat_) tokens. API keys from every other provider NemoClaw supports passed through unredacted to CLI output, error messages, and debug dumps. Add patterns for: - OpenAI (sk-proj-, sk-) - Anthropic (sk-ant-) - Slack (xoxb-, xoxp-, xoxe.xoxp-) - AWS (AKIA followed by 16 uppercase alphanumeric) - HuggingFace (hf_) - GitLab (glpat-) - Groq (gsk_) - PyPI (pypi-) Update both secret-patterns.ts (TypeScript, used by runner.ts) and debug.sh (shell, used by nemoclaw debug) per the "single source of truth" contract documented in the file header. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtended secret-detection and redaction rules: added regexes and shell-prefix entries for additional token formats (OpenAI sk-proj/sk, Slack xox*, AWS AKIA/ASIA, Hugging Face hf_, GitLab glpat-, Groq gsk_, PyPI pypi-, Anthropic sk-ant-). Core flow and other patterns unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/secret-patterns.ts (1)
60-75:⚠️ Potential issue | 🟠 MajorAdd missing
xoxe.xoxp-toEXPECTED_SHELL_PREFIXES.Line 29 introduces
xoxe.xoxp-detection, but Lines 60-75 omit that prefix from the shell consistency list. This leaves a gap where shell redaction drift for this token type won’t be caught by the consistency test.🔧 Proposed fix
export const EXPECTED_SHELL_PREFIXES = [ "nvapi-", "nvcf-", "ghp_", "github_pat_", "sk-proj-", "sk-", "xoxb-", "xoxp-", + "xoxe.xoxp-", "AKIA", "hf_", "glpat-", "gsk_", "pypi-", "sk-ant-", ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/secret-patterns.ts` around lines 60 - 75, The EXPECTED_SHELL_PREFIXES array in src/lib/secret-patterns.ts is missing the "xoxe.xoxp-" prefix referenced elsewhere; update the EXPECTED_SHELL_PREFIXES constant to include "xoxe.xoxp-" (e.g., add "xoxe.xoxp-" to the exported array) so the shell redaction consistency test covering tokens like xoxe.xoxp- will detect drift; modify the array in the EXPECTED_SHELL_PREFIXES declaration accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/debug.sh`:
- Around line 134-144: The redact() rules in scripts/debug.sh are missing Slack
tokens of the form "xoxe.xoxp-…"; update the sed rules inside redact() to add a
regex that matches that pattern (for example a new line analogous to the
existing Slack rules such as: a substitution matching
"xoxe\.xoxp-[A-Za-z0-9_.-]{10,}" and replacing with "<REDACTED>"). Place this
new rule alongside the other Slack token rules (e.g., near the existing
xoxb-/xoxp- rules) so xoxe.xoxp- tokens are also redacted.
---
Outside diff comments:
In `@src/lib/secret-patterns.ts`:
- Around line 60-75: The EXPECTED_SHELL_PREFIXES array in
src/lib/secret-patterns.ts is missing the "xoxe.xoxp-" prefix referenced
elsewhere; update the EXPECTED_SHELL_PREFIXES constant to include "xoxe.xoxp-"
(e.g., add "xoxe.xoxp-" to the exported array) so the shell redaction
consistency test covering tokens like xoxe.xoxp- will detect drift; modify the
array in the EXPECTED_SHELL_PREFIXES declaration accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c362677f-0159-43e5-b590-28711109d907
📒 Files selected for processing (2)
scripts/debug.shsrc/lib/secret-patterns.ts
Signed-off-by: ColinM-sys <cmcdonough@50words.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/secret-patterns.ts`:
- Around line 30-31: The regex /AKIA[A-Z0-9]{16}/g in src/lib/secret-patterns.ts
only matches long-term AWS keys and misses temporary keys that start with ASIA;
update the pattern to match both prefixes by replacing /AKIA[A-Z0-9]{16}/g with
/A(?:K|S)IA[A-Z0-9]{16}/g (and apply the same replacement for the other
occurrence in the file), ensuring both AKIA and ASIA access key IDs are
redacted.
- Line 69: The new secret pattern "xoxe.xoxp-" added in
src/lib/secret-patterns.ts must be redacted by the debug script; update
scripts/debug.sh in the redact() sed rules to add a rule that matches the
"xoxe.xoxp-" prefix (e.g., a sed expression like -e
's/xoxe\\.xoxp-[A-Za-z0-9_-]{10,}/<REDACTED>/g' ) alongside the existing xoxb-
and xoxp- rules so tokens with the xoxe.xoxp- prefix are replaced in debug
output.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a8cbe1d-7ae2-42cf-bf24-1ee600dea636
📒 Files selected for processing (1)
src/lib/secret-patterns.ts
|
✨ Thanks for submitting this PR that proposes a fix for the secret redaction to add missing provider token patterns, which could help improve the security of the system by properly redacting sensitive information. |
1 similar comment
|
✨ Thanks for submitting this PR that proposes a fix for the secret redaction to add missing provider token patterns, which could help improve the security of the system by properly redacting sensitive information. |
Per CodeRabbit feedback on NVIDIA#1970: - secret-patterns.ts: expand AKIA regex to match both long-term (AKIA) and temporary/session (ASIA) AWS access key ID prefixes. - debug.sh: add sed rules for xoxe.xoxp- Slack config tokens and ASIA AWS session keys, alongside the existing AKIA rule. EXPECTED_SHELL_PREFIXES now includes ASIA so the consistency test stays authoritative. All 30 redaction-consistency tests pass. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/debug.sh (1)
138-140: Use standard comments instead of backticked pseudo-comments for improved maintainability.Lines 138 and 140 use backticks (
`# ...`) to include documentation, which are command substitutions in bash, not comments. While syntactically valid and passing ShellCheck, this non-idiomatic pattern is fragile and harder to maintain in a security-sensitive redaction path. Consider moving these explanations to standard comments on separate lines above the corresponding-epatterns.🔧 Suggested alternative
+ # xoxe.xoxp- = Slack config token prefix -e 's/xoxe\.xoxp-[A-Za-z0-9_-]{10,}/<REDACTED>/g' \ + # AWS access key IDs: AKIA = long-term, ASIA = temporary/session -e 's/AKIA[A-Z0-9]{16}/<REDACTED>/g' \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/debug.sh` around lines 138 - 140, The script uses backticked pseudo-comments (e.g., the sequences starting with `# xoxe.xoxp-` and `# AWS access key IDs:`) which invoke command substitution instead of real comments; replace each backticked pseudo-comment with a proper bash comment line (start with #) placed immediately above the related sed -e pattern (for example above the -e 's/xoxe\.xoxp-[A-Za-z0-9_-]{10,}/<REDACTED>/g' and above the -e pattern for AKIA/ASIA keys) so the explanatory text is non-executing, readable, and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/debug.sh`:
- Around line 138-140: The script uses backticked pseudo-comments (e.g., the
sequences starting with `# xoxe.xoxp-` and `# AWS access key IDs:`) which invoke
command substitution instead of real comments; replace each backticked
pseudo-comment with a proper bash comment line (start with #) placed immediately
above the related sed -e pattern (for example above the -e
's/xoxe\.xoxp-[A-Za-z0-9_-]{10,}/<REDACTED>/g' and above the -e pattern for
AKIA/ASIA keys) so the explanatory text is non-executing, readable, and
maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b045d71-f280-46b6-87ee-15e0eb043488
📒 Files selected for processing (2)
scripts/debug.shsrc/lib/secret-patterns.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/secret-patterns.ts
Per CodeRabbit nit on NVIDIA#1970: backticked comments in the middle of the sed pipeline (e.g. `# AWS access key IDs...`) invoke command substitution rather than acting as real shell comments. While ShellCheck-clean, the pattern is non-idiomatic and fragile in a security-sensitive redaction path. Move the explanation of xoxe.xoxp- (Slack config token) and AKIA/ASIA (AWS long-term vs session keys) up into the function-level comment block, where real `#` comments belong. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
2 participants
Summary
secret-patterns.tscovering OpenAI, Anthropic, Slack, AWS, HuggingFace, GitLab, Groq, and PyPI.scripts/debug.shper the single-source-of-truth contract.Why
The redaction system only caught NVIDIA and GitHub tokens. Every other provider's API keys passed through unredacted. Verified locally:
NemoClaw explicitly supports OpenAI, Anthropic, Slack, and HuggingFace as providers. Their tokens appear in error messages, subprocess output, and
nemoclaw debugdumps — all of which flow throughredact(). CWE-532 (Insertion of Sensitive Information into Log File).What changed
src/lib/secret-patterns.ts— added 10 new prefix patterns toTOKEN_PREFIX_PATTERNSand corresponding entries toEXPECTED_SHELL_PREFIXESscripts/debug.sh— added matching sed patterns toredact()Test plan
npm run build:cli— compiles cleanlytest/secret-redaction.test.tsconsistency check should pass (verifies shell prefixes match TS prefixes)Signed-off-by: ColinM-sys cmcdonough@50words.com
Summary by CodeRabbit