diff --git a/AGENTS.md b/AGENTS.md index f32f810..8b245b0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,13 +81,27 @@ store.Repository**. - `resolver.go` - inheritance merge over `extends`/`excludes`. Later parents win, the child's own slots win last, cycles produce `CycleError`, and an unreadable ancestor propagates `ErrParentUnreadable` rather than silently dropping content. + - `service.go` - `publish` is the single write path behind both front doors. It authorizes before + parsing caller-supplied content, enforces `MaxContentBytes` on the stored document, refuses a + version that does not advance `latest_version`, and refuses a write whose declared base version + is no longer current. The Connect RPC stores the author's exact bytes; `PublishDoc` marshals the + document instead, so comments and formatting survive a CLI or web publish. - `backend/internal/store` defines `Repository` plus an in-memory implementation used by tests; `store/sqlite` is the real one, with goose migrations in `store/sqlite/migrations`. Publishes go through `CreateFrameVersion`, which inserts the frame row, version, inheritance edges, and grants atomically. **SQLite is single-writer, so the deployment is pinned to one replica.** - `backend/internal/mcp` is a thin protocol adapter over `frames.Service`, exposing frames as MCP resources under `nebari-frame:///[@]` plus RFC 9728 metadata at - `/.well-known/oauth-protected-resource`. URI parsing rejects anything structurally off (extra path + `/.well-known/oauth-protected-resource`. It is also a write surface: `create_frame` and + `update_frame` go through `frames.Service.PublishDocFrom`, the same RBAC-enforcing path the + Connect API uses, so the adapter itself performs no permission or validation logic. Two rules + matter when changing it. `update_frame` merges onto the frame's own document from `SourceDoc` and + never onto the composed form `get_frame` returns by default - merging onto a resolved document + would copy every parent's slots into the child and drop its `extends` edges. And the base version + it asserts against comes from the caller (`base_version`, read via `get_frame source=true`), never + from a fresh server-side read, which would always match and make the check inert. Request bodies + are capped at `mcp.MaxRequestBytes`; the cap must wrap the outermost handler, since the bearer + middleware is only installed when auth is on. URI parsing rejects anything structurally off (extra path segments, `.`/`..`) instead of misrouting it. - `web/` is the SPA. `web/embed.go` embeds `web/dist` into the Go binary and serves it with an index.html fallback and a CSP assembled from the OIDC issuer origin and branded image origins. @@ -107,6 +121,11 @@ store.Repository**. the git tag (`version` = tag without `v`, `appVersion` = the literal tag). - The SPA ships inside the image, not the chart. A frontend change reaches a cluster only through a new image tag. +- A test that asserts only "this was rejected" usually proves nothing: an unrelated 401, or a parse + failure, satisfies it just as well. Assert the specific code or message, and pair a rejection with + a control that must succeed. Reflective guards in `backend/internal/mcp/resources_test.go` walk + `frames.SlotTable` and `frames.Doc`, so adding a slot without wiring it through the MCP input + fails rather than silently dropping data. - Comments in this repo explain *why* a constraint exists (pinned CI versions, fail-closed readiness, the vite `@bufbuild/protobuf` aliases). Preserve that rationale when editing near it, and keep new comments in the same register. diff --git a/backend/internal/frames/service.go b/backend/internal/frames/service.go index dc1f242..9bd62b9 100644 --- a/backend/internal/frames/service.go +++ b/backend/internal/frames/service.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "errors" "fmt" + "strconv" "strings" "connectrpc.com/connect" @@ -68,32 +69,127 @@ func (s *Service) GetMe(ctx context.Context, _ *connect.Request[framesv1.GetMeRe }), nil } +// MaxContentBytes caps a single Frame version's stored document. The limit is +// per-version and applies to the canonical stored bytes, not to the resolved +// form: a Frame that inherits heavily can still compose to more than this. +const MaxContentBytes = 512 * 1024 + +// PublishIntent tells PublishDoc whether the caller means to create a new +// frame, update an existing one, or either. It exists so the create/update +// distinction is enforced next to the RBAC checks rather than by each caller: +// the MCP tools rely on it, and the Connect RPC keeps its upsert behavior. +type PublishIntent int + +const ( + // PublishUpsert creates the frame when absent and updates it when present. + PublishUpsert PublishIntent = iota + // PublishCreate requires the frame not to exist yet. + PublishCreate + // PublishUpdate requires the frame to already exist. + PublishUpdate +) + func (s *Service) PublishFrame(ctx context.Context, req *connect.Request[framesv1.PublishFrameRequest]) (*connect.Response[framesv1.PublishFrameResponse], error) { - caller, err := s.resolveCaller(ctx) + // Authorization stays ahead of parsing, as it was before this path was + // split: parsing is the expensive, caller-controlled step, and someone who + // may not publish should never reach it. + caller, err := s.authorizePublish(ctx) if err != nil { return nil, err } + doc, err := Parse(req.Msg.Content) + if err != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, err) + } + // The submitted bytes are stored verbatim rather than re-marshalled from + // doc: an author's comments and formatting survive, and the digest stays + // stable for a document that did not change. + frame, version, err := s.publish(ctx, caller, doc, req.Msg.Content, req.Msg.Changelog, PublishUpsert, "") + if err != nil { + return nil, err + } + return connect.NewResponse(&framesv1.PublishFrameResponse{Frame: frame, Version: version}), nil +} + +// authorizePublish resolves the caller and checks the role required to publish +// at all. Both front doors call it before touching caller-supplied content. +func (s *Service) authorizePublish(ctx context.Context) (rbac.Caller, error) { + caller, err := s.resolveCaller(ctx) + if err != nil { + return rbac.Caller{}, err + } if !rbac.CanPublish(caller) { - return nil, connect.NewError(connect.CodePermissionDenied, errors.New("publisher or admin role required")) + return rbac.Caller{}, connect.NewError(connect.CodePermissionDenied, errors.New("publisher or admin role required")) } + return caller, nil +} - doc, err := Parse(req.Msg.Content) +// PublishDoc validates and publishes doc as a new version, enforcing RBAC: +// creating a frame needs the publisher or admin role, and writing to an +// existing frame needs edit permission on it. It is the single write path +// shared by the Connect RPC and the MCP tools, so neither can drift from the +// other or skip a check. +// +// Errors are connect errors so both front doors can map them without +// translation: PermissionDenied, InvalidArgument (with field violations), +// AlreadyExists, NotFound, Internal. +func (s *Service) PublishDoc(ctx context.Context, doc *Doc, changelog string, intent PublishIntent) (*framesv1.Frame, *framesv1.FrameVersion, error) { + return s.PublishDocFrom(ctx, doc, changelog, intent, "") +} + +// PublishDocFrom is PublishDoc with a concurrency check. baseVersion is the +// version the caller read before composing doc; the publish is rejected with +// CodeFailedPrecondition when the frame has moved on since. An empty +// baseVersion means the caller did not check, which is the unguarded behaviour +// the Connect RPC has always had. +// +// A read-modify-write without this check silently loses one of two concurrent +// updates: both merge onto the same base, both pick different version strings +// so nothing collides, and both report success. +func (s *Service) PublishDocFrom(ctx context.Context, doc *Doc, changelog string, intent PublishIntent, baseVersion string) (*framesv1.Frame, *framesv1.FrameVersion, error) { + caller, err := s.authorizePublish(ctx) if err != nil { - return nil, connect.NewError(connect.CodeInvalidArgument, err) + return nil, nil, err + } + content, err := Marshal(doc) + if err != nil { + return nil, nil, connect.NewError(connect.CodeInternal, err) } + return s.publish(ctx, caller, doc, content, changelog, intent, baseVersion) +} + +// publish is the shared implementation, called only with a caller that +// authorizePublish has already cleared. content is the canonical stored form of +// doc; callers holding the author's original bytes pass those so they are not +// normalized away. +func (s *Service) publish(ctx context.Context, caller rbac.Caller, doc *Doc, content []byte, changelog string, intent PublishIntent, baseVersion string) (*framesv1.Frame, *framesv1.FrameVersion, error) { if verr := Validate(doc); verr != nil { - return nil, violationErr(verr) + return nil, nil, violationErr(verr) + } + // Enforced here rather than at either entry point so the Connect API and the + // MCP tools share one limit. It matters more now that an LLM can author a + // Frame: the content lands verbatim in a single-writer SQLite database and is + // re-read on every read and every child's inheritance walk. + if len(content) > MaxContentBytes { + return nil, nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf( + "frame content is %d bytes, over the %d byte limit", len(content), MaxContentBytes)) } org, err := s.repo.GetOrgByID(ctx, caller.OrgID) if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, nil, connect.NewError(connect.CodeInternal, err) } existing, err := s.repo.GetFrameBySlugName(ctx, org.Slug, doc.Name) isNew := errors.Is(err, store.ErrNotFound) if err != nil && !isNew { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, nil, connect.NewError(connect.CodeInternal, err) + } + switch { + case isNew && intent == PublishUpdate: + return nil, nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("no frame named %q to update", doc.Name)) + case !isNew && intent == PublishCreate: + return nil, nil, connect.NewError(connect.CodeAlreadyExists, fmt.Errorf("a frame named %q already exists; update it instead", doc.Name)) } now := timestamppb.Now() @@ -107,10 +203,31 @@ func (s *Service) PublishFrame(ctx context.Context, req *connect.Request[framesv // editing an existing frame requires edit permission allowed, err := rbac.Can(ctx, s.lookup, caller, existing.OrgId, existing.Id, rbac.PermEdit) if err != nil { - return nil, connect.NewError(connect.CodeInternal, err) + return nil, nil, connect.NewError(connect.CodeInternal, err) } if !allowed { - return nil, connect.NewError(connect.CodePermissionDenied, errors.New("edit permission required")) + return nil, nil, connect.NewError(connect.CodePermissionDenied, errors.New("edit permission required")) + } + if baseVersion != "" && existing.LatestVersion != baseVersion { + return nil, nil, connect.NewError(connect.CodeFailedPrecondition, fmt.Errorf( + "frame %q has moved on: you based this change on %s but the latest is %s; re-read it and apply your change again", + doc.Name, baseVersion, existing.LatestVersion)) + } + // latest_version must only move forward. Otherwise publishing an older + // version silently unpublishes newer content: every default read, and + // the merge base of the next update, resolves to whatever is "latest". + // Strictly below only: republishing the current version is a duplicate, + // which CreateFrameVersion reports as AlreadyExists - a more precise + // answer than "does not advance". + if cmp, ok := compareVersions(doc.Version, existing.LatestVersion); ok && cmp < 0 { + // Reported as a field violation on `version`, so the web form marks + // the offending field rather than showing a form-level error, and + // the CLI names the field too. + return nil, nil, violationErr(&ValidationError{Errors: []FieldError{{ + Path: "version", + Message: fmt.Sprintf("must be higher than the current version %s", + existing.LatestVersion), + }}}) } frame = existing frame.Description = doc.Description @@ -120,18 +237,18 @@ func (s *Service) PublishFrame(ctx context.Context, req *connect.Request[framesv edges, err := s.resolveEdges(ctx, caller, org.Slug, doc.Extends) if err != nil { - return nil, err + return nil, nil, err } excludeIDs, err := s.resolveExcludes(ctx, caller, org.Slug, doc.Excludes) if err != nil { - return nil, err + return nil, nil, err } - digest := sha256.Sum256(req.Msg.Content) + digest := sha256.Sum256(content) version := &framesv1.FrameVersion{ - Version: doc.Version, Changelog: req.Msg.Changelog, Digest: hex.EncodeToString(digest[:]), - SizeBytes: int64(len(req.Msg.Content)), PublishedBy: caller.Subject, PublishedAt: now, - Content: req.Msg.Content, + Version: doc.Version, Changelog: changelog, Digest: hex.EncodeToString(digest[:]), + SizeBytes: int64(len(content)), PublishedBy: caller.Subject, PublishedAt: now, + Content: content, } in := store.CreateFrameVersionInput{ @@ -145,11 +262,11 @@ func (s *Service) PublishFrame(ctx context.Context, req *connect.Request[framesv } if err := s.repo.CreateFrameVersion(ctx, in); err != nil { if errors.Is(err, store.ErrAlreadyExists) { - return nil, connect.NewError(connect.CodeAlreadyExists, errors.New("frame version already exists")) + return nil, nil, connect.NewError(connect.CodeAlreadyExists, errors.New("frame version already exists")) } - return nil, connect.NewError(connect.CodeInternal, err) + return nil, nil, connect.NewError(connect.CodeInternal, err) } - return connect.NewResponse(&framesv1.PublishFrameResponse{Frame: frame, Version: version}), nil + return frame, version, nil } // readableFramesInOrg resolves the caller and returns the frames in their org @@ -302,6 +419,31 @@ func (s *Service) ListReadable(ctx context.Context) ([]ReadableFrame, error) { return out, nil } +// SourceDoc returns a frame's own stored document, scoped to the caller's org +// and read-enforced (a denied or missing read is CodeNotFound, no existence +// leak). Unlike ResolveDoc it does NOT merge ancestors, which is what makes it +// the safe input to a write: feeding a resolved document back into a publish +// would copy every parent's slots into the child and drop its extends edges. +func (s *Service) SourceDoc(ctx context.Context, name, version string) (*Doc, error) { + caller, err := s.resolveCaller(ctx) + if err != nil { + return nil, err + } + org, err := s.repo.GetOrgByID(ctx, caller.OrgID) + if err != nil { + return nil, connect.NewError(connect.CodeInternal, err) + } + _, v, _, _, err := s.loadForRead(ctx, caller, org.Slug, name, version) + if err != nil { + return nil, err + } + doc, err := Parse(v.Content) + if err != nil { + return nil, connect.NewError(connect.CodeInternal, err) + } + return doc, nil +} + // ResolveDoc returns the inheritance-merged Doc for a frame, read-enforced. // A denied or missing read returns connect.CodeNotFound (no existence leak). func (s *Service) ResolveDoc(ctx context.Context, orgSlug, name, version string) (*Doc, error) { @@ -544,3 +686,43 @@ func (s *Service) ConvertFrame(ctx context.Context, req *connect.Request[framesv errors.New("exactly one of yaml or markdown must be set")) } } + +// compareVersions orders two validated semantic versions, reporting false when +// either cannot be parsed. Validate already enforces the shape, so a false here +// means an unexpected form rather than user error - the caller lets it through +// rather than rejecting a document it cannot reason about. +func compareVersions(a, b string) (int, bool) { + pa, ok := parseVersion(a) + if !ok { + return 0, false + } + pb, ok := parseVersion(b) + if !ok { + return 0, false + } + for i := range pa { + if pa[i] != pb[i] { + if pa[i] < pb[i] { + return -1, true + } + return 1, true + } + } + return 0, true +} + +func parseVersion(v string) ([3]int, bool) { + var out [3]int + parts := strings.Split(v, ".") + if len(parts) != 3 { + return out, false + } + for i, p := range parts { + n, err := strconv.Atoi(p) + if err != nil || n < 0 { + return out, false + } + out[i] = n + } + return out, true +} diff --git a/backend/internal/frames/service_test.go b/backend/internal/frames/service_test.go index 5b65e04..3152d93 100644 --- a/backend/internal/frames/service_test.go +++ b/backend/internal/frames/service_test.go @@ -576,3 +576,452 @@ func TestConvertFrame_RequiresMembership(t *testing.T) { t.Fatal("want error for a caller with no org membership") } } + +// docFor builds a minimal valid Doc for PublishDoc tests. +func docFor(name, version string, rules ...string) *frames.Doc { + return &frames.Doc{ + Name: name, + Description: name + " description", + Version: version, + Slots: frames.Slots{Rules: rules}, + } +} + +func TestService_PublishDoc(t *testing.T) { + tests := []struct { + name string + // role of the calling user in org o1 + role string + // seed publishes brand-voice@1.0.0 as "owner" first + seedExisting bool + docName string + version string + intent frames.PublishIntent + wantCode connect.Code // 0 means success + }{ + { + name: "publisher creates a new frame", role: "publisher", + docName: "brand-voice", version: "1.0.0", intent: frames.PublishCreate, + }, + { + name: "viewer cannot create", role: "viewer", + docName: "brand-voice", version: "1.0.0", intent: frames.PublishCreate, + wantCode: connect.CodePermissionDenied, + }, + { + name: "admin creates a new frame", role: "admin", + docName: "brand-voice", version: "1.0.0", intent: frames.PublishCreate, + }, + { + name: "create refuses a name that already exists", role: "admin", + seedExisting: true, + docName: "brand-voice", version: "2.0.0", intent: frames.PublishCreate, + wantCode: connect.CodeAlreadyExists, + }, + { + name: "update requires the frame to exist", role: "admin", + docName: "brand-voice", version: "1.0.0", intent: frames.PublishUpdate, + wantCode: connect.CodeNotFound, + }, + { + name: "admin updates an existing frame", role: "admin", + seedExisting: true, + docName: "brand-voice", version: "2.0.0", intent: frames.PublishUpdate, + }, + { + name: "upsert creates when absent, preserving the RPC's behavior", role: "publisher", + docName: "brand-voice", version: "1.0.0", intent: frames.PublishUpsert, + }, + { + name: "upsert updates when present", role: "admin", + seedExisting: true, + docName: "brand-voice", version: "2.0.0", intent: frames.PublishUpsert, + }, + { + name: "a republished version is rejected", role: "admin", + seedExisting: true, + docName: "brand-voice", version: "1.0.0", intent: frames.PublishUpdate, + wantCode: connect.CodeAlreadyExists, + }, + { + name: "an invalid document is rejected before any write", role: "admin", + docName: "Not A Valid Name", version: "1.0.0", intent: frames.PublishCreate, + wantCode: connect.CodeInvalidArgument, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "caller", tt.role) + svc := frames.NewService(repo) + + if tt.seedExisting { + ownerCtx := auth.WithClaims(context.Background(), &auth.Claims{Subject: "owner", Email: "owner@x"}) + _ = repo.UpsertMembership(context.Background(), &framesv1.Membership{OrgId: "o1", UserSub: "owner", Role: "publisher"}) + if _, _, err := svc.PublishDoc(ownerCtx, docFor("brand-voice", "1.0.0", "seeded"), "seed", frames.PublishCreate); err != nil { + t.Fatalf("seed publish: %v", err) + } + } + + frame, version, err := svc.PublishDoc(ctx, docFor(tt.docName, tt.version, "a rule"), "changelog", tt.intent) + if tt.wantCode != 0 { + if connect.CodeOf(err) != tt.wantCode { + t.Fatalf("code = %v (err %v), want %v", connect.CodeOf(err), err, tt.wantCode) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if frame.Name != tt.docName { + t.Errorf("frame name = %q, want %q", frame.Name, tt.docName) + } + if version.Version != tt.version { + t.Errorf("version = %q, want %q", version.Version, tt.version) + } + if frame.LatestVersion != tt.version { + t.Errorf("latest version = %q, want %q", frame.LatestVersion, tt.version) + } + }) + } +} + +// A publisher who is not the owner and holds no edit grant must not be able to +// overwrite someone else's frame, whichever intent they pass. +func TestService_PublishDocDoesNotBypassEditPermission(t *testing.T) { + for _, intent := range []frames.PublishIntent{frames.PublishUpsert, frames.PublishUpdate, frames.PublishCreate} { + repo := store.NewMemory() + ownerCtx := seedOrg(t, repo, "owner", "publisher") + svc := frames.NewService(repo) + if _, _, err := svc.PublishDoc(ownerCtx, docFor("brand-voice", "1.0.0", "owned"), "", frames.PublishCreate); err != nil { + t.Fatalf("seed publish: %v", err) + } + + ctx := context.Background() + _ = repo.UpsertMembership(ctx, &framesv1.Membership{OrgId: "o1", UserSub: "other", Role: "publisher"}) + otherCtx := auth.WithClaims(ctx, &auth.Claims{Subject: "other", Email: "other@x"}) + + _, _, err := svc.PublishDoc(otherCtx, docFor("brand-voice", "9.9.9", "hijacked"), "", intent) + if err == nil { + t.Fatalf("intent %v: a non-owner publisher overwrote a frame they cannot edit", intent) + } + code := connect.CodeOf(err) + if code != connect.CodePermissionDenied && code != connect.CodeAlreadyExists { + t.Errorf("intent %v: code = %v, want PermissionDenied or AlreadyExists", intent, code) + } + } +} + +// The Connect path must store the exact bytes the client submitted: normalizing +// them through a Doc round trip would strip comments and change the digest of a +// logically identical document. +func TestService_PublishFramePreservesSubmittedBytes(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + + content := []byte("# a comment the author cares about\n" + sampleFrame) + if _, err := svc.PublishFrame(ctx, connect.NewRequest(&framesv1.PublishFrameRequest{Content: content})); err != nil { + t.Fatalf("publish: %v", err) + } + + resp, err := svc.GetFrame(ctx, connect.NewRequest(&framesv1.GetFrameRequest{OrgSlug: "openteams", Name: "brand-voice"})) + if err != nil { + t.Fatalf("get: %v", err) + } + if !bytes.Equal(resp.Msg.Version.Content, content) { + t.Errorf("stored content was rewritten:\n got: %q\nwant: %q", resp.Msg.Version.Content, content) + } +} + +// Authorization must precede parsing: a caller who may not publish should be +// denied without the server first parsing content they supplied. Malformed YAML +// from a viewer therefore surfaces PermissionDenied, not InvalidArgument. The +// ordering predates the create/update split; this pins it so extracting the +// shared publish path cannot quietly invert it. +func TestService_PublishFrameAuthorizesBeforeParsing(t *testing.T) { + repo := store.NewMemory() + viewerCtx := seedOrg(t, repo, "viewer-user", "viewer") + svc := frames.NewService(repo) + + _, err := svc.PublishFrame(viewerCtx, connect.NewRequest(&framesv1.PublishFrameRequest{ + Content: []byte("this: is: not: valid: yaml: at: all"), + })) + if got := connect.CodeOf(err); got != connect.CodePermissionDenied { + t.Fatalf("code = %v (err %v), want PermissionDenied: parsing ran before the role check", got, err) + } +} + +// SourceDoc returns a frame's own stored document, NOT the inheritance-resolved +// one. A write path that fed a resolved doc back in would flatten the parent's +// content into the child and drop the extends edges, so this distinction is the +// difference between a safe update and silent inheritance loss. +func TestService_SourceDocIsUnresolved(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + + parent := `name: base +description: Base +version: 1.0.0 +slots: + rules: + - from parent +` + child := `name: child +description: Child +version: 1.0.0 +visibility: private +scope: company +maintainer: platform team +extends: + - ref: openteams/base + version: 1.0.0 +slots: + rules: + - from child +` + for _, content := range []string{parent, child} { + if _, err := svc.PublishFrame(ctx, connect.NewRequest(&framesv1.PublishFrameRequest{Content: []byte(content)})); err != nil { + t.Fatalf("publish: %v", err) + } + } + + src, err := svc.SourceDoc(ctx, "child", "") + if err != nil { + t.Fatalf("SourceDoc: %v", err) + } + if got := src.Slots.Rules; len(got) != 1 || got[0] != "from child" { + t.Errorf("rules = %v, want only the child's own rule (parent content must not be merged in)", got) + } + if len(src.Extends) != 1 || src.Extends[0].Ref != "openteams/base" { + t.Errorf("extends = %+v, want the child's own pinned parent", src.Extends) + } + if src.Visibility != "private" || src.Scope != "company" || src.Maintainer != "platform team" { + t.Errorf("metadata lost: visibility=%q scope=%q maintainer=%q", src.Visibility, src.Scope, src.Maintainer) + } + + // Contrast: ResolveDoc merges the parent in and is therefore unsafe to + // round-trip back into a write. + resolved, err := svc.ResolveDoc(ctx, "openteams", "child", "") + if err != nil { + t.Fatalf("ResolveDoc: %v", err) + } + if len(resolved.Slots.Rules) != 2 { + t.Errorf("resolved rules = %v, want both parent and child rules", resolved.Slots.Rules) + } +} + +func TestService_SourceDocRespectsRead(t *testing.T) { + repo := store.NewMemory() + ownerCtx := seedOrg(t, repo, "owner", "publisher") + svc := frames.NewService(repo) + if _, _, err := svc.PublishDoc(ownerCtx, docFor("private-frame", "1.0.0", "secret"), "", frames.PublishCreate); err != nil { + t.Fatalf("seed: %v", err) + } + + // A member of a different org must not read it, and must not learn it exists. + otherCtx := seedSecondOrg(t, repo, "outsider", "admin") + if _, err := svc.SourceDoc(otherCtx, "private-frame", ""); connect.CodeOf(err) != connect.CodeNotFound { + t.Errorf("code = %v, want NotFound for a cross-org read", connect.CodeOf(err)) + } +} + +// An LLM-driven write path can emit arbitrarily large content, so the cap the +// design doc promises has to be real - and enforced for both front doors. +func TestService_PublishRejectsOversizedContent(t *testing.T) { + huge := strings.Repeat("x", frames.MaxContentBytes+1) + + t.Run("connect path", func(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + content := "name: big\ndescription: d\nversion: 1.0.0\nslots:\n goals: " + huge + "\n" + _, err := svc.PublishFrame(ctx, connect.NewRequest(&framesv1.PublishFrameRequest{Content: []byte(content)})) + if connect.CodeOf(err) != connect.CodeInvalidArgument { + t.Errorf("code = %v (err %v), want InvalidArgument", connect.CodeOf(err), err) + } + }) + + t.Run("publish doc path", func(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + doc := docFor("big", "1.0.0") + doc.Slots.Goals = huge + _, _, err := svc.PublishDoc(ctx, doc, "", frames.PublishCreate) + if connect.CodeOf(err) != connect.CodeInvalidArgument { + t.Errorf("code = %v (err %v), want InvalidArgument", connect.CodeOf(err), err) + } + }) + + // Pins the boundary exactly: content of precisely MaxContentBytes is allowed + // and one byte more is not, so the comparison cannot drift between > and >=. + t.Run("the boundary is inclusive", func(t *testing.T) { + // Binary-search the padding that makes the marshalled document land on + // exactly the limit; YAML framing makes the offset awkward to hardcode. + sizeFor := func(pad int) int { + d := docFor("ok", "1.0.0") + d.Slots.Goals = strings.Repeat("y", pad) + b, err := frames.Marshal(d) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return len(b) + } + lo, hi := 0, frames.MaxContentBytes + for lo < hi { + mid := (lo + hi + 1) / 2 + if sizeFor(mid) <= frames.MaxContentBytes { + lo = mid + } else { + hi = mid - 1 + } + } + if got := sizeFor(lo); got != frames.MaxContentBytes { + t.Fatalf("could not construct content of exactly %d bytes (closest %d)", frames.MaxContentBytes, got) + } + + atLimit := docFor("ok", "1.0.0") + atLimit.Slots.Goals = strings.Repeat("y", lo) + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + if _, _, err := frames.NewService(repo).PublishDoc(ctx, atLimit, "", frames.PublishCreate); err != nil { + t.Errorf("content of exactly %d bytes was rejected: %v", frames.MaxContentBytes, err) + } + + over := docFor("ok", "1.0.0") + over.Slots.Goals = strings.Repeat("y", lo+1) + repo2 := store.NewMemory() + ctx2 := seedOrg(t, repo2, "pub", "publisher") + _, _, err := frames.NewService(repo2).PublishDoc(ctx2, over, "", frames.PublishCreate) + if connect.CodeOf(err) != connect.CodeInvalidArgument { + t.Errorf("one byte over the limit: code = %v (err %v), want InvalidArgument", connect.CodeOf(err), err) + } + }) +} + +// latest_version must not move backwards. Publishing an older version would +// otherwise make every default read - GetFrame, ListFrames, MCP get_frame, and +// the merge base of the next update - resolve to the older document, quietly +// unpublishing newer content. +func TestService_PublishRejectsNonAdvancingVersion(t *testing.T) { + tests := []struct { + name string + versions []string // published in order; the last one is the assertion + wantCode connect.Code + }{ + {name: "advancing patch", versions: []string{"1.0.0", "1.0.1"}}, + {name: "advancing minor", versions: []string{"1.0.0", "1.1.0"}}, + {name: "advancing major", versions: []string{"1.9.9", "2.0.0"}}, + {name: "double digits sort numerically", versions: []string{"1.9.0", "1.10.0"}}, + {name: "going backwards is rejected", versions: []string{"2.0.0", "1.0.1"}, wantCode: connect.CodeInvalidArgument}, + {name: "minor going backwards is rejected", versions: []string{"1.2.0", "1.1.9"}, wantCode: connect.CodeInvalidArgument}, + {name: "republishing the same version is rejected", versions: []string{"1.0.0", "1.0.0"}, wantCode: connect.CodeAlreadyExists}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + var err error + for i, v := range tt.versions { + intent := frames.PublishUpdate + if i == 0 { + intent = frames.PublishCreate + } + _, _, err = svc.PublishDoc(ctx, docFor("brand-voice", v, "r"), "", intent) + if i < len(tt.versions)-1 && err != nil { + t.Fatalf("seeding %s: %v", v, err) + } + } + if got := connect.CodeOf(err); tt.wantCode == 0 && err != nil { + t.Fatalf("unexpected error: %v", err) + } else if tt.wantCode != 0 && got != tt.wantCode { + t.Fatalf("code = %v (err %v), want %v", got, err, tt.wantCode) + } + }) + } +} + +// Two callers that both read version 1.0.0 and then publish must not silently +// lose one another's changes. The second publish is rejected because the Frame +// moved on beneath it. +func TestService_PublishDetectsAStaleBase(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + if _, _, err := svc.PublishDoc(ctx, docFor("brand-voice", "1.0.0", "original"), "", frames.PublishCreate); err != nil { + t.Fatalf("seed: %v", err) + } + + // Both callers read 1.0.0 as their base. + first := docFor("brand-voice", "1.1.0", "original", "from the first caller") + second := docFor("brand-voice", "1.2.0", "original", "from the second caller") + + if _, _, err := svc.PublishDocFrom(ctx, first, "", frames.PublishUpdate, "1.0.0"); err != nil { + t.Fatalf("first publish: %v", err) + } + _, _, err := svc.PublishDocFrom(ctx, second, "", frames.PublishUpdate, "1.0.0") + if connect.CodeOf(err) != connect.CodeFailedPrecondition { + t.Fatalf("code = %v (err %v), want FailedPrecondition: the second caller's base was stale", + connect.CodeOf(err), err) + } + + // The first caller's change survived. + doc, err := svc.SourceDoc(ctx, "brand-voice", "") + if err != nil { + t.Fatalf("source: %v", err) + } + if doc.Version != "1.1.0" { + t.Errorf("latest = %q, want 1.1.0", doc.Version) + } +} + +// An empty base version means "I did not check", which keeps the Connect API's +// existing behaviour rather than forcing every caller to supply one. +func TestService_PublishWithoutABaseVersionIsUnchecked(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + if _, _, err := svc.PublishDoc(ctx, docFor("brand-voice", "1.0.0", "a"), "", frames.PublishCreate); err != nil { + t.Fatalf("seed: %v", err) + } + if _, _, err := svc.PublishDoc(ctx, docFor("brand-voice", "1.1.0", "b"), "", frames.PublishUpdate); err != nil { + t.Errorf("unchecked publish should succeed: %v", err) + } +} + +// The version error is reported as a field violation so the web form can mark +// the version input, the way it already does for a duplicate version. +func TestService_NonAdvancingVersionIsAFieldViolation(t *testing.T) { + repo := store.NewMemory() + ctx := seedOrg(t, repo, "pub", "publisher") + svc := frames.NewService(repo) + if _, _, err := svc.PublishDoc(ctx, docFor("brand-voice", "2.0.0", "a"), "", frames.PublishCreate); err != nil { + t.Fatalf("seed: %v", err) + } + _, _, err := svc.PublishDoc(ctx, docFor("brand-voice", "1.0.0", "b"), "", frames.PublishUpdate) + var ce *connect.Error + if !errors.As(err, &ce) { + t.Fatalf("want a connect error, got %v", err) + } + found := false + for _, d := range ce.Details() { + v, derr := d.Value() + if derr != nil { + continue + } + if fv, ok := v.(*framesv1.FieldViolations); ok { + for _, viol := range fv.Violations { + if viol.Field == "version" { + found = true + } + } + } + } + if !found { + t.Errorf("no field violation on 'version'; details = %v", ce.Details()) + } +} diff --git a/backend/internal/mcp/compose.go b/backend/internal/mcp/compose.go index c0c9737..f887cfb 100644 --- a/backend/internal/mcp/compose.go +++ b/backend/internal/mcp/compose.go @@ -24,6 +24,11 @@ func composeMarkdown(doc *frames.Doc, resolvedAt time.Time) string { if doc.Description != "" { fmt.Fprintf(&b, "%s\n\n", doc.Description) } + // The version is what update_frame's base_version must be set to, so it has + // to be visible to a client that intends to edit this frame. + if doc.Version != "" { + fmt.Fprintf(&b, "> Version: %s\n", doc.Version) + } if len(doc.Extends) > 0 { parts := make([]string, len(doc.Extends)) for i, e := range doc.Extends { diff --git a/backend/internal/mcp/integration_test.go b/backend/internal/mcp/integration_test.go index a08e118..3e2da35 100644 --- a/backend/internal/mcp/integration_test.go +++ b/backend/internal/mcp/integration_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" gomcp "github.com/modelcontextprotocol/go-sdk/mcp" "google.golang.org/protobuf/types/known/timestamppb" @@ -28,6 +29,18 @@ func (stubVerifier) Validate(context.Context, string) (*auth.Claims, error) { return nil, errors.New("stub: no validation") } +// acceptingVerifier admits any token as the dev user, with a live expiry. The +// expiry matters: the bearer middleware treats a zero Expiration as expired and +// answers 401, which would make a body-size test pass without ever reaching the +// handler being tested. +type acceptingVerifier struct{} + +func (acceptingVerifier) Validate(context.Context, string) (*auth.Claims, error) { + c := auth.DevClaims() + c.Expiry = time.Now().Add(time.Hour) + return c, nil +} + // newTestServer builds an httptest server mounting only the MCP component. In // non-dev mode it supplies a stub verifier (Mount requires one); the metadata // and 401-challenge tests never present a token, and the dev-mode test sets @@ -207,3 +220,447 @@ func connectSDK(t *testing.T, ctx context.Context, endpoint string) *gomcp.Clien } return session } + +// newWriteTestSession wires the real frames.Service to an in-process MCP client +// session in dev mode, with the dev user holding role in org o1. Nothing is +// stubbed on the permission path, so these tests fail if the MCP layer ever +// gains a shortcut around RBAC. +func newWriteTestSession(t *testing.T, role string) (*gomcp.ClientSession, *store.Memory) { + t.Helper() + ctx := context.Background() + mem := store.NewMemory() + seedOrgAndReadableFrame(t, mem) + if err := mem.UpsertMembership(ctx, &framesv1.Membership{ + OrgId: "o1", UserSub: "dev-user", Role: role, + }); err != nil { + t.Fatalf("UpsertMembership: %v", err) + } + + svc := frames.NewService(mem) + comp := mcppkg.NewComponent(mcppkg.Config{DevMode: true, PublicURL: "https://frames.example.com"}, svc, nil) + mux := http.NewServeMux() + comp.Mount(mux) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + client := gomcp.NewClient(&gomcp.Implementation{Name: "test", Version: "v1"}, nil) + cs, err := client.Connect(ctx, &gomcp.StreamableClientTransport{Endpoint: srv.URL + "/mcp"}, nil) + if err != nil { + t.Fatalf("client connect: %v", err) + } + t.Cleanup(func() { _ = cs.Close() }) + return cs, mem +} + +// callTool invokes a tool and returns its text plus whether it reported an error. +func callTool(t *testing.T, cs *gomcp.ClientSession, name string, args map[string]any) (string, bool) { + t.Helper() + res, err := cs.CallTool(context.Background(), &gomcp.CallToolParams{Name: name, Arguments: args}) + if err != nil { + t.Fatalf("call %s: %v", name, err) + } + var b strings.Builder + for _, c := range res.Content { + if tc, ok := c.(*gomcp.TextContent); ok { + b.WriteString(tc.Text) + } + } + return b.String(), res.IsError +} + +func TestMCPWritesEnforceRBAC(t *testing.T) { + newFrame := map[string]any{ + "name": "brand-voice", + "description": "How we write", + "version": "1.0.0", + "rules": []any{"Cite benchmarks."}, + } + + t.Run("a viewer cannot create a frame", func(t *testing.T) { + cs, mem := newWriteTestSession(t, "viewer") + text, isErr := callTool(t, cs, "create_frame", newFrame) + if !isErr { + t.Fatalf("viewer created a frame: %q", text) + } + if !strings.Contains(text, "permission denied") { + t.Errorf("text = %q, want a permission denial", text) + } + if _, err := mem.GetFrameBySlugName(context.Background(), "openteams", "brand-voice"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("a denied create still wrote a frame (err=%v)", err) + } + }) + + t.Run("a publisher can create a frame and read it back", func(t *testing.T) { + cs, mem := newWriteTestSession(t, "publisher") + text, isErr := callTool(t, cs, "create_frame", newFrame) + if isErr { + t.Fatalf("publisher denied: %q", text) + } + if !strings.Contains(text, "brand-voice@1.0.0") { + t.Errorf("text = %q, want the published name and version", text) + } + f, err := mem.GetFrameBySlugName(context.Background(), "openteams", "brand-voice") + if err != nil { + t.Fatalf("frame not persisted: %v", err) + } + if f.OwnerSub != "dev-user" { + t.Errorf("owner = %q, want dev-user", f.OwnerSub) + } + // The new Frame is immediately readable through the read tool. + got, isErr := callTool(t, cs, "get_frame", map[string]any{"name": "brand-voice"}) + if isErr || !strings.Contains(got, "Cite benchmarks.") { + t.Errorf("get_frame after create = %q (isErr=%v)", got, isErr) + } + }) + + t.Run("creating over an existing name fails", func(t *testing.T) { + cs, _ := newWriteTestSession(t, "publisher") + if _, isErr := callTool(t, cs, "create_frame", newFrame); isErr { + t.Fatal("first create should succeed") + } + // A different version, so this can only be the create-intent check and + // not the version-uniqueness check. + second := map[string]any{ + "name": "brand-voice", "description": "How we write", "version": "2.0.0", + "rules": []any{"Cite benchmarks."}, + } + text, isErr := callTool(t, cs, "create_frame", second) + if !isErr { + t.Fatalf("second create succeeded: %q", text) + } + if !strings.Contains(text, "already exists") { + t.Errorf("text = %q, want an already-exists error", text) + } + }) + + t.Run("the owner can update their frame", func(t *testing.T) { + cs, mem := newWriteTestSession(t, "publisher") + if _, isErr := callTool(t, cs, "create_frame", newFrame); isErr { + t.Fatal("create should succeed") + } + updated := map[string]any{ + "name": "brand-voice", + "description": "How we write, revised", + "version": "1.1.0", + "base_version": "1.0.0", + "rules": []any{"Cite benchmarks.", "Avoid jargon."}, + "changelog": "added a rule", + } + text, isErr := callTool(t, cs, "update_frame", updated) + if isErr { + t.Fatalf("owner denied update: %q", text) + } + f, err := mem.GetFrameBySlugName(context.Background(), "openteams", "brand-voice") + if err != nil { + t.Fatalf("get frame: %v", err) + } + if f.LatestVersion != "1.1.0" { + t.Errorf("latest version = %q, want 1.1.0", f.LatestVersion) + } + }) + + t.Run("updating a frame the caller cannot edit is denied", func(t *testing.T) { + // alpha is seeded owned by "someone" with only an org-level read grant. + cs, _ := newWriteTestSession(t, "publisher") + text, isErr := callTool(t, cs, "update_frame", map[string]any{ + "name": "alpha", + "description": "hijacked", + "version": "2.0.0", + "base_version": "1.0.0", + "rules": []any{"mine now"}, + }) + if !isErr { + t.Fatalf("edit permission was bypassed: %q", text) + } + if !strings.Contains(text, "permission denied") { + t.Errorf("text = %q, want a permission denial", text) + } + }) + + t.Run("updating an unknown frame reports not found", func(t *testing.T) { + cs, _ := newWriteTestSession(t, "publisher") + text, isErr := callTool(t, cs, "update_frame", map[string]any{ + "name": "ghost", "description": "x", + "version": "1.0.0", "base_version": "1.0.0", + "rules": []any{"r"}, + }) + if !isErr { + t.Fatalf("update of an unknown frame succeeded: %q", text) + } + if !strings.Contains(text, "not found") { + t.Errorf("text = %q, want a not-found error", text) + } + }) + + t.Run("an invalid document is rejected by the canonical validator", func(t *testing.T) { + cs, mem := newWriteTestSession(t, "publisher") + text, isErr := callTool(t, cs, "create_frame", map[string]any{ + "name": "Not A Valid Name", + "description": "x", + "version": "1.0.0", + "rules": []any{"r"}, + }) + if !isErr { + t.Fatalf("invalid name accepted: %q", text) + } + if !strings.Contains(text, "invalid frame") { + t.Errorf("text = %q, want an invalid-frame error", text) + } + if _, err := mem.GetFrameBySlugName(context.Background(), "openteams", "Not A Valid Name"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("an invalid create still wrote something (err=%v)", err) + } + }) +} + +// An update must not destroy what the caller did not mention. Absent fields keep +// their current values; supplied fields replace them; an explicitly empty list +// clears. Without this, an AI that updates one slot silently wipes the Frame's +// visibility, maintainer, and - worst - its inheritance edges. +func TestMCPUpdatePreservesOmittedFields(t *testing.T) { + cs, mem := newWriteTestSession(t, "publisher") + ctx := context.Background() + + // A parent to inherit from, then a child that pins it and carries metadata. + if _, isErr := callTool(t, cs, "create_frame", map[string]any{ + "name": "base", "description": "Base", "version": "1.0.0", + "rules": []any{"from parent"}, + }); isErr { + t.Fatal("create base failed") + } + if _, isErr := callTool(t, cs, "create_frame", map[string]any{ + "name": "child", "description": "Child", "version": "1.0.0", + "rules": []any{"from child"}, + "visibility": "private", + "scope": "company", + "maintainer": "platform team", + "extends": []any{map[string]any{"ref": "openteams/base", "version": "1.0.0"}}, + "goals": "ship the thing", + }); isErr { + t.Fatal("create child failed") + } + + // Update only the rules. Everything else must survive. + text, isErr := callTool(t, cs, "update_frame", map[string]any{ + "name": "child", "version": "1.1.0", "base_version": "1.0.0", + "rules": []any{"from child", "and another"}, + }) + if isErr { + t.Fatalf("update failed: %q", text) + } + + v, _, _, err := mem.GetFrameVersion(ctx, mustFrameID(t, mem, "child"), "1.1.0") + if err != nil { + t.Fatalf("get version: %v", err) + } + doc, err := frames.Parse(v.Content) + if err != nil { + t.Fatalf("parse stored content: %v", err) + } + + if doc.Visibility != "private" { + t.Errorf("visibility = %q, want private (omitted fields must be preserved)", doc.Visibility) + } + if doc.Scope != "company" { + t.Errorf("scope = %q, want company", doc.Scope) + } + if doc.Maintainer != "platform team" { + t.Errorf("maintainer = %q, want %q", doc.Maintainer, "platform team") + } + if len(doc.Extends) != 1 || doc.Extends[0].Ref != "openteams/base" || doc.Extends[0].Version != "1.0.0" { + t.Errorf("extends = %+v, want the pinned parent preserved: inheritance must survive an update", doc.Extends) + } + if doc.Slots.Goals != "ship the thing" { + t.Errorf("goals = %q, want the original prose preserved", doc.Slots.Goals) + } + if doc.Description != "Child" { + t.Errorf("description = %q, want Child", doc.Description) + } + // The parent's rule must NOT have been copied into the child. + if len(doc.Slots.Rules) != 2 { + t.Errorf("rules = %v, want exactly the two supplied (no inherited content flattened in)", doc.Slots.Rules) + } + for _, r := range doc.Slots.Rules { + if r == "from parent" { + t.Errorf("parent content was flattened into the child: %v", doc.Slots.Rules) + } + } + + t.Run("supplied fields replace, and an explicit empty list clears", func(t *testing.T) { + text, isErr := callTool(t, cs, "update_frame", map[string]any{ + "name": "child", "version": "1.2.0", "base_version": "1.1.0", + "maintainer": "data team", + "extends": []any{}, + }) + if isErr { + t.Fatalf("update failed: %q", text) + } + v, _, _, err := mem.GetFrameVersion(ctx, mustFrameID(t, mem, "child"), "1.2.0") + if err != nil { + t.Fatalf("get version: %v", err) + } + doc, err := frames.Parse(v.Content) + if err != nil { + t.Fatalf("parse: %v", err) + } + if doc.Maintainer != "data team" { + t.Errorf("maintainer = %q, want the supplied value", doc.Maintainer) + } + if len(doc.Extends) != 0 { + t.Errorf("extends = %+v, want cleared by the explicit empty list", doc.Extends) + } + if doc.Visibility != "private" { + t.Errorf("visibility = %q, still want private (untouched)", doc.Visibility) + } + }) +} + +func mustFrameID(t *testing.T, mem *store.Memory, name string) string { + t.Helper() + f, err := mem.GetFrameBySlugName(context.Background(), "openteams", name) + if err != nil { + t.Fatalf("frame %q: %v", name, err) + } + return f.Id +} + +// The most likely instruction this tool will ever get is "add a rule to X". +// Doing that requires reading the Frame's current rules, and if the only read +// available returns the inheritance-composed form, the model has no choice but +// to send the parent's content back as the child's own - which validates, looks +// identical when composed, and silently detaches the child from its parent's +// future revisions. +func TestMCPReadModifyWriteDoesNotFlattenInheritance(t *testing.T) { + cs, mem := newWriteTestSession(t, "publisher") + ctx := context.Background() + + if _, isErr := callTool(t, cs, "create_frame", map[string]any{ + "name": "company-base", "description": "Company", "version": "1.0.0", + "rules": []any{"Use inclusive language."}, + "goals": "Grow the platform.", + }); isErr { + t.Fatal("create parent failed") + } + if _, isErr := callTool(t, cs, "create_frame", map[string]any{ + "name": "team-api", "description": "API team", "version": "1.0.0", + "rules": []any{"Version every endpoint."}, + "extends": []any{map[string]any{"ref": "openteams/company-base", "version": "1.0.0"}}, + }); isErr { + t.Fatal("create child failed") + } + + // A source read must exist, and must return only the child's own content. + src, isErr := callTool(t, cs, "get_frame", map[string]any{"name": "team-api", "source": true}) + if isErr { + t.Fatalf("get_frame source mode failed: %q", src) + } + if strings.Contains(src, "Use inclusive language.") { + t.Errorf("source read leaked inherited content, so a model editing it would copy the parent in:\n%s", src) + } + if !strings.Contains(src, "Version every endpoint.") { + t.Errorf("source read is missing the frame's own rule:\n%s", src) + } + if strings.Contains(src, "Grow the platform.") { + t.Errorf("source read leaked the parent's prose slot:\n%s", src) + } + + // The default read stays composed, which is what a consumer wants. + composed, isErr := callTool(t, cs, "get_frame", map[string]any{"name": "team-api"}) + if isErr { + t.Fatalf("get_frame failed: %q", composed) + } + if !strings.Contains(composed, "Use inclusive language.") { + t.Errorf("default read should still compose inherited content:\n%s", composed) + } + + // Editing from the source read leaves inheritance intact and un-flattened. + if _, isErr := callTool(t, cs, "update_frame", map[string]any{ + "name": "team-api", "version": "1.1.0", "base_version": "1.0.0", + "rules": []any{"Version every endpoint.", "Prefer cursor pagination."}, + }); isErr { + t.Fatal("update failed") + } + v, _, _, err := mem.GetFrameVersion(ctx, mustFrameID(t, mem, "team-api"), "1.1.0") + if err != nil { + t.Fatalf("get version: %v", err) + } + doc, err := frames.Parse(v.Content) + if err != nil { + t.Fatalf("parse: %v", err) + } + for _, r := range doc.Slots.Rules { + if r == "Use inclusive language." { + t.Errorf("the parent's rule was copied into the child: %v", doc.Slots.Rules) + } + } + if doc.Slots.Goals != "" { + t.Errorf("goals = %q, want empty: the parent's prose must not be frozen into the child", doc.Slots.Goals) + } + if len(doc.Extends) != 1 { + t.Errorf("extends = %+v, want the parent still pinned", doc.Extends) + } +} + +// The SDK reads a request body in full before any tool handler runs, so the +// body limit is the only thing standing between an authenticated caller and the +// memory of a single-replica deployment. +// +// Both auth modes are covered on purpose: the bearer middleware is installed +// only when DevMode is false, and wrapping the wrong handler there silently +// drops the cap in exactly the deployments that need it. +func TestMCPRejectsOversizedRequestBody(t *testing.T) { + tests := []struct { + name string + devMode bool + verifier auth.TokenValidator + token string + }{ + {name: "dev mode", devMode: true}, + {name: "with bearer auth", devMode: false, verifier: acceptingVerifier{}, token: "any-token"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mem := store.NewMemory() + seedOrgAndReadableFrame(t, mem) + comp := mcppkg.NewComponent( + mcppkg.Config{DevMode: tt.devMode, PublicURL: "https://frames.example.com"}, + frames.NewService(mem), tt.verifier) + mux := http.NewServeMux() + comp.Mount(mux) + srv := httptest.NewServer(mux) + defer srv.Close() + + post := func(size int) int { + t.Helper() + body := `{"jsonrpc":"2.0","id":1,"method":"initialize","params":` + + `{"protocolVersion":"2025-06-18","capabilities":{},` + + `"clientInfo":{"name":"probe","version":"1"},"padding":"` + + strings.Repeat("a", size) + `"}}` + req, err := http.NewRequest(http.MethodPost, srv.URL+"/mcp", strings.NewReader(body)) + if err != nil { + t.Fatalf("request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if tt.token != "" { + req.Header.Set("Authorization", "Bearer "+tt.token) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + // A connection reset is an acceptable refusal. + return http.StatusRequestEntityTooLarge + } + defer func() { _ = resp.Body.Close() }() + return resp.StatusCode + } + + // Control: a small body must succeed, so a refusal below cannot be + // mistaken for an unrelated rejection such as a 401. + if got := post(100); got >= 400 { + t.Fatalf("small body rejected with %d; the test is not reaching the handler", got) + } + if got := post(mcppkg.MaxRequestBytes + 1024); got < 400 { + t.Errorf("oversized body accepted with %d: the body cap is not in force on this path", got) + } + }) + } +} diff --git a/backend/internal/mcp/resources.go b/backend/internal/mcp/resources.go index c5cbeb6..d574bc8 100644 --- a/backend/internal/mcp/resources.go +++ b/backend/internal/mcp/resources.go @@ -13,6 +13,7 @@ import ( "github.com/nebari-dev/nebari-frames/backend/internal/auth" "github.com/nebari-dev/nebari-frames/backend/internal/frames" + framesv1 "github.com/nebari-dev/nebari-frames/gen/go/frames/v1" ) // compile-time assertion that the real service satisfies the adapter's interface. @@ -23,6 +24,12 @@ var _ FrameSource = (*frames.Service)(nil) type FrameSource interface { ListReadable(ctx context.Context) ([]frames.ReadableFrame, error) ResolveDoc(ctx context.Context, orgSlug, name, version string) (*frames.Doc, error) + // SourceDoc reads a frame's own unresolved document, for use as the merge + // base of an update. Distinct from ResolveDoc on purpose: see updateFrameTool. + SourceDoc(ctx context.Context, name, version string) (*frames.Doc, error) + // PublishDocFrom is the RBAC-enforcing write path shared with the Connect + // API. The base version it takes is what makes concurrent updates safe. + PublishDocFrom(ctx context.Context, doc *frames.Doc, changelog string, intent frames.PublishIntent, baseVersion string) (*framesv1.Frame, *framesv1.FrameVersion, error) } type resourceServer struct { @@ -86,8 +93,19 @@ func (rs *resourceServer) getServer(req *http.Request) *gomcp.Server { }, rs.listFramesTool(claims)) gomcp.AddTool(srv, &gomcp.Tool{ Name: "get_frame", - Description: "Get the full composed Markdown of a Frame by name (optionally a specific version). Use this to load an organization Frame as context before writing.", + Description: "Get the Markdown of a Frame by name (optionally a specific version). By default returns the composed form, including everything it inherits - use that to load an organization Frame as context. Pass source=true to get only the Frame's own content, which is what you must read before editing it with update_frame.", }, rs.getFrameTool(claims)) + // Writes. Permission is enforced entirely by frames.PublishDoc, the same + // path the Connect API uses; a caller without the role or grant gets an + // error result rather than a partial write. + gomcp.AddTool(srv, &gomcp.Tool{ + Name: "create_frame", + Description: "Create a new Frame in the user's organization. Fails if a Frame with that name already exists, or if the user may not publish. Call list_frames first to check the name is free.", + }, rs.createFrameTool(claims)) + gomcp.AddTool(srv, &gomcp.Tool{ + Name: "update_frame", + Description: "Publish a new version of an existing Frame, changing only the fields you supply. Read it first with get_frame source=true and pass the version it reports as base_version; the update is refused if someone else published in the meantime. The new version must be higher than the current one. Anything you omit keeps its current value, so send just what changes; pass an empty list to clear a list. To modify a list or a text section, first read the current value with get_frame source=true - never with the default composed form, whose inherited content would be copied into this Frame and detach it from its parents. Fails if no Frame with that name exists, or if the user may not edit it.", + }, rs.updateFrameTool(claims)) return srv } @@ -142,6 +160,11 @@ func (rs *resourceServer) listFramesTool(claims *auth.Claims) gomcp.ToolHandlerF type getFrameInput struct { Name string `json:"name" jsonschema:"the Frame name, e.g. nebari-platform"` Version string `json:"version,omitempty" jsonschema:"optional version; defaults to the latest"` + // Source exists so a client that intends to EDIT a Frame can see what the + // Frame itself says. The default composed view merges every ancestor's + // slots, and sending that back through update_frame would copy the parents' + // content into the child and detach it from their future revisions. + Source bool `json:"source,omitempty" jsonschema:"when true, return only this Frame's own content without inherited content. Use this before update_frame; use the default (false) when reading a Frame as context"` } // getFrameTool returns a Frame's composed Markdown. It finds the named frame @@ -168,7 +191,7 @@ func (rs *resourceServer) getFrameTool(claims *auth.Claims) gomcp.ToolHandlerFor if version == "" { version = match.Version } - doc, err := rs.src.ResolveDoc(ctx, match.OrgSlug, in.Name, version) + doc, err := rs.docFor(ctx, in, match.OrgSlug, version) if err != nil { return errorResult("frame not found: " + in.Name), nil, nil } @@ -184,3 +207,13 @@ func textResult(s string) *gomcp.CallToolResult { func errorResult(s string) *gomcp.CallToolResult { return &gomcp.CallToolResult{IsError: true, Content: []gomcp.Content{&gomcp.TextContent{Text: s}}} } + +// docFor returns the Frame's own document when the caller asked for source, +// and the inheritance-composed one otherwise. Both are read-enforced by the +// service. +func (rs *resourceServer) docFor(ctx context.Context, in getFrameInput, orgSlug, version string) (*frames.Doc, error) { + if in.Source { + return rs.src.SourceDoc(ctx, in.Name, version) + } + return rs.src.ResolveDoc(ctx, orgSlug, in.Name, version) +} diff --git a/backend/internal/mcp/resources_test.go b/backend/internal/mcp/resources_test.go index 234d5e0..65ebc61 100644 --- a/backend/internal/mcp/resources_test.go +++ b/backend/internal/mcp/resources_test.go @@ -5,12 +5,16 @@ import ( "errors" "net/http" "net/http/httptest" + "reflect" "strings" "testing" + "connectrpc.com/connect" gomcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/nebari-dev/nebari-frames/backend/internal/auth" "github.com/nebari-dev/nebari-frames/backend/internal/frames" + framesv1 "github.com/nebari-dev/nebari-frames/gen/go/frames/v1" ) type stubSource struct { @@ -22,6 +26,19 @@ type stubSource struct { func (s stubSource) ListReadable(context.Context) ([]frames.ReadableFrame, error) { return s.readable, s.listErr } + +// PublishDoc and SourceDoc keep stubSource satisfying FrameSource for the +// read-path tests. Those tests never write, so reaching either is a bug in the +// code under test rather than an expected error - panic so it cannot be mistaken +// for a normal error result somewhere downstream. +func (s stubSource) PublishDocFrom(context.Context, *frames.Doc, string, frames.PublishIntent, string) (*framesv1.Frame, *framesv1.FrameVersion, error) { + panic("stubSource: unexpected publish call from a read path") +} + +func (s stubSource) SourceDoc(context.Context, string, string) (*frames.Doc, error) { + panic("stubSource: unexpected SourceDoc call from a read path") +} + func (s stubSource) ResolveDoc(_ context.Context, org, name, _ string) (*frames.Doc, error) { d, ok := s.docs[org+"/"+name] if !ok { @@ -167,3 +184,393 @@ func TestGetFrameTool(t *testing.T) { } }) } + +// publishCall records what the stub was asked to publish so tests can assert the +// adapter passed the caller's input through faithfully. +type publishCall struct { + doc *frames.Doc + changelog string + intent frames.PublishIntent + baseVersion string +} + +type stubWriter struct { + stubSource + calls []publishCall + err error + // current is the document SourceDoc returns; srcErr overrides it. + current *frames.Doc + srcErr error +} + +func (s *stubWriter) SourceDoc(context.Context, string, string) (*frames.Doc, error) { + if s.srcErr != nil { + return nil, s.srcErr + } + if s.current != nil { + return s.current, nil + } + return &frames.Doc{}, nil +} + +func (s *stubWriter) PublishDocFrom(_ context.Context, doc *frames.Doc, changelog string, intent frames.PublishIntent, baseVersion string) (*framesv1.Frame, *framesv1.FrameVersion, error) { + s.calls = append(s.calls, publishCall{doc: doc, changelog: changelog, intent: intent, baseVersion: baseVersion}) + if s.err != nil { + return nil, nil, s.err + } + return &framesv1.Frame{Name: doc.Name}, &framesv1.FrameVersion{Version: doc.Version}, nil +} + +// ptr is shorthand for the optional string fields. +func ptr(s string) *string { return &s } + +func TestWriteFrameTools(t *testing.T) { + validInput := func() writeFrameInput { + return writeFrameInput{ + Name: "brand-voice", + Description: ptr("How we write"), + Version: "1.0.0", + // Required by update_frame and ignored by create_frame. + BaseVersion: "0.9.0", + Rules: []string{"Cite benchmarks."}, + } + } + + tests := []struct { + name string + tool string // "create" or "update" + input writeFrameInput + publishErr error + + wantIsError bool + wantIntent frames.PublishIntent + wantCalls int + wantText string // substring the result must contain + }{ + { + name: "create publishes with create intent", + tool: "create", input: validInput(), + wantIntent: frames.PublishCreate, wantCalls: 1, wantText: "brand-voice", + }, + { + name: "update publishes with update intent", + tool: "update", input: validInput(), + wantIntent: frames.PublishUpdate, wantCalls: 1, wantText: "brand-voice", + }, + { + name: "a denied create surfaces an error result, not a transport error", + tool: "create", input: validInput(), + publishErr: connect.NewError(connect.CodePermissionDenied, errors.New("publisher or admin role required")), + wantIsError: true, wantCalls: 1, wantIntent: frames.PublishCreate, wantText: "permission", + }, + { + name: "an update of a missing frame surfaces an error result", + tool: "update", input: validInput(), + publishErr: connect.NewError(connect.CodeNotFound, errors.New(`no frame named "ghost" to update`)), + wantIsError: true, wantCalls: 1, wantIntent: frames.PublishUpdate, wantText: "not found", + }, + { + name: "a create over an existing name surfaces an error result", + tool: "create", input: validInput(), + publishErr: connect.NewError(connect.CodeAlreadyExists, errors.New("already exists")), + wantIsError: true, wantCalls: 1, wantIntent: frames.PublishCreate, wantText: "already exists", + }, + { + name: "an internal fault does not leak detail to the client", + tool: "create", input: validInput(), + publishErr: connect.NewError(connect.CodeInternal, errors.New("sqlite: disk image is malformed")), + wantIsError: true, wantCalls: 1, wantIntent: frames.PublishCreate, wantText: "could not publish frame", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + src := &stubWriter{err: tt.publishErr} + rs := &resourceServer{src: src, cfg: Config{DevMode: true}} + + h := rs.createFrameTool(auth.DevClaims()) + if tt.tool == "update" { + h = rs.updateFrameTool(auth.DevClaims()) + } + res, _, err := h(context.Background(), &gomcp.CallToolRequest{}, tt.input) + if err != nil { + t.Fatalf("tool returned transport error: %v", err) + } + if res.IsError != tt.wantIsError { + t.Fatalf("IsError = %v, want %v; text=%q", res.IsError, tt.wantIsError, toolText(res)) + } + if len(src.calls) != tt.wantCalls { + t.Fatalf("PublishDoc called %d times, want %d", len(src.calls), tt.wantCalls) + } + if tt.wantText != "" && !strings.Contains(strings.ToLower(toolText(res)), strings.ToLower(tt.wantText)) { + t.Errorf("result text %q should mention %q", toolText(res), tt.wantText) + } + if tt.wantCalls > 0 { + call := src.calls[0] + if call.intent != tt.wantIntent { + t.Errorf("intent = %v, want %v", call.intent, tt.wantIntent) + } + if call.doc.Name != tt.input.Name || call.doc.Version != tt.input.Version { + t.Errorf("doc = %+v, does not match input %+v", call.doc, tt.input) + } + } + }) + } +} + +// Every field of the input must reach the published document; one silently +// dropped by the adapter would lose organizational context with no error. +func TestWriteFrameInputCarriesEveryField(t *testing.T) { + src := &stubWriter{} + rs := &resourceServer{src: src, cfg: Config{DevMode: true}} + h := rs.createFrameTool(auth.DevClaims()) + + in := writeFrameInput{ + Name: "full", Description: ptr("d"), Version: "1.0.0", + Visibility: ptr("private"), + Scope: ptr("company"), + Maintainer: ptr("platform team"), + Terminology: []termInput{{Term: "Frame", Definition: "a context artifact"}}, + Rules: []string{"rule"}, + Skills: []string{"skill"}, + Prompts: []string{"prompt"}, + ToolSpecs: ptr("tools"), + Goals: ptr("goals"), + Style: ptr("style"), + Norms: ptr("norms"), + Architecture: ptr("architecture"), + BusinessProcess: ptr("process"), + Extends: []extendInput{{Ref: "openteams/base", Version: "1.0.0"}}, + Excludes: []string{"openteams/legacy"}, + } + if _, _, err := h(context.Background(), &gomcp.CallToolRequest{}, in); err != nil { + t.Fatalf("create_frame: %v", err) + } + if len(src.calls) != 1 { + t.Fatalf("PublishDoc called %d times, want 1", len(src.calls)) + } + got := src.calls[0].doc + want := &frames.Doc{ + Name: "full", Description: "d", Version: "1.0.0", + Visibility: "private", Scope: "company", Maintainer: "platform team", + Extends: []frames.ExtendRef{{Ref: "openteams/base", Version: "1.0.0"}}, + Excludes: []string{"openteams/legacy"}, + Slots: frames.Slots{ + Terminology: []frames.Term{{Term: "Frame", Definition: "a context artifact"}}, + Rules: []string{"rule"}, + Skills: []string{"skill"}, + Prompts: []string{"prompt"}, + ToolSpecs: "tools", + Goals: "goals", + Style: "style", + Norms: "norms", + Architecture: "architecture", + BusinessProcess: "process", + }, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("doc mismatch\n got: %+v\nwant: %+v", got, want) + } +} + +// A real drift guard. The previous version of this test compared against a +// hand-written literal, so a newly added slot or document field was zero on both +// sides and passed - which is exactly how visibility, scope, and maintainer came +// to be silently dropped. This walks the canonical definitions instead, so +// extending frames.Doc or frames.SlotTable without extending writeFrameInput +// fails here rather than in production. +func TestWriteFrameInputCoversDocFields(t *testing.T) { + inputFields := map[string]bool{} + inT := reflect.TypeOf(writeFrameInput{}) + for i := range inT.NumField() { + name, _, _ := strings.Cut(inT.Field(i).Tag.Get("json"), ",") + inputFields[name] = true + } + + t.Run("every slot has an input field", func(t *testing.T) { + for _, d := range frames.SlotTable { + if !inputFields[d.Key] { + t.Errorf("slot %q has no writeFrameInput field: MCP writes would silently drop it", d.Key) + } + } + // Guard the other direction too: frames.Slots must not grow a field that + // SlotTable does not describe. + if got, want := reflect.TypeOf(frames.Slots{}).NumField(), len(frames.SlotTable); got != want { + t.Errorf("frames.Slots has %d fields but SlotTable describes %d", got, want) + } + }) + + t.Run("every document field is accounted for", func(t *testing.T) { + // Doc-level fields that are not slots. "slots" is the container itself; + // the rest are the Frame Spec metadata plus inheritance. + expected := map[string]bool{ + "name": true, "description": true, "version": true, + "visibility": true, "scope": true, "maintainer": true, + "extends": true, "excludes": true, "slots": true, + } + docT := reflect.TypeOf(frames.Doc{}) + for i := range docT.NumField() { + name, _, _ := strings.Cut(docT.Field(i).Tag.Get("yaml"), ",") + if !expected[name] { + t.Errorf("frames.Doc gained field %q: decide whether MCP writes must carry it, then add it here", name) + continue + } + if name == "slots" { + continue // covered by the slot walk above + } + if !inputFields[name] { + t.Errorf("document field %q has no writeFrameInput field: MCP writes would silently drop it", name) + } + } + }) +} + +// The write tools must be advertised, or a client has no way to call them. +func TestGetServerAdvertisesWriteTools(t *testing.T) { + src := &stubWriter{} + rs := &resourceServer{src: src, cfg: Config{DevMode: true}} + srv := rs.getServer(httptest.NewRequest("POST", "/mcp", nil)) + if srv == nil { + t.Fatal("getServer returned nil") + } + // The SDK exposes no tool listing on the server value, so assert through a + // connected client session instead. + ctx := context.Background() + clientTransport, serverTransport := gomcp.NewInMemoryTransports() + serverSession, err := srv.Connect(ctx, serverTransport, nil) + if err != nil { + t.Fatalf("server connect: %v", err) + } + defer func() { _ = serverSession.Close() }() + client := gomcp.NewClient(&gomcp.Implementation{Name: "test", Version: "v1"}, nil) + cs, err := client.Connect(ctx, clientTransport, nil) + if err != nil { + t.Fatalf("client connect: %v", err) + } + defer func() { _ = cs.Close() }() + + res, err := cs.ListTools(ctx, &gomcp.ListToolsParams{}) + if err != nil { + t.Fatalf("list tools: %v", err) + } + got := map[string]bool{} + for _, tool := range res.Tools { + got[tool.Name] = true + } + for _, want := range []string{"list_frames", "get_frame", "create_frame", "update_frame"} { + if !got[want] { + t.Errorf("tool %q not advertised; got %v", want, got) + } + } +} + +// Guards the remaining half of the drift problem: TestWriteFrameInputCoversDocFields +// forces a new slot or document field to gain an input field, but nothing forced +// that field to be wired into applyTo. An unwired field would leave the target +// at its zero value, which a hand-written want literal cannot catch because it +// is zero on both sides. This sets every input field to a distinct sentinel and +// asserts nothing in the resulting document is still zero. +func TestApplyToWiresEveryInputField(t *testing.T) { + in := writeFrameInput{} + v := reflect.ValueOf(&in).Elem() + inT := v.Type() + + // Fill every field with a non-zero sentinel derived from its name. + for i := range inT.NumField() { + name := inT.Field(i).Name + f := v.Field(i) + switch f.Kind() { + case reflect.String: + f.SetString("sentinel-" + name) + case reflect.Pointer: + sv := reflect.New(f.Type().Elem()) + sv.Elem().SetString("sentinel-" + name) + f.Set(sv) + case reflect.Bool: + f.SetBool(true) + case reflect.Slice: + elem := f.Type().Elem() + ev := reflect.New(elem).Elem() + switch elem.Kind() { + case reflect.String: + ev.SetString("sentinel-" + name) + case reflect.Struct: + for j := range elem.NumField() { + if ev.Field(j).Kind() == reflect.String { + ev.Field(j).SetString("sentinel-" + name) + } + } + default: + t.Fatalf("field %s: unhandled slice element kind %s", name, elem.Kind()) + } + f.Set(reflect.Append(f, ev)) + default: + t.Fatalf("field %s: unhandled kind %s; extend this test", name, f.Kind()) + } + } + // Name and Version must look like a valid frame for nothing else to matter, + // but their values are irrelevant to the zero-check below. + in.Name, in.Version = "sentinel-name", "1.0.0" + + got := in.applyTo(&frames.Doc{}) + + // Changelog is publish metadata, not part of the document. + docV := reflect.ValueOf(*got) + docT := docV.Type() + for i := range docT.NumField() { + name := docT.Field(i).Name + if name == "Slots" { + continue + } + if docV.Field(i).IsZero() { + t.Errorf("Doc.%s is zero after applyTo: the input field exists but is not wired in", name) + } + } + slotsV := reflect.ValueOf(got.Slots) + slotsT := slotsV.Type() + for i := range slotsT.NumField() { + if slotsV.Field(i).IsZero() { + t.Errorf("Slots.%s is zero after applyTo: the input field exists but is not wired in", slotsT.Field(i).Name) + } + } +} + +// The concurrency guard only works if update_frame reports the version it +// actually merged onto. Passing an empty base would leave the check inert while +// still looking correct. +func TestUpdateFrameSendsTheBaseVersionItRead(t *testing.T) { + src := &stubWriter{current: &frames.Doc{ + Name: "brand-voice", Description: "d", Version: "3.4.5", + Slots: frames.Slots{Rules: []string{"existing"}}, + }} + rs := &resourceServer{src: src, cfg: Config{DevMode: true}} + h := rs.updateFrameTool(auth.DevClaims()) + + if _, _, err := h(context.Background(), &gomcp.CallToolRequest{}, writeFrameInput{ + Name: "brand-voice", Version: "3.5.0", BaseVersion: "3.4.5", + Rules: []string{"existing", "new"}, + }); err != nil { + t.Fatalf("update_frame: %v", err) + } + if len(src.calls) != 1 { + t.Fatalf("publish called %d times, want 1", len(src.calls)) + } + if got := src.calls[0].baseVersion; got != "3.4.5" { + t.Errorf("baseVersion = %q, want the value the caller supplied, not one re-read server-side", got) + } +} + +// create_frame has nothing to be stale against, so it must not send a base. +func TestCreateFrameSendsNoBaseVersion(t *testing.T) { + src := &stubWriter{} + rs := &resourceServer{src: src, cfg: Config{DevMode: true}} + h := rs.createFrameTool(auth.DevClaims()) + if _, _, err := h(context.Background(), &gomcp.CallToolRequest{}, writeFrameInput{ + Name: "n", Description: ptr("d"), Version: "1.0.0", Rules: []string{"r"}, + }); err != nil { + t.Fatalf("create_frame: %v", err) + } + if got := src.calls[0].baseVersion; got != "" { + t.Errorf("baseVersion = %q, want empty for a create", got) + } +} diff --git a/backend/internal/mcp/server.go b/backend/internal/mcp/server.go index e10e79b..eba104a 100644 --- a/backend/internal/mcp/server.go +++ b/backend/internal/mcp/server.go @@ -9,6 +9,19 @@ import ( "github.com/nebari-dev/nebari-frames/backend/internal/auth" ) +// MaxRequestBytes caps a single MCP request body. It is deliberately larger +// than frames.MaxContentBytes so a create_frame at the content limit still fits +// with its JSON-RPC framing. +const MaxRequestBytes = 8 << 20 // 8 MiB + +// maxBodyBytes limits how much of a request body the next handler can read. +func maxBodyBytes(next http.Handler, limit int64) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, limit) + next.ServeHTTP(w, r) + }) +} + // Component bundles the MCP endpoint routes and can be mounted onto any // http.ServeMux. Create it with NewComponent. type Component struct { @@ -39,13 +52,19 @@ func (c *Component) Mount(mux *http.ServeMux) { rs := &resourceServer{src: c.src, cfg: c.cfg} mcpHandler := gomcp.NewStreamableHTTPHandler(rs.getServer, nil) - var handler http.Handler = mcpHandler + // Bound what a request can make the server buffer. The SDK reads the body in + // full before a tool handler - and therefore before RBAC - is ever reached, + // so without this an authenticated caller who may not write at all can + // exhaust the memory of a single-replica deployment. + handler := maxBodyBytes(mcpHandler, MaxRequestBytes) if !c.cfg.DevMode { verifier := newTokenVerifier(c.verifier) middleware := mcpauth.RequireBearerToken(verifier, &mcpauth.RequireBearerTokenOptions{ ResourceMetadataURL: c.cfg.metadataURL(), }) - handler = middleware(mcpHandler) + // Wraps handler, not mcpHandler: wrapping the latter would discard the + // body cap in exactly the deployments that have authentication on. + handler = middleware(handler) } mux.Handle("/mcp", handler) diff --git a/backend/internal/mcp/write.go b/backend/internal/mcp/write.go new file mode 100644 index 0000000..1e6ea96 --- /dev/null +++ b/backend/internal/mcp/write.go @@ -0,0 +1,210 @@ +package mcp + +import ( + "context" + "errors" + "fmt" + "strings" + + "connectrpc.com/connect" + gomcp "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/nebari-dev/nebari-frames/backend/internal/auth" + "github.com/nebari-dev/nebari-frames/backend/internal/frames" +) + +// termInput is one vocabulary entry in the terminology slot. +type termInput struct { + Term string `json:"term" jsonschema:"the term being defined"` + Definition string `json:"definition" jsonschema:"what the term means in this organization"` +} + +// extendInput is a pinned reference to a parent Frame. +type extendInput struct { + Ref string `json:"ref" jsonschema:"parent Frame reference as org_slug/frame_name"` + Version string `json:"version" jsonschema:"the parent version to pin, e.g. 1.0.0"` +} + +// writeFrameInput is the typed input shared by create_frame and update_frame. It +// mirrors frames.Doc field for field so an MCP client is guided by the tool +// schema instead of authoring YAML blind. +// +// Every optional field is a pointer or a slice so that "not mentioned" is +// distinguishable from "set to empty". update_frame relies on that distinction: +// an omitted field keeps the Frame's current value, while an explicitly empty +// one clears it. Without it, an AI updating a single slot would silently erase +// the Frame's metadata and - far worse - its inheritance edges. +// +// TestWriteFrameInputCoversDocFields walks frames.SlotTable and the frames.Doc +// field set, so adding a slot or a document field without adding it here fails. +type writeFrameInput struct { + Name string `json:"name" jsonschema:"Frame name: lowercase letters, digits and dashes, e.g. brand-voice"` + Version string `json:"version" jsonschema:"semantic version for the new revision, e.g. 1.1.0; must not already exist"` + Changelog string `json:"changelog,omitempty" jsonschema:"optional note describing what changed in this version"` + // Required by update_frame. Inferring it server-side would defeat the + // purpose: the window that loses a change is between the client's read and + // its write, and only the client knows what it read. + BaseVersion string `json:"base_version,omitempty" jsonschema:"required for update_frame: the version shown by get_frame when you read this Frame. The update is refused if someone else has published since, so you can re-read and reapply instead of silently overwriting their change"` + + Description *string `json:"description,omitempty" jsonschema:"one-line summary of what this Frame carries. Required when creating; when updating, omit to keep the current one"` + Visibility *string `json:"visibility,omitempty" jsonschema:"declared intent, one of private, internal, shared, public; omit to keep the current one, pass an empty string to clear it. Access is decided by registry permissions, not by this field"` + Scope *string `json:"scope,omitempty" jsonschema:"who this Frame applies to, e.g. company or team-platform; omit to keep the current one, pass an empty string to clear it"` + Maintainer *string `json:"maintainer,omitempty" jsonschema:"who owns this Frame; omit to keep the current one, pass an empty string to clear it"` + + Terminology []termInput `json:"terminology,omitempty" jsonschema:"named concepts and their definitions; omit to keep the current list, pass an empty list to clear it"` + Rules []string `json:"rules,omitempty" jsonschema:"constraints that must be followed; omit to keep the current list, pass an empty list to clear it"` + Skills []string `json:"skills,omitempty" jsonschema:"capabilities this Frame expects; omit to keep, empty list to clear"` + Prompts []string `json:"prompts,omitempty" jsonschema:"reusable prompts; omit to keep, empty list to clear"` + ToolSpecs *string `json:"tool_specs,omitempty" jsonschema:"tool specifications, as markdown; omit to keep the current text, pass an empty string to clear it"` + Goals *string `json:"goals,omitempty" jsonschema:"what the organization is trying to achieve, as markdown; omit to keep the current text, pass an empty string to clear it"` + Style *string `json:"style,omitempty" jsonschema:"voice and formatting conventions, as markdown; omit to keep the current text, pass an empty string to clear it"` + Norms *string `json:"norms,omitempty" jsonschema:"team norms and expectations, as markdown; omit to keep the current text, pass an empty string to clear it"` + Architecture *string `json:"architecture,omitempty" jsonschema:"system architecture context, as markdown; omit to keep the current text, pass an empty string to clear it"` + BusinessProcess *string `json:"business_process,omitempty" jsonschema:"business process context, as markdown; omit to keep the current text, pass an empty string to clear it"` + + Extends []extendInput `json:"extends,omitempty" jsonschema:"parent Frames this one inherits from, each pinned to a version; later parents win. Omit to keep the current inheritance, pass an empty list to remove all parents"` + Excludes []string `json:"excludes,omitempty" jsonschema:"parent references to exclude from inheritance; omit to keep, empty list to clear"` +} + +// applyTo overlays the supplied fields onto base, which is the Frame's current +// document for an update and an empty document for a create. Fields the caller +// omitted are left as they were. Validation is deliberately not performed here: +// frames.PublishDoc runs the canonical validator, so there is one definition of +// a valid Frame. +func (in writeFrameInput) applyTo(base *frames.Doc) *frames.Doc { + d := *base + d.Name = in.Name + d.Version = in.Version + + setString(&d.Description, in.Description) + setString(&d.Visibility, in.Visibility) + setString(&d.Scope, in.Scope) + setString(&d.Maintainer, in.Maintainer) + + setString(&d.Slots.ToolSpecs, in.ToolSpecs) + setString(&d.Slots.Goals, in.Goals) + setString(&d.Slots.Style, in.Style) + setString(&d.Slots.Norms, in.Norms) + setString(&d.Slots.Architecture, in.Architecture) + setString(&d.Slots.BusinessProcess, in.BusinessProcess) + + if in.Rules != nil { + d.Slots.Rules = in.Rules + } + if in.Skills != nil { + d.Slots.Skills = in.Skills + } + if in.Prompts != nil { + d.Slots.Prompts = in.Prompts + } + if in.Terminology != nil { + terms := make([]frames.Term, len(in.Terminology)) + for i, t := range in.Terminology { + terms[i] = frames.Term{Term: t.Term, Definition: t.Definition} + } + d.Slots.Terminology = terms + } + if in.Extends != nil { + refs := make([]frames.ExtendRef, len(in.Extends)) + for i, e := range in.Extends { + refs[i] = frames.ExtendRef{Ref: e.Ref, Version: e.Version} + } + d.Extends = refs + } + if in.Excludes != nil { + d.Excludes = in.Excludes + } + return &d +} + +// setString assigns only when the caller supplied the field. +func setString(dst *string, src *string) { + if src != nil { + *dst = *src + } +} + +// createFrameTool publishes a new Frame. It performs no permission check of its +// own: PublishDoc enforces the publisher/admin role and rejects a name that +// already exists, so the MCP surface cannot drift from the Connect API. +func (rs *resourceServer) createFrameTool(claims *auth.Claims) gomcp.ToolHandlerFor[writeFrameInput, any] { + return func(ctx context.Context, _ *gomcp.CallToolRequest, in writeFrameInput) (*gomcp.CallToolResult, any, error) { + ctx = auth.WithClaims(ctx, claims) + return rs.publish(ctx, in, in.applyTo(&frames.Doc{}), frames.PublishCreate, "") + } +} + +// updateFrameTool publishes a new version of an existing Frame, merging the +// caller's changes onto the Frame's current document. PublishDoc enforces edit +// permission on the target and rejects an unknown name. +// +// The merge base is SourceDoc - the Frame's OWN document - and not the composed +// form get_frame returns. Merging onto a resolved document would copy every +// parent's slots into the child and drop its extends edges, quietly destroying +// the inheritance graph. +func (rs *resourceServer) updateFrameTool(claims *auth.Claims) gomcp.ToolHandlerFor[writeFrameInput, any] { + return func(ctx context.Context, _ *gomcp.CallToolRequest, in writeFrameInput) (*gomcp.CallToolResult, any, error) { + ctx = auth.WithClaims(ctx, claims) + if in.BaseVersion == "" { + return errorResult("base_version is required: read the Frame first with " + + "get_frame source=true and pass the version it reports, so a change " + + "published by someone else in the meantime is not silently overwritten"), nil, nil + } + current, err := rs.src.SourceDoc(ctx, in.Name, "") + if err != nil { + return errorResult(writeErrorText(err)), nil, nil + } + // Merge onto the current document, but assert against the version the + // caller actually read. Deriving the base from this read instead would + // make the check vacuous - it would always match. + return rs.publish(ctx, in, in.applyTo(current), frames.PublishUpdate, in.BaseVersion) + } +} + +func (rs *resourceServer) publish(ctx context.Context, in writeFrameInput, doc *frames.Doc, intent frames.PublishIntent, baseVersion string) (*gomcp.CallToolResult, any, error) { + frame, version, err := rs.src.PublishDocFrom(ctx, doc, in.Changelog, intent, baseVersion) + if err != nil { + return errorResult(writeErrorText(err)), nil, nil + } + return textResult(fmt.Sprintf("Published %s@%s.", frame.Name, version.Version)), nil, nil +} + +// writeErrorText turns a service error into text an AI client can act on. The +// connect code carries the meaning, so it is mapped rather than pattern-matched +// on message strings. +func writeErrorText(err error) string { + msg := connectMessage(err) + switch connect.CodeOf(err) { + case connect.CodePermissionDenied: + return "permission denied: " + msg + case connect.CodeNotFound: + return "frame not found: " + msg + case connect.CodeAlreadyExists: + // Deliberately reveals that the name is taken, which is what makes the + // error actionable for a client picking a name. The pre-existing Connect + // path already disclosed as much by returning a permission error for a + // name the caller cannot edit. + return "already exists: " + msg + case connect.CodeInvalidArgument: + return "invalid frame: " + msg + case connect.CodeUnauthenticated: + return "not authenticated: " + msg + case connect.CodeFailedPrecondition: + // A concurrent update moved the frame on after this one read it. The + // message names both versions, so a client can re-read and retry. + return "frame changed while you were editing it: " + msg + default: + // Internal faults must not leak storage or wiring detail to the client. + return "could not publish frame" + } +} + +// connectMessage extracts the human-readable part of a connect error, dropping +// the code prefix connect adds to Error(). +func connectMessage(err error) string { + var cerr *connect.Error + if errors.As(err, &cerr) { + return cerr.Message() + } + return strings.TrimSpace(err.Error()) +} diff --git a/dev/keycloak/frames-realm.json b/dev/keycloak/frames-realm.json index 808bda7..7865e99 100644 --- a/dev/keycloak/frames-realm.json +++ b/dev/keycloak/frames-realm.json @@ -26,6 +26,16 @@ "id.token.claim": "false", "access.token.claim": "true" } + }, + { + "name": "frames-mcp-audience", + "protocol": "openid-connect", + "protocolMapper": "oidc-audience-mapper", + "config": { + "included.custom.audience": "http://localhost:5173/mcp", + "id.token.claim": "false", + "access.token.claim": "true" + } } ] } diff --git a/docs/connect/chatgpt.md b/docs/connect/chatgpt.md index 5cab64b..0874646 100644 --- a/docs/connect/chatgpt.md +++ b/docs/connect/chatgpt.md @@ -2,7 +2,8 @@ ChatGPT can use an organization's Frames as a remote MCP connector. It registers itself via Dynamic Client Registration (DCR) against Keycloak, signs the user in -with their Nebari account, and then exposes the `list_frames` and `get_frame` +with their Nebari account, and then exposes the `list_frames`, `get_frame`, +`create_frame`, and `update_frame` tools in the conversation. ## Prerequisites @@ -35,7 +36,7 @@ tools in the conversation. reads the protected-resource metadata and registers a client via **DCR** (CIMD is skipped because the server doesn't advertise it), then opens the OAuth login. **Sign in with your Nebari (Keycloak) account** and approve. -5. The app shows **connected** with two tools: `list_frames` and `get_frame`. +5. The app shows **connected** with four tools: `list_frames`, `get_frame`, `create_frame`, and `update_frame`. The write tools are RBAC-gated: a viewer sees them listed but every call is denied. ## Using it @@ -58,7 +59,7 @@ load the content, and writes grounded in that Frame (respecting its rules). - **Connects but tool calls 401:** the token lacks the `/mcp` audience. Confirm the audience mapper is on a scope every client gets (see keycloak-setup.md). - **No tools shown:** confirm the server is the tool-bearing build (it exposes - `list_frames`/`get_frame`), not resources-only. + `list_frames`/`get_frame`/`create_frame`/`update_frame`), not resources-only. ## References diff --git a/docs/connect/gemini.md b/docs/connect/gemini.md index 190c84c..d2eb820 100644 --- a/docs/connect/gemini.md +++ b/docs/connect/gemini.md @@ -49,8 +49,8 @@ This doc covers the Gemini CLI path. /mcp list ``` - You should see `nebari-frames` connected with the `list_frames` and - `get_frame` tools. + You should see `nebari-frames` connected with the `list_frames`, + `get_frame`, `create_frame`, and `update_frame` tools. ## Using it diff --git a/docs/design/2026-05-21-mcp-endpoint-design.md b/docs/design/2026-05-21-mcp-endpoint-design.md index b8c9a24..635b5ad 100644 --- a/docs/design/2026-05-21-mcp-endpoint-design.md +++ b/docs/design/2026-05-21-mcp-endpoint-design.md @@ -273,10 +273,18 @@ Rejected. Claude.ai's connector mechanism IS MCP. There is no shorter path. "Bui ## 6. Security Considerations - **All RBAC server-side.** The `/mcp` endpoint never returns frame content without `rbac.Can(caller, Read, frame)` returning allow. -- **OAuth scopes.** MCP client gets read-only scope (`frames:read`). No publish or admin capability via MCP - those flow through CLI / web app. Reduces blast radius if an MCP token is compromised. +- **OAuth scopes.** ~~MCP client gets read-only scope (`frames:read`). No publish or admin capability via MCP - those flow through CLI / web app. Reduces blast radius if an MCP token is compromised.~~ **Superseded by [#51](https://github.com/nebari-dev/nebari-frames/issues/51):** the endpoint now also exposes `create_frame` and `update_frame`. The blast-radius argument above was the reason writes were originally excluded, and it still applies - a compromised MCP token can now publish as its owner. What limits it is that writes carry no privilege of their own: they run through `frames.Service.PublishDoc`, so a token belonging to a viewer cannot write at all, and one belonging to a publisher can only create Frames and edit those it holds an edit grant on. An **admin** token is the real worst case, since `rbac.Can` allows an admin every frame in the org before grants are consulted. Deletion is still not reachable over MCP. + A second threat is specific to exposing writes as an AI tool rather than a CLI: Frame content is + untrusted text that an AI client reads into its context, so a prompt-injection payload can attempt + to drive `update_frame` and rewrite an organization's shared context with no token theft at all. + What blunts it is that an update carries only the caller's own permissions, and that every version + is retained, so a bad write is auditable and revertable rather than destructive. Merge semantics + are *not* a mitigation here: an injected instruction is well-formed, and `"extends": []` + deliberately clears every parent. Merging defends against accidental loss by a well-intentioned + client, not against a deliberate call. - **Token TTL.** MCP OAuth tokens follow standard OAuth refresh semantics (short access token + refresh token). The user can revoke at the OIDC provider level. - **Same-origin and CSRF.** Not applicable; MCP is API-to-API after OAuth. Token in bearer header. -- **Content size caps.** Server enforces the same 512KB per-frame content cap as the rest of the system. A Frame with 100MB of inherited content is rejected at publish time, not at MCP-read time. +- **Content size caps.** Server enforces a 512KB per-version cap on stored content (`frames.MaxContentBytes`), checked in the shared publish path so the Connect API and the MCP write tools are both covered. The cap applies to the stored document, not the resolved form: a Frame that inherits heavily can still compose to more than this, so a resolved-size limit remains unimplemented. - **Information disclosure via list.** `resources/list` returns Frame names and descriptions even before content is fetched. Names and descriptions are intentionally shareable within an org (that's the point of a registry); cross-org names are not listed because `rbac` filters by org. - **Connector trust prompts.** Enterprise admins at the consumer side (claude.ai org admin etc.) often gate third-party connectors. We document the trust prompts in the per-provider Connect pages. diff --git a/docs/site/src/content/docs/architecture.md b/docs/site/src/content/docs/architecture.md index 0c56ef2..29926e9 100644 --- a/docs/site/src/content/docs/architecture.md +++ b/docs/site/src/content/docs/architecture.md @@ -8,7 +8,7 @@ title: Architecture - **Web app** (`web/`) - a Vite-built SPA, embedded into the backend binary at build time (`make build-web`) so the shipped artifact is one binary and one container image. - **Store** (`backend/internal/store/sqlite`) - SQLite via `modernc.org/sqlite` (pure Go, no cgo), on a PVC in Kubernetes. Single-writer by design: `replicaCount` is pinned to `1` and the Deployment uses the `Recreate` strategy so the previous pod releases the volume before the next one mounts it. - **CLI** (`cli/`) - the `frames` binary (built on `github.com/spf13/cobra`), talking to the backend over Connect RPC. See the [CLI Reference](/reference/cli/frames/). -- **MCP endpoint** (`backend/internal/mcp`) - a remote MCP server mounted at `/mcp`, letting any MCP-capable AI client (Claude, ChatGPT, Gemini, and others) read Frames the authenticated caller can access. +- **MCP endpoint** (`backend/internal/mcp`) - a remote MCP server mounted at `/mcp`, letting any MCP-capable AI client (Claude, ChatGPT, Gemini, and others) read the Frames the authenticated caller can access, and create or update Frames they have permission to write. It is a protocol adapter only: reads go through `frames.Service.ResolveDoc` and writes through `frames.Service.PublishDoc`, the same RBAC-enforcing methods the Connect API uses, so the two surfaces cannot disagree about who may do what. - **NebariApp / operator integration** (`chart/templates/nebariapp.yaml`) - on a Nebari cluster, the chart creates a `NebariApp` custom resource; the nebari-operator reconciles it into routing, TLS, a landing-page tile, and (optionally) an OIDC client. ## Request flow