Repository navigation
Conversation
|
Stacked consumer: Pinvou/pinvou-agent#653 pins |
27ccc01 to
82e3e38
Compare
The memory docs told the model not to store secrets, but nothing enforced it: normalize_note only checked empty/oversize. Memory files are plain text on disk and are injected verbatim into future prompts, so a captured secret leaks into every later session. Port the sensitive-content heuristics from the app-side embedder (features/memory/util.rs::looks_sensitive), where they filter captured memory in production: 11+ digit runs, credential needles (including the Chinese markers 身份证/手机号/密码/口令/密钥/私钥 — real-world secret markers, kept deliberately), credential words tied to an assignment marker, URLs, emails, and home-directory paths. Matching runs on a whitespace-collapsed projection so a tab or NBSP cannot split a needle or marker, and the digit run bridges the separators grouped numbers are written with (spaces, hyphen/dash variants, and dots), so "138 1234 5678" and "138.1234.5678" are caught while "2024-02-01" (8 bridged digits) is not. One disclosed deviation from the source: the embedder screens a 500-character clean_text projection, this gate screens the full note — strictly more, in the same err-closed direction. On top of the port: AWS access-key ID shapes (AKIA/ASIA + 16 alphanumerics, matched case-insensitively), the 'private key' and 'private_key' needles for PEM armor and config spellings, the compound key kinds 'ssh key'/'deploy key'/'access key', the English copulas " is "/" was " and the arrow '->' as assignment markers mirroring the ported 是/为, the api-key header spelling, pwd/passcode in the assignment-word list, the full-width :assignment marker, scheme-agnostic URL matching (a credentialed postgres:// or redis:// URL leaks like https:// does), and a wider path net (/root/, mid-note ~/, \users\ beyond sentence start, any Windows drive letter). The gate runs where content enters the store only — append, the revise replacement, and import_legacy (screened as a whole, not line by line, so a secret split across lines cannot slip between per-line checks; refusing the whole import means a pre-gate file cannot bulk-inject what it captured under the advisory-only regime) — and fails closed with a typed SensitiveNoteRefusal whose stable reason string the remember tool surfaces as invalid input. The tool description now says the rule is enforced. Retrieval (memory_search/memory_get) is read-only and untouched; retire and the revise source are never gated, so a sensitive note that predates the gate can always be revised away or retired (both still journal the original text in plaintext JOURNAL.md — never indexed or injected, documented as such). The two generic-api-key-shaped synthetic fixtures (sk-live-abc123, ghp_0123456789abcdef) carry exact-value gitleaks allowlist entries per the repo convention; the AKIA docs-example fixture rides gitleaks' default example allowlist like the pre-existing fixtures do. Signed-off-by: asto <asto18089@126.com>
82e3e38 to
b1c6654
Compare
asto18089
left a comment
There was a problem hiding this comment.
Fresh full review — head b1c665462
Fresh 12-shard review (claim audit, port provenance vs the app-side embedder, ingress/threat-model enumeration, heuristic line-by-line logic, tool wiring and error contract, test-quality, docs-vs-code, smuggled-changes audit, empirical gitleaks, process/placement, elegance/deps, CI audit, plus empirical adversarial probes executed against the real gate in a scratch worktree).
Rebase state: no rebase needed — pinvou3-clean head is 44921cf02, exactly this PR's base (verified via API; zero drift, mergeable: CLEAN).
Verdict: no blockers (0×P0, 0×P1, 11×P2, ~25×P3)
- Useful / complete / root-caused — yes. The leak surface is real (plain-Markdown store injected verbatim into prompts via
prompt_block, verified end-to-end), and the gate sits at the single chokepoint every programmatic ingress shares:remember(store level, so tool/slash-command/#quick-add/HTTP API all inherit it), therevisereplacement, andimport_legacyscreened whole-file before any write. Removal escape hatches (retire, revise source) are deliberately and correctly ungated; the only productionimport_legacycaller is the/memory importcommand, so a wholesale refusal cannot break startup. All revisions claimed in the PR body are verified in code and empirically: tab/NBSP folding, dot-bridged digit runs, any-scheme credentialed URLs, whole-file import screening (split-across-lines test), and the gitleaks allowlists (empirically load-bearing: 7 findings with them removed, 0 with;AKIAIOSFODNN7EXAMPLErides the default.+EXAMPLE$allowlist exactly as claimed). - Elegant — yes, two deductions below (P2-5, P2-6). Hand-rolled scanning is justified despite
regexbeing available (the split-based AWS tokenizer is strictly more err-closed than the natural\bregex; the needle list is clearer ascontainsthan as a 19-alternative regex). The typedSensitiveNoteRefusal+ downcast toinvalid_inputis the right weight and verified downcastable at every call site (no.context()wrappers;with_write_lockis transparent). - No smuggled changes — confirmed. 15/15 hunks in scope; the 13 deleted lines fully reconciled (doc rewrite, fixture rename with preserved test semantics, error-mapping refactor preserving strings verbatim); single commit; commit message accurate against the diff; all fixtures canonical synthetic values.
- Bugs — none found. Deterministic, panic-free (byte-window is UTF-8-safe), linear scans (~2.5 MB worst case at the 64 KiB cap; 60 KB note through the full store path measured at 62 ms). CI green on the exact head: 14,563 tests including every new test by name,
gate/check/Gitleaks/DCO all pass (all four are the branch-protection requirements).
P2 findings (should fix or disclose; none block)
- Compound key kinds lack the underscore/hyphen spellings —
access_key/ssh_key/deploy_key/access-key/ssh-key/deploy-keyare in neitherSENSITIVE_NEEDLESnor the assignment-word list, soprod access_key = hunter2walks through, while the doc wording implies the spellings were considered (native_memory.rs:1017-1037,:1181-1184). One-line list change + test rows, or extend the disclosed slack. - Undisclosed false-negative shapes (empirically confirmed through the real gate):
DB_PASS=hunter2(passabbreviation not in the assignment-word list);I reset the password to hunter2and baremy password hunter2(credential word without a" is "/" was "/=/:copula);Authorization: Bearer <JWT>(assignment colon present,authorizationnot a credential word — the disclosed bare-token slack arguably doesn't cover this shape). Worth a follow-up needle/couplet pass or explicit disclosure. - Version-string false positive contradicts the docs —
130.0.6723.116bridges to exactly 11 digits and is refused, but docs and module comments say "version numbers stay under it" (true for2024-02-01= 8, false for Chrome/Windows-style versions). Doc accuracy fix. - Two missing test pins: no
ASIA-prefix payload (anAKIA-only typo would ship green), and the documented "evidence is never screened" contract is unpinned (a future "fix" gating evidence would silently break quoting secrets during removal). - 9 of 12 credential words in the assignment list are unreachable (needle check runs first;
tokenis pre-consumed by its own rule) — the "deliberate repetition" comment doesn't say they're a backstop only, so an auditor reading the code gets the wrong trigger set (native_memory.rs:1178-1186). Trim to the live four or annotate. - Twin prose blocks — the ~68-line rustdoc and the new
docs/MEMORY.mdsection carry near-verbatim family/slack enumerations; two sync points for one behavior set. Consider pointing one at the other; the tests already pin everything. - Cross-repo duplication is unmitigated — Pinvou/pinvou-agent#653 explicitly says "#82 — no app chop needed", so the app-side embedder copy stays in production while the engine copy is now strictly stronger (AKIA shape, more needles/markers, scheme-agnostic URLs, bridged runs), with no shared test vector or sync note; and the engine-side gate fns are private, so there is no API to chop onto even if wanted. Merge-time decision: expose the predicate set and chop in codewhale-hq#653, or add a cross-reference/sync-contract note to both copies. (The PR comment here overstates codewhale-hq#653's chop scope — true for the batch's other PRs, not this one.)
- PR body nit: "The full lib suite runs in CI's
gatejob" — it runs in thecheckjob;gateonly aggregatescheck+Gitleaksresults (fork-ci.yml:55-58,:97-110).
Notable P3s
Evidence field is model-controlled, unscreened, and lands in the journal — docs name retire/revise-source as the journal channels but not evidence (one sentence); pkg@1.3.0 trips the email rule (dependency pins); CN marker-without-value FPs (数据库的访问口令为空, the old token was revoked); / and · don't bridge digit runs; no exact-string pin of the stable reason; pwd/passcode are undisclosed additions to the port inventory; AWS "16 uppercase alphanumerics, matched case-insensitively" wording is loose and the 20-char token boundary is undisclosed; workflow redaction.rs already has a private AWS-shape predicate (second implementation in the workspace).
Verification of verification
Targeted suite: 36 passed / 0 failed locally (matches the body). fmt clean; clippy clean on all touched files (a local full-workspace clippy with a floating toolchain flags pre-existing clone_on_copy in engine tests/shell_dispatcher/js_execution — untouched by this PR; CI's pinned clippy -D warnings is green). Local full-lib run showed 5 failures outside this PR's files (remote_control/runtime_threads/sandbox no_new_privs); serial rerun drops them to 2 prctl-bound container artifacts — environment, not the PR.
Recommendation: mergeable. P2-1/2/3 are cheap pre-merge fixes (one list + a few doc sentences + test rows); P2-7 needs a decision (chop vs anchor) but shouldn't block this PR landing its own gate.
Summary
The native memory write path gains a code-level sensitive-content refusal. Until now the secrets guidance lived only in
docs/MEMORY.mdand theremembertool description — advisory text — whilenormalize_notechecked only empty/oversize. Memory files are plain text on disk and are injected verbatim into future prompts, so a captured secret leaks into every later session.The write path (
NativeMemoryStore::remember, thetohalf ofrevise, andimport_legacyscreened as a whole) now runs each normalized note through heuristic screens ported from the app-side embedder (features/memory/util.rs::looks_sensitiveand its helpers), where they have filtered captured memory in production. A note that trips any screen is refused fail-closed with a typed error — never written, never silently dropped.Heuristic inventory
Matching runs on a whitespace-collapsed projection of the note, so a tab or NBSP inside
the password<tab>is hunter2cannot split a needle or a marker; folding only removes whitespace variation, so nothing that matched on the raw note matches less on the projection.138 1234 5678and138.1234.5678are caught while ordinary dates and version numbers stay under it (2024-02-01bridges to 8, and punctuation between unrelated numbers breaks the run). Adapted from the embedder's total-digit count because a total across full-sentence notes false-positives on date/version-heavy notes.api_key,apikey,api key,api-key(the HTTP header spelling),secret,private key,private_key(the config/underscore spelling), the compound key kindsssh key/deploy key/access key, and the Chinese markers 身份证 / 手机号 / 手机号码 / 电话号码 / 联系电话 / 密码 / 口令 / 密钥 / 私钥, kept deliberately: they are the real-world secret markers in the locales the source heuristics run in production.token(orpassword/passwd/pwd/passcode/secret/api key/密钥/私钥/口令/密码) tied to=,:, the full-width:, the arrow->, the CJK copulas 是/为, plus the English copulas " is " / " was " added as the mirrors of 是/为 (without them "the password is hunter2" walks through).AKIA/ASIA+ 16 alphanumerics, matched case-insensitively so a lowercased spelling cannot walk through; added on top of the port so a bare key is refused even with no assignment or needle around it.scheme://matching is deliberately not limited to http(s): a credentialedpostgres://user:pw@db/apporredis://:pass@hostleaks likehttps://does. Paths:/home/,/root/,/tmp/,/users/,/var/,/etc/,/opt/and~/anywhere in the note, plus\users\,\appdata\, and any Windows drive root (c:\,d:/keys), not justc:.the database key is …). The net also over-refuses on purpose: documentation links,/tmp/mentions, server addresses (an IPv4 with 11+ digits like192.168.100.100), space-separated number lists (ports, backoffs), and a credential word near an unrelated colon all trip the rules; long order or epoch-millisecond numbers trip the digit rule.clean_textprojection; this gate screens the full note — strictly more, in the same err-closed direction.Refusal semantics
Fail-closed.
SensitiveNoteRefusal(unit struct,Display+std::error::Error) carries one stable, model-actionable string; theremembertool downcasts it and surfaces it verbatim as invalid input instead of a generic execution failure, and the tool description now states that the rule is enforced:The gate covers only what enters the store. Retrieval (
memory_search/memory_get,search,get) is read-only and intentionally unchanged.retireand the revise source are never gated: a sensitive note that predates the gate must stay revisable/removable, or the gate would preserve the very leak it exists to prevent — note that both still record the original text in the plaintext journal (JOURNAL.md), which is never indexed or injected but is not scrubbed (documented).import_legacyis screened as a whole — not line by line, so a secret split across lines (the password is\nhunter2) cannot slip between per-line checks — and one tripping note refuses the whole import: nothing partially migrates and the legacy file is left untouched for cleaning. Evidence is journal-only text that is never indexed or injected, so it is not screened either. Writes through the approval-gated file tools are out of scope: with the user approving each write, they carry the same trust as hand-editing the file.Placement note: the heuristics live as a section inside
crates/tui/src/native_memory.rsrather than a new module — the repository already designates that file as the single home for native memory, no existing sensitivity helper existed to reuse (the workflowredactionmodule is replace-style outbound-text semantics with different coverage), and the gate sharesnormalize_noteoutput with its call sites.Reviewer note: the heuristics are ported from the app-side embedder, where they run in production over captured memory notes; naming/style adapted to CodeWhale, and the additions beyond the port are flagged inline in the code docs.
One existing test fixture changed:
tools_enforce_workspace_scope_boundaryused the note "second workspace secret", which now (correctly) trips the gate; it uses "second workspace exclusion" — the test covers scope boundaries, not content.Changes in this revision (post-review tightening)
import_legacynow screens the file as a whole instead of line by line (closes the cross-line split evasion, e.g.the password is+hunter2on consecutive lines); dedicated test added..joined the digit-run separators (dot-grouped phones/cards like138.1234.5678no longer walk through); IPv4 collateral documented.private_key(underscore) and the compound key kindsssh key/deploy key/access keyadded as needles (a bare generic "key" stays un-gated for keybindings/key-value prose, pinned by a pass test).wasand->added as assignment markers (mirroring the already-present " is ")./tmp/wording fixed, provenance reworded to "app-side embedder".Verification
cargo fmt --all— clean;cargo clippy -p codewhale-tui— 0 diagnostics (arm64 Linux, floating stable toolchain)cargo test -p codewhale-tui --lib -- native_memory:: tools::remember tools::native_memory— 36 passed, 0 failed, covering every refusal family (AWS key shape incl. lowercase, PEM header, CN 身份证 18-digit,密码:xxx, full-width-colonpassword:xxx,api_key=..., hyphenatedX-API-Key: ...,token: ..., baresecret, 11-digit phone in contiguous / hyphen / space / dot forms, 12-digit order id, tab-separatedpassword is,password was,password -> x, credentialed non-http URL, plain https URL, email,/root/.ssh, mid-note~/,D:/keys, split-across-lines import) and benign passes (dates2024-01-15 14:30, version numbers, a 10-digit bridged phone, a spaced retry-backoff list, "key insight" prose, ordinary sentences); empty/oversize bounds unchanged and pinned to run before the gate; revise-away and retire of stored sensitive notes still possible; workspace-scope capture screened; refused import (single-line and cross-line) leaves both files untouched. The full lib suite runs in CI'sgatejob.sk-live-abc123,ghp_0123456789abcdef) carry exact-value allowlist entries and the AKIA docs-example rides gitleaks' default example allowlist, as the pre-existing fixtures doNo-Issue: enforcement hardening for an existing subsystem; the repo has issues disabled