~bigbes/sr-ht-spec

ref: 64cae3af81d4b0039edc8ec3946bed36166a447b sr-ht-spec/mcpsrv/mcpsrv.go -rw-r--r-- 18.1 KiB
824788ab — Eugene Blikh 2 days ago
mcpsrv: mark /mcp uncacheable, fail closed on origin, split tool errors from faults

Three gaps between this surface and the cov/dolt pattern the siblings settled
on. The write tools are untouched, the per-tool grant scheme is untouched, and
the transport stays stateful.

The endpoint set no Cache-Control and no Vary at all. Every answer here depends
entirely on the credential the request carried and says nothing about it in its
URL, and some of them are the whole approved corpus, so a shared cache was free
to keep one and replay it to the next caller. cache.go is a local copy of
ecore/mcphttp.PrivateCache, byte for byte on the header values so the retrofit
is a delete and an import once that commit is published. It commits the headers
on Write and Flush as well as WriteHeader: the SDK answers a POST with an event
stream that never calls WriteHeader, so a wrapper hooking only that one sets
nothing on the response an agent actually gets, while passing every other header
test. Measured — with only WriteHeader hooked the streamed answer leaves with
the SDK's own `no-cache, no-transform` and no Vary.

Vary names both planes, where dolt.sr.ht names Authorization alone. dolt is
right for dolt: its /mcp is bearer-only, so naming Cookie would promise a cache
a dependency the surface never reads. It is wrong here. authn.Resolver.Resolve
prefers a bearer token when one is present but falls through to
login.UsernameFromRequest when none is, and an owner cookie resolves to
KindOwner — which is exactly what Gate admits. On this service the cookie is
the difference between the whole corpus and a 401. This is the one string a
future mcphttp retrofit has to reconcile between the two services.

An origin with no host in it was a warning and then an unguarded endpoint. The
Host allowlist is the only thing protecting /mcp once the SDK's own rebinding
guard is disabled, so that path turned one unparseable config value into a
silently open endpoint indistinguishable in every functional test from a
correctly guarded one. It is a construction error now. The daemon cannot reach
it either way: service.Config.Validate already refuses to start unless the
origin parses and carries a host.

Errors from below travelled to the agent as tool results carrying their own
text, so a dead git object store and a missing document were the same kind of
answer. A tool result means "the call was understood and the thing you asked
for is not there", so an agent reading one for a store outage concludes the
document does not exist and rewrites a specification around a document that is
perfectly real — and the store's own words reached it. errors.go splits the
two: service.ErrNotFound is a tool result whose sentence is built from the
call's own arguments, and everything else is a jsonrpc protocol error saying
"internal server error" with the detail logged. The old tests asserted the
behaviour being removed — that the agent was shown the words "on fire" — and
are replaced by ones that pin the split in both directions.
b643b0be — Eugene Blikh 9 days ago
bearer: refuse through the shared table and challenge
be33cce1 — Eugene Blikh 9 days ago
instconf: one reading of this instance's origins
c7477607 — Eugene Blikh 10 days ago
authn: accept tokens.sr.ht working tokens beside the agent token

A second agent credential plane, next to the existing one rather than in
place of it. The agent_token table, every agent configured with it, and
the refs rule and provenance requirement around it are untouched; the
local plane is removed in a later phase, not this one.

The resolver tries the instance plane first and falls back to the local
store on exactly two refusals, bearer.ErrInvalid and bearer.ErrNotOurs.
spec's local token has no prefix to discriminate on — it is 32 random
bytes in base64, which is precisely what "did not decode as one of ours"
looks like — so the fallback replaces the shape test bench and cover can
afford. ErrRevoked, ErrForbidden and ErrUnavailable are terminal: a
withdrawn credential must not get a second chance at the old door, and an
unreachable daemon must not silently degrade into the legacy plane.

Grants ride on the principal and are checked where the action is known,
never in the middleware, which runs upstream of the router: spec:propose
in service.Propose, below both write surfaces, and spec:read in each read
surface's gate. /mcp checks per tool rather than at its Gate, because one
endpoint carries both kinds and a surface-wide read grant would refuse a
propose-only token at initialize. Principal.Authorize is a no-op off the
instance plane, which is what keeps the local token working.

The instance plane brings an owner where the local token had none, so a
working token belonging to anybody but [sr.ht] owner-name is refused
rather than admitted as a second identity: Principal.Owner is read by the
provenance committer, the refs rule's principal kind and the coreauth
AuthContext, all written for one human.

StatusFor is the one status table. ErrUnavailable is 503 and never 401 —
reading "I could not ask tokens.sr.ht" as "revoked" would refuse every
live instance token while a daemon that is deliberately off the hot path
restarts.

An instance with no [tokens.sr.ht] section builds no instance plane and
starts anyway, serving its own agent token as before.
f82d90a4 — Eugene Blikh 24 days ago
feat(mcpsrv): spec_comment closes the agent half of the review loop (spec-by6.3.4)

