Centralize credential redaction in src/utils/redaction.ts + channel gate tests - #1711
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR centralizes redaction helpers, updates consumers and tests to use them, and changes MCP channel lookup, relay filtering, dev-channel setup, and heartbeat emission to use plugin-source-aware wiring. ChangesCentralized Redaction Utility
MCP Channel Gating and CLI Wiring
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/log.ts (1)
181-198:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSend the sanitized error to the reporting sink.
The new redaction only protects
errorInfo; the queued path and attached sink still receiveerr, so secrets inmessage/stackcan leave through error reporting. Build a sanitizedErrorand use it for both paths.🛡️ Proposed fix
- const errorStr = err.stack || err.message - const sanitizedErrorStr = redactSensitiveInfo(errorStr) + const sanitizedMessage = redactSensitiveInfo(err.message) + const sanitizedStack = err.stack ? redactSensitiveInfo(err.stack) : undefined + const sanitizedErrorStr = sanitizedStack ?? sanitizedMessage + const sanitizedError = new Error(sanitizedMessage) + sanitizedError.name = err.name + if (sanitizedStack) { + sanitizedError.stack = sanitizedStack + } const errorInfo = { error: sanitizedErrorStr, timestamp: new Date().toISOString(), @@ // If sink not attached, queue the event if (errorLogSink === null) { - errorQueue.push({ type: 'error', error: err }) + errorQueue.push({ type: 'error', error: sanitizedError }) return } - errorLogSink.logError(err) + errorLogSink.logError(sanitizedError)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/log.ts` around lines 181 - 198, The sanitized error information is only used for the in-memory log, but the original unsanitized error object is still passed to errorLogSink.logError() and pushed to errorQueue, allowing sensitive data to leak through error reporting. Create a new sanitized Error object using the sanitizedErrorStr (which already contains the redacted error information) and use this sanitized error object instead of the original err when calling errorLogSink.logError() and when pushing to errorQueue.src/services/mcp/channelNotification.ts (1)
249-355:⚠️ Potential issue | 🔴 CriticalAdd tests for the channel-gating trust boundary.
No test file exists for
channelNotification.ts, andgateChannelServer(called fromuseManageMCPConnections.tsandcli/print.ts) has zero test coverage. This function implements a critical MCP trust boundary with 6+ gates: capability check, runtime gate, session allowlist, marketplace verification, plugin allowlist, and dev bypass logic. The behavior changed materially (OAuth/org policy removed; marketplace disambiguation added).Per AGENTS.md and CONTRIBUTING.md, add or update tests when behavior changes, especially for trust boundaries. Create targeted tests covering:
- Capability rejection (missing or false
experimental['claude/channel'])- Missing
--channelsentry- Marketplace mismatch (requested vs. installed plugin source)
- Plugin allowlist rejection and approval
- Plugin
devbypass behavior- Server-entry
devrequirement- Runtime gate when
isChannelsEnabled()is false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/mcp/channelNotification.ts` around lines 249 - 355, Create a new test suite for the gateChannelServer function in channelNotification.ts to cover all six trust boundary gates. The tests should validate: (1) capability rejection when experimental['claude/channel'] is missing or false, (2) runtime gate when isChannelsEnabled() returns false, (3) session allowlist rejection when findChannelEntry returns null, (4) marketplace mismatch detection when parsePluginIdentifier result doesn't match entry.marketplace, (5) plugin allowlist approval and rejection using getChannelAllowlist() results, (6) the dev bypass behavior for both plugin-kind entries (entry.dev bypasses allowlist) and server-kind entries (entry.dev required), and (7) successful registration when all gates pass. Ensure each test case isolates a specific gate condition and mocks the relevant dependencies appropriately.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/FeedbackSurvey/submitTranscriptShare.ts`:
- Line 16: The issue is that redactSensitiveInfo is being applied to an
already-stringified JSON payload, which means the redactor only sees text and
cannot identify JSON structure, allowing credentials with generic key names like
"value" to leak through. Instead, use a JSON replacer function (jsonRedactor)
during the JSON.stringify operation itself so the redactor can see and properly
redact the JSON structure before it becomes a string, then optionally keep the
text redactor as a second pass. Also remove the jsonStringify import if it is no
longer being used after making this change.
In `@src/services/mcp/channelNotification.ts`:
- Around line 166-169: The getEffectiveChannelAllowlist() function can return
orgList as the effective allowlist, but the gateChannelServer() function only
enforces the ledger-based getChannelAllowlist(), creating a mismatch between
what is exported as the effective allowlist and what is actually enforced at the
MCP trust boundary. Either remove the org override logic in
getEffectiveChannelAllowlist() (lines 166-169) and related code (lines 323-327)
to always use getChannelAllowlist() consistently, or update gateChannelServer()
to enforce the same allowlist source that getEffectiveChannelAllowlist() returns
so that the trust boundary enforcement matches the exported contract.
In `@src/utils/redaction.ts`:
- Around line 78-93: The private_key and privateKey fields are not being
redacted because GENERIC_HEADER_FIELD_PATTERN does not include a pattern
matching them, and SENSITIVE_FIELD_SUBSTRINGS does not include the normalized
form 'privatekey'. To fix this, add a pattern for private_key to the
GENERIC_HEADER_FIELD_PATTERN regex (similar to how other sensitive header
patterns like api-key and api_key are handled), and add 'privatekey' as an entry
to the SENSITIVE_FIELD_SUBSTRINGS array to ensure jsonRedactor catches private
key fields in JSON credentials.
- Around line 30-35: The ANTHROPIC_KEY_PATTERN and OPENAI_KEY_PATTERN regex
patterns currently include quotes in their negative lookbehind and lookahead
assertions, preventing API keys from being redacted when they appear as quoted
string values (like in JSON). Remove the quote characters `"'` from both the
negative lookbehind `(?<![A-Za-z0-9"']` and negative lookahead
`(?![A-Za-z0-9"'])` in both patterns so that quotes act as delimiters rather
than blockers. Additionally, add tests that verify API keys are properly
redacted when they appear within single or double quotes to ensure the redaction
behavior works correctly with quoted values.
---
Outside diff comments:
In `@src/services/mcp/channelNotification.ts`:
- Around line 249-355: Create a new test suite for the gateChannelServer
function in channelNotification.ts to cover all six trust boundary gates. The
tests should validate: (1) capability rejection when
experimental['claude/channel'] is missing or false, (2) runtime gate when
isChannelsEnabled() returns false, (3) session allowlist rejection when
findChannelEntry returns null, (4) marketplace mismatch detection when
parsePluginIdentifier result doesn't match entry.marketplace, (5) plugin
allowlist approval and rejection using getChannelAllowlist() results, (6) the
dev bypass behavior for both plugin-kind entries (entry.dev bypasses allowlist)
and server-kind entries (entry.dev required), and (7) successful registration
when all gates pass. Ensure each test case isolates a specific gate condition
and mocks the relevant dependencies appropriately.
In `@src/utils/log.ts`:
- Around line 181-198: The sanitized error information is only used for the
in-memory log, but the original unsanitized error object is still passed to
errorLogSink.logError() and pushed to errorQueue, allowing sensitive data to
leak through error reporting. Create a new sanitized Error object using the
sanitizedErrorStr (which already contains the redacted error information) and
use this sanitized error object instead of the original err when calling
errorLogSink.logError() and when pushing to errorQueue.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 4c592333-ea64-4070-9fd4-b36447c83314
📒 Files selected for processing (7)
src/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/services/api/logging.tssrc/services/mcp/channelNotification.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/logging.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/logging.tssrc/services/mcp/channelNotification.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/logging.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/services/mcp/channelNotification.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/logging.tssrc/services/mcp/channelNotification.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/logging.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/logging.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/logging.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/logging.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/logging.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/logging.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/debug.tssrc/utils/log.tssrc/utils/redaction.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.ts
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI implementations
Files:
src/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/Feedback.tsx
src/{skills,utils/plugins,services/mcp}/**
⚙️ CodeRabbit configuration file
src/{skills,utils/plugins,services/mcp}/**: Review skill/plugin/MCP behavior as a trust boundary. Check registry fetches, local and remote installs, path normalization, hash verification, revocation/trust metadata, tools_required handling, config-home behavior, and startup-time loading. Block on path traversal risk, unverified downloads, silent trust promotion, or unexpected code/tool activation.
Files:
src/services/mcp/channelNotification.ts
🪛 ast-grep (0.43.0)
src/utils/redaction.ts
[warning] 110-113: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(?<![A-Za-z0-9_])(${escaped.join('|')})\\s*[=:]\\s*["']?[^"'\\s)}\\]]+["']?,
'gi',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (6)
src/components/Feedback.tsx (1)
24-24: LGTM!Also applies to: 72-73
src/services/api/logging.ts (1)
24-24: LGTM!Also applies to: 272-272
src/utils/debug.ts (1)
16-16: LGTM!Also applies to: 221-223
src/services/mcp/channelNotification.ts (3)
10-30: LGTM!
122-150: LGTM!
192-230: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
I do like the idea of a central owner for this.. but there is some issues.
Findings
-
[P2] Reconcile this with the existing redaction utilities
src/utils/redaction.ts:1
This PR says the new utility is the "single source of truth" for logs, bug reports, transcript shares, and diagnostics, but the merged doctor-report work from #1647 still has its ownsrc/utils/diagnostics/redaction.ts, merged #1672 addedsrc/utils/statusRedaction.ts, and open #1673 is actively changingproviderSecrets/urlRedaction/status.tsxfor route-secret redaction. Those PRs do not make #1711 a clean duplicate, and they are not a direct file-level merge conflict with these changed files, but they do mean this patch creates yet another independent security redaction path with different semantics. Please either consolidate the existing diagnostics/status URL redaction onto this shared utility, or keep this PR narrowly scoped to feedback/logging and remove the single-source-of-truth claim until the other redaction paths are actually migrated. -
[P2] Complete CodeRabbit's request to test the channel trust-boundary rewrite
src/services/mcp/channelNotification.ts:278
CodeRabbit's channel-gate review item is still valid: this PR removes the OAuth/org-policy gates and changes the effective allowlist/session/marketplace behavior ingateChannelServer(), but it does not add focused tests for that trust boundary. The existing neighboring MCP tests pass, but they do not exercisegateChannelServer()or the changed policy and allowlist paths. Please complete that review request with targeted coverage for the capability, runtime, session, marketplace, plugin allowlist/dev-bypass, and server-entry dev gates before shipping this behavior change. -
[P2] Complete CodeRabbit's request to use the structural JSON redactor for transcript shares
src/components/FeedbackSurvey/submitTranscriptShare.ts:72
CodeRabbit asked for transcript sharing to usejsonRedactorwhile stringifying, but the current patch still callsjsonStringify(data)first and then runsredactSensitiveInfo()over the resulting text. That skips the key-aware protection the new utility was added to provide, and leaves this sensitive payload path dependent on regexes seeing enough string context after serialization. Please complete that review request by applying the JSON replacer during stringify, with the text redactor retained only as a defense-in-depth second pass if needed.
|
All three P2 findings addressed in commit [P2] #1 — Consolidate the four redaction modulesMerged all four redaction files into
Updated the file's header comment to document the four surface groups (logs / URL display / /status / diagnostic reports) and every exported function, so the "single source of truth" claim is now accurate rather than aspirational. Direct consumers re-pointed at
Test files updated to match: [P2] #2 —
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/mcp/channelNotification.test.ts`:
- Around line 123-150: Add a new regression test case after the existing
marketplace gate tests that validates the multi-candidate disambiguation
scenario. Create a test where the allowed channels list contains two entries
with the same plugin name (slack) but different marketplace values (e.g.,
anthropic and evilcorp). Call gateChannelServer with a pluginSource that matches
one of the marketplaces (e.g., plugin:slack@anthropic) and verify that the
function correctly selects the matching entry and returns action 'register'.
This test ensures the findChannelEntry function's multi-candidate branch is
exercised when there are duplicate plugin names with different marketplaces,
validating that the runtime pluginSource parameter properly disambiguates
between candidates before allowlist evaluation.
In `@src/utils/redaction.ts`:
- Around line 377-382: The home-prefix redaction logic in the code block
starting with the startsWith check on normalizedCandidate does not verify
path-segment boundaries, causing incorrect matches like `/home/alice2/project`
to match the `/home/alice` prefix. Add a boundary check after the startsWith
condition to ensure the character immediately following the normalizedCandidate
match is either a path separator (forward slash) or the end of the string before
applying the redaction with the tilde replacement. This prevents partial
directory name matches from being incorrectly redacted.
- Around line 305-312: The fallback URL redaction logic in the catch block uses
a hardcoded list of query parameter names that may not align with the
`shouldRedactUrlQueryParam` function, creating a potential credential leak for
malformed URLs. Extract the parameter names being checked in
`shouldRedactUrlQueryParam` and ensure the fallback regex pattern in the catch
block covers the same set of credential-like parameters. Additionally, add or
update test cases to verify that malformed URLs are properly redacted with the
same credential parameters that the primary redaction path handles.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e233801b-2bc8-4fa2-a414-d3e0af1ae1e0
📒 Files selected for processing (15)
scripts/system-check.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/api/openaiShim.tssrc/services/mcp/channelNotification.test.tssrc/utils/diagnostics/issueReport.tssrc/utils/diagnostics/redaction.test.tssrc/utils/diagnostics/redaction.tssrc/utils/redaction.tssrc/utils/requestSizeBreakdown.tssrc/utils/status.tsxsrc/utils/statusRedaction.test.tssrc/utils/statusRedaction.tssrc/utils/urlRedaction.test.tssrc/utils/urlRedaction.ts
💤 Files with no reviewable changes (3)
- src/utils/diagnostics/redaction.ts
- src/utils/statusRedaction.ts
- src/utils/urlRedaction.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (16)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/diagnostics/redaction.test.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/status.tsxsrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/diagnostics/redaction.test.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/diagnostics/redaction.test.tsscripts/system-check.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/status.tsxsrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/diagnostics/redaction.test.tssrc/utils/urlRedaction.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/utils/diagnostics/redaction.test.tssrc/utils/urlRedaction.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/diagnostics/redaction.test.tsscripts/system-check.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/status.tsxsrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/diagnostics/redaction.test.tsscripts/system-check.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/status.tsxsrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/diagnostics/redaction.test.tsscripts/system-check.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/status.tsxsrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/diagnostics/redaction.test.tssrc/utils/urlRedaction.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/diagnostics/redaction.test.tsscripts/system-check.tssrc/utils/urlRedaction.test.tssrc/services/api/openaiShim.tssrc/utils/status.tsxsrc/utils/diagnostics/issueReport.tssrc/utils/requestSizeBreakdown.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsxsrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
scripts/system-check.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/openaiShim.tssrc/services/mcp/channelNotification.test.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/openaiShim.tssrc/services/mcp/channelNotification.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/openaiShim.ts
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI implementations
Files:
src/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/ProviderManager.tsx
src/{skills,utils/plugins,services/mcp}/**
⚙️ CodeRabbit configuration file
src/{skills,utils/plugins,services/mcp}/**: Review skill/plugin/MCP behavior as a trust boundary. Check registry fetches, local and remote installs, path normalization, hash verification, revocation/trust metadata, tools_required handling, config-home behavior, and startup-time loading. Block on path traversal risk, unverified downloads, silent trust promotion, or unexpected code/tool activation.
Files:
src/services/mcp/channelNotification.test.ts
🪛 ast-grep (0.43.0)
src/utils/redaction.ts
[warning] 459-459: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${escapeRegExp(normalizedHome)}(?=$|[/\\\\]), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (10)
src/components/FeedbackSurvey/submitTranscriptShare.ts (1)
16-16: LGTM!Also applies to: 72-84
scripts/system-check.ts (1)
26-26: LGTM!src/utils/statusRedaction.test.ts (1)
8-8: LGTM!src/utils/urlRedaction.test.ts (1)
3-3: LGTM!src/utils/diagnostics/issueReport.ts (1)
36-36: LGTM!src/components/ProviderManager.tsx (1)
78-78: LGTM!src/services/api/openaiShim.ts (1)
86-86: LGTM!src/utils/requestSizeBreakdown.ts (1)
4-4: LGTM!src/utils/status.tsx (1)
25-25: LGTM!src/utils/diagnostics/redaction.test.ts (1)
10-10: LGTM!
|
Added a regression test for the New test case (
Without exercising this path, the gate would lock onto whichever entry sorts first in All 14 cases in |
|
Pushed two redaction correctness fixes in commit [1] URL fallback regex now derives from the canonical token list.
New tests in
[2] The previous New test in process.env.HOME = '/home/alice'
expect(redactPathForStatus('/home/alice2/project')).toBe('/home/alice2/project') // unchanged
expect(redactPathForStatus('/home/alice.bak/file')).toBe('/home/alice.bak/file') // unchanged
expect(redactPathForStatus('/home/alice/project')).toBe('~/project') // still redactsVerification: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/redaction.ts`:
- Around line 299-302: The MALFORMED_URL_PARAM_PATTERN regex only performs exact
token matching but the primary parsing uses includes() semantics to match keys
containing sensitive tokens. This causes keys like x_access_token or my_api_key
to leak in the fallback catch path. Update the regex pattern in
MALFORMED_URL_PARAM_PATTERN to match keys that contain (rather than exactly
match) the sensitive tokens, and add regression test cases for the malformed-URL
fallback scenario that verify keys with prefixed or encoded sensitive tokens
like x_access_token and my_api_key are properly redacted.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: a08e1bf1-a5ae-4da9-bc1e-8798ccea11c9
📒 Files selected for processing (3)
src/utils/redaction.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/urlRedaction.test.tssrc/utils/statusRedaction.test.tssrc/utils/redaction.ts
🪛 ast-grep (0.43.0)
src/utils/redaction.ts
[warning] 298-301: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
([?&](?:${SENSITIVE_URL_QUERY_PARAM_TOKENS.join('|')})=)[^&#]*,
'gi',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (1)
src/utils/statusRedaction.test.ts (1)
163-181: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the development-channel warning after removing the OAuth gate
src/interactiveHelpers.tsx:276
gateChannelServer()now removes the OAuth/org-policy blockers, but this onboarding path still skipsDevChannelsDialogwhenever there is no Claude OAuth token and immediately appends the requested development channels withdev: true. That skip was safe only while the downstream gate blocked no-OAuth users; with this PR, an API-key/no-OAuth session can pass--dangerously-load-development-channels, avoid the warning/confirmation entirely, and still register the channel because server entries withdev: truenow pass the channel gate. Please update the dev-channel confirmation/notice path together with the gate change so dangerous development channels cannot be enabled without the explicit warning acceptance. -
[P2] Complete CodeRabbit's request to align malformed URL redaction
src/utils/redaction.ts:299
CodeRabbit's latest review item is still valid: the primaryURLparser path redacts query keys when they contain a sensitive token viashouldRedactUrlQueryParam(), but the malformed-URL fallback regex only matches exact parameter names. For example,shouldRedactUrlQueryParam('my_api_key')andshouldRedactUrlQueryParam('x_access_token')return true, whileredactUrlForDisplay('//host/path?my_api_key=SECRET&x_access_token=TOKEN')leaves both values intact because the catch path only matchesapi_key=oraccess_token=exactly. Please complete that review request by using the same predicate semantics in the fallback path and adding malformed-URL regressions for prefixed/encoded sensitive keys.
|
Both findings fixed in commit [P1] Dev-channel warning restored. The dialog now shows whenever the flag is passed and [P2] Malformed-URL fallback now uses the same substring predicate as the primary path. Three new tests in
Verification: |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/interactiveHelpers.tsx`:
- Around line 264-274: The comment block explaining the dev-channel gating logic
(lines 264-274) contains inaccurate information about when the confirmation
dialog runs. The comment states the dialog runs regardless of tengu_harbor
state, but the actual code gates on isChannelsEnabled() which represents the
tengu_harbor feature flag. Update this comment block to accurately reflect that
the confirmation dialog is only shown when isChannelsEnabled() returns true, and
simplify the explanation to clearly express the actual security boundary without
the misleading claim about running regardless of tengu_harbor state. Keep the
comment concise and focused on the actual code behavior.
- Around line 263-310: Add comprehensive tests for the dev-channels dialog
acceptance behavior in the showSetupScreens() function to ensure the
security-sensitive behavior is covered. Create test cases that mock
isChannelsEnabled() to return both true and false, call showSetupScreens() with
devChannels parameter, and verify that the dialog appears when channels are
enabled but is skipped when disabled. For the enabled case, assert that the
DevChannelsDialog onAccept callback correctly invokes setAllowedChannels() and
setHasDevChannels(true) with each entry marked as dev: true. For the disabled
case, verify that entries are still registered via setAllowedChannels() and
setHasDevChannels(true) without displaying the dialog. These tests should target
the conditional logic around the isChannelsEnabled() check and the different
code paths it controls.
In `@src/utils/redaction.ts`:
- Around line 320-323: The redaction logic has two security issues: First, the
userinfo regex pattern needs to be updated to exclude question marks and hash
symbols in the character class to prevent consuming `@` characters that appear
in query string values. Second, in the fallback query parameter redaction code
shown in the diff (the section with `shouldRedactUrlQueryParam(key)`), the key
variable must be decoded using `decodeURIComponent` before being passed to the
`shouldRedactUrlQueryParam` predicate, since the predicate expects decoded
parameter names but the fallback path provides encoded keys. Additionally, add
regression test cases to verify that encoded parameter names like `%74oken`
match their decoded equivalents and that `@` symbols within query values are not
incorrectly treated as userinfo separators.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: cd9330a9-7f72-4b83-9016-53ad41a0773a
📒 Files selected for processing (3)
src/interactiveHelpers.tsxsrc/utils/redaction.tssrc/utils/urlRedaction.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/urlRedaction.test.tssrc/interactiveHelpers.tsxsrc/utils/redaction.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/urlRedaction.test.tssrc/utils/redaction.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/utils/urlRedaction.test.tssrc/interactiveHelpers.tsxsrc/utils/redaction.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/urlRedaction.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/utils/urlRedaction.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/utils/urlRedaction.test.tssrc/interactiveHelpers.tsxsrc/utils/redaction.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/utils/urlRedaction.test.tssrc/interactiveHelpers.tsxsrc/utils/redaction.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/urlRedaction.test.tssrc/interactiveHelpers.tsxsrc/utils/redaction.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/urlRedaction.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/utils/urlRedaction.test.tssrc/interactiveHelpers.tsxsrc/utils/redaction.ts
|
Pushed two follow-ups from the latest CodeRabbit review in commit [1] Boundary class widened on key-prefix patterns — Changed Adversarial probe confirms each pattern now catches its key when preceded by a JSON string quote. [2] Verified both paths are covered:
Comment on [3] Dev-channel dialog comment tightened — Previous comment said the dialog skip was gated on "KAIROS / KAIROS_CHANNELS". The actual gate is Skipped with reason
Verification
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Finish redacting malformed URL fallback secrets
src/utils/redaction.ts:323
The malformed-URL fallback still does not match the primaryURLSearchParamspath for sensitive query keys. It passes the raw key intoshouldRedactUrlQueryParam(), so an encoded sensitive name like//host/path?%74oken=SECRETis returned withSECRETintact. The userinfo fallback also keeps matching through?and#, so//api.example.com?email=user@example.com&token=SECRETbecomes//redacted@example.com&token=SECRETand the token is no longer in a parseable query segment. These URL redactors feed diagnostic/status surfaces, so malformed proxy/provider URLs can still leak credentials. Please decode fallback keys before applying the predicate and stop the userinfo regex at query/fragment delimiters, with regressions for both cases. -
[P2] Keep channel notice allowlist in sync with the gate
src/services/mcp/channelNotification.ts:166
getEffectiveChannelAllowlist()can returnpolicy.allowedChannelPlugins, andChannelsNoticeuses that result to decide whether a requested plugin should warn as unmatched. ButgateChannelServer()still enforces onlygetChannelAllowlist()at the trust boundary, so a plugin present in the org/policy list but absent from the ledger will show no "not on the allowlist" warning and then be skipped when the MCP channel handler registers. Please either remove the org override from the notice/effective helper or make the gate enforce the same effective allowlist source so startup guidance matches runtime behavior.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Apply marketplace matching to permission relays too
src/hooks/toolPermission/handlers/interactiveHandler.ts:325
The PR makesgateChannelServer()marketplace-aware, but permission prompts still filter relay clients withfindChannelEntry(name, allowedChannels)and nopluginSource. If a session allowsplugin:slack@anthropicwhile a connected server is actuallyplugin:slack@evilcorp, the channel registration gate correctly skips the evilcorp plugin, but the permission-relay path still treats the bareplugin:slackname as allowed and sends itnotifications/claude/channel/permission_requestpreviews. That leaks tool names/descriptions/input previews to a plugin marketplace the user did not approve. Please thread the runtime plugin source through this allowlist check, or reuse the same marketplace-verified gate result, before sending permission requests. -
[P2] Remove stale OAuth/org-policy blocking from the channel notice
src/components/LogoV2/ChannelsNotice.tsx:193
gateChannelServer()now explicitly removes the Claude OAuth and org-policy gates, so an API-key/no-OAuth session, or a managed settings session withoutchannelsEnabled: true, can still register channel notifications when the capability/session/allowlist checks pass. The startup notice still computesnoAuthandpolicyBlockedfirst and renders "--channelsignored" / "blocked by org policy" before it ever reaches the listening message. That makes the UI say inbound messages are unavailable even though the handler can be registered and receive them, which is especially confusing for the non-OAuth flow this PR is trying to enable. Please keepChannelsNoticealigned with the runtime gate by removing or rewriting those stale blockers. -
[P2] Add the missing dev-channel dialog coverage CodeRabbit requested
src/interactiveHelpers.tsx:263
CodeRabbit's request to cover the--dangerously-load-development-channelsconfirmation path is still valid: this PR changes a security-sensitive branch so the dialog is now the only warning/acceptance step for non-OAuth sessions whenisChannelsEnabled()is true, but there are no focused tests forshowSetupScreens()orDevChannelsDialogregistration behavior. Please add coverage that mocksisChannelsEnabled()both true and false, verifies the dialog is shown and onlyonAcceptappendsdev: trueentries in the enabled case, and verifies the disabled branch registers entries without showing the dialog.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/bugfixes.test.ts`:
- Around line 419-433: The test assertion for the exact count of `.map(c => ({
...c, dev: true }))` occurrences in the showSetupScreens test is fragile and may
break on unrelated refactoring. Add a more detailed comment explaining why
exactly 2 occurrences are semantically required in interactiveHelpers.tsx (one
for the disabled branch and one for the onAccept handler), making it clear this
is a logical requirement rather than just an implementation detail count. This
documents the security concern being validated and makes the assertion's purpose
more explicit for future maintainers.
- Around line 442-471: The test function uses mock.module() to mock
isChannelsEnabled but does not include cleanup between tests, causing mock state
to persist across the test suite. Add an afterEach hook within the describe
block that contains this test and call mock.restoreModule() to restore the
module state between each test execution. This will prevent the mocking of
isChannelsEnabled from one test affecting the behavior of subsequent tests in
the same describe block.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: b875fe0e-d223-4d8e-8883-86faefc5dfa8
📒 Files selected for processing (4)
src/__tests__/bugfixes.test.tssrc/components/LogoV2/ChannelsNotice.tsxsrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (15)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/__tests__/bugfixes.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.tssrc/components/LogoV2/ChannelsNotice.tsx
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/__tests__/bugfixes.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.tssrc/components/LogoV2/ChannelsNotice.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/__tests__/bugfixes.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/__tests__/bugfixes.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/__tests__/bugfixes.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.tssrc/components/LogoV2/ChannelsNotice.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/__tests__/bugfixes.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.tssrc/components/LogoV2/ChannelsNotice.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/__tests__/bugfixes.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.tssrc/components/LogoV2/ChannelsNotice.tsx
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/__tests__/bugfixes.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/__tests__/bugfixes.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/services/mcp/channelPermissions.tssrc/components/LogoV2/ChannelsNotice.tsx
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**
⚙️ CodeRabbit configuration file
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**: Review permission prompts, auto-allow logic, sandbox behavior, SDK permission schemas, shell/PowerShell execution, and background execution paths as security-sensitive. Block on bypasses, unclear trust boundaries, unsafe path handling, missing user visibility, or changes that broaden allowed behavior without an explicit maintainer decision.
Files:
src/hooks/toolPermission/handlers/interactiveHandler.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/mcp/channelPermissions.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/mcp/channelPermissions.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/mcp/channelPermissions.ts
src/{skills,utils/plugins,services/mcp}/**
⚙️ CodeRabbit configuration file
src/{skills,utils/plugins,services/mcp}/**: Review skill/plugin/MCP behavior as a trust boundary. Check registry fetches, local and remote installs, path normalization, hash verification, revocation/trust metadata, tools_required handling, config-home behavior, and startup-time loading. Block on path traversal risk, unverified downloads, silent trust promotion, or unexpected code/tool activation.
Files:
src/services/mcp/channelPermissions.ts
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI implementations
Files:
src/components/LogoV2/ChannelsNotice.tsx
🔇 Additional comments (3)
src/components/LogoV2/ChannelsNotice.tsx (1)
16-95: LGTM!src/hooks/toolPermission/handlers/interactiveHandler.ts (1)
323-334: LGTM!src/services/mcp/channelPermissions.ts (1)
176-202: LGTM!
07bf819 to
65f152e
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P1] Finish marketplace-gating permission relays
src/hooks/toolPermission/handlers/interactiveHandler.ts:333
The permission relay path still treats a same-name plugin from the wrong marketplace as allowed. This callback only checksfindChannelEntry(name, allowedChannels, pluginSource) !== undefined, butfindChannelEntry()returns the first candidate immediately when there is only oneplugin:slack@anthropicentry, before comparing the runtimepluginSource. I verified thatfindChannelEntry('plugin:slack', [{kind:'plugin', name:'slack', marketplace:'anthropic'}], 'plugin:slack@evilcorp')returns the Anthropic entry.gateChannelServer()has a separate marketplace check after this lookup, but the permission relay path never runs that gate, so an unapprovedplugin:slack@evilcorpclient can still receivenotifications/claude/channel/permission_requestpreviews for tool names/descriptions/input. Please complete the marketplace verification here by reusing the actual gate result or by making the relay predicate reject mismatched or missing plugin sources. -
[P2] Complete CodeRabbit's request to clean up the channelAllowlist mock
src/__tests__/bugfixes.test.ts:450
CodeRabbit's test-isolation request is still valid. The new dev-channel coverage registersmock.module('../services/mcp/channelAllowlist.js', ...)but theafterEachonly callsmock.restore(), which this repo already documents does not clearmock.module()overrides. Running the new test file with the neighboring channel gate tests fails afterbugfixes.test.tspasses:bun test src/__tests__/bugfixes.test.ts src/services/mcp/channelNotification.test.tsreportsExport named 'getChannelAllowlist' not found in module ... channelAllowlist.ts, whilebun test src/services/mcp/channelNotification.test.tspasses by itself. Please complete that review request by restoring the realchannelAllowlistmodule, or otherwise isolating this mock, so CI and adjacent tests do not depend on file ordering.
|
Pushed two follow-ups in commit [P1] Permission relay mirrors the marketplace gate. The predicate no longer just calls
A Added the missing [P2] channelAllowlist mock cleanup.
Also expanded the dev-map count comment to document the security invariant (a dev entry must never be confused with a production entry in the allowlist check) per CodeRabbit's request. Verification
|
0936c93 to
4ef3a65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/urlRedaction.test.ts`:
- Around line 42-48: The test 'fallback redaction also drops fragments for
malformed URLs' has an expectation that conflicts with another test at lines
123-128 which asserts fragment preservation. Update the expectation in the
expect statement at line 47 to align with the consistent fragment preservation
behavior demonstrated in the other test, ensuring that fragments are handled the
same way across both test cases for malformed URLs.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 72eb120d-149c-4803-95a8-14a48766017d
📒 Files selected for processing (25)
scripts/system-check.tssrc/__tests__/bugfixes.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/LogoV2/ChannelsNotice.tsxsrc/components/ProviderManager.tsxsrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/interactiveHelpers.tsxsrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelNotification.test.tssrc/services/mcp/channelNotification.tssrc/services/mcp/channelPermissions.tssrc/utils/debug.tssrc/utils/diagnostics/issueReport.tssrc/utils/diagnostics/redaction.test.tssrc/utils/diagnostics/redaction.tssrc/utils/log.tssrc/utils/redaction.tssrc/utils/requestSizeBreakdown.tssrc/utils/status.tsxsrc/utils/statusRedaction.test.tssrc/utils/statusRedaction.tssrc/utils/urlRedaction.test.tssrc/utils/urlRedaction.ts
💤 Files with no reviewable changes (3)
- src/utils/statusRedaction.ts
- src/utils/diagnostics/redaction.ts
- src/utils/urlRedaction.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
scripts/system-check.tssrc/utils/status.tsxsrc/components/ProviderManager.tsxsrc/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/interactiveHelpers.tsxsrc/components/LogoV2/ChannelsNotice.tsxsrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
scripts/system-check.tssrc/utils/status.tsxsrc/components/ProviderManager.tsxsrc/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/interactiveHelpers.tsxsrc/components/LogoV2/ChannelsNotice.tsxsrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
scripts/system-check.tssrc/utils/status.tsxsrc/components/ProviderManager.tsxsrc/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/interactiveHelpers.tsxsrc/components/LogoV2/ChannelsNotice.tsxsrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
scripts/system-check.tssrc/utils/status.tsxsrc/components/ProviderManager.tsxsrc/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/interactiveHelpers.tsxsrc/components/LogoV2/ChannelsNotice.tsxsrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
scripts/system-check.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
scripts/system-check.tssrc/utils/status.tsxsrc/components/ProviderManager.tsxsrc/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/interactiveHelpers.tsxsrc/components/LogoV2/ChannelsNotice.tsxsrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/utils/status.tsxsrc/components/ProviderManager.tsxsrc/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/interactiveHelpers.tsxsrc/components/LogoV2/ChannelsNotice.tsxsrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/components/Feedback.tsxsrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI implementations
Files:
src/components/ProviderManager.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/LogoV2/ChannelsNotice.tsxsrc/components/Feedback.tsx
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/utils/debug.tssrc/utils/diagnostics/redaction.test.tssrc/utils/diagnostics/issueReport.tssrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/services/mcp/channelNotification.test.tssrc/utils/requestSizeBreakdown.tssrc/utils/log.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.tssrc/services/mcp/channelNotification.tssrc/utils/redaction.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/utils/diagnostics/redaction.test.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/utils/diagnostics/redaction.test.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/diagnostics/redaction.test.tssrc/__tests__/bugfixes.test.tssrc/services/mcp/channelNotification.test.tssrc/utils/statusRedaction.test.tssrc/utils/urlRedaction.test.ts
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**
⚙️ CodeRabbit configuration file
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**: Review permission prompts, auto-allow logic, sandbox behavior, SDK permission schemas, shell/PowerShell execution, and background execution paths as security-sensitive. Block on bypasses, unclear trust boundaries, unsafe path handling, missing user visibility, or changes that broaden allowed behavior without an explicit maintainer decision.
Files:
src/hooks/toolPermission/handlers/interactiveHandler.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/services/mcp/channelNotification.test.tssrc/services/mcp/channelNotification.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelPermissions.tssrc/services/mcp/channelNotification.test.tssrc/services/mcp/channelNotification.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/logging.tssrc/services/api/openaiShim.ts
src/{skills,utils/plugins,services/mcp}/**
⚙️ CodeRabbit configuration file
src/{skills,utils/plugins,services/mcp}/**: Review skill/plugin/MCP behavior as a trust boundary. Check registry fetches, local and remote installs, path normalization, hash verification, revocation/trust metadata, tools_required handling, config-home behavior, and startup-time loading. Block on path traversal risk, unverified downloads, silent trust promotion, or unexpected code/tool activation.
Files:
src/services/mcp/channelPermissions.tssrc/services/mcp/channelNotification.test.tssrc/services/mcp/channelNotification.ts
🪛 ast-grep (0.43.0)
src/utils/redaction.ts
[warning] 131-134: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(?<![A-Za-z0-9_])(${escaped.join('|')})\\s*[=:]\\s*["']?[^"'\\s)}\\]]+["']?,
'gi',
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 520-520: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${escapeRegExp(normalizedHome)}(?=$|[/\\\\]), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 GitHub Actions: PR Checks / 1_smoke-and-tests.txt
src/utils/urlRedaction.test.ts
[error] 47-47: Test assertion failed in Jest/Vitest: expect(received).toBe(expected). Redaction output did not match expected string.
🪛 GitHub Actions: PR Checks / smoke-and-tests
src/utils/urlRedaction.test.ts
[error] 47-47: Test failed: expect(received).toBe(expected). Expected and received URL redaction strings differ (URL redaction output mismatch).
🪛 GitHub Check: smoke-and-tests
src/utils/urlRedaction.test.ts
[failure] 47-47: error: expect(received).toBe(expected)
Expected: "//redacted@localhost:11434?token=redacted"
Received: "//redacted@localhost:11434?token=redacted#access_token=fragment-secret"
at <anonymous> (/home/runner/work/openclaude/openclaude/src/utils/urlRedaction.test.ts:47:22)
🔇 Additional comments (34)
src/services/mcp/channelNotification.ts (7)
10-30: LGTM!
137-150: LGTM!
152-165: LGTM!
196-225: LGTM!
273-293: LGTM!
319-333: LGTM!
334-347: LGTM!src/components/LogoV2/ChannelsNotice.tsx (4)
16-27: LGTM!
57-92: LGTM!
99-117: LGTM!
179-179: LGTM!src/interactiveHelpers.tsx (1)
264-312: LGTM!src/hooks/toolPermission/handlers/interactiveHandler.ts (2)
13-13: LGTM!
324-347: LGTM!src/services/mcp/channelPermissions.ts (1)
176-198: LGTM!src/services/mcp/channelNotification.test.ts (1)
1-232: LGTM!src/__tests__/bugfixes.test.ts (3)
11-11: LGTM!
413-444: LGTM!
446-551: LGTM!src/utils/redaction.ts (1)
1-576: LGTM!src/services/api/logging.ts (1)
24-24: LGTM!Also applies to: 272-272
src/utils/log.ts (1)
23-23: LGTM!Also applies to: 182-206
src/utils/status.tsx (1)
24-33: LGTM!Also applies to: 87-89, 124-424, 647-770
src/utils/statusRedaction.test.ts (1)
8-8: LGTM!Also applies to: 163-182
src/utils/diagnostics/issueReport.ts (1)
36-36: LGTM!src/components/Feedback.tsx (1)
24-24: LGTM!Also applies to: 72-73
src/components/FeedbackSurvey/submitTranscriptShare.ts (1)
16-16: LGTM!Also applies to: 72-83
src/utils/debug.ts (1)
16-16: LGTM!Also applies to: 221-223
scripts/system-check.ts (1)
26-26: LGTM!src/components/ProviderManager.tsx (1)
78-78: LGTM!src/services/api/openaiShim.ts (2)
86-86: LGTM!
2858-2872: No action needed—both changed behaviors are already covered by focused regression tests.The precedence logic at lines 2858–2872 is tested by the "opencode go %s direct env routing ignores stale custom auth" test (openaiShim.test.ts:2458), which verifies that
catalogEntry.transportOverrides.openaiShim.defaultAuthHeadertakes precedence overOPENAI_AUTH_HEADER_VALUEenv vars using real gateway catalog data.The response URL preservation at lines 3207–3248 is tested by "non-streaming: preserves response.url routing metadata after body read" (openaiShim.test.ts:4657), which confirms that
response.urlsurvives Response recreation by asserting the correct routing path (/messages) is preserved.src/utils/requestSizeBreakdown.ts (1)
4-4: LGTM!src/utils/diagnostics/redaction.test.ts (1)
10-10: LGTM!
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/urlRedaction.test.ts`:
- Around line 42-48: The test 'fallback redaction also drops fragments for
malformed URLs' has an expectation that conflicts with another test at lines
123-128 which asserts fragment preservation. Update the expectation in the
expect statement at line 47 to align with the consistent fragment preservation
behavior demonstrated in the other test, ensuring that fragments are handled the
same way across both test cases for malformed URLs.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 72eb120d-149c-4803-95a8-14a48766017d
📒 Files selected for processing (25)
scripts/system-check.tssrc/__tests__/bugfixes.test.tssrc/components/Feedback.tsxsrc/components/FeedbackSurvey/submitTranscriptShare.tssrc/components/LogoV2/ChannelsNotice.tsxsrc/components/ProviderManager.tsxsrc/hooks/toolPermission/handlers/interactiveHandler.tssrc/interactiveHelpers.tsxsrc/services/api/logging.tssrc/services/api/openaiShim.tssrc/services/mcp/channelNotification.test.tssrc/services/mcp/channelNotification.tssrc/services/mcp/channelPermissions.tssrc/utils/debug.tssrc/utils/diagnostics/issueReport.tssrc/utils/diagnostics/redaction.test.tssrc/utils/diagnostics/redaction.tssrc/utils/log.tssrc/utils/redaction.tssrc/utils/requestSizeBreakdown.tssrc/utils/status.tsxsrc/utils/statusRedaction.test.tssrc/utils/statusRedaction.tssrc/utils/urlRedaction.test.tssrc/utils/urlRedaction.ts
💤 Files with no reviewable changes (3)
- src/utils/statusRedaction.ts
- src/utils/diagnostics/redaction.ts
- src/utils/urlRedaction.ts
📜 Review details
🔇 Additional comments (34)
src/services/mcp/channelNotification.ts (7)
10-30: LGTM!
137-150: LGTM!
152-165: LGTM!
196-225: LGTM!
273-293: LGTM!
319-333: LGTM!
334-347: LGTM!src/components/LogoV2/ChannelsNotice.tsx (4)
16-27: LGTM!
57-92: LGTM!
99-117: LGTM!
179-179: LGTM!src/interactiveHelpers.tsx (1)
264-312: LGTM!src/hooks/toolPermission/handlers/interactiveHandler.ts (2)
13-13: LGTM!
324-347: LGTM!src/services/mcp/channelPermissions.ts (1)
176-198: LGTM!src/services/mcp/channelNotification.test.ts (1)
1-232: LGTM!src/__tests__/bugfixes.test.ts (3)
11-11: LGTM!
413-444: LGTM!
446-551: LGTM!src/utils/redaction.ts (1)
1-576: LGTM!src/services/api/logging.ts (1)
24-24: LGTM!Also applies to: 272-272
src/utils/log.ts (1)
23-23: LGTM!Also applies to: 182-206
src/utils/status.tsx (1)
24-33: LGTM!Also applies to: 87-89, 124-424, 647-770
src/utils/statusRedaction.test.ts (1)
8-8: LGTM!Also applies to: 163-182
src/utils/diagnostics/issueReport.ts (1)
36-36: LGTM!src/components/Feedback.tsx (1)
24-24: LGTM!Also applies to: 72-73
src/components/FeedbackSurvey/submitTranscriptShare.ts (1)
16-16: LGTM!Also applies to: 72-83
src/utils/debug.ts (1)
16-16: LGTM!Also applies to: 221-223
scripts/system-check.ts (1)
26-26: LGTM!src/components/ProviderManager.tsx (1)
78-78: LGTM!src/services/api/openaiShim.ts (2)
86-86: LGTM!
2858-2872: No action needed—both changed behaviors are already covered by focused regression tests.The precedence logic at lines 2858–2872 is tested by the "opencode go %s direct env routing ignores stale custom auth" test (openaiShim.test.ts:2458), which verifies that
catalogEntry.transportOverrides.openaiShim.defaultAuthHeadertakes precedence overOPENAI_AUTH_HEADER_VALUEenv vars using real gateway catalog data.The response URL preservation at lines 3207–3248 is tested by "non-streaming: preserves response.url routing metadata after body read" (openaiShim.test.ts:4657), which confirms that
response.urlsurvives Response recreation by asserting the correct routing path (/messages) is preserved.src/utils/requestSizeBreakdown.ts (1)
4-4: LGTM!src/utils/diagnostics/redaction.test.ts (1)
10-10: LGTM!
🛑 Comments failed to post (1)
src/utils/urlRedaction.test.ts (1)
42-48:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the malformed-URL fragment expectation at Line 47 (this is currently breaking CI).
Line 47 expects fragment removal, but this conflicts with the malformed fallback contract and with your own test at Lines 123-128 that asserts fragment preservation. That contradiction is the smoke-and-tests failure.
Suggested patch
- test('fallback redaction also drops fragments for malformed URLs', () => { + test('fallback redaction preserves fragments for malformed URLs', () => { const redacted = redactUrlForDisplay( '//user:pass@localhost:11434?token=abc#access_token=fragment-secret', ) - expect(redacted).toBe('//redacted@localhost:11434?token=redacted') + expect(redacted).toBe( + '//redacted@localhost:11434?token=redacted#access_token=fragment-secret', + ) })As per coding guidelines, “If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.”
🧰 Tools
🪛 GitHub Actions: PR Checks / 1_smoke-and-tests.txt
[error] 47-47: Test assertion failed in Jest/Vitest: expect(received).toBe(expected). Redaction output did not match expected string.
🪛 GitHub Actions: PR Checks / smoke-and-tests
[error] 47-47: Test failed: expect(received).toBe(expected). Expected and received URL redaction strings differ (URL redaction output mismatch).
🪛 GitHub Check: smoke-and-tests
[failure] 47-47: error: expect(received).toBe(expected)
Expected: "//redacted@localhost:11434?token=redacted"
Received: "//redacted@localhost:11434?token=redacted#access_token=fragment-secret"at <anonymous> (/home/runner/work/openclaude/openclaude/src/utils/urlRedaction.test.ts:47:22)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/urlRedaction.test.ts` around lines 42 - 48, The test 'fallback redaction also drops fragments for malformed URLs' has an expectation that conflicts with another test at lines 123-128 which asserts fragment preservation. Update the expectation in the expect statement at line 47 to align with the consistent fragment preservation behavior demonstrated in the other test, ensuring that fragments are handled the same way across both test cases for malformed URLs.Sources: Coding guidelines, Pipeline failures
…-drop, embedded URL query redaction - Restore &#; delimiters in generic pattern value classes (F1) so safe query tails (&mode=test) survive. Re-add &-tail post-processor for non-URL abc&def case. - Gate redactUrlForDisplay in jsonRedactor to https?:// strings only (F2) to prevent #-drop on ordinary text like 'fails after #setup'. - Add URL query redaction step to redactSensitiveInfo (F3) that extracts https?:// URLs from free-form text and routes them through redactUrlForDisplay, catching signature/sig params that generic patterns miss. Skip already-redacted URLs to avoid double-redaction.
…permission truthy check
8063f2c to
7fea750
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found a couple of issues that still need to be addressed.
Findings
-
[P2] Do not exempt token containers from structural redaction
src/utils/redaction.ts:325
jsonRedactor()now excludes the exact keytokensso usage counters can pass through, but this also exempts arbitrary token containers before the sensitive-key check runs. For example,JSON.stringify({ tokens: ["opaque-secret-value"] }, jsonRedactor)currently preserves the opaque value, and the same leak survives throughsanitizeError()on enumerable error payloads such as{ cause: { tokens: ["opaque-secret-value"] } }. Because this replacer is now used for feedback/transcript payloads, MCP header/debug logging, permission previews, and error sanitization, a dependency or provider error that uses a generictokensfield can leak credentials that do not match a hard-coded token regex. Please limit the token-count exemption to numeric usage shapes, or recurse/redact non-numerictokenscontainers, and add a regression for an opaque token value under atokensarray/object. -
[P2] Preserve semicolon query tails for cookie params
src/utils/redaction.ts:119
The cookie header pass is scoped away from?cookie=...and&cookie=..., but it still matches semicolon-delimited URL query segments because;cookie=is not excluded. That meansredactSensitiveInfo("https://example.com/v1?foo=bar;cookie=secret;mode=test")returnshttps://example.com/v1?foo=bar;cookie=redacted, dropping the safemode=testparameter before the URL redactor can preserve it;set-cookiebehaves the same. Please keep the full-header cookie matcher out of URL query segments separated by;, while still lettingredactUrlForDisplay()redact the cookie value and preserve safe trailing params.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and found one issue that still needs to be addressed.
Summary
CodeRabbit previously flagged that the generic secret redactors now stop at & in non-URL text, and CONTRIBUTING.md says PR authors must address CodeRabbit findings before waiting for maintainer override. That concern is still valid on the current head, so please complete that automated-review item as part of this PR.
Findings
- [P2] Complete CodeRabbit's request to keep generic secret values whole outside URLs
src/utils/redaction.ts:80
The current regexes stop at URL delimiters globally, not only while redacting URL query strings. That preserves safe URL tails, but it leaves the rest of compound secret values in ordinary log/error/preview text:redactSensitiveInfo("DATABASE_PASSWORD=correct&horse=battery")returnsDATABASE_PASSWORD=[REDACTED]&horse=battery, andtruncateForPreview({ command: "export DATABASE_PASSWORD=correct&horse=battery" })keeps the same&horse=batterysuffix. The same happens forx-api-key: abc&def=ghiandtoken=abc;def=ghi. Because this helper feeds debug logs, error sanitization, transcript/feedback payloads, and channel permission previews, non-URL credentials containing shell/query delimiters can still leak. Please scope the delimiter-preserving behavior to URL-shaped query spans or otherwise let the generic env/header redactors consume the full non-URL value, and add regressions that keep bothDATABASE_PASSWORD=correct&horse=batteryfully redacted andhttps://example.com/v1?OPENAI_API_KEY=secret&mode=testpreservingmode=test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
|
@kevincodex1 check it please. LGTM |
|
@kevincodex1 MARGE IT PLEASE |
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
LGTM.
thanks for contribution @Gravirei please join discord
…ate tests (Gitlawb#1711) * feat(utils): add centralized redaction utility Single source of truth for stripping API keys, tokens, and other secrets from strings and JSON. Provider env-var coverage is generated from getKnownProviderSecretEnvKeys() so adding a new provider cannot silently create an unredacted path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(Feedback): import redactSensitiveInfo from utils Remove the inline 40-line regex implementation in favor of the centralized redaction utility, eliminating drift between Feedback and the transcript share path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(submitTranscriptShare): import redactSensitiveInfo from utils Update import path to point at the centralized utility instead of the Feedback component. Removes the implicit re-export contract that required Feedback.tsx to keep redactSensitiveInfo exported. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(log,debug): redact secrets in default error and debug output Wire the centralized redaction utility into logError and logForDebugging so secrets cannot leak into in-memory error logs or the debug file even if a caller forgets to pass through redactSensitiveInfo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(api/logging): redact error message in logAPIError Apply the centralized redaction utility to the error string passed to logEvent so analytics events cannot capture unredacted credentials from upstream API failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: resolve merge conflict from upstream sync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(channelNotification): allow null in getEffectiveChannelAllowlist signature ChannelsNotice.tsx passes getSubscriptionType() which returns SubscriptionType | null, but the signature only accepted string | undefined. Widen to string | null so the call site typechecks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(redaction): exclude specific token fields from redaction process * fix(redaction): lower AIza minimum length to {10,} Real GCP/Gemini keys are 39 chars total (4 prefix + 35 suffix), but the {35} suffix bound missed short tokens like 'AIzaSyDUMMY-secret-token' (21 chars after AIza). Lower to {10,} to match the diagnostics module and catch any AIza-shaped value. Same precision trade-off the diagnostics redaction makes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction,log): address review feedback - Drop quotes from ANTHROPIC/OPENAI key negative lookarounds so JSON-shaped values like "sk-ant-..." redact. - Add private_key pattern to GENERIC_HEADER_FIELD_PATTERN and privatekey to SENSITIVE_FIELD_SUBSTRINGS. - logError now builds a sanitized Error (redacted message + stack) before passing to the sink and queue, not just the in-memory log. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(redaction): consolidate into single module + add channel gate tests Address the three P2 review findings on the central-redaction PR: [1] Consolidate four redaction modules into src/utils/redaction.ts. Previously lived in: - src/utils/redaction.ts (logs/bug reports/transcript shares) - src/utils/urlRedaction.ts (URL display) - src/utils/statusRedaction.ts (/status output) - src/utils/diagnostics/redaction.ts (doctor reports) The four surfaces share the same regex set / credential lists but had drifted into separate per-domain files. Merged into one module; deleted the three shim files. Updated six direct consumers (openaiShim.ts, ProviderManager.tsx, status.tsx, requestSizeBreakdown.ts, diagnostics/issueReport.ts, scripts/system-check.ts) and three test files to import from redaction.js. [2] Add gateChannelServer() test coverage. src/services/mcp/channelNotification.test.ts: 13 cases for the six gate paths (capability, runtime, session, marketplace, plugin allowlist, server-entry dev) plus end-to-end register. Mocks channelAllowlist.js (GrowthBook-backed) so tests stay independent of feature-flag state. [3] Apply jsonRedactor in transcript share. src/components/FeedbackSurvey/submitTranscriptShare.ts now does redactSensitiveInfo(jsonStringify(data, jsonRedactor)) — the key-aware redaction applies during serialization, and the text pass stays as defense in depth for free-form fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(channelNotification): cover findChannelEntry multi-candidate branch Regression test for the disambiguation path in `findChannelEntry` (channelNotification.ts:201-230): when two same-name plugin entries exist in the allowed-channels list with different marketplaces, `pluginSource` must select the matching entry before the marketplace and allowlist gates evaluate. Without this branch being exercised, the gate could lock onto whichever entry sorts first and either skip the user's real installation or wrongly authorize a typo-squatted one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction): align URL fallback regex + add path-prefix boundary check Two related redaction correctness fixes: [1] URL fallback regex covers the same parameter set as the primary path. The malformed-URL branch in `redactUrlForDisplay` previously had a hand-rolled alternation of credential parameter names that could drift behind `SENSITIVE_URL_QUERY_PARAM_TOKENS`. New `MALFORMED_URL_PARAM_PATTERN` derives from that same list, so the two paths can never diverge. Tests cover the full credential set (`api_key`, `access_token`, `refresh_token`, `signature`, `sig`, `secret`, `password`, `apikey`) plus a non-sensitive `model` that must survive. [2] `redactPathForStatus` now requires a path-separator boundary after the home prefix. The previous `startsWith` check matched `/home/alice2/project` against `/home/alice` and emitted `~2/project`. The fix requires the character at `normalizedCandidate.length` to be `/` or `\` so `alice` no longer matches `alice2` or `alice.bak`. Test pins the false-positive paths and the true-positive (`/home/alice/project` → `~/project`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(channel,redaction): restore dev-channel warning + align URL fallback Two related security fixes: [1] Restore DevChannelsDialog when --dangerously-load-development-channels is passed and the channels feature is enabled. The previous logic skipped the dialog when OAuth was absent, which was safe only while gateChannelServer() blocked no-OAuth sessions. With the OAuth/org- policy gates removed in this PR, an API-key session could pass the flag, skip the warning, and still register the dev channel. The only remaining skip is the genuinely-disabled feature case (`!isChannelsEnabled()`), where the dialog is moot. [2] Malformed-URL fallback now uses the same substring predicate as the primary `URL` parser path. The previous regex matched only exact parameter names (`api_key=`, `access_token=`, …), so `my_api_key=SECRET` and `x_access_token=TOKEN` slipped through unchanged even though `shouldRedactUrlQueryParam` flags them as sensitive. New `redactMalformedQuery` walks the query pairs and runs the predicate on each key. Three new tests cover prefixed keys, non-sensitive keys, and fragment preservation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction): widen key boundary class + tighten dev-channel comment Two small follow-ups from the latest CodeRabbit review: [1] Boundary class on key-prefix patterns widened from `[A-Za-z0-9]` to `[A-Za-z0-9_-]` so a raw key embedded in a JSON string value (`"sk-ant-..."`, `"AIza..."`, `"ghp_..."`, etc.) is still caught. Quotes act as delimiters, not blockers — the previous boundary class was correct for unquoted text but let quoted keys slip through. [2] Tighten the dev-channel dialog comment in interactiveHelpers.tsx so future readers don't misread the security boundary. Skip condition is `isChannelsEnabled()` (the channels feature flag gate), not KAIROS / KAIROS_CHANNELS as the previous wording implied. Comment now matches the code. Skipped with reason: - getEffectiveChannelAllowlist divergence from gateChannelServer allowlist — by design; the effective-list override is a UI hint consumed only by ChannelsNotice for the org-override indicator. Trust boundary is enforced by gateChannelServer() reading the hardcoded ledger. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction,channel): address P1/P2 review findings P1 - malformed URL fallback secrets: - Decode percent-encoded query param keys via decodeURIComponent() before applying shouldRedactUrlQueryParam (e.g. %74oken -> token) - Stop userinfo regex at ? and # delimiters to avoid consuming query params when matching @ signs in email addresses or fragment delimiters P2 - channel notice/gate allowlist sync: - Remove org override path from getEffectiveChannelAllowlist() so ChannelsNotice startup guidance uses the same ledger source as gateChannelServer's runtime enforcement - Simplify ChannelsNotice to drop unused sub/policy params and the source === 'org' conditional * fix(channel): apply marketplace matching to permission relays, remove stale OAuth/org-policy blockers, add dev-channel dialog coverage P1: Thread runtime pluginSource through filterPermissionRelayClients so findChannelEntry disambiguates same-name plugin entries from different marketplaces before sending permission request previews. P2: Remove stale noAuth and policyBlocked branches from ChannelsNotice that would render '--channels ignored' before reaching the listening message, confusing non-OAuth users. P2: Add test coverage that mocks isChannelsEnabled() both true and false, verifies DevChannelsDialog appears with onAccept marking entries dev:true in the enabled case, and verifies the disabled branch registers entries directly without dialog. * test(dev-channel): clarify count assertion comment + add afterEach with mock.restore() * fix(channel): mirror marketplace gate in permission relay + restore mock Two follow-ups from the latest review: [1] Permission relay predicate no longer relies on findChannelEntry alone. After resolving the entry, the predicate now requires a runtime pluginSource whose marketplace matches the session entry's marketplace for plugin-kind entries — mirroring the gateChannelServer check at channelNotification.ts:303-312. A `plugin:slack@evilcorp` client whose session allows `plugin:slack@anthropic` is now rejected instead of piggy-backing on the approved entry to receive permission-request previews. Server-kind entries still match on bare name. [2] bugfixes.test.ts now re-registers the real channelAllowlist module in afterEach via a cache-busted reference, so the neighbor channelNotification.test.ts continues to import getChannelAllowlist after this suite runs. mock.restore() does not clear module-level mock.module() overrides in bun (the registry is process-global). Pattern matches compact.test.ts:27-36. Also expanded the dev-map count comment in bugfixes.test.ts to document the security invariant (a dev entry must never be confused with a production entry in the allowlist check) per CodeRabbit's request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(redaction): consolidate into single module + add channel gate tests Address the three P2 review findings on the central-redaction PR: [1] Consolidate four redaction modules into src/utils/redaction.ts. Previously lived in: - src/utils/redaction.ts (logs/bug reports/transcript shares) - src/utils/urlRedaction.ts (URL display) - src/utils/statusRedaction.ts (/status output) - src/utils/diagnostics/redaction.ts (doctor reports) The four surfaces share the same regex set / credential lists but had drifted into separate per-domain files. Merged into one module; deleted the three shim files. Updated six direct consumers (openaiShim.ts, ProviderManager.tsx, status.tsx, requestSizeBreakdown.ts, diagnostics/issueReport.ts, scripts/system-check.ts) and three test files to import from redaction.js. [2] Add gateChannelServer() test coverage. src/services/mcp/channelNotification.test.ts: 13 cases for the six gate paths (capability, runtime, session, marketplace, plugin allowlist, server-entry dev) plus end-to-end register. Mocks channelAllowlist.js (GrowthBook-backed) so tests stay independent of feature-flag state. [3] Apply jsonRedactor in transcript share. src/components/FeedbackSurvey/submitTranscriptShare.ts now does redactSensitiveInfo(jsonStringify(data, jsonRedactor)) — the key-aware redaction applies during serialization, and the text pass stays as defense in depth for free-form fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): align malformed URL fragment expectation with preservation behavior * fix: address review findings P1 and P2 [P1] Enforce dev flag for server-kind entries in permission relay predicate, matching gateChannelServer() behavior. Add coverage for both dev and non-dev server relay paths. [P2] Drop fragments in malformed URL fallback (redactMalformedQuery) to match the valid-URL path, preventing credential leaks via fragment-carried tokens. Update existing tests and add regression for fragment-only malformed URLs. * test(relay): add plugin-kind marketplace regression tests * fix: address review findings P1 and P2 [P1] Add PEM private key redaction pattern to redactSensitiveInfo so multi-line PEM values are fully consumed instead of leaking after the first whitespace. Add [ to generic header pattern's value exclusion set to prevent re-consuming [REDACTED] tokens. [P2] Use truthy check (Boolean()) for claude/channel capability in filterPermissionRelayClients to match gateChannelServer's behavior, rejecting explicit false capabilities. * fix(debug): redact before JSON-stringify multiline messages Reorder logForDebugging so redactSensitiveInfo runs before jsonStringify, ensuring PEM/private-key patterns match the raw (unescaped) message text rather than the JSON-encoded form where colons and quotes are escaped. * test(debug): add end-to-end regression for multiline PEM redaction in logForDebugging Uses mock.module on process.js to capture stderr output and exercises the full logForDebugging path with multiline PEM private_key input, verifying the redact-before-JSON-stringify ordering produces redacted output. * fix(test): preserve original process.env.DEBUG and process.argv in logForDebugging test hooks * fix: address PR review findings P1-P3/P5-P7 - P1: clear isDebugMode/isDebugToStdErr memoize caches in test beforeEach + cache-busting query param for fresh debug.ts imports - P2: restore mock.module afterAll instead of leaking mock + mutate err in-place in logError to preserve name/cause - P3: post-processing regex absorbs trailing bracket content after [REDACTED] - P5: (was P3) expand jsonRedactor EXCLUDED_KEYS for maxTokens etc. - P7: capture HOME/USERPROFILE per-test instead of at module scope * fix: address CodeRabbit review findings - interactiveHelpers.tsx: update dev-channel comment — OAuth/org-policy gates removed from gateChannelServer(), org policy is not enforced - channelNotification.test.ts: add afterAll mock.restore() to clean up process-global channelAllowlist.js mock - channelNotification.ts: fix comments — isChannelsEnabled() still reads tengu_harbor, not always true - log.ts: sanitize err.message and err.stack separately so message doesn't get replaced with full stack trace - redaction.ts: add 'i' flag to redactHomePath regex for Windows case-insensitive path matching * fix: address second review round - interactiveHandler.ts: [P2] redact input_preview via redactSensitiveInfo before sending to channel servers - log.ts: [P3] copy error via Object.assign(Object.create(err), err) before sanitizing instead of mutating in-place * fix: address CodeRabbit second round - channelPermissions.ts: redact before truncate in truncateForPreview so partial credentials don't leak at the 200-char boundary - interactiveHandler.ts: remove outer redactSensitiveInfo — now handled inside truncateForPreview - log.ts: derive errorInfo.error from already-sanitized sanitizedErr; fix Object.assign comment to accurately describe what is copies * fix: improve permission relay client filtering and enhance redaction functions * fix: address third review round (P1, P2, P3) - P1: update test expectations for [REDACTED_*] output format - P2: add total_tokens, prompt_tokens, completion_tokens to jsonRedactor EXCLUDED_KEYS - P3: remove ) and } from GENERIC_HEADER_FIELD_PATTERN value capture to prevent content leak after embedded parens - Fix buildKnownEnvVarPattern capture group to preserve env-var separator ([REDACTED]) - Add & to GENERIC_CREDENTIAL_ENV_PATTERN value exclusion to prevent URL query over-consumption * fix: address latest reviewer P2/P3 findings (errorLogSink redaction, X_API_KEY/AUTHORIZATION patterns, regression tests) * fix: address reviewer P1/P2 — bracketed values and multi-word header values - P1: Remove and from value captures in X_API_KEY_PATTERN, AUTHORIZATION_PATTERN, GENERIC_HEADER_FIELD_PATTERN, GENERIC_CREDENTIAL_ENV_PATTERN so bracketed secrets like are fully redacted instead of passing through unchanged. - P2: Widen header-style value captures to include spaces by removing from exclusions, using as delimiter (stops at newlines and URL query separators). Fixes multi-word leaks: , , , . - GENERIC_CREDENTIAL_ENV_PATTERN: add to negative lookbehind to prevent matching inside when the latter is already redacted. - GENERIC_HEADER_FIELD_PATTERN replacer: skip values starting with to preserve specific labels from earlier passes. - Add 7 regression tests covering both finding categories. * fix: address reviewer findings P1-P4 P1: Custom enumerable error properties now redacted in log.ts logError iterates all own enumerable properties on the original error and applies redactSensitiveInfo to string values and jsonRedactor to object values, preventing credential-bearing custom fields from leaking through the sanitized error. Regression tests added in log.test.ts. P2: Soften single-source-of-truth claim; migrate easy call sites Header comment in redaction.ts updated to acknowledge that specialized scanners (secretScanner.ts, xaa.ts) are intentional exceptions. src/services/mcp/client.ts and src/services/mcp/auth.ts now use jsonRedactor for header redaction instead of ad-hoc key checks. P3: Fix mock.restore cleanup in channelNotification.test.ts Cache-bust the real channelAllowlist module at describe-entry and re-register it in afterAll, following the pattern from bugfixes.test.ts. mock.restore alone does not clear mock.module overrides in Bun. P4: Remove unused ChannelGateResult kinds Removed 'auth' and 'policy' from the skip kind union and removed corresponding dead branches in useManageMCPConnections.ts. * fix: extract sanitizeError() to fix CI test fragility The logError tests were failing in CI due to parallel test execution racing on the module-level errorLogSink singleton. Extract the inline sanitization logic into an exported sanitizeError() helper and test that directly — it's pure, has no env-var or sink dependencies, and doesn't interact with shared mutable state. * fix: use Object.getPrototypeOf(err) instead of err as prototype in sanitizeError Object.create(err) sets the original error instance as the prototype of the sanitized copy, leaking non-enumerable own properties through the prototype chain. Use Object.getPrototypeOf(err) instead so the prototype is the error constructor's prototype (e.g. TypeError.prototype), preserving instanceof checks without exposing the original error's non-enumerable fields. Add a regression test verifying non-enumerable properties do not leak and update the prototype-chain test to assert Object.getPrototypeOf result. * fix: apply key-aware redaction and fail closed on non-serializable error props - String properties: use jsonRedactor(key, value) instead of redactSensitiveInfo(value) so keys like apiKey with innocuous values (e.g. 'my-key') are still caught via SENSITIVE_FIELD_SUBSTRINGS. - Object path: catch now replaces non-serializable/circular references with '[REDACTED]' instead of leaving the original object reference. - Add 2 regression tests for key-aware redaction and fail-closed behavior. * Update src/utils/log.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: redact bare auth header keys in JSON/header objects - Add 'auth' to SENSITIVE_FIELD_SUBSTRINGS in src/utils/redaction.ts:109 to match URL/diagnostic redactors treatment of auth - Add regression test for bare auth header keys in src/utils/diagnostics/redaction.test.ts:88 Co-authored-by: openhands <openhands@all-hands.dev> * fix: narrow auth matching, redact nested transcript JSONL, fix channel skip message * fix: address CodeRabbit nits — comment, hint, JSONL fallback redaction * fix: key-aware malformed JSONL fallback and auth/x-auth in free-form text * fix: strengthen redactJsonLines trailing rest redaction and auth test assertions * fix: preserve non-JSON prefix in redactJsonLines fallback and redact it * fix: tighten redactJsonLines prefix test to exact output assertion * fix: redact MCP log sink payloads and errorStr before writing to disk * fix: address P1 findings — URL #-in-password, ;-delimited query params, split channel trust-boundary - Allow in URL userinfo password on malformed-URL fallback path (new URL() fails when password contains fragment delimiter). - Redact -delimited sensitive query params by splitting on both & and ; in redactMalformedQuery, plus redactSemicolonQueryParams post-processor for valid-URL output. - Restore channelNotification.ts to upstream/main to fully split OAuth/org-policy trust-boundary changes from credential redaction PR. * fix: update callers to match upstream/main function signatures channelNotification.ts was restored to upstream/main to split trust-boundary changes from the redaction PR. This commit updates the three caller sites that previously passed extra arguments: - ChannelsNotice.tsx: pass getSubscriptionType() + undefined to getEffectiveChannelAllowlist (needs 2 args upstream) - interactiveHandler.ts, channelNotification.test.ts: drop 3rd pluginSource arg from findChannelEntry (takes 2 args upstream) * fix: address reviewer findings — OAuth mock, notice states, marketplace disambiguation P1: Mock getClaudeAIOAuthTokens and getSubscriptionType in channel notification tests so they pass on CI where no real OAuth exists. P2: Restore blocked-auth/org-policy notice states in ChannelsNotice.tsx so the UI shows the correct blocker when gateChannelServer rejects unauthenticated users or orgs without channelsEnabled. P2: Add pluginSource disambiguation to findChannelEntry so same-name plugin entries from different marketplaces are matched by runtime source rather than first-match order. Add regression test with non-matching marketplace first to cover the bug. * fix: address reviewer findings — relay gate parity and allowlist regression test - Replace filterPermissionRelayClients in interactiveHandler with inline gateChannelServer call so the relay predicate checks ALL gates including disabled-channel, auth, org policy, and approved-plugin allowlist, not just session entry + marketplace. - Clean up unused imports (getAllowedChannels, parsePluginIdentifier, findChannelEntry, filterPermissionRelayClients). - Add regression test: gateChannelServer rejects marketplace-matched plugin not on approved allowlist (full-gate path). * fix: redact mixed semicolon secrets in valid-URL path and route OpenAI shim through centralized redactor P1: Pre-redact semicolon-delimited sensitive query params from the raw query string in redactUrlForDisplay BEFORE URLSearchParams encodes ; as %3B. Previously model=ok;token=SECRET leaked because parsed.toString() reserialized to model=ok%3Btoken%3DSECRET, making it invisible to the post-process pass. P1: Route openaiShim's redactUrlForDiagnostics through the centralized redactUrlForDisplay so the semicolon fix, malformed-URL fallback, and all future redaction improvements apply to OpenAI-compatible diagnostic logs too. Keep redactSecretValueForDisplay as an additional safety net after the centralized pass. Add 3 regression tests for mixed-separator queries. * Update src/utils/redaction.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: add fragment-query credential regression test and correct dev-channel gate comments P2: Add regression test for redactUrlForDisplay with query-like credential in fragment (e.g. #debug?token=SECRET). Fix raw-query pre-processing to only extract query before the first #, preventing fragment content from being treated as query parameters. P3: Update comments in interactiveHelpers.tsx to match the actual gate order — OAuth and org-policy gates still exist in gateChannelServer() after restoring to upstream/main; the --dangerously-load-development- channels flag only bypasses the allowlist gate. * fix: add port+fragment+@ fallback test and restructure dev-channels dialog tests * fix: registerDevChannels seam, bare-host #-in-password heuristic, and coverage restructure * fix: add OAuth and org-policy gate test coverage - Refactor auth module mock to use mutable variables per test - Auth gate test: empty OAuth tokens -> kind:auth - Policy gate test: team subscription without channelsEnabled -> kind:policy * fix: prefer exact server channel entries before plugin disambiguation - Return exact server-kind candidate first when candidates include both server and plugin entries with same name - Added regression test covering mixed server/plugin --channels entries to ensure exact server opt-in is not overridden by plugin candidate - This prevents a plugin marketplace mismatch from incorrectly rejecting a server the user explicitly selected via server:plugin:slack * fix: only trust exact [REDACTED] placeholder in generic header field pattern - Changed GENERIC_HEADER_FIELD_PATTERN to only bypass exact '[REDACTED]' canonical placeholder - Prevents non-canonical placeholders like '[REDACTED_API_KEY]' or '[REDACTED_actual_secret]' from leaking through - Updated tests to expect canonical '[REDACTED]' output for generic pattern * fix: handle bare hosts in malformed URL userinfo fallback - Added regex to recognize bare hostnames (with optional port) in the fragment heuristic - Added tests for //alice:sec#ret@host and //alice:sec#ret@host:443 * fix: add relay dispatch path test for non-allowlisted plugin - Added test using full gateChannelServer predicate in filterPermissionRelayClients - Mirrors the exact relay dispatch path used in interactiveHandler - Ensures marketplace-matched plugin not on allowlist is excluded from permission preview * fix: enhance URL redaction logic to handle valid hosts before fragment * fix: refine URL redaction logic to ensure valid host checks before fragment * fix: enhance redaction logic to handle embedded URLs in free-form text * fix: update redaction logic to remove user info from OpenAI base URL in diagnostic report * fix: ensure findChannelEntry returns undefined when no exact matches are found * fix: improve URL redaction logic to remove user info and ensure proper formatting * fix: enhance redactDiagnosticUrl to preserve query-param values and trailing slashes * fix: refine redaction logic to preserve meaningful path segments and handle trailing slashes correctly * fix: enhance redactDiagnosticUrl to preserve literal path segments and handle trailing slashes correctly * fix: preserve semicolon-delimited query params during redaction * fix: update redaction logic to support semicolon-delimited query parameters * fix: enhance redactUrlForDisplay to handle bare hosts and improve fragment redaction * fix: enhance redactUrlForDisplay to correctly handle username-only userinfo with fragments * fix: address privacy findings — URL redaction in jsonRedactor, base URL redaction, diagnostic object collapsing, structural channel previews, pluginSource telemetry * fix: preserve falsey env-presence values in diagnostic redaction - false, "", and 0 under isEnvPresenceKey keys are now preserved as-is instead of misrepresented as "[set]" - Added regression test for absent/falsey env-presence inputs * fix: address CodeRabbit findings — sync describe, heartbeat emitter, responsesBody filtering, dev entry precedence * chore: remove stray Windows path artifact * fix: update redaction import path in taskReport module * fix: address CodeRabbit P1-P3 findings and rebase regressions - F1: rebase onto upstream/main, fix taskReport.ts import path - F2: Ollama native chat code recovered via rebase (6 functions) - F3: &-truncation in credential regexes fixed via post-processing pass - F4: 'tokens' added to jsonRedactor EXCLUDED_KEYS - F5: redactHomePath case-sensitivity aligned with redactPathForStatus - F6: credential metadata object preserved in issue report (sensitive-key check moved inside type branches) - F7: heartbeat tests updated for pre-drain write behavior - F8: reportTask test expects [REDACTED] (matches centralized output) - rm: stray C:\repo\ Windows path artifact * fix: address reviewer findings — generic regex &-handling and diagnostic secret-key masking - Remove & from excluded char classes in 4 generic patterns so they consume full secret values (URL-query &-splitting belongs in redactUrlForDisplay). - Remove now-obsolete &-tail post-processor pass. - Remove credential from DIAGNOSTIC_SECRET_KEY_PATTERN so issue report credential metadata objects are traversed, not collapsed. - Restore broad isDiagnosticSecretKey check before type dispatch in redactDiagnosticObjectInternal so objects/arrays under secret-marked keys (auth, password, token, etc.) are masked. - Update issue report test baseUrl expectation (no trailing &mode=test after generic redactor consumes past &). * fix: address reviewer findings — URL delimiter safety, jsonRedactor #-drop, embedded URL query redaction - Restore &#; delimiters in generic pattern value classes (F1) so safe query tails (&mode=test) survive. Re-add &-tail post-processor for non-URL abc&def case. - Gate redactUrlForDisplay in jsonRedactor to https?:// strings only (F2) to prevent #-drop on ordinary text like 'fails after #setup'. - Add URL query redaction step to redactSensitiveInfo (F3) that extracts https?:// URLs from free-form text and routes them through redactUrlForDisplay, catching signature/sig params that generic patterns miss. Skip already-redacted URLs to avoid double-redaction. * fix: add Cookie/Set-Cookie semicolon-safe redaction pass, tighten &-tail regex * fix: COOKIE_PATTERN consume comma-joined multi-cookie values * fix: address P2 findings — URL redact skip, pre-drain write promise, permission truthy check * fix: update log.test.ts expectation, add protocol-relative URL support * fix: enhance redaction for provider env-vars in URLs, preserve safe query params * fix: enhance redaction for uppercase provider keys and cookie query params * fix: enhance redaction for bare Bearer and JWT tokens in sensitive info * fix: update report task test expectations for new redaction format * fix: limit token exemption to numeric values, protect semicolon cookie query tails * test: add tests for truncateForPreview to ensure sensitive data redaction --------- Co-authored-by: Gravirei <gravirei@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: openhands <openhands@all-hands.dev> (cherry picked from commit aa936cd) (cherry picked from commit c578fb5a5ee6122c810dc49c13ec55e954fb4957)
…ate tests (Gitlawb#1711) * feat(utils): add centralized redaction utility Single source of truth for stripping API keys, tokens, and other secrets from strings and JSON. Provider env-var coverage is generated from getKnownProviderSecretEnvKeys() so adding a new provider cannot silently create an unredacted path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(Feedback): import redactSensitiveInfo from utils Remove the inline 40-line regex implementation in favor of the centralized redaction utility, eliminating drift between Feedback and the transcript share path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(submitTranscriptShare): import redactSensitiveInfo from utils Update import path to point at the centralized utility instead of the Feedback component. Removes the implicit re-export contract that required Feedback.tsx to keep redactSensitiveInfo exported. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(log,debug): redact secrets in default error and debug output Wire the centralized redaction utility into logError and logForDebugging so secrets cannot leak into in-memory error logs or the debug file even if a caller forgets to pass through redactSensitiveInfo. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(api/logging): redact error message in logAPIError Apply the centralized redaction utility to the error string passed to logEvent so analytics events cannot capture unredacted credentials from upstream API failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: resolve merge conflict from upstream sync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(channelNotification): allow null in getEffectiveChannelAllowlist signature ChannelsNotice.tsx passes getSubscriptionType() which returns SubscriptionType | null, but the signature only accepted string | undefined. Widen to string | null so the call site typechecks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(redaction): exclude specific token fields from redaction process * fix(redaction): lower AIza minimum length to {10,} Real GCP/Gemini keys are 39 chars total (4 prefix + 35 suffix), but the {35} suffix bound missed short tokens like 'AIzaSyDUMMY-secret-token' (21 chars after AIza). Lower to {10,} to match the diagnostics module and catch any AIza-shaped value. Same precision trade-off the diagnostics redaction makes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction,log): address review feedback - Drop quotes from ANTHROPIC/OPENAI key negative lookarounds so JSON-shaped values like "sk-ant-..." redact. - Add private_key pattern to GENERIC_HEADER_FIELD_PATTERN and privatekey to SENSITIVE_FIELD_SUBSTRINGS. - logError now builds a sanitized Error (redacted message + stack) before passing to the sink and queue, not just the in-memory log. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(redaction): consolidate into single module + add channel gate tests Address the three P2 review findings on the central-redaction PR: [1] Consolidate four redaction modules into src/utils/redaction.ts. Previously lived in: - src/utils/redaction.ts (logs/bug reports/transcript shares) - src/utils/urlRedaction.ts (URL display) - src/utils/statusRedaction.ts (/status output) - src/utils/diagnostics/redaction.ts (doctor reports) The four surfaces share the same regex set / credential lists but had drifted into separate per-domain files. Merged into one module; deleted the three shim files. Updated six direct consumers (openaiShim.ts, ProviderManager.tsx, status.tsx, requestSizeBreakdown.ts, diagnostics/issueReport.ts, scripts/system-check.ts) and three test files to import from redaction.js. [2] Add gateChannelServer() test coverage. src/services/mcp/channelNotification.test.ts: 13 cases for the six gate paths (capability, runtime, session, marketplace, plugin allowlist, server-entry dev) plus end-to-end register. Mocks channelAllowlist.js (GrowthBook-backed) so tests stay independent of feature-flag state. [3] Apply jsonRedactor in transcript share. src/components/FeedbackSurvey/submitTranscriptShare.ts now does redactSensitiveInfo(jsonStringify(data, jsonRedactor)) — the key-aware redaction applies during serialization, and the text pass stays as defense in depth for free-form fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(channelNotification): cover findChannelEntry multi-candidate branch Regression test for the disambiguation path in `findChannelEntry` (channelNotification.ts:201-230): when two same-name plugin entries exist in the allowed-channels list with different marketplaces, `pluginSource` must select the matching entry before the marketplace and allowlist gates evaluate. Without this branch being exercised, the gate could lock onto whichever entry sorts first and either skip the user's real installation or wrongly authorize a typo-squatted one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction): align URL fallback regex + add path-prefix boundary check Two related redaction correctness fixes: [1] URL fallback regex covers the same parameter set as the primary path. The malformed-URL branch in `redactUrlForDisplay` previously had a hand-rolled alternation of credential parameter names that could drift behind `SENSITIVE_URL_QUERY_PARAM_TOKENS`. New `MALFORMED_URL_PARAM_PATTERN` derives from that same list, so the two paths can never diverge. Tests cover the full credential set (`api_key`, `access_token`, `refresh_token`, `signature`, `sig`, `secret`, `password`, `apikey`) plus a non-sensitive `model` that must survive. [2] `redactPathForStatus` now requires a path-separator boundary after the home prefix. The previous `startsWith` check matched `/home/alice2/project` against `/home/alice` and emitted `~2/project`. The fix requires the character at `normalizedCandidate.length` to be `/` or `\` so `alice` no longer matches `alice2` or `alice.bak`. Test pins the false-positive paths and the true-positive (`/home/alice/project` → `~/project`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(channel,redaction): restore dev-channel warning + align URL fallback Two related security fixes: [1] Restore DevChannelsDialog when --dangerously-load-development-channels is passed and the channels feature is enabled. The previous logic skipped the dialog when OAuth was absent, which was safe only while gateChannelServer() blocked no-OAuth sessions. With the OAuth/org- policy gates removed in this PR, an API-key session could pass the flag, skip the warning, and still register the dev channel. The only remaining skip is the genuinely-disabled feature case (`!isChannelsEnabled()`), where the dialog is moot. [2] Malformed-URL fallback now uses the same substring predicate as the primary `URL` parser path. The previous regex matched only exact parameter names (`api_key=`, `access_token=`, …), so `my_api_key=SECRET` and `x_access_token=TOKEN` slipped through unchanged even though `shouldRedactUrlQueryParam` flags them as sensitive. New `redactMalformedQuery` walks the query pairs and runs the predicate on each key. Three new tests cover prefixed keys, non-sensitive keys, and fragment preservation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction): widen key boundary class + tighten dev-channel comment Two small follow-ups from the latest CodeRabbit review: [1] Boundary class on key-prefix patterns widened from `[A-Za-z0-9]` to `[A-Za-z0-9_-]` so a raw key embedded in a JSON string value (`"sk-ant-..."`, `"AIza..."`, `"ghp_..."`, etc.) is still caught. Quotes act as delimiters, not blockers — the previous boundary class was correct for unquoted text but let quoted keys slip through. [2] Tighten the dev-channel dialog comment in interactiveHelpers.tsx so future readers don't misread the security boundary. Skip condition is `isChannelsEnabled()` (the channels feature flag gate), not KAIROS / KAIROS_CHANNELS as the previous wording implied. Comment now matches the code. Skipped with reason: - getEffectiveChannelAllowlist divergence from gateChannelServer allowlist — by design; the effective-list override is a UI hint consumed only by ChannelsNotice for the org-override indicator. Trust boundary is enforced by gateChannelServer() reading the hardcoded ledger. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(redaction,channel): address P1/P2 review findings P1 - malformed URL fallback secrets: - Decode percent-encoded query param keys via decodeURIComponent() before applying shouldRedactUrlQueryParam (e.g. %74oken -> token) - Stop userinfo regex at ? and # delimiters to avoid consuming query params when matching @ signs in email addresses or fragment delimiters P2 - channel notice/gate allowlist sync: - Remove org override path from getEffectiveChannelAllowlist() so ChannelsNotice startup guidance uses the same ledger source as gateChannelServer's runtime enforcement - Simplify ChannelsNotice to drop unused sub/policy params and the source === 'org' conditional * fix(channel): apply marketplace matching to permission relays, remove stale OAuth/org-policy blockers, add dev-channel dialog coverage P1: Thread runtime pluginSource through filterPermissionRelayClients so findChannelEntry disambiguates same-name plugin entries from different marketplaces before sending permission request previews. P2: Remove stale noAuth and policyBlocked branches from ChannelsNotice that would render '--channels ignored' before reaching the listening message, confusing non-OAuth users. P2: Add test coverage that mocks isChannelsEnabled() both true and false, verifies DevChannelsDialog appears with onAccept marking entries dev:true in the enabled case, and verifies the disabled branch registers entries directly without dialog. * test(dev-channel): clarify count assertion comment + add afterEach with mock.restore() * fix(channel): mirror marketplace gate in permission relay + restore mock Two follow-ups from the latest review: [1] Permission relay predicate no longer relies on findChannelEntry alone. After resolving the entry, the predicate now requires a runtime pluginSource whose marketplace matches the session entry's marketplace for plugin-kind entries — mirroring the gateChannelServer check at channelNotification.ts:303-312. A `plugin:slack@evilcorp` client whose session allows `plugin:slack@anthropic` is now rejected instead of piggy-backing on the approved entry to receive permission-request previews. Server-kind entries still match on bare name. [2] bugfixes.test.ts now re-registers the real channelAllowlist module in afterEach via a cache-busted reference, so the neighbor channelNotification.test.ts continues to import getChannelAllowlist after this suite runs. mock.restore() does not clear module-level mock.module() overrides in bun (the registry is process-global). Pattern matches compact.test.ts:27-36. Also expanded the dev-map count comment in bugfixes.test.ts to document the security invariant (a dev entry must never be confused with a production entry in the allowlist check) per CodeRabbit's request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor(redaction): consolidate into single module + add channel gate tests Address the three P2 review findings on the central-redaction PR: [1] Consolidate four redaction modules into src/utils/redaction.ts. Previously lived in: - src/utils/redaction.ts (logs/bug reports/transcript shares) - src/utils/urlRedaction.ts (URL display) - src/utils/statusRedaction.ts (/status output) - src/utils/diagnostics/redaction.ts (doctor reports) The four surfaces share the same regex set / credential lists but had drifted into separate per-domain files. Merged into one module; deleted the three shim files. Updated six direct consumers (openaiShim.ts, ProviderManager.tsx, status.tsx, requestSizeBreakdown.ts, diagnostics/issueReport.ts, scripts/system-check.ts) and three test files to import from redaction.js. [2] Add gateChannelServer() test coverage. src/services/mcp/channelNotification.test.ts: 13 cases for the six gate paths (capability, runtime, session, marketplace, plugin allowlist, server-entry dev) plus end-to-end register. Mocks channelAllowlist.js (GrowthBook-backed) so tests stay independent of feature-flag state. [3] Apply jsonRedactor in transcript share. src/components/FeedbackSurvey/submitTranscriptShare.ts now does redactSensitiveInfo(jsonStringify(data, jsonRedactor)) — the key-aware redaction applies during serialization, and the text pass stays as defense in depth for free-form fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(test): align malformed URL fragment expectation with preservation behavior * fix: address review findings P1 and P2 [P1] Enforce dev flag for server-kind entries in permission relay predicate, matching gateChannelServer() behavior. Add coverage for both dev and non-dev server relay paths. [P2] Drop fragments in malformed URL fallback (redactMalformedQuery) to match the valid-URL path, preventing credential leaks via fragment-carried tokens. Update existing tests and add regression for fragment-only malformed URLs. * test(relay): add plugin-kind marketplace regression tests * fix: address review findings P1 and P2 [P1] Add PEM private key redaction pattern to redactSensitiveInfo so multi-line PEM values are fully consumed instead of leaking after the first whitespace. Add [ to generic header pattern's value exclusion set to prevent re-consuming [REDACTED] tokens. [P2] Use truthy check (Boolean()) for claude/channel capability in filterPermissionRelayClients to match gateChannelServer's behavior, rejecting explicit false capabilities. * fix(debug): redact before JSON-stringify multiline messages Reorder logForDebugging so redactSensitiveInfo runs before jsonStringify, ensuring PEM/private-key patterns match the raw (unescaped) message text rather than the JSON-encoded form where colons and quotes are escaped. * test(debug): add end-to-end regression for multiline PEM redaction in logForDebugging Uses mock.module on process.js to capture stderr output and exercises the full logForDebugging path with multiline PEM private_key input, verifying the redact-before-JSON-stringify ordering produces redacted output. * fix(test): preserve original process.env.DEBUG and process.argv in logForDebugging test hooks * fix: address PR review findings P1-P3/P5-P7 - P1: clear isDebugMode/isDebugToStdErr memoize caches in test beforeEach + cache-busting query param for fresh debug.ts imports - P2: restore mock.module afterAll instead of leaking mock + mutate err in-place in logError to preserve name/cause - P3: post-processing regex absorbs trailing bracket content after [REDACTED] - P5: (was P3) expand jsonRedactor EXCLUDED_KEYS for maxTokens etc. - P7: capture HOME/USERPROFILE per-test instead of at module scope * fix: address CodeRabbit review findings - interactiveHelpers.tsx: update dev-channel comment — OAuth/org-policy gates removed from gateChannelServer(), org policy is not enforced - channelNotification.test.ts: add afterAll mock.restore() to clean up process-global channelAllowlist.js mock - channelNotification.ts: fix comments — isChannelsEnabled() still reads tengu_harbor, not always true - log.ts: sanitize err.message and err.stack separately so message doesn't get replaced with full stack trace - redaction.ts: add 'i' flag to redactHomePath regex for Windows case-insensitive path matching * fix: address second review round - interactiveHandler.ts: [P2] redact input_preview via redactSensitiveInfo before sending to channel servers - log.ts: [P3] copy error via Object.assign(Object.create(err), err) before sanitizing instead of mutating in-place * fix: address CodeRabbit second round - channelPermissions.ts: redact before truncate in truncateForPreview so partial credentials don't leak at the 200-char boundary - interactiveHandler.ts: remove outer redactSensitiveInfo — now handled inside truncateForPreview - log.ts: derive errorInfo.error from already-sanitized sanitizedErr; fix Object.assign comment to accurately describe what is copies * fix: improve permission relay client filtering and enhance redaction functions * fix: address third review round (P1, P2, P3) - P1: update test expectations for [REDACTED_*] output format - P2: add total_tokens, prompt_tokens, completion_tokens to jsonRedactor EXCLUDED_KEYS - P3: remove ) and } from GENERIC_HEADER_FIELD_PATTERN value capture to prevent content leak after embedded parens - Fix buildKnownEnvVarPattern capture group to preserve env-var separator ([REDACTED]) - Add & to GENERIC_CREDENTIAL_ENV_PATTERN value exclusion to prevent URL query over-consumption * fix: address latest reviewer P2/P3 findings (errorLogSink redaction, X_API_KEY/AUTHORIZATION patterns, regression tests) * fix: address reviewer P1/P2 — bracketed values and multi-word header values - P1: Remove and from value captures in X_API_KEY_PATTERN, AUTHORIZATION_PATTERN, GENERIC_HEADER_FIELD_PATTERN, GENERIC_CREDENTIAL_ENV_PATTERN so bracketed secrets like are fully redacted instead of passing through unchanged. - P2: Widen header-style value captures to include spaces by removing from exclusions, using as delimiter (stops at newlines and URL query separators). Fixes multi-word leaks: , , , . - GENERIC_CREDENTIAL_ENV_PATTERN: add to negative lookbehind to prevent matching inside when the latter is already redacted. - GENERIC_HEADER_FIELD_PATTERN replacer: skip values starting with to preserve specific labels from earlier passes. - Add 7 regression tests covering both finding categories. * fix: address reviewer findings P1-P4 P1: Custom enumerable error properties now redacted in log.ts logError iterates all own enumerable properties on the original error and applies redactSensitiveInfo to string values and jsonRedactor to object values, preventing credential-bearing custom fields from leaking through the sanitized error. Regression tests added in log.test.ts. P2: Soften single-source-of-truth claim; migrate easy call sites Header comment in redaction.ts updated to acknowledge that specialized scanners (secretScanner.ts, xaa.ts) are intentional exceptions. src/services/mcp/client.ts and src/services/mcp/auth.ts now use jsonRedactor for header redaction instead of ad-hoc key checks. P3: Fix mock.restore cleanup in channelNotification.test.ts Cache-bust the real channelAllowlist module at describe-entry and re-register it in afterAll, following the pattern from bugfixes.test.ts. mock.restore alone does not clear mock.module overrides in Bun. P4: Remove unused ChannelGateResult kinds Removed 'auth' and 'policy' from the skip kind union and removed corresponding dead branches in useManageMCPConnections.ts. * fix: extract sanitizeError() to fix CI test fragility The logError tests were failing in CI due to parallel test execution racing on the module-level errorLogSink singleton. Extract the inline sanitization logic into an exported sanitizeError() helper and test that directly — it's pure, has no env-var or sink dependencies, and doesn't interact with shared mutable state. * fix: use Object.getPrototypeOf(err) instead of err as prototype in sanitizeError Object.create(err) sets the original error instance as the prototype of the sanitized copy, leaking non-enumerable own properties through the prototype chain. Use Object.getPrototypeOf(err) instead so the prototype is the error constructor's prototype (e.g. TypeError.prototype), preserving instanceof checks without exposing the original error's non-enumerable fields. Add a regression test verifying non-enumerable properties do not leak and update the prototype-chain test to assert Object.getPrototypeOf result. * fix: apply key-aware redaction and fail closed on non-serializable error props - String properties: use jsonRedactor(key, value) instead of redactSensitiveInfo(value) so keys like apiKey with innocuous values (e.g. 'my-key') are still caught via SENSITIVE_FIELD_SUBSTRINGS. - Object path: catch now replaces non-serializable/circular references with '[REDACTED]' instead of leaving the original object reference. - Add 2 regression tests for key-aware redaction and fail-closed behavior. * Update src/utils/log.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: redact bare auth header keys in JSON/header objects - Add 'auth' to SENSITIVE_FIELD_SUBSTRINGS in src/utils/redaction.ts:109 to match URL/diagnostic redactors treatment of auth - Add regression test for bare auth header keys in src/utils/diagnostics/redaction.test.ts:88 Co-authored-by: openhands <openhands@all-hands.dev> * fix: narrow auth matching, redact nested transcript JSONL, fix channel skip message * fix: address CodeRabbit nits — comment, hint, JSONL fallback redaction * fix: key-aware malformed JSONL fallback and auth/x-auth in free-form text * fix: strengthen redactJsonLines trailing rest redaction and auth test assertions * fix: preserve non-JSON prefix in redactJsonLines fallback and redact it * fix: tighten redactJsonLines prefix test to exact output assertion * fix: redact MCP log sink payloads and errorStr before writing to disk * fix: address P1 findings — URL #-in-password, ;-delimited query params, split channel trust-boundary - Allow in URL userinfo password on malformed-URL fallback path (new URL() fails when password contains fragment delimiter). - Redact -delimited sensitive query params by splitting on both & and ; in redactMalformedQuery, plus redactSemicolonQueryParams post-processor for valid-URL output. - Restore channelNotification.ts to upstream/main to fully split OAuth/org-policy trust-boundary changes from credential redaction PR. * fix: update callers to match upstream/main function signatures channelNotification.ts was restored to upstream/main to split trust-boundary changes from the redaction PR. This commit updates the three caller sites that previously passed extra arguments: - ChannelsNotice.tsx: pass getSubscriptionType() + undefined to getEffectiveChannelAllowlist (needs 2 args upstream) - interactiveHandler.ts, channelNotification.test.ts: drop 3rd pluginSource arg from findChannelEntry (takes 2 args upstream) * fix: address reviewer findings — OAuth mock, notice states, marketplace disambiguation P1: Mock getClaudeAIOAuthTokens and getSubscriptionType in channel notification tests so they pass on CI where no real OAuth exists. P2: Restore blocked-auth/org-policy notice states in ChannelsNotice.tsx so the UI shows the correct blocker when gateChannelServer rejects unauthenticated users or orgs without channelsEnabled. P2: Add pluginSource disambiguation to findChannelEntry so same-name plugin entries from different marketplaces are matched by runtime source rather than first-match order. Add regression test with non-matching marketplace first to cover the bug. * fix: address reviewer findings — relay gate parity and allowlist regression test - Replace filterPermissionRelayClients in interactiveHandler with inline gateChannelServer call so the relay predicate checks ALL gates including disabled-channel, auth, org policy, and approved-plugin allowlist, not just session entry + marketplace. - Clean up unused imports (getAllowedChannels, parsePluginIdentifier, findChannelEntry, filterPermissionRelayClients). - Add regression test: gateChannelServer rejects marketplace-matched plugin not on approved allowlist (full-gate path). * fix: redact mixed semicolon secrets in valid-URL path and route OpenAI shim through centralized redactor P1: Pre-redact semicolon-delimited sensitive query params from the raw query string in redactUrlForDisplay BEFORE URLSearchParams encodes ; as %3B. Previously model=ok;token=SECRET leaked because parsed.toString() reserialized to model=ok%3Btoken%3DSECRET, making it invisible to the post-process pass. P1: Route openaiShim's redactUrlForDiagnostics through the centralized redactUrlForDisplay so the semicolon fix, malformed-URL fallback, and all future redaction improvements apply to OpenAI-compatible diagnostic logs too. Keep redactSecretValueForDisplay as an additional safety net after the centralized pass. Add 3 regression tests for mixed-separator queries. * Update src/utils/redaction.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: add fragment-query credential regression test and correct dev-channel gate comments P2: Add regression test for redactUrlForDisplay with query-like credential in fragment (e.g. #debug?token=SECRET). Fix raw-query pre-processing to only extract query before the first #, preventing fragment content from being treated as query parameters. P3: Update comments in interactiveHelpers.tsx to match the actual gate order — OAuth and org-policy gates still exist in gateChannelServer() after restoring to upstream/main; the --dangerously-load-development- channels flag only bypasses the allowlist gate. * fix: add port+fragment+@ fallback test and restructure dev-channels dialog tests * fix: registerDevChannels seam, bare-host #-in-password heuristic, and coverage restructure * fix: add OAuth and org-policy gate test coverage - Refactor auth module mock to use mutable variables per test - Auth gate test: empty OAuth tokens -> kind:auth - Policy gate test: team subscription without channelsEnabled -> kind:policy * fix: prefer exact server channel entries before plugin disambiguation - Return exact server-kind candidate first when candidates include both server and plugin entries with same name - Added regression test covering mixed server/plugin --channels entries to ensure exact server opt-in is not overridden by plugin candidate - This prevents a plugin marketplace mismatch from incorrectly rejecting a server the user explicitly selected via server:plugin:slack * fix: only trust exact [REDACTED] placeholder in generic header field pattern - Changed GENERIC_HEADER_FIELD_PATTERN to only bypass exact '[REDACTED]' canonical placeholder - Prevents non-canonical placeholders like '[REDACTED_API_KEY]' or '[REDACTED_actual_secret]' from leaking through - Updated tests to expect canonical '[REDACTED]' output for generic pattern * fix: handle bare hosts in malformed URL userinfo fallback - Added regex to recognize bare hostnames (with optional port) in the fragment heuristic - Added tests for //alice:sec#ret@host and //alice:sec#ret@host:443 * fix: add relay dispatch path test for non-allowlisted plugin - Added test using full gateChannelServer predicate in filterPermissionRelayClients - Mirrors the exact relay dispatch path used in interactiveHandler - Ensures marketplace-matched plugin not on allowlist is excluded from permission preview * fix: enhance URL redaction logic to handle valid hosts before fragment * fix: refine URL redaction logic to ensure valid host checks before fragment * fix: enhance redaction logic to handle embedded URLs in free-form text * fix: update redaction logic to remove user info from OpenAI base URL in diagnostic report * fix: ensure findChannelEntry returns undefined when no exact matches are found * fix: improve URL redaction logic to remove user info and ensure proper formatting * fix: enhance redactDiagnosticUrl to preserve query-param values and trailing slashes * fix: refine redaction logic to preserve meaningful path segments and handle trailing slashes correctly * fix: enhance redactDiagnosticUrl to preserve literal path segments and handle trailing slashes correctly * fix: preserve semicolon-delimited query params during redaction * fix: update redaction logic to support semicolon-delimited query parameters * fix: enhance redactUrlForDisplay to handle bare hosts and improve fragment redaction * fix: enhance redactUrlForDisplay to correctly handle username-only userinfo with fragments * fix: address privacy findings — URL redaction in jsonRedactor, base URL redaction, diagnostic object collapsing, structural channel previews, pluginSource telemetry * fix: preserve falsey env-presence values in diagnostic redaction - false, "", and 0 under isEnvPresenceKey keys are now preserved as-is instead of misrepresented as "[set]" - Added regression test for absent/falsey env-presence inputs * fix: address CodeRabbit findings — sync describe, heartbeat emitter, responsesBody filtering, dev entry precedence * chore: remove stray Windows path artifact * fix: update redaction import path in taskReport module * fix: address CodeRabbit P1-P3 findings and rebase regressions - F1: rebase onto upstream/main, fix taskReport.ts import path - F2: Ollama native chat code recovered via rebase (6 functions) - F3: &-truncation in credential regexes fixed via post-processing pass - F4: 'tokens' added to jsonRedactor EXCLUDED_KEYS - F5: redactHomePath case-sensitivity aligned with redactPathForStatus - F6: credential metadata object preserved in issue report (sensitive-key check moved inside type branches) - F7: heartbeat tests updated for pre-drain write behavior - F8: reportTask test expects [REDACTED] (matches centralized output) - rm: stray C:\repo\ Windows path artifact * fix: address reviewer findings — generic regex &-handling and diagnostic secret-key masking - Remove & from excluded char classes in 4 generic patterns so they consume full secret values (URL-query &-splitting belongs in redactUrlForDisplay). - Remove now-obsolete &-tail post-processor pass. - Remove credential from DIAGNOSTIC_SECRET_KEY_PATTERN so issue report credential metadata objects are traversed, not collapsed. - Restore broad isDiagnosticSecretKey check before type dispatch in redactDiagnosticObjectInternal so objects/arrays under secret-marked keys (auth, password, token, etc.) are masked. - Update issue report test baseUrl expectation (no trailing &mode=test after generic redactor consumes past &). * fix: address reviewer findings — URL delimiter safety, jsonRedactor #-drop, embedded URL query redaction - Restore &#; delimiters in generic pattern value classes (F1) so safe query tails (&mode=test) survive. Re-add &-tail post-processor for non-URL abc&def case. - Gate redactUrlForDisplay in jsonRedactor to https?:// strings only (F2) to prevent #-drop on ordinary text like 'fails after #setup'. - Add URL query redaction step to redactSensitiveInfo (F3) that extracts https?:// URLs from free-form text and routes them through redactUrlForDisplay, catching signature/sig params that generic patterns miss. Skip already-redacted URLs to avoid double-redaction. * fix: add Cookie/Set-Cookie semicolon-safe redaction pass, tighten &-tail regex * fix: COOKIE_PATTERN consume comma-joined multi-cookie values * fix: address P2 findings — URL redact skip, pre-drain write promise, permission truthy check * fix: update log.test.ts expectation, add protocol-relative URL support * fix: enhance redaction for provider env-vars in URLs, preserve safe query params * fix: enhance redaction for uppercase provider keys and cookie query params * fix: enhance redaction for bare Bearer and JWT tokens in sensitive info * fix: update report task test expectations for new redaction format * fix: limit token exemption to numeric values, protect semicolon cookie query tails * test: add tests for truncateForPreview to ensure sensitive data redaction --------- Co-authored-by: Gravirei <gravirei@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: openhands <openhands@all-hands.dev>
4 participants
Summary
redactSensitiveInfofromFeedback.tsxinto a centralizedsrc/utils/redaction.tsso every diagnostic path (transcript share, in-memory error log, debug log, API error analytics) uses the same regex set.redactSensitiveInfocovers sk-ant-, sk-/sk-proj-/sk-or-v1-, AKIA, AIza, Vertex service accounts, ghp_/gho_/ghs_/ghu_/ghr_/github_pat_, AWS_/GOOGLE_ env vars, x-api-key, Authorization Bearer, private_key, and generic _API_KEY/_SECRET/_TOKEN/_PASSWORD.getKnownProviderSecretEnvKeys(), so a new provider added via the descriptor registry is automatically redacted.jsonRedactor(key, value)is aJSON.stringifyreplacer that flags credential-shaped keys and runsredactSensitiveInfoover string values.logError,logForDebugging, andlogAPIErrorso secrets cannot leak via paths that don't pass throughredactSensitiveInfomanually.logErrornow builds a sanitized Error object before passing to the sink and queue — the in-memory log and downstream sinks all receive the redacted error.Test plan
bun run typecheckexits 0Feedback.test.ts,FeedbackSurvey/,diagnostics/redaction.test.ts,providerSecrets.test.ts)"sk-ant-...","sk-proj-...", single-quoted variants)private_keyandprivateKeyare caught by bothredactSensitiveInfoandjsonRedactorredactSensitiveInfoover a realistic transcript payload (6 secret shapes) — all redacted, no leakslogForDebuggingwith secret-shaped strings, grep'd the resulting debug file — no leakslogErrorwith a credential-shaped message routes a sanitized Error to the sink (verified message + stack redacted, raw token absent)/bugand/feedbackend-to-end to confirm transcripts still reach the server with secrets redacted--debugin a real session and grep the debug file forsk-ant-to confirm no leaksSummary by CodeRabbit