fix(mcp): close claim-envelope bypass on value-only brain_edit diffs
CI / Lint / Test / Vet (pull_request) Successful in 33s
CI / Mirror to GitHub (pull_request) Skipped

Review of PR #136 found the gate trivially bypassable: touchesClaimEnvelope
only fired when old_str/new_str contained the claim-envelope FIELD NAME as a
substring, but a natural value-only diff (old_str: "asserted", new_str:
"guessed" against a claim_provenance line) never mentions the field name and
walked straight past it — notes.Edit wrote the invalid enum value to disk
with zero error.

Redesign the gate to be position-based instead of name-based: checkClaimEnvelope
now checks whether old_str is physically contained within the note's
frontmatter block (via the new frontmatterBlock helper), previews the exact
strings.Replace the real edit would perform, and validates the resulting
envelope before the edit is allowed to run. frontmatterScalars now builds on
the shared frontmatterBlock helper.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-09-23 10:21:53 +02:00
co-authored by Claude Sonnet 5
parent f7fe00cecf
commit 062574dad8
2 changed files with 63 additions and 26 deletions
+38 -26
View File
@@ -187,10 +187,9 @@ func (s *Server) editWithScope(args json.RawMessage, scope notes.Scope) (notes.R
// replace has no frontmatter awareness at all, so it could rewrite an
// existing half_life:/applies:/claim_provenance: line straight past the
// validator entirely. Scoped to notes only (governance files have no
// claim-envelope concept) and gated on a cheap substring check so an
// edit that never touches these field names pays nothing extra.
if scope == notes.ScopeNotes && touchesClaimEnvelope(a.OldStr, a.NewStr) {
if verr := validateEnvelopeAfterEdit(s.brainDir, a.Path, a.OldStr, a.NewStr); verr != nil {
// claim-envelope concept).
if scope == notes.ScopeNotes {
if verr := checkClaimEnvelope(s.brainDir, a.Path, a.OldStr, a.NewStr); verr != nil {
return notes.Result{}, nil, fmt.Errorf("claim envelope: %w", verr)
}
}
@@ -245,42 +244,59 @@ func touchesFrontmatter(oldStr, newStr string) bool {
return false
}
// touchesClaimEnvelope reports whether oldStr/newStr mention any of the
// three claim-envelope field names — the cheap gate that lets an ordinary
// body edit skip the read-and-validate round trip entirely.
func touchesClaimEnvelope(oldStr, newStr string) bool {
for _, s := range []string{oldStr, newStr} {
if strings.Contains(s, claim.FieldHalfLife) || strings.Contains(s, claim.FieldApplies) || strings.Contains(s, claim.FieldProvenance) {
return true
}
}
return false
}
// validateEnvelopeAfterEdit previews what oldStr->newStr would produce —
// the exact strings.Replace(before, oldStr, newStr, 1) formula notes.Edit
// checkClaimEnvelope previews what old_str->new_str would produce — the
// exact strings.Replace(before, oldStr, newStr, 1) formula notes.Edit
// itself uses internally — and validates the resulting frontmatter's claim
// envelope before the real edit is allowed to run. Fail-closed: an invalid
// result blocks the edit entirely, the same posture writeHallNote/UpdateNote
// already apply on create/supersede.
//
// Gated on old_str appearing within the note's frontmatter BLOCK, not on
// old_str/new_str mentioning a claim-envelope field NAME — found in review:
// a field-name-substring gate misses a value-only edit like
// "asserted"->"guessed" on a claim_provenance line, which never mentions
// the field name at all. That is exactly the shape of edit a minimal,
// natural diff produces, so a field-name gate was closing the issue in
// name only. Checking block containment instead means any edit that
// couldn't possibly touch the frontmatter (its target text only exists in
// the body) is still skipped cheaply, while nothing inside the fence can
// slip through unvalidated regardless of which token it targets.
//
// This does not replicate notes.Edit's uniqueness/CRLF/conflict-marker
// checks — if old_str is absent or ambiguous, this preview may not match
// what Edit ultimately does, but Edit independently and correctly rejects
// those cases at write time regardless of what this function decides, so
// the only thing riding on this preview's accuracy is the envelope check
// itself, not write safety.
func validateEnvelopeAfterEdit(brainDir, path, oldStr, newStr string) error {
func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error {
before, err := notes.Read(brainDir, path)
if err != nil {
// Let notes.Edit itself produce the real error (missing file, etc.)
// — this function's job is the envelope, not read failures.
return nil
}
block, ok := frontmatterBlock(string(before))
if !ok || !strings.Contains(block, oldStr) {
return nil // old_str isn't in the frontmatter block at all
}
after := strings.Replace(string(before), oldStr, newStr, 1)
return claim.ValidateEnvelope(frontmatterScalars(after))
}
// frontmatterBlock returns the raw text between doc's opening and closing
// `---` fences, or ok=false if doc has no (properly closed) fence.
func frontmatterBlock(doc string) (block string, ok bool) {
if !strings.HasPrefix(doc, "---\n") {
return "", false
}
rest := doc[len("---\n"):]
end := strings.Index(rest, "\n---\n")
if end < 0 {
return "", false
}
return rest[:end], true
}
// frontmatterScalars extracts top-level "key: value" scalar pairs from
// doc's leading fence block, for the claim-envelope check only — not a
// general frontmatter parser. Indented lines (YAML block-list items,
@@ -288,15 +304,11 @@ func validateEnvelopeAfterEdit(brainDir, path, oldStr, newStr string) error {
// hyperguild#126/#128 in the sibling packages this mirrors.
func frontmatterScalars(doc string) map[string]string {
out := map[string]string{}
if !strings.HasPrefix(doc, "---\n") {
block, ok := frontmatterBlock(doc)
if !ok {
return out
}
rest := doc[len("---\n"):]
end := strings.Index(rest, "\n---\n")
if end < 0 {
return out
}
for _, line := range strings.Split(rest[:end], "\n") {
for _, line := range strings.Split(block, "\n") {
if line != "" && (line[0] == ' ' || line[0] == '\t') {
continue
}
@@ -116,6 +116,31 @@ func TestBrainEdit_RejectsAnInvalidClaimEnvelopeWrittenViaStringReplace(t *testi
assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
}
func TestBrainEdit_RejectsAValueOnlyEditThatBreaksTheEnvelope(t *testing.T) {
// Found in review: the original gate (touchesClaimEnvelope) checked
// whether old_str/new_str mention the FIELD NAME — but a minimal diff
// (the natural shape of any real edit, model-generated or human) can
// target just the VALUE token and never mention the field name at all.
// "asserted" -> "guessed" on a claim_provenance line walked straight
// past the field-name substring check and wrote an invalid enum value
// with no error — the exact bypass #123 was supposed to close, closed
// in name only.
dir := newBrainRepo(t)
note := filepath.Join(dir, "wiki/homelab/facts/n.md")
require.NoError(t, os.WriteFile(note,
[]byte("---\ntitle: 'n'\nhalf_life: durable\napplies: universal\nclaim_provenance: asserted\n---\n\nbody\n"), 0o644))
srv := mcp.NewServer(dir, nil, nil, nil)
resp := call(t, srv, "brain_edit", map[string]any{
"path": "wiki/homelab/facts/n.md", "old_str": "asserted", "new_str": "guessed",
})
require.Contains(t, resp, "error", "%v", resp)
got, err := os.ReadFile(note)
require.NoError(t, err)
assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
}
func TestBrainEdit_AllowsAValidClaimEnvelopeEdit(t *testing.T) {
dir := newBrainRepo(t)
note := filepath.Join(dir, "wiki/homelab/facts/n.md")