An agent can now read the review threads on a proposal and reply to them. It
cannot open a thread or resolve one, and that is enforced by the type rather
than by the handler remembering: spec_comment is written against a narrow
Commenter interface naming only Threads, ReplyTo, GetProposal and ProposalDiff,
so service.CommentOn and service.ResolveThread are unreachable from it however
service/ later grows. An unresolved thread suppresses policy auto-merge, so an
agent able to open or resolve one would hold the gate that exists to hold its
own output back. Writer is now the union of Proposer and Commenter, one narrow
interface per write tool, and each handler takes only its own half.

Every listed thread carries its anchor state, resolved against the branch as it
stands now rather than as it stood when the comment was written — often the
same agent has revised it since. An agent told only "fix this paragraph", with
no signal that the critique no longer describes any block, edits the wrong
thing. The tool description spells out what anchored/edited/outdated mean and
says plainly that replying does not close a thread, so an agent answers the
critique and pushes a revision instead of replying and waiting.

Replying requires the proposal as well as the thread. A thread id is a global
integer and service.ReplyTo needs nothing else, so a mistyped id would post a
reply onto a stranger's proposal, out of sight of the agent that wrote it; the
membership check reuses the threads already read for the ACL and costs nothing.

Reads stay on the uniform owner+agents gate rather than being narrowed to the
proposal an agent authored. Agent identity is self-declared in X-Agent headers
and all agents share one token, so an authorship check would constrain a string
the caller picks — stricter on paper than the read plane it sits in, and
enforcing nothing.

No wiring change was needed outside this package: main.go already passes
Write: svc, and *service.Service satisfies the widened Writer.

spec-by6.3.4
a0fa81b3 — Eugene Blikh 24 days ago
refactor(authn): one Principal.CanRead() for the read-plane ACL (spec-ejq.1)

graph's gate, web's mayRead and mcpsrv's Gate each hand-spelled
'IsOwner() || IsAgent()' — three copies of the read ACL, which graph's own
comment warned is how a corpus leaks when they drift. Define it once as
authn.Principal.CanRead and route all three through it.

coreauth.Derive keeps its own owner||agent test on purpose: it answers a
different question (is this an owner-backed identity to bridge to
AUTH_INTERNAL), and coupling it to the read ACL would misroute a future
read-only viewer kind to the owner's UserID.

Closes spec-ejq.1
7cc652d5 — Eugene Blikh 24 days ago
fix(mcpsrv): gate read tools to owner+agents (spec-jjo)

The MCP read tools (spec_search/spec_read/spec_list) enforced no ACL:
anyone clearing the Host allowlist could read approved content. graph's
/query and the web UI gate reads to owner+agents; /mcp did not. Add the
same fail-closed gate to the whole MCP surface — initialize, tools/list
and tools/call — mounted inside the resolver middleware so it sees the
resolved principal. spec_propose was already fail-closed in service.Propose;
this makes the read tools match.

Pre-existing since Phase 2, cheap now that /mcp resolves a principal.

Closes spec-jjo
42d4ee13 — Eugene Blikh 26 days ago
feat(mcpsrv): spec_propose write tool (Phase 3)

The agent-facing half of the write plane over MCP. spec_propose uploads
whole documents and returns {proposal, url, merged}, calling the same
service.Propose the REST PUT will — one implementation of If-Match,
provenance and auto-merge behind both surfaces, never two that drift.

- Writer is an optional Backend field: nil keeps the server read-only
  (the three read tools, unchanged), so a read-only deploy or a test
  needs no mutable backend. Set, it registers spec_propose with neither
  the read-only nor the idempotent hint — proposing twice opens two
  proposals.
- The acting agent is resolved from the bearer token on the tool call by
  the resolver middleware now installed on /mcp; the read tools never
  needed it. The ACL stays in service/: service.Propose refuses a
  non-agent, so an anonymous or owner caller is rejected there.
8edca94e — bigbes 27 days ago
feat(mcpsrv): MCP read tools, with Host validation replacing the SDK guard

Adds the Phase 2 read tools (spec_search, spec_read, spec_list) over the
service layer.

Two security fixes came out of building them.

The read plane could serve proposal content. gitx resolves ref names, and
service.resolveRev passed any string through, so rev=proposals/42 made the
READ plane hand back unreviewed text — which would then flow into agent
context as though approved, the single failure this service exists to
prevent. ValidateReadRev now admits only the approved-head sentinel or a
full 40-character object name, at the layer all three surfaces share.
Abbreviations are refused too: one that is unique today can become
ambiguous later, so a pinned revision would silently stop meaning one
thing.

The MCP SDK's DNS-rebinding guard rejects a loopback listener whose Host
is not loopback, which is exactly nginx forwarding to 127.0.0.1 — it would
403 only in production, passing every local test. The SDK offers no
allowlist, so the guard is disabled and replaced by a stricter check: Host
must equal the configured origin, or a loopback name for development. A
rebinding attack carries the attacker's name in Host and fails it. An
unusable origin logs loudly rather than quietly unguarding the endpoint.