feat(gateway): persist tool approval decisions to audit log#72769
feat(gateway): persist tool approval decisions to audit log#72769chinadbo wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds an append-only JSONL audit log for tool approval decisions and a new Confidence Score: 3/5Not safe to merge as-is: the advertised secret redaction is ineffective for the actual arg type used, causing credential leakage to disk. One P1 security finding (secrets in commandArgv bypass redaction) blocks the merge. Two P2 findings (blocking sync read in async handler, unreachable error label) are non-blocking but worth addressing. src/infra/exec-approval-audit.ts — redaction logic and synchronous file read both need attention.
|
| for (const [key, value] of Object.entries(args as Record<string, unknown>)) { | ||
| result[key] = SECRET_KEY_PATTERNS.test(key) ? "[REDACTED]" : value; | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| export function appendApprovalAuditEntry( | ||
| entry: ApprovalAuditEntry, |
There was a problem hiding this comment.
commandArgv
redactSecretArgs short-circuits and returns the input unchanged when it receives an array (Array.isArray(args) check on line 42). At the call site, args is set to resolvedEvent.request?.commandArgv, which is typed string[]. Any CLI arguments carrying bearer tokens or API keys are therefore written to disk in plaintext. The PR description states that sensitive values are redacted, but because commandArgv is always an array rather than a plain object, no redaction ever occurs in practice.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approval-audit.ts
Line: 40-47
Comment:
**Secret redaction does not apply to `commandArgv`**
`redactSecretArgs` short-circuits and returns the input unchanged when it receives an array (`Array.isArray(args)` check on line 42). At the call site, `args` is set to `resolvedEvent.request?.commandArgv`, which is typed `string[]`. Any CLI arguments carrying bearer tokens or API keys are therefore written to disk in plaintext. The PR description states that sensitive values are redacted, but because `commandArgv` is always an array rather than a plain object, no redaction ever occurs in practice.
How can I resolve this? If you propose a fix, please make it concise.| env: NodeJS.ProcessEnv = process.env, | ||
| homedir: () => string = () => os.homedir(), | ||
| ): ApprovalAuditEntry[] { | ||
| const auditPath = resolveApprovalAuditLogPath(env, homedir); | ||
| let raw: string; | ||
| try { | ||
| raw = fs.readFileSync(auditPath, "utf-8"); | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code === "ENOENT") { | ||
| return []; | ||
| } | ||
| throw err; | ||
| } | ||
|
|
||
| const lines = raw.split("\n").filter((line) => line.trim().length > 0); | ||
| const entries: ApprovalAuditEntry[] = []; | ||
| for (const line of lines) { | ||
| try { | ||
| const entry = JSON.parse(line) as ApprovalAuditEntry; | ||
| if (opts.since !== undefined && entry.ts < opts.since) { | ||
| continue; | ||
| } | ||
| if (opts.agentId !== undefined && entry.agentId !== opts.agentId) { | ||
| continue; | ||
| } | ||
| entries.push(entry); | ||
| } catch { | ||
| // skip malformed lines | ||
| } | ||
| } | ||
|
|
||
| if (opts.limit !== undefined && opts.limit > 0 && entries.length > opts.limit) { | ||
| return entries.slice(entries.length - opts.limit); | ||
| } | ||
| return entries; | ||
| } |
There was a problem hiding this comment.
readFileSync loads the full approvals.jsonl into a single string before any filtering. Combined with the limit slice operating on the already-filtered entries array, both the I/O and the in-process filtering scale linearly with total file size. For a long-lived gateway instance with many tool calls, a single exec.approval.auditLog RPC will block the Node.js event loop for an unbounded duration. The existing io.audit.ts pattern uses fs.promises for async writes; a streaming line-by-line read (or at minimum an async readFile) would avoid blocking.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/exec-approval-audit.ts
Line: 75-110
Comment:
**Entire file read into memory on every query**
`readFileSync` loads the full `approvals.jsonl` into a single string before any filtering. Combined with the `limit` slice operating on the already-filtered `entries` array, both the I/O and the in-process filtering scale linearly with total file size. For a long-lived gateway instance with many tool calls, a single `exec.approval.auditLog` RPC will block the Node.js event loop for an unbounded duration. The existing `io.audit.ts` pattern uses `fs.promises` for async writes; a streaming line-by-line read (or at minimum an async `readFile`) would avoid blocking.
How can I resolve this? If you propose a fix, please make it concise.| command: resolvedEvent.request?.command ?? "", | ||
| decision: auditDecision, | ||
| resolvedBy: { | ||
| deviceId: client?.connect?.device?.id ?? null, | ||
| clientId: client?.connect?.client?.id ?? null, | ||
| connId: client?.connId ?? null, | ||
| }, | ||
| agentId: resolvedEvent.request?.agentId ?? null, | ||
| sessionKey: resolvedEvent.request?.sessionKey ?? null, | ||
| args: resolvedEvent.request?.commandArgv, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
doAppend is fire-and-forget; errorLabel is unreachable
appendApprovalAuditEntry is synchronous and swallows all errors internally via try/catch. The run callback therefore never throws, so the errorLabel: "exec approvals: audit log write failed" will never be surfaced through whatever error-handling pipeline extraResolvedHandlers uses. If audit write failures need to be observable (e.g. via logging/metrics), appendApprovalAuditEntry would need to propagate the error rather than absorbing it, or the swallowing should be documented as intentional with a note that errorLabel is vestigial here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/exec-approval.ts
Line: 407-418
Comment:
**`doAppend` is fire-and-forget; `errorLabel` is unreachable**
`appendApprovalAuditEntry` is synchronous and swallows all errors internally via `try/catch`. The `run` callback therefore never throws, so the `errorLabel: "exec approvals: audit log write failed"` will never be surfaced through whatever error-handling pipeline `extraResolvedHandlers` uses. If audit write failures need to be observable (e.g. via logging/metrics), `appendApprovalAuditEntry` would need to propagate the error rather than absorbing it, or the swallowing should be documented as intentional with a note that `errorLabel` is vestigial here.
How can I resolve this? If you propose a fix, please make it concise.|
The failing CI checks (
|
33867e6 to
48c769d
Compare
a670cec to
9c9c58f
Compare
7727bb0 to
88b8407
Compare
|
Addressed Greptile review:
|
Append approved/denied decisions to {stateDir}/audit/approvals.jsonl.
Adds exec.approval.auditLog gateway method (READ scope) for querying
with since/limit/agentId filters. Args with secret-looking keys are
redacted before writing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a286311 to
0cfe443
Compare
|
Codex review: found issues before merge. Summary Reproducibility: yes. A focused branch check can assert Next step before merge Security Review findings
Review detailsBest possible solution: Keep the feature direction, but revise the branch so approval history is approval-scoped, argv persistence uses the shared redaction contract, audit reads are async and bounded, protocol artifacts/docs are synchronized, and the user-facing change has a changelog entry. Do we have a high-confidence way to reproduce the issue? Yes. A focused branch check can assert Is this the best way to solve the issue? No. Persisting approval decisions is a reasonable additive gateway feature, but this implementation is not the narrowest maintainable solution until it uses the existing approval scope, shared redaction behavior, bounded async reads, and the full protocol update path. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 15bbf4f2f304. |
Summary
{stateDir}/audit/approvals.jsonlas append-only JSONLexec.approval.auditLoggateway method (READ scope) for querying the log with optionalsince,limit, andagentIdfilterssrc/config/io.audit.tsTest plan
approvals.jsonldecision: "denied"exec.approval.auditLogreturns persisted entries across gateway restartsagentId,since, andlimitfilters work correctlypnpm check:changedpassesThanks @ioodu