fix(mcp): drop the containment gate, always validate the post-edit envelope
Round-2 review found a third bypass: checkClaimEnvelope required old_str to be fully contained within the extracted frontmatter block (which excludes both "---" fence lines). An old_str disambiguated across the closing fence into the body — a natural choice, since notes.Edit requires old_str to be unique — failed that containment test, so the gate skipped validation entirely while notes.Edit applied the replace verbatim, reaching the same invalid-enum outcome as the two bypasses already closed. Remove the containment gate structurally instead of patching another edge: checkClaimEnvelope now always computes the post-edit doc and validates its resulting frontmatter, regardless of old_str's textual relationship to the fence. A body-only edit leaves the frontmatter unchanged (still valid, since the note started valid), so this is a no-op for the common case and fail-closed for every case that touches the envelope, however old_str is phrased. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -275,10 +275,15 @@ func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error {
|
|||||||
// — this function's job is the envelope, not read failures.
|
// — this function's job is the envelope, not read failures.
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
block, ok := frontmatterBlock(string(before))
|
// No gate on where old_str falls relative to the frontmatter fence:
|
||||||
if !ok || !strings.Contains(block, oldStr) {
|
// round 2 found that requiring old_str to be fully contained in the
|
||||||
return nil // old_str isn't in the frontmatter block at all
|
// extracted block was itself bypassable by an old_str disambiguated
|
||||||
}
|
// across the closing "---" into the body. Simpler and bypass-proof:
|
||||||
|
// always compute what the edit would produce and validate that
|
||||||
|
// resulting state's envelope. A body-only edit leaves the frontmatter
|
||||||
|
// (and thus the validation outcome) unchanged, so this is a no-op for
|
||||||
|
// the common case and fail-closed for every case that touches the
|
||||||
|
// envelope, regardless of how old_str is phrased.
|
||||||
after := strings.Replace(string(before), oldStr, newStr, 1)
|
after := strings.Replace(string(before), oldStr, newStr, 1)
|
||||||
return claim.ValidateEnvelope(frontmatterScalars(after))
|
return claim.ValidateEnvelope(frontmatterScalars(after))
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -141,6 +141,33 @@ func TestBrainEdit_RejectsAValueOnlyEditThatBreaksTheEnvelope(t *testing.T) {
|
|||||||
assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
|
assert.Contains(t, string(got), "claim_provenance: asserted", "rejected edit must leave the note byte-identical")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestBrainEdit_RejectsAnOldStrThatCrossesTheClosingFence(t *testing.T) {
|
||||||
|
// Found in round-2 review: the position-based gate required old_str to
|
||||||
|
// be fully CONTAINED in the extracted frontmatter block (which excludes
|
||||||
|
// both "---" fence lines). An old_str disambiguated by extending past
|
||||||
|
// the closing fence into the body — a natural choice, since notes.Edit
|
||||||
|
// requires old_str to be unique in the file — fails that containment
|
||||||
|
// test, so checkClaimEnvelope skips validation entirely while
|
||||||
|
// notes.Edit still applies the replace verbatim. Same invalid-enum
|
||||||
|
// outcome as the original two bypasses, reached a third way.
|
||||||
|
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\n---\n\nbody",
|
||||||
|
"new_str": "guessed\n---\n\nbody",
|
||||||
|
})
|
||||||
|
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) {
|
func TestBrainEdit_AllowsAValidClaimEnvelopeEdit(t *testing.T) {
|
||||||
dir := newBrainRepo(t)
|
dir := newBrainRepo(t)
|
||||||
note := filepath.Join(dir, "wiki/homelab/facts/n.md")
|
note := filepath.Join(dir, "wiki/homelab/facts/n.md")
|
||||||
|
|||||||
Reference in New Issue
Block a user