fix(security): URL/unicode encoding coverage, unify action classification taxonomy - #157
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements comprehensive security hardening by centralizing prompt-injection pattern detection, integrating tool-output sanitization in the orchestrator, wiring an ApprovalQueue system for approval-based policy enforcement, and adding extensive adversarial test coverage to validate the new security measures. Changes
Sequence DiagramsequenceDiagram
participant Client as Request/Tool Output
participant Orchestrator
participant PolicyEngine as Policy Engine
participant ApprovalQueue
participant Callback as Flag Callback
Client->>Orchestrator: tool_result event
Orchestrator->>Orchestrator: sanitizeToolOutput()
Orchestrator->>PolicyEngine: createCanUseTool(action)
activate PolicyEngine
alt always_flag matches action
PolicyEngine->>PolicyEngine: compute detail
alt flagCallback registered
PolicyEngine->>Callback: flagCallback(action, detail)
Callback->>PolicyEngine: boolean response
else flagCallback absent
alt ApprovalQueue enabled
PolicyEngine->>ApprovalQueue: request(jobId, score)
ApprovalQueue->>PolicyEngine: boolean response
else neither registered
PolicyEngine->>PolicyEngine: log warning & fail-closed deny
end
end
else action not flagged
PolicyEngine->>PolicyEngine: allow by default
end
PolicyEngine->>Orchestrator: behavior (allow/deny)
deactivate PolicyEngine
Orchestrator->>Client: decision + response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's security posture by addressing two key areas: robust injection detection and consistent action classification. It introduces a more sophisticated mechanism for identifying and neutralizing encoded injection attempts across various input types, and centralizes the definition of security patterns to prevent drift. Additionally, it improves the reliability of policy enforcement by ensuring that actions requiring approval are always routed through an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
codeant-ai
Bot
added
the
size:XL
Sequence DiagramThis PR closes an approval bypass by wiring ApprovalQueue into PolicyEngine before runtime checks, and adds mandatory sanitization of tool results before they re-enter the LLM loop. It also centralizes pattern usage so sanitization uses a shared security pattern source. sequenceDiagram
participant CLI
participant Orchestrator
participant PolicyEngine
participant ApprovalQueue
participant Tool
participant PromptDefense
participant LLM
CLI->>Orchestrator: Register ApprovalQueue then boot
Orchestrator->>PolicyEngine: Wire ApprovalQueue for always flag enforcement
Orchestrator->>PolicyEngine: Check risky tool action
PolicyEngine->>ApprovalQueue: Request approval for flagged action
ApprovalQueue-->>PolicyEngine: Approve or deny
PolicyEngine-->>Orchestrator: Return allow or deny decision
Tool-->>Orchestrator: Return tool result content
Orchestrator->>PromptDefense: Sanitize result with shared patterns
Orchestrator-->>LLM: Send sanitized tool result only
Generated by CodeAnt AI |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two important security enhancements. First, it centralizes prompt injection detection patterns and applies sanitization to tool outputs, which is a significant defense-in-depth improvement against attacks from external data sources. Second, it closes a security gap by wiring in the ApprovalQueue as a fallback, ensuring that actions flagged for approval do not silently pass if a callback is not registered. The changes are well-structured, and the addition of new adversarial tests and a security audit script significantly strengthens the project's security posture. My review found one opportunity for code simplification and improved type safety.
| ); | ||
| // Replace the event content with the sanitized result so it reaches the LLM safely. | ||
| // Mutating .content is safe: AgentEvent is a plain object (not frozen), content is `unknown`. | ||
| (event.content as Record<string, unknown>)['result'] = sanitizedResult; |
There was a problem hiding this comment.
The type casting to Record<string, unknown> can be made more type-safe and readable. Since toolResultContent is already cast as ToolResultEventContent and its result property is of type unknown, you can directly assign the sanitizedResult to it. This avoids a less-safe cast and makes the code clearer.
| (event.content as Record<string, unknown>)['result'] = sanitizedResult; | |
| toolResultContent.result = sanitizedResult; |
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const preScreenSuspicious = ALL_CHANNEL_PATTERNS.some(pattern => | ||
| pattern.test(message.content) |
There was a problem hiding this comment.
Suggestion: The pre-screen only tests raw message text against plain regexes, so URL-encoded, Unicode-escaped, or base64-encoded injection strings bypass this block and still reach the quarantine LLM. Normalize/decode common encodings before pattern matching so encoded payloads are flagged consistently. [security]
Severity Level: Major ⚠️
- ⚠️ Channel pre-screen misses encoded injection strings.
- ⚠️ Suspicious messages still consume quarantine LLM processing.
- ❌ Detection consistency differs across security components.| const preScreenSuspicious = ALL_CHANNEL_PATTERNS.some(pattern => | |
| pattern.test(message.content) | |
| const normalizedCandidates = [message.content]; | |
| try { | |
| normalizedCandidates.push(decodeURIComponent(message.content)); | |
| } catch { | |
| // ignore malformed URL encoding | |
| } | |
| normalizedCandidates.push( | |
| message.content.replace(/\\u([0-9a-fA-F]{4})/g, (_, hex: string) => | |
| String.fromCharCode(parseInt(hex, 16)), | |
| ), | |
| ); | |
| try { | |
| normalizedCandidates.push(Buffer.from(message.content, "base64").toString("utf8")); | |
| } catch { | |
| // ignore malformed base64 | |
| } | |
| const preScreenSuspicious = normalizedCandidates.some(candidate => | |
| ALL_CHANNEL_PATTERNS.some(pattern => pattern.test(candidate)) |
Steps of Reproduction ✅
1. Start channel mode so `QuarantineProcessor` is wired in `src/cli/daemon.ts:243-247`
(`new QuarantineProcessor(...)`, then passed into `new ChannelManager(...)`).
2. Send a channel message through any registered adapter; callback path is
`src/channels/channel-manager.ts:64-66` (`adapter.onMessage` → `handleMessage`).
3. In `handleMessage`, execution reaches quarantine at
`src/channels/channel-manager.ts:159` (`this._quarantine.process(msg, capability)`).
4. Pre-screen in `src/channels/quarantine-processor.ts:60-62` checks only
`pattern.test(message.content)` (raw text). `ALL_CHANNEL_PATTERNS`
(`src/security/patterns.ts:84-87`) excludes encoded patterns, while encoded signatures
exist separately in `ENCODED_INJECTION_PATTERNS` (`src/security/patterns.ts:28-33`) and
are not used here.
5. Use encoded payload like
`%69%67%6e%6f%72%65%20%70%72%65%76%69%6f%75%73%20%69%6e%73%74%72%75%63%74%69%6f%6e%73`;
pre-screen returns false, so message continues to `_runQuarantineLLM(...)` at
`src/channels/quarantine-processor.ts:75`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/channels/quarantine-processor.ts
**Line:** 60:61
**Comment:**
*Security: The pre-screen only tests raw message text against plain regexes, so URL-encoded, Unicode-escaped, or base64-encoded injection strings bypass this block and still reach the quarantine LLM. Normalize/decode common encodings before pattern matching so encoded payloads are flagged consistently.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const approved = await this._approvalQueue.request({ | ||
| action, | ||
| score: 65, | ||
| jobId: String(input['__jobId'] ?? 'unknown'), |
There was a problem hiding this comment.
Suggestion: jobId is taken from input['__jobId'], but callers do not populate this field, so approval requests are recorded as "unknown" and lose traceability/correlation with the running session. Use the engine session id as a fallback to preserve correct approval/audit linkage. [logic error]
Severity Level: Major ⚠️
- ⚠️ Approval prompts show unknown job for flagged actions.
- ⚠️ Operator cannot quickly map approval to running task.
- ⚠️ Incident/audit correlation quality degrades for approvals.| jobId: String(input['__jobId'] ?? 'unknown'), | |
| jobId: String(input['__jobId'] ?? this._sessionId ?? 'unknown'), |
Steps of Reproduction ✅
1. Start daemon path where ApprovalQueue is wired (`src/cli/daemon.ts:190-194`) and submit
a task with a real job id (`src/cli/daemon.ts:54-56` or
`src/orchestrator/orchestrator.ts:37-52`).
2. Trigger an `always_flag` action so PolicyEngine routes to ApprovalQueue
(`src/security/policy-engine.ts:616-641`), e.g. `Bash git push`.
3. The canUseTool wrapper passes SDK input unchanged
(`src/orchestrator/orchestrator.ts:1795-1810`); it never injects `__jobId` into `input`.
4. At `src/security/policy-engine.ts:638`, `jobId` becomes `"unknown"`; ApprovalQueue
message template (`src/core/approval-queue.ts:167-172`) then displays `Job: unknown`,
losing task correlation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/security/policy-engine.ts
**Line:** 638:638
**Comment:**
*Logic Error: `jobId` is taken from `input['__jobId']`, but callers do not populate this field, so approval requests are recorded as `"unknown"` and lose traceability/correlation with the running session. Use the engine session id as a fallback to preserve correct approval/audit linkage.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // No enforcement path available — log and allow (config present, no gate wired yet). | ||
| log.warn({ action, tool: toolName }, 'always_flag matched but no approval path registered — allowing'); |
There was a problem hiding this comment.
Suggestion: The new always_flag fallback is fail-open: when no callback or enabled queue is available, the code logs and allows the action even though always_flag means approval is required. This creates a policy bypass for high-risk actions. Deny the action in this branch so enforcement is fail-closed. [security]
Severity Level: Critical 🚨
- ❌ always_flag approval gate bypasses in CLI ask mode.
- ❌ Flagged destructive Bash actions can execute unapproved.
- ⚠️ Security policy intent weakens despite always_flag configuration.| // No enforcement path available — log and allow (config present, no gate wired yet). | |
| log.warn({ action, tool: toolName }, 'always_flag matched but no approval path registered — allowing'); | |
| log.warn({ action, tool: toolName }, 'always_flag matched but no approval path registered — denying'); | |
| return { | |
| behavior: 'deny' as const, | |
| message: `Action '${action}' requires approval but no approval path is registered`, | |
| }; |
Steps of Reproduction ✅
1. Run the normal CLI `ask` flow in `src/cli/index.ts:29-31`, which creates `Orchestrator`
and calls `boot()` without ever calling `orchestrator.setApprovalQueue(...)`.
2. In `src/orchestrator/orchestrator.ts:243-254`, `PolicyEngine` is created, but
`_approvalQueue` is only propagated when present; in this path it remains unset.
3. Submit a task that triggers a flagged action (policy field exists at
`src/types.ts:661-665`, and `_shouldFlag` checks it at
`src/security/policy-engine.ts:733-735`), e.g. a `Bash` tool call classified as
`git_push`.
4. The tool call flows through `taskContext.canUseTool`
(`src/orchestrator/orchestrator.ts:780`, `1793-1810`) into
`PolicyEngine.createCanUseTool()`; when `always_flag` matches and neither callback nor
enabled queue exists, branch `src/security/policy-engine.ts:647-650` logs warning and
returns allow, so flagged action executes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/security/policy-engine.ts
**Line:** 648:649
**Comment:**
*Security: The new `always_flag` fallback is fail-open: when no callback or enabled queue is available, the code logs and allows the action even though `always_flag` means approval is required. This creates a policy bypass for high-risk actions. Deny the action in this branch so enforcement is fail-closed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| // GENERAL_PATTERNS should not include channel-specific patterns | ||
| expect(GENERAL_PATTERNS.some(p => p.source === pattern.source)).toBe(false); |
There was a problem hiding this comment.
Suggestion: The test claims to verify that channel-only patterns are not present in ALL_PATTERNS, but it checks GENERAL_PATTERNS instead. This creates a false negative: accidental inclusion of channel patterns directly in ALL_PATTERNS would not be detected. Assert against ALL_PATTERNS so the test validates the actual contract. [logic error]
Severity Level: Major ⚠️
- ⚠️ Test misses ALL_PATTERNS contamination regressions.
- ❌ Prompt sanitization may over-flag benign user content.
- ❌ Tool-output sanitization may add incorrect untrusted tags.| // GENERAL_PATTERNS should not include channel-specific patterns | |
| expect(GENERAL_PATTERNS.some(p => p.source === pattern.source)).toBe(false); | |
| // ALL_PATTERNS should not include channel-specific patterns | |
| expect(ALL_PATTERNS.some(p => p.source === pattern.source)).toBe(false); |
Steps of Reproduction ✅
1. Run unit tests via `npm run test:unit` (configured in `package.json:25`; also executed
in release workflow at `.github/workflows/release.yml:35`), including
`tests/security/adversarial/patterns.test.ts`.
2. In `tests/security/adversarial/patterns.test.ts:55-60`, the test named
`CHANNEL_PATTERNS are NOT in ALL_PATTERNS` checks `GENERAL_PATTERNS` at line 59, not
`ALL_PATTERNS`.
3. `ALL_PATTERNS` is the actual aggregate used by sanitization
(`src/security/patterns.ts:77-81`) and consumed by `sanitizeInput`/`sanitizeToolOutput`
(`src/security/prompt-defense.ts:46` and `:65`), then used on live orchestration paths
(`src/orchestrator/orchestrator.ts:731` and `:984`).
4. If a future edit accidentally appends `CHANNEL_PATTERNS` directly into `ALL_PATTERNS`
(without touching `GENERAL_PATTERNS`), this test still passes, so the declared contract is
not actually verified.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/security/adversarial/patterns.test.ts
**Line:** 58:59
**Comment:**
*Logic Error: The test claims to verify that channel-only patterns are not present in `ALL_PATTERNS`, but it checks `GENERAL_PATTERNS` instead. This creates a false negative: accidental inclusion of channel patterns directly in `ALL_PATTERNS` would not be detected. Assert against `ALL_PATTERNS` so the test validates the actual contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
Replace unsafe `(event.content as Record<string, unknown>)['result']` cast with direct assignment `toolResultContent.result = sanitizedResult` as suggested in Gemini review. Co-Authored-By: Claude <noreply@anthropic.com>
… pre-screen, test correctness - quarantine-processor: decode URL/unicode/base64 candidates before pre-screen pattern match so encoded injection payloads are caught before reaching quarantine LLM - policy-engine: always_flag with no approval path now denies (fail-closed) instead of logging and allowing — prevents policy bypass when no gate is wired - policy-engine: jobId fallback uses this._sessionId before 'unknown' for better approval audit traceability - patterns.test.ts: fix test to assert against ALL_PATTERNS (not GENERAL_PATTERNS) to correctly verify channel pattern path separation - Update two tests that asserted the old fail-open behaviour to expect deny Co-Authored-By: Claude <noreply@anthropic.com>
…config validation, structured result preservation - patterns.ts: fix system/assistant detector to allow leading whitespace (^\s*system: instead of ^system:) so inputs with leading spaces/tabs are caught - daemon.ts: validate timeout_s is finite and positive before converting to milliseconds — malformed config falls back to default 300s - orchestrator.ts: preserve structured tool result shape when injection was only found in JSON representation — only overwrite result when original was already a string to avoid breaking downstream consumers Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/security/prompt-defense.ts (1)
46-52:⚠️ Potential issue | 🟠 MajorTag encoded payloads before they reach the model.
These loops only replace matches found in the raw string. Inputs like
ignore%20previous%20instructions,\\u0069\\u0067..., or URL-encoded base64 never produce a raw match, so both sanitizers still let encoded injections through unchanged. Normalize candidates first and, if any decoded form matches, mark the original payload as untrusted (or map the match back to raw offsets) before returning.Also applies to: 65-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/prompt-defense.ts` around lines 46 - 52, The current loop using ALL_PATTERNS and result.replace with globalPattern only matches raw text and misses encoded payloads; update the replacement logic to first normalize/try decoding candidate forms (URL-decode, percent/unicode escape sequences like \\uXXXX, and common Base64 variants) for each input chunk, test each decoded form against ALL_PATTERNS/globalPattern, and if a decoded form matches either (a) map the decoded match back to the original raw offsets and wrap that original substring with <untrusted_content> in result, or (b) if mapping is complex, conservatively mark the full input substring as untrusted before returning; apply the same decoding+matching fix to the other replacement loop referenced (the block around lines 65-73) so encoded injections are caught before reaching the model.src/security/policy-engine.ts (1)
515-526:⚠️ Potential issue | 🟠 MajorRace approval waits against cancellation.
The new
ApprovalQueuepath ignores the SDKAbortSignal, so a canceled tool call can still sit in the approval wait until timeout.Suggested hardening
- _options: { signal: AbortSignal }, + options: { signal: AbortSignal }, @@ - const approved = await this._approvalQueue.request({ - action, - score: 65, - jobId: String(input['__jobId'] ?? this._sessionId ?? 'unknown'), - tool: toolName, - }); + const abortPromise = new Promise<'aborted'>((resolve) => { + if (options.signal.aborted) { + resolve('aborted'); + return; + } + options.signal.addEventListener('abort', () => resolve('aborted'), { once: true }); + }); + const approvalResult = await Promise.race([ + this._approvalQueue.request({ + action, + score: 65, + jobId: this._sessionId ?? 'unknown', + tool: toolName, + }), + abortPromise, + ]); + if (approvalResult === 'aborted') { + return { behavior: 'deny' as const, message: 'Approval request aborted' }; + } + const approved = approvalResult;Ideally pair this with a queue-side cancel API so aborted approvals do not linger until timeout.
Also applies to: 631-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/security/policy-engine.ts` around lines 515 - 526, The ApprovalQueue path in createCanUseTool ignores the provided AbortSignal so a canceled tool call can remain waiting; update createCanUseTool and the parallel approval-wait code (also at the similar block around the 631-646 region) to race the approval promise against options.signal by either (a) passing the signal into the ApprovalQueue wait call if ApprovalQueue supports cancellation, or (b) attach a signal listener that rejects/short-circuits the wait and calls the queue-side cancel API (e.g., approvalQueue.cancel(requestId) or equivalent) to remove the pending request, then return a { behavior: 'deny', message: 'cancelled' } (or propagate AbortError) immediately; ensure you reference the async function createCanUseTool, the ApprovalQueue wait call, and any requestId/queue cancel API when implementing the race so aborted approvals do not linger until timeout.
🧹 Nitpick comments (1)
tests/security/adversarial/patterns.test.ts (1)
64-110: Add encoded regression cases here.This suite only exercises plain-text payloads. URL-encoded, unicode-escaped, and URL-encoded-base64 variants are the exact cases that can bypass the new sanitization/pre-screen logic, so the security gap can regress without any test failure. A few fixtures like
ignore%20previous%20instructions,\\u0069\\u0067..., andaWdub3JlIHByZXZpb3VzIGluc3RydWN0aW9ucw%3D%3Dwould make this suite pull its weight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/security/adversarial/patterns.test.ts` around lines 64 - 110, The tests only cover plain-text payloads and miss encoded variants that can bypass sanitization; update the test suite to include URL-encoded, unicode-escaped, and base64/URL-encoded-base64 forms of the same payloads (e.g., URL-encoded like "ignore%20previous%20instructions", unicode-escaped like "\\u0069\\u0067..." and base64 like "aWdub3JlIHByZXZpb3VzIGluc3RydWN0aW9ucw%3D%3D") alongside the existing coreInjections and channelOnlyInjections arrays, and assert that sanitizeInput(payload), sanitizeToolOutput(payload), and ALL_CHANNEL_PATTERNS.some(...) produce the same flags/boolean results as the plain-text cases so encoded variants cannot regress the protections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/security-audit.sh`:
- Around line 5-8: The current audit only greps for token presence; instead
update scripts/security-audit.sh to verify concrete call sites and ordering:
check that sanitizeToolOutput is actually invoked (grep for
"sanitizeToolOutput("), verify ApprovalQueue is instantiated/registered (grep
for "new ApprovalQueue" or "ApprovalQueue.register" or "approvalQueue =") and
that that registration/instantiation occurs before boot() (ensure the match for
"approvalQueue" appears earlier than "boot("), and replace the loose
"from.*patterns" checks with searches for specific exported symbols from
patterns.ts that are actually used in prompt-defense.ts and
quarantine-processor.ts (e.g., grep for the concrete pattern names or their
function calls rather than the import line). Fail the audit if any of these
concrete checks do not pass.
In `@src/channels/quarantine-processor.ts`:
- Around line 59-73: The current pre-screen only applies each normalization once
to message.content so wrapped encodings (e.g., URL-encoded base64) slip through;
replace the one-off transforms with a small iterative helper (e.g.,
decodeTransitively or decodeAndCheck) that: starting from message.content,
repeatedly attempts URL-decoding, unicode-unescape, and base64-decode (each
wrapped in try/catch) and collects each new decoded variant until no new variant
appears or a safe maxDepth (e.g., 5) is reached; dedupe collected candidates and
then run the existing ALL_CHANNEL_PATTERNS check against all candidates to
produce preScreenSuspicious. Ensure the helper and its use reference the
existing variables (message.content, candidates, ALL_CHANNEL_PATTERNS) and avoid
infinite loops by tracking seen strings.
In `@src/orchestrator/orchestrator.ts`:
- Around line 979-991: The current sanitization overwrites the original
structured value by assigning the serialized string back to
toolResultContent.result; instead, preserve the original type by creating a
cloned LLM-facing copy (e.g., sanitizedResultText from
sanitizeToolOutput(resultText)) and use that clone when adding to history or
when passing data to the LLM/hook invocations, leaving toolResultContent.result
unchanged; locate the sanitization logic around sanitizeToolOutput and the
assignment to toolResultContent.result and change the code to only replace the
LLM/history payload (or set a separate sanitized field) while keeping the
original structured result for runAfter() and other consumers.
In `@src/security/policy-engine.ts`:
- Line 638: Replace the untrusted input['__jobId'] usage when constructing jobId
with a trusted source: stop reading input['__jobId'] in the jobId assignment
inside the policy engine and instead accept a trusted jobId passed via the
canUseTool context (or fall back to this._sessionId), and update callers
(notably orchestrator::_buildTokenAwareCanUseTool()) to pass the real jobId into
that context; in short, remove/read-ignore input['__jobId'], add/consume a
trustedJobId field on the canUseTool/context used by the policy engine, and
ensure orchestrator::_buildTokenAwareCanUseTool() supplies that trustedJobId.
- Around line 631-637: The comment is wrong: using score: 65 can be auto-allowed
by a blanket-allow (blanketMaxScore defaults to 80). Update the ApprovalQueue
request in the else branch that calls this._approvalQueue.request({ action,
score: ... }) to use a sentinel high score (e.g., Number.MAX_SAFE_INTEGER) so
the auto-allow check (opts.score < _blanketMaxScore) cannot pass; ensure the
call still uses the same action/payload but replaces score: 65 with the sentinel
to force explicit approval and avoid the blanket-allow window.
---
Outside diff comments:
In `@src/security/policy-engine.ts`:
- Around line 515-526: The ApprovalQueue path in createCanUseTool ignores the
provided AbortSignal so a canceled tool call can remain waiting; update
createCanUseTool and the parallel approval-wait code (also at the similar block
around the 631-646 region) to race the approval promise against options.signal
by either (a) passing the signal into the ApprovalQueue wait call if
ApprovalQueue supports cancellation, or (b) attach a signal listener that
rejects/short-circuits the wait and calls the queue-side cancel API (e.g.,
approvalQueue.cancel(requestId) or equivalent) to remove the pending request,
then return a { behavior: 'deny', message: 'cancelled' } (or propagate
AbortError) immediately; ensure you reference the async function
createCanUseTool, the ApprovalQueue wait call, and any requestId/queue cancel
API when implementing the race so aborted approvals do not linger until timeout.
In `@src/security/prompt-defense.ts`:
- Around line 46-52: The current loop using ALL_PATTERNS and result.replace with
globalPattern only matches raw text and misses encoded payloads; update the
replacement logic to first normalize/try decoding candidate forms (URL-decode,
percent/unicode escape sequences like \\uXXXX, and common Base64 variants) for
each input chunk, test each decoded form against ALL_PATTERNS/globalPattern, and
if a decoded form matches either (a) map the decoded match back to the original
raw offsets and wrap that original substring with <untrusted_content> in result,
or (b) if mapping is complex, conservatively mark the full input substring as
untrusted before returning; apply the same decoding+matching fix to the other
replacement loop referenced (the block around lines 65-73) so encoded injections
are caught before reaching the model.
---
Nitpick comments:
In `@tests/security/adversarial/patterns.test.ts`:
- Around line 64-110: The tests only cover plain-text payloads and miss encoded
variants that can bypass sanitization; update the test suite to include
URL-encoded, unicode-escaped, and base64/URL-encoded-base64 forms of the same
payloads (e.g., URL-encoded like "ignore%20previous%20instructions",
unicode-escaped like "\\u0069\\u0067..." and base64 like
"aWdub3JlIHByZXZpb3VzIGluc3RydWN0aW9ucw%3D%3D") alongside the existing
coreInjections and channelOnlyInjections arrays, and assert that
sanitizeInput(payload), sanitizeToolOutput(payload), and
ALL_CHANNEL_PATTERNS.some(...) produce the same flags/boolean results as the
plain-text cases so encoded variants cannot regress the protections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d954364f-69ed-4ca3-bcc6-01d5dc36e333
📒 Files selected for processing (11)
scripts/security-audit.shsrc/channels/quarantine-processor.tssrc/cli/daemon.tssrc/orchestrator/orchestrator.tssrc/security/patterns.tssrc/security/policy-engine.tssrc/security/prompt-defense.tstests/security/adversarial/fix1-tool-output.test.tstests/security/adversarial/fix2-approval.test.tstests/security/adversarial/patterns.test.tstests/unit/security/policy-engine.test.ts
| grep -q "sanitizeToolOutput" src/orchestrator/orchestrator.ts || { echo "FAIL: sanitizeToolOutput not wired in orchestrator"; exit 1; } | ||
| grep -q "ApprovalQueue" src/orchestrator/orchestrator.ts || { echo "FAIL: ApprovalQueue not imported in orchestrator"; exit 1; } | ||
| grep -q "from.*patterns" src/security/prompt-defense.ts || { echo "FAIL: prompt-defense not importing from patterns.ts"; exit 1; } | ||
| grep -q "from.*patterns" src/channels/quarantine-processor.ts || { echo "FAIL: quarantine-processor not importing from patterns.ts"; exit 1; } |
There was a problem hiding this comment.
The audit is too token-based to be a trustworthy gate.
These greps pass if the string exists anywhere in the file — including comments, dead imports, or unused type references. They also never verify the startup invariant this PR depends on: approvalQueue must be registered before boot(). Tighten the checks to concrete call sites so the audit fails when the wiring is actually broken.
Suggested checks
-grep -q "sanitizeToolOutput" src/orchestrator/orchestrator.ts || { echo "FAIL: sanitizeToolOutput not wired in orchestrator"; exit 1; }
-grep -q "ApprovalQueue" src/orchestrator/orchestrator.ts || { echo "FAIL: ApprovalQueue not imported in orchestrator"; exit 1; }
-grep -q "from.*patterns" src/security/prompt-defense.ts || { echo "FAIL: prompt-defense not importing from patterns.ts"; exit 1; }
-grep -q "from.*patterns" src/channels/quarantine-processor.ts || { echo "FAIL: quarantine-processor not importing from patterns.ts"; exit 1; }
+grep -Eq '\bsanitizeToolOutput\s*\(' src/orchestrator/orchestrator.ts || { echo "FAIL: sanitizeToolOutput not called in orchestrator"; exit 1; }
+grep -Eq '\bsetApprovalQueue\s*\(\s*approvalQueue\s*\)' src/cli/daemon.ts || { echo "FAIL: ApprovalQueue not registered before orchestrator boot"; exit 1; }
+grep -Eq '\bthis\._policyEngine\.setApprovalQueue\s*\(' src/orchestrator/orchestrator.ts || { echo "FAIL: ApprovalQueue not propagated into PolicyEngine"; exit 1; }
+grep -Eq "from ['\"].*patterns\.js['\"]" src/security/prompt-defense.ts || { echo "FAIL: prompt-defense not importing from patterns.ts"; exit 1; }
+grep -Eq "from ['\"].*patterns\.js['\"]" src/channels/quarantine-processor.ts || { echo "FAIL: quarantine-processor not importing from patterns.ts"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/security-audit.sh` around lines 5 - 8, The current audit only greps
for token presence; instead update scripts/security-audit.sh to verify concrete
call sites and ordering: check that sanitizeToolOutput is actually invoked (grep
for "sanitizeToolOutput("), verify ApprovalQueue is instantiated/registered
(grep for "new ApprovalQueue" or "ApprovalQueue.register" or "approvalQueue =")
and that that registration/instantiation occurs before boot() (ensure the match
for "approvalQueue" appears earlier than "boot("), and replace the loose
"from.*patterns" checks with searches for specific exported symbols from
patterns.ts that are actually used in prompt-defense.ts and
quarantine-processor.ts (e.g., grep for the concrete pattern names or their
function calls rather than the import line). Fail the audit if any of these
concrete checks do not pass.
| // Pre-screen for known injection patterns before even calling LLM. | ||
| // Normalize encoded variants (URL, unicode escapes, base64) so encoded payloads | ||
| // are caught before they reach the quarantine LLM. | ||
| const candidates = [message.content]; | ||
| try { candidates.push(decodeURIComponent(message.content)); } catch { /* malformed URL encoding — skip */ } | ||
| candidates.push( | ||
| message.content.replace(/\\u([0-9a-fA-F]{4})/g, (_, hex: string) => | ||
| String.fromCharCode(parseInt(hex, 16)), | ||
| ), | ||
| ); | ||
| try { candidates.push(Buffer.from(message.content, 'base64').toString('utf8')); } catch { /* malformed base64 — skip */ } | ||
|
|
||
| const preScreenSuspicious = candidates.some(candidate => | ||
| ALL_CHANNEL_PATTERNS.some(pattern => pattern.test(candidate)) | ||
| ); |
There was a problem hiding this comment.
Decode transitively; one-pass normalization still misses wrapped payloads.
Every transform here is derived only from message.content. A payload like aWdub3JlIHByZXZpb3VzIGluc3RydWN0aW9ucw%3D%3D becomes raw base64 after decodeURIComponent(), but that candidate is never base64-decoded again, so the pre-screen never sees ignore previous instructions. Reuse a shared iterative decodeAndCheck()-style helper instead of independent one-off decodes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/channels/quarantine-processor.ts` around lines 59 - 73, The current
pre-screen only applies each normalization once to message.content so wrapped
encodings (e.g., URL-encoded base64) slip through; replace the one-off
transforms with a small iterative helper (e.g., decodeTransitively or
decodeAndCheck) that: starting from message.content, repeatedly attempts
URL-decoding, unicode-unescape, and base64-decode (each wrapped in try/catch)
and collects each new decoded variant until no new variant appears or a safe
maxDepth (e.g., 5) is reached; dedupe collected candidates and then run the
existing ALL_CHANNEL_PATTERNS check against all candidates to produce
preScreenSuspicious. Ensure the helper and its use reference the existing
variables (message.content, candidates, ALL_CHANNEL_PATTERNS) and avoid infinite
loops by tracking seen strings.
9547928 to
2582ad9
Compare
2 participants
User description
Summary
Two security fixes from the v0.12 review.
FIX 4: Encoding-aware injection detection
Replaces fragile hardcoded base64 regex literals with
decodeAndCheck()— a decode-then-check function that normalizes three encoding schemes before running patterns:%69%67%6E%6F%72%65...)\u0069\u0067\u006E\u006F\u0072\u0065...)All decode attempts are
try/catchguarded — malformed input is skipped, never thrown.FIX 6: Unified action classification taxonomy
Three incompatible classifiers previously mapped tool names to divergent string sets:
PolicyEngine._classifyAction()→write_file | shell_exec | ...IrreversibilityScorerHook.toolToAction()→file_delete | http_request | git_commit | ...MemoryRiskForecaster.categorize()→write | shell | network | ...New
src/security/action-classifier.tsprovides a single canonicalclassifyAction()function. All three delegate to it. Legacy adapter functions (toIrreversibilityCategory,toForecasterCategory) preserve exact string outputs for downstream consumers.scripts/security-audit.shupdated with 4 new wiring invariant checks.Test results
Files changed
src/security/action-classifier.tssrc/security/patterns.tsdecodeAndCheck(), keep ENCODED_INJECTION_PATTERNS for compatsrc/security/policy-engine.ts_classifyAction()delegates toclassifyAction()src/hooks/built-in/irreversibility-scorer.tstoolToAction()delegates to canonicalsrc/core/memory-risk-forecaster.tscategorize()delegates to canonicalscripts/security-audit.shtests/unit/security/encoding.test.tstests/unit/security/action-classifier.test.ts🤖 Generated with Claude Code
CodeAnt-AI Description
Harden tool output handling and approval checks
What Changed
Impact
✅ Fewer prompt injection escapes from tool results✅ Fewer silent approval bypasses✅ Safer channel message handling💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Bug Fixes
Tests