mathias/hyperguild
1
0
Fork 0
Code Issues 30 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

UpdateNote: supersede_reason settable via caller's own frontmatter fragment, bypassing the Reason option #125

New Issue
Closed
opened 2026-09-22 21:28:34 +00:00 by mathias · 1 comment
mathias commented 2026-09-22 21:28:34 +00:00
Owner
Copy Link
Copy Source

Non-blocking observation from #111's review (PR #124, merge commit 5a5d938e).

UpdateNote's merge loop (ingestion/internal/api/update.go) applies every field from the caller's leading frontmatter fragment (contentFM) onto fm, then unconditionally re-sets updated_at/supersedes but only sets supersede_reason when opts.Reason != "". So a caller can set supersede_reason directly via its own fragment (---\nsupersede_reason: whatever\n---\n...) even when the Reason option is empty, bypassing the intended single entry point for that field.

Judged non-blocking at review time: it's the same actor able to set the same field two ways, not a cross-provenance leak (unlike the original #111 bug, where a different, earlier author's envelope data could survive unvalidated). No confirmed exploit, just an inconsistency worth deciding on deliberately rather than leaving implicit.

Ask

Decide whether the merge loop's field set should be constrained (e.g. skip updated_at/supersedes/supersede_reason if present in the caller's fragment, since those three are meant to be server-derived, not caller-set) — or whether this is fine as-is and the fragment is intentionally a general-purpose "set anything" mechanism now that #111 validates its envelope subset.

Low priority — speculative severity, not a confirmed defect.

Created via git-mcp on behalf of @mathias

Non-blocking observation from #111's review (PR #124, merge commit 5a5d938e). `UpdateNote`'s merge loop (`ingestion/internal/api/update.go`) applies every field from the caller's leading frontmatter fragment (`contentFM`) onto `fm`, then unconditionally re-sets `updated_at`/`supersedes` but only sets `supersede_reason` when `opts.Reason != ""`. So a caller can set `supersede_reason` directly via its own fragment (`---\nsupersede_reason: whatever\n---\n...`) even when the `Reason` option is empty, bypassing the intended single entry point for that field. Judged non-blocking at review time: it's the same actor able to set the same field two ways, not a cross-provenance leak (unlike the original #111 bug, where a *different, earlier* author's envelope data could survive unvalidated). No confirmed exploit, just an inconsistency worth deciding on deliberately rather than leaving implicit. ## Ask Decide whether the merge loop's field set should be constrained (e.g. skip `updated_at`/`supersedes`/`supersede_reason` if present in the caller's fragment, since those three are meant to be server-derived, not caller-set) — or whether this is fine as-is and the fragment is intentionally a general-purpose "set anything" mechanism now that #111 validates its envelope subset. Low priority — speculative severity, not a confirmed defect. --- _Created via git-mcp on behalf of @mathias_
mathias commented 2026-09-23 08:18:51 +00:00
Author
Owner
Copy Link
Copy Source

Merged PR #137 (squash) as merge commit a5f502eb9c.

Review evidence:

  • Diff: ingestion/internal/api/update.go's merge loop now skips l.key in {"", "updated_at", "supersedes", "supersede_reason"} instead of only "". Two new tests in update_test.go (absence assertion for the leak, regression pin for the legit opts.Reason path).
  • Flip-tested independently: reverted the skip condition to l.key == "" in a scratch worktree -> TestUpdateNoteSupersedeReasonNotSettableViaCallerFragmentWhenReasonOptionEmpty went red with the leaked supersede_reason: snuck in via the fragment visible in the note body. Restored the fix -> both new tests green.
  • Checked for legitimate callers relying on the old fragment-sets-reserved-fields behavior: only caller of UpdateNote is brainstore.Store.Update, which passes Reason via capture.Note.Reason (the proper option), never via the content fragment. No MCP tool description or handler sets these fields through the fragment either. This is a pure gap-close, no behavior change for any real caller.
  • Confirmed no interaction with #111's claim-envelope validation: claim.ValidateEnvelope runs on the full merged frontmatter (half_life/applies/claim_provenance), none of which are in the new skip list.
  • Full module suite from ingestion/: go build ./..., go vet ./..., go test ./... -race -count=1 all clean at head SHA fb69f4bf3d.
  • CI green at that SHA: run 7906, "CI / Lint / Test / Vet" success.

Closing.

Merged PR #137 (squash) as merge commit a5f502eb9c4bbe13b6720ccaf930d41f265a2d16. Review evidence: - Diff: `ingestion/internal/api/update.go`'s merge loop now skips `l.key` in `{"", "updated_at", "supersedes", "supersede_reason"}` instead of only `""`. Two new tests in `update_test.go` (absence assertion for the leak, regression pin for the legit `opts.Reason` path). - Flip-tested independently: reverted the skip condition to `l.key == ""` in a scratch worktree -> `TestUpdateNoteSupersedeReasonNotSettableViaCallerFragmentWhenReasonOptionEmpty` went red with the leaked `supersede_reason: snuck in via the fragment` visible in the note body. Restored the fix -> both new tests green. - Checked for legitimate callers relying on the old fragment-sets-reserved-fields behavior: only caller of `UpdateNote` is `brainstore.Store.Update`, which passes `Reason` via `capture.Note.Reason` (the proper option), never via the content fragment. No MCP tool description or handler sets these fields through the fragment either. This is a pure gap-close, no behavior change for any real caller. - Confirmed no interaction with #111's claim-envelope validation: `claim.ValidateEnvelope` runs on the full merged frontmatter (`half_life`/`applies`/`claim_provenance`), none of which are in the new skip list. - Full module suite from `ingestion/`: `go build ./...`, `go vet ./...`, `go test ./... -race -count=1` all clean at head SHA fb69f4bf3d46c164b9dd326c60d359f1a2beebc4. - CI green at that SHA: run 7906, "CI / Lint / Test / Vet" success. Closing.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agent/ready
Meets the agent-ready contract; authorizes unattended execution
 blocked/client
Waiting on the client
 blocked/supplier
Waiting on a supplier
 blocked/upstream
Waiting on an upstream project or vendor
 risk/high
Irreversible, security-relevant, or control plane
 risk/low
Reversible, local, test-covered
 risk/medium
Touches shared state, deploys or data
 size/l
Too big - split it
 size/m
Under two days
 size/s
Under half a day
 size/xs
Under an hour
 status:dispatch
status:dispatched
status:review
status:reviewing
type/chore
Maintenance, no behaviour change
 type/decision
Human decision required; never agent/ready
 type/docs
Documentation only
 type/feat
New capability
 type/fix
Corrects broken behaviour
 type/ops
Infrastructure, deploy, runtime
 type/research
Investigate; output is a finding, not code
 type/test
Test coverage only
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate-bot mathias
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mathias/hyperguild#125