fix(capture): decide capture and gate bypass by channel, not by content (#365) - #375
Conversation
d81a83e to
c700e3a
Compare
…nt (#365) Fetched web content could install itself in durable, cross-session memory by shaping itself, through two content-controlled decisions: 1. hooks/post_tool_capture._should_capture keyed WebFetch/WebSearch capture on `kw in output.lower()` over HIGH_VALUE_PATTERNS — the fetched page decided whether it was persisted. 2. core/write_gate.determine_bypass grants bypass_error / bypass_decision from the content itself (core/thermodynamics), so text merely SHAPED like an error or a decision skipped the novelty REJECT. hooks/session_start then replays stored memories verbatim into later sessions, so both decisions are writes to durable state made from attacker-supplied input. core/capture_origin is a third concept on purpose, and the docstring says why neither neighbour could carry this: - core/provenance grades REFERENCE VERIFIABILITY by reading the content's references. - core/source_monitoring attributes EPISTEMIC ORIGIN by regex over the content's wording. Both are content-derived, so the attacker supplies the classifier's input. A hostile page dense with file paths and URLs grades `verified` and classifies `perceived` — the most credible value in each vocabulary. Origin is resolved from the producing TOOL NAME instead, which is known out-of-band at the hook boundary and cannot be forged by the payload; a test asserts that invariant directly. Asymmetry in the refusal is deliberate: `force` and a `deliberate` write class are out-of-band human signals and stay valid at any origin. Only the two content-read bypasses are refused, and only for network origin. The `important`/`critical` tag bypass stays available because _build_tags never derives those from output — verified, not assumed. Unrecognised tools classify UNKNOWN rather than LOCAL_ACTION, so a newly added tool is visibly unclassified instead of silently trusted. ORIGIN_UNKNOWN is permissive so no existing caller's behaviour changes; every untrusted channel reaches the gate through the classification table, never through that default. Also removes an unreachable branch: `if tool_name == "Bash"` sat inside the _CONDITIONAL_TOOLS block while Bash was in _HIGH_VALUE_TOOLS — unreachable in the very commit that introduced both (5eba011), so not a leftover from a move and not an unplugged wire. Its intent (capture Bash without keyword signals) is already implemented by the high-value branch, which is why deleting it loses no behaviour. origin_tool is declared in the input schema and exposed on both registered MCP wrappers, which the #98 parity gate caught and required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…og (#365) The scoped §12 run leaves exactly one survivor: `(tool_name or "")` -> `(tool_name or "XXXX")`. It is equivalent, not a coverage gap — an empty name and an unrecognised name both return ORIGIN_UNKNOWN by construction of the two membership tests, so no test can distinguish them. Rationale recorded at the use site per §12.4, where a reviewer can refute it. Simplified while there: the separate empty-input guard duplicated the fallthrough (an empty key matches neither set and already returns ORIGIN_UNKNOWN), so it is removed rather than left as unreachable-in-effect branching. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#365) The origin decided the gate outcome in flight but never reached the row, so it could not be audited afterwards and no consumer could read it. New memories.capture_origin column on both backends, with a one-shot migration for existing installs (no back-compat shim). The upgrade path is verified old-code-to-new-code against live PostgreSQL, not reasoned about: a database seeded by pre-#365 code (column absent, ddl_hash cfaedf74) then initialised by this code gains the column on the TABLE and on the current_memories VIEW, keeps its pre-existing row, and backfills it to 'unknown'. That view is `SELECT * FROM memories`, whose column list PostgreSQL freezes at creation, so an upgraded database only sees the new column because get_all_ddl already orders MIGRATIONS_DDL before CURRENT_MEMORIES_VIEW_DDL. That ordering is invisible from the fresh-install path and now has a test. Two false verifications caught while testing this, both recorded because the technique matters more than the result: - `psql -c` returns 0 on SQL error without ON_ERROR_STOP, so a DROP COLUMN that failed on a view dependency reported success. The "migration test" it produced proved nothing. - schema init is guarded by a ddl_hash in schema_meta, so mutating the DB out-of-band and re-running init skips all DDL. Only a genuine old-code -> new-code run exercises a migration. capture_origin is queryable by SQL on both backends but deliberately NOT added to recall results: _WRRF_CONTRACT_FIELDS pins the injected-candidate key set to the exact RETURNS TABLE column set of the recall_memories() PL/pgSQL function, and adding the field to the spreading-activation path alone created exactly the divergence that contract exists to prevent (caught by its own test). Carrying it onto recall results requires changing that stored function's signature and belongs with #363, the consumer that needs it. I2's ALLOWED_WRITERS pins heat_base write sites by line number; the six entries shifted +4 and were confirmed byte-identical at their new lines before the registry was updated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed. The core argument is right and well made: a content-derived classifier cannot carry a security property because the attacker supplies its input, so origin has to come from the channel. Coverage is thorough on the intended behaviour: network refused, One design point I would like a decision on before this is load-bearing. The bypass rule is a denylist: _ORIGINS_REFUSED_CONTENT_BYPASS: frozenset[str] = frozenset({ORIGIN_NETWORK})so The concrete trigger is not hypothetical. Inverting to an allowlist makes it fail closed: _ORIGINS_ALLOWED_CONTENT_BYPASS = frozenset({ORIGIN_DELIBERATE, ORIGIN_LOCAL_ACTION})The cost is that callers currently relying on the If you would rather keep the denylist for compatibility, the cheaper mitigation is a drift guard: a test that pins the host's known tool names and fails when the set it was written against changes, so a new web tool is a red test rather than a silent regression. Not blocking — the PR is a clear improvement over the status quo either way, and the choice between "fail closed now, touch every caller" and "denylist plus a drift guard" is yours. Happy to implement whichever. |
c700e3a to
f8c34af
Compare
#375's entry described the denylist it shipped: "network-origin content is refused the two content-derived bypasses" and "unrecognised tools classify as unknown rather than trusted, so a newly added tool is visibly unclassified". Both are now wrong in the same direction — unknown is refused, not merely visible — so the release notes would have contradicted the code. Restated as what it is: an allowlist of {deliberate, local_action}, with the reason a denylist was abandoned (it fails open the moment the host adds or renames a network tool) and the distinction between a tool that was NAMED but is unrecognised and no tool being named at all. docs/mcp-tools.md's write-path description carried the same denylist framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ow-up) (#380) * fix(capture): make the write-gate bypass rule an allowlist (#365 follow-up) Follow-up to #375, per review. The origin control was a denylist: _ORIGINS_REFUSED_CONTENT_BYPASS = frozenset({ORIGIN_NETWORK}) so anything not literally named webfetch or websearch could claim a content-derived bypass. Classification already failed safe — an unrecognised tool lands in UNKNOWN rather than being promoted to LOCAL_ACTION — but the security decision built on it failed open. The trigger is not hypothetical: _NETWORK_TOOLS hardcodes two names against a host whose tool surface changes. A third off-machine tool, or a rename, classifies UNKNOWN and silently regains the bypass — the control stopping with no failing test and no signal, which is the shape of the issue #375 closes. Inverted to an allowlist of {DELIBERATE, LOCAL_ACTION}. An unclassified origin is refused; the cost of a missing classification is a rejected write instead of a trusted one. Two things fell out of it. ORIGIN_DELIBERATE was defined, documented in the vocabulary and produced by NOTHING — dead since it was written. Under an allowlist a direct `remember` would resolve UNKNOWN and lose the content bypass it has always had, so the value now has a producer: a `remember` carrying no producing tool is the user asking directly. The condition is the ABSENCE of a tool name, not an UNKNOWN classification — those differ exactly where it counts, since a named-but- unrecognised tool also classifies UNKNOWN and promoting that would reinstate the fail-open just removed. tests_py/infrastructure/ test_capture_origin_persistence.py caught that in the first draft. The issue #147 ordering (deliberate never novelty-rejected, but a specific content reason still wins over the generic one) broke: a deliberate write's reason degraded from bypass_error to bypass_write_class_deliberate. Such a write bypasses either way, so refusing it the specific label changed no outcome and only destroyed a diagnostic. The origin rule now governs whether content may BUY a bypass, not how an already-granted one is labelled. Seven tests in test_write_gate.py were silently relying on the permissive default and now state which channel produced their content — which is what they were asserting all along: error-shaped content FROM A LOCAL TOOL bypasses, not error-shaped content from anywhere. Verified: pytest -k "gate or capture or remember or origin or write_class" -> 453 passed, 13 subtests passed ruff check / format --check -> clean Refs #365 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: reconcile the #365 changelog and tool docs with the allowlist #375's entry described the denylist it shipped: "network-origin content is refused the two content-derived bypasses" and "unrecognised tools classify as unknown rather than trusted, so a newly added tool is visibly unclassified". Both are now wrong in the same direction — unknown is refused, not merely visible — so the release notes would have contradicted the code. Restated as what it is: an allowlist of {deliberate, local_action}, with the reason a denylist was abandoned (it fails open the moment the host adds or renames a network tool) and the distinction between a tool that was NAMED but is unrecognised and no tool being named at all. docs/mcp-tools.md's write-path description carried the same denylist framing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #365.
Fetched web content could install itself in durable, cross-session memory by shaping itself. Two decisions read attacker-supplied text:
hooks/post_tool_capture._should_capturekeyed WebFetch/WebSearch capture onkw in output.lower()overHIGH_VALUE_PATTERNS— the fetched page decided whether it was persisted.core/write_gate.determine_bypassgrantsbypass_error/bypass_decisionfrom the content itself, so text merely shaped like an error or a decision skipped the novelty REJECT.hooks/session_startthen replays stored memories verbatim into later sessions, so both are writes to durable state made from untrusted input.Why a third concept, not a reuse
core/capture_originresolves origin from the producing tool name — known out-of-band at the hook boundary, unforgeable by the payload. The two neighbours could not carry this:core/provenancegrades reference verifiability by reading the content's references.core/source_monitoringattributes epistemic origin by regex over the content's wording.Both are content-derived, so the attacker supplies the classifier's input. A hostile page dense with file paths and URLs grades
verifiedand classifiesperceived— the most credible value in each vocabulary. A test asserts the new classifier ignores content entirely.Deliberate asymmetries
forceand adeliberatewrite class are out-of-band human signals and stay valid at any origin. Only the two content-read bypasses are refused, and only for network origin.important/criticaltag bypass also stays, because_build_tagsnever derives those from output — verified, not assumed.unknown, neverlocal_action, so a newly added tool is visibly unclassified rather than silently trusted.ORIGIN_UNKNOWNis permissive so no existing caller changes behaviour; every untrusted channel reaches the gate through the classification table, never that default.Persistence
New
memories.capture_originon both backends with a one-shot migration. The upgrade path is verified old-code-to-new-code against live PostgreSQL, not reasoned about: a DB seeded by pre-#365 code (column absent,ddl_hash cfaedf74) then initialised by this code gains the column on the TABLE and on thecurrent_memoriesVIEW, keeps its pre-existing row, and backfills it tounknown.That view is
SELECT * FROM memories, whose column list PostgreSQL freezes at creation, so an upgraded DB only sees the column becauseget_all_ddlordersMIGRATIONS_DDLbefore the view. That ordering is invisible from the fresh-install path and now has a test.Completion Ledger
Test names enumerated from
git diff origin/main...HEAD— not recalled.networktest_network_tools_classify_as_networklocal_actiontest_local_tools_classify_as_local_actionunknown, never a guesstest_unrecognised_or_empty_is_unknown_never_a_guesstest_result_is_always_in_the_vocabularytest_whitespace_and_case_are_normalisedtest_classification_ignores_content_entirelytest_network_origin_may_not_claim_a_content_bypasstest_non_network_origins_maytest_unrecognised_origin_is_permissive_by_designtest_error_shaped_content_bypasses_at_local_origin,test_error_shaped_content_is_refused_at_network_origintest_decision_shaped_content_bypasses_at_local_origin,test_decision_shaped_content_is_refused_at_network_originforcestill wins at network origintest_force_still_wins_at_network_origindeliberatestill winstest_deliberate_write_class_still_wins_at_network_originimportanttag still winstest_important_tag_still_wins_at_network_origintest_default_origin_preserves_pre_change_behaviourtest_same_verdict_regardless_of_keywords,test_capture_decision_does_not_depend_on_the_payloadtest_reason_names_the_tool_not_a_keywordtest_length_floor_still_appliesremembertest_the_hook_reports_the_producing_tool_to_remembertest_hostile_payload_cannot_claim_a_content_bypasstest_hostile_payload_is_stored_labelled_networktest_origin_tool_is_persisted_as_its_origin_classunknowntest_absent_origin_tool_persists_unknownunknowntest_unrecognised_tool_persists_unknown_not_a_guesstest_pg_base_schema_declares_the_column,test_sqlite_base_schema_declares_the_columntest_pg_migration_adds_the_column_for_existing_databases,test_sqlite_migration_list_includes_the_columntest_migrations_run_before_the_view_is_recreated§13.1 checklist
(False, None); capture refusal reason names the tool, not the payloadorigin_toolis a new optional input; no field removedtest_absent_origin_tool_persists_unknownasserts absence of a guessruff check+ruff format --checkcleancapture_origin.py139 lines, stdlib-only, noos/pathlib/infrastructure importsdocs/mcp-tools.mdwrite path + CHANGELOG(tool_name or "")→or "XXXX"is unobservable because an empty and an unrecognised name both returnORIGIN_UNKNOWN. Rationale at the use site, refutableScoped deliberately out
capture_originis queryable by SQL but not added to recall results._WRRF_CONTRACT_FIELDSpins the injected-candidate key set to the exactRETURNS TABLEcolumn set of therecall_memories()PL/pgSQL function; adding the field to the spreading-activation path alone created that divergence and its own contract test caught it. Carrying it onto recall results means changing that stored function's signature and belongs with #363, the consumer that needs it, so change and consumer are tested together.Also fixed en route
if tool_name == "Bash"sat inside the_CONDITIONAL_TOOLSblock whileBashwas in_HIGH_VALUE_TOOLS— unreachable in the commit that introduced both (5eba011), so not a leftover and not an unplugged wire. Its intent is already implemented by the high-value branch, which is why deleting it loses no behaviour.origin_toolexposed on both registered MCP wrappers (the Registered tool signatures drift from handler schemas: remember lacks supersedes_id/write_class, wiki_write lacks memory_ids — promotion loop cannot be closed over MCP #98 parity gate required it).I2'sALLOWED_WRITERSpinsheat_basesites by line number; six entries shifted +4 and were confirmed byte-identical at their new lines before the registry was updated.Two false verifications caught
psql -creturns 0 on SQL error withoutON_ERROR_STOP, so aDROP COLUMNthat failed on a view dependency reported success — the migration test it produced proved nothing.schema_meta.ddl_hash, so mutating the DB out-of-band and re-running init skips all DDL. Only a genuine old-code → new-code run exercises a migration.🤖 Generated with Claude Code