From 4f75279ceb3a70e947776cffceea8a727daccdb6 Mon Sep 17 00:00:00 2001 From: Mathias Date: Wed, 23 Sep 2026 10:40:00 +0200 Subject: [PATCH] fix(mcp): normalize CRLF before frontmatter fence detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review found a fifth bypass, and this one needs no adversarial old_str/new_str crafting at all: frontmatterBlock's fence detection matches the literal byte sequence "---\n". A note whose fence lines use CRLF ("---\r\n") fails strings.HasPrefix(doc, "---\n") on the precondition read purely from line-ending encoding — hadFence is false, so the round-3/round-4 checks can never fire, frontmatterScalars falls back to an empty map, and claim.ValidateEnvelope({}) is nil by design. An invalid claim_provenance value lands with zero error. Zero CRLF-fenced notes exist in the live corpus today, but nothing in the code prevented one, and this class of encoding-dependent false negative could resurface in any of the other fence-based checks. Normalize once inside frontmatterBlock so every caller (fence-presence detection and frontmatterScalars) agrees regardless of line ending. The broader structural concern — three independent, subtly inconsistent fence-parsing implementations across internal/mcp, internal/api, and internal/frontmatter, of which this CRLF gap is a direct symptom — is out of scope for this PR and filed separately as hyperguild#138. Co-Authored-By: Claude Sonnet 5 --- ingestion/internal/mcp/tools_notes.go | 7 ++++++ ingestion/internal/mcp/tools_notes_test.go | 27 ++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/ingestion/internal/mcp/tools_notes.go b/ingestion/internal/mcp/tools_notes.go index 1a743a8..043a217 100644 --- a/ingestion/internal/mcp/tools_notes.go +++ b/ingestion/internal/mcp/tools_notes.go @@ -310,6 +310,13 @@ func checkClaimEnvelope(brainDir, path, oldStr, newStr string) error { // 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) { + // Round 4 found a note with CRLF fence lines ("---\r\n") silently + // defeats HasPrefix(doc, "---\n") on the precondition read — not an + // adversarial old_str/new_str, just an encoding difference — which + // makes checkClaimEnvelope no-op entirely. Normalize once, here, so + // every caller (hadFence/hasFence detection and frontmatterScalars) + // agrees regardless of line ending. + doc = strings.ReplaceAll(doc, "\r\n", "\n") if !strings.HasPrefix(doc, "---\n") { return "", false } diff --git a/ingestion/internal/mcp/tools_notes_test.go b/ingestion/internal/mcp/tools_notes_test.go index c2421a5..d991bf2 100644 --- a/ingestion/internal/mcp/tools_notes_test.go +++ b/ingestion/internal/mcp/tools_notes_test.go @@ -196,6 +196,33 @@ func TestBrainEdit_RejectsAnEditThatRemovesTheClosingFence(t *testing.T) { assert.Contains(t, string(got), "claim_provenance: asserted\n---\n", "rejected edit must leave the note byte-identical, fence intact") } +func TestBrainEdit_RejectsAnInvalidEnvelopeOnACRLFFencedNote(t *testing.T) { + // Found in round-4 review: frontmatterBlock's fence detection matches + // the literal byte sequence "---\n" / "\n---\n". A note whose fence + // lines use CRLF ("---\r\n") fails strings.HasPrefix(doc, "---\n") on + // the BEFORE read, so hadFence is false — not because the fence is + // missing, but because \r sits in front of every \n. hadFence&&!hasFence + // can then never fire, frontmatterScalars falls back to an empty map, + // and claim.ValidateEnvelope({}) is nil by design. No adversarial + // old_str/new_str crafting needed: encoding alone silently disables the + // whole envelope check for this note. Zero CRLF-fenced notes exist in + // the live corpus today, but nothing in the code prevents one. + dir := newBrainRepo(t) + note := filepath.Join(dir, "wiki/homelab/facts/n.md") + require.NoError(t, os.WriteFile(note, + []byte("---\r\ntitle: 'n'\r\nhalf_life: durable\r\napplies: universal\r\nclaim_provenance: asserted\r\n---\r\n\r\nbody\r\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")