Skip to content

feat(memory): refuse sensitive notes at write time - #82

Open
asto18089 wants to merge 1 commit into
pinvou3-cleanfrom
upstream/u5-memory-sensitivity
Open

asto18089 wants to merge 1 commit into
pinvou3-cleanfrom
upstream/u5-memory-sensitivity

Conversation

@asto18089

@asto18089 asto18089 commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The native memory write path gains a code-level sensitive-content refusal. Until now the secrets guidance lived only in docs/MEMORY.md and the remember tool description — advisory text — while normalize_note checked 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, the to half of revise, and import_legacy screened as a whole) now runs each normalized note through heuristic screens ported from the app-side embedder (features/memory/util.rs::looks_sensitive and 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 hunter2 cannot split a needle or a marker; folding only removes whitespace variation, so nothing that matched on the raw note matches less on the projection.

  • 11+ digit runs, separator-bridged — CN mobile numbers are 11 digits, resident ID cards 18; the run bridges the spaces, hyphen/dash variants, and dots grouped numbers are conventionally written with, so 138 1234 5678 and 138.1234.5678 are caught while ordinary dates and version numbers stay under it (2024-02-01 bridges 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.
  • Credential needles — api_key, apikey, api key, api-key (the HTTP header spelling), secret, private key, private_key (the config/underscore spelling), the compound key kinds ssh 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.
  • Credential word + assignment marker — token (or password/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).
  • AWS access-key IDs — 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.
  • URLs with any scheme, email addresses, filesystem paths — scheme:// matching is deliberately not limited to http(s): a credentialed postgres://user:pw@db/app or redis://:pass@host leaks like https:// 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 just c:.
  • Known slack, documented in the module docs: a bare token with no credential word or assignment around it (a GitHub PAT, a JWT, a PEM body without its armor line) passes, as do homoglyph, full-width digit, and invisible-character spellings, and a generic "… key is …" shape with no compound kind (the database key is …). The net also over-refuses on purpose: documentation links, /tmp/ mentions, server addresses (an IPv4 with 11+ digits like 192.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.
  • 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.

Refusal semantics

Fail-closed. SensitiveNoteRefusal (unit struct, Display + std::error::Error) carries one stable, model-actionable string; the remember tool 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:

memory note refused: it looks like it contains sensitive data such as a credential, token, password, phone or ID number. Memory files are plain text on disk and are injected verbatim into future prompts, so never store secrets there. Restate the durable fact without the sensitive values, or keep the secret in a dedicated credential store instead.

The gate covers only what enters the store. Retrieval (memory_search / memory_get, search, get) is read-only and intentionally unchanged. retire and 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_legacy is 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.rs rather 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 workflow redaction module is replace-style outbound-text semantics with different coverage), and the gate shares normalize_note output 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_boundary used 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_legacy now screens the file as a whole instead of line by line (closes the cross-line split evasion, e.g. the password is + hunter2 on consecutive lines); dedicated test added.
  • . joined the digit-run separators (dot-grouped phones/cards like 138.1234.5678 no longer walk through); IPv4 collateral documented.
  • private_key (underscore) and the compound key kinds ssh key / deploy key / access key added as needles (a bare generic "key" stays un-gated for keybindings/key-value prose, pinned by a pass test).
  • was and -> added as assignment markers (mirroring the already-present " is ").
  • AWS shape matched case-insensitively.
  • Disclosure/precision: 500-char-truncation deviation from the port stated in the commit and module docs, journal retention documented, /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)
  • Targeted: 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-colon password:xxx, api_key=..., hyphenated X-API-Key: ..., token: ..., bare secret, 11-digit phone in contiguous / hyphen / space / dot forms, 12-digit order id, tab-separated password 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 (dates 2024-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's gate job.
  • The refusal tests fail without the fix: disabling the gate fails exactly the refusal-family tests while pass-through/bounds/retire tests stay green (verified on the prior head; the gate structure is unchanged in this revision)
  • Local gitleaks with the repo config: no new secret-shaped fixtures are introduced by this revision; the two generic-api-key fixtures (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 do

No-Issue: enforcement hardening for an existing subsystem; the repo has issues disabled

@asto18089

Copy link
Copy Markdown
Collaborator Author

Stacked consumer: Pinvou/pinvou-agent#653 pins upstream/integration (= pinjou3-clean + these upstream contributions) and chops the app-side duplicates onto the new APIs. That parent PR is draft until this PR merges — its verify-public-submodule gate requires the parent gitlink to equal the public pinjou3-clean head. After merge, the pin re-points to the branch head and the chop lands.

@asto18089
asto18089 force-pushed the upstream/u5-memory-sensitivity branch from 27ccc01 to 82e3e38 Compare October 6, 2026 10:09
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>
@asto18089
asto18089 force-pushed the upstream/u5-memory-sensitivity branch from 82e3e38 to b1c6654 Compare October 6, 2026 11:57

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. 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), the revise replacement, and import_legacy screened whole-file before any write. Removal escape hatches (retire, revise source) are deliberately and correctly ungated; the only production import_legacy caller is the /memory import command, 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; AKIAIOSFODNN7EXAMPLE rides the default .+EXAMPLE$ allowlist exactly as claimed).
  2. Elegant — yes, two deductions below (P2-5, P2-6). Hand-rolled scanning is justified despite regex being available (the split-based AWS tokenizer is strictly more err-closed than the natural \b regex; the needle list is clearer as contains than as a 19-alternative regex). The typed SensitiveNoteRefusal + downcast to invalid_input is the right weight and verified downcastable at every call site (no .context() wrappers; with_write_lock is transparent).
  3. 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.
  4. 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)

  1. Compound key kinds lack the underscore/hyphen spellings — access_key/ssh_key/deploy_key/access-key/ssh-key/deploy-key are in neither SENSITIVE_NEEDLES nor the assignment-word list, so prod access_key = hunter2 walks 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.
  2. Undisclosed false-negative shapes (empirically confirmed through the real gate): DB_PASS=hunter2 (pass abbreviation not in the assignment-word list); I reset the password to hunter2 and bare my password hunter2 (credential word without a " is "/" was "/=/: copula); Authorization: Bearer <JWT> (assignment colon present, authorization not a credential word — the disclosed bare-token slack arguably doesn't cover this shape). Worth a follow-up needle/couplet pass or explicit disclosure.
  3. Version-string false positive contradicts the docs — 130.0.6723.116 bridges to exactly 11 digits and is refused, but docs and module comments say "version numbers stay under it" (true for 2024-02-01 = 8, false for Chrome/Windows-style versions). Doc accuracy fix.
  4. Two missing test pins: no ASIA-prefix payload (an AKIA-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).
  5. 9 of 12 credential words in the assignment list are unreachable (needle check runs first; token is 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.
  6. Twin prose blocks — the ~68-line rustdoc and the new docs/MEMORY.md section carry near-verbatim family/slack enumerations; two sync points for one behavior set. Consider pointing one at the other; the tests already pin everything.
  7. 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.)
  8. PR body nit: "The full lib suite runs in CI's gate job" — it runs in the check job; gate only aggregates check+Gitleaks results (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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant