diff --git a/handlers.go b/handlers.go index 3dc8001..4b77085 100644 --- a/handlers.go +++ b/handlers.go @@ -97,11 +97,11 @@ func (s *Server) auth(h http.HandlerFunc) http.HandlerFunc { // ":workspace" suffix) leave the choice to the caller. if bound != "" { ws, confined = bound, true - } else if hdr := r.Header.Get("X-Workspace"); hdr != "" { - ws = hdr + } else if sel := workspaceSelector(r); sel != "" { + ws = sel } - } else if hdr := r.Header.Get("X-Workspace"); hdr != "" { - ws = hdr + } else if sel := workspaceSelector(r); sel != "" { + ws = sel } if !validWorkspace(ws) { writeError(w, http.StatusBadRequest, "bad_workspace", ErrBadWorkspace.Error(), @@ -148,6 +148,19 @@ type reqWS struct { type reqWSKey struct{} +// workspaceSelector reads the caller's requested workspace. The X-Workspace +// header wins; ?workspace= is honoured as a fallback because the MCP tools take +// a per-call `workspace` argument and the /fleet routes already read the query +// param, so a reader that passed ?workspace= silently got the default workspace +// back with a 200 — a wrong answer that looked like a right one. Neither form +// can widen a confined token; that check stays above. +func workspaceSelector(r *http.Request) string { + if hdr := strings.TrimSpace(r.Header.Get("X-Workspace")); hdr != "" { + return hdr + } + return strings.TrimSpace(r.URL.Query().Get("workspace")) +} + func requestWorkspace(ctx context.Context) reqWS { v, _ := ctx.Value(reqWSKey{}).(reqWS) return v @@ -399,6 +412,17 @@ func (s *Server) getDoc(w http.ResponseWriter, r *http.Request) { // matches, ".../raw" streams the prefix's bytes and ".../lock" reports // its lease — so folio-file slugs get the same sub-routes as {id}. if rest := r.PathValue("rest"); rest != "" { + // Order matters: ".../revisions/{n}/raw" also ends in "/raw", so it + // has to be tested before the plain-content case or a revision read + // would stream the current bytes instead. + if base, version, ok := cutRevisionRaw(rest); ok { + s.rawRevisionID(w, r, base, version) + return + } + if base, ok := strings.CutSuffix(rest, "/revisions"); ok { + s.listRevisionsID(w, r, base) + return + } if base, ok := strings.CutSuffix(rest, "/raw"); ok { s.streamContent(w, r, base) return @@ -435,8 +459,27 @@ func (s *Server) patchDoc(w http.ResponseWriter, r *http.Request) { Title *string `json:"title"` Kind *string `json:"kind"` } - if err := json.NewDecoder(r.Body).Decode(&in); err != nil { - badRequest(w, "json body required (tags / add_tags / remove_tags / metadata / title / kind)") + // Strict, so a field this endpoint cannot apply is refused rather than + // dropped. PATCH used to accept {"content": ...}, return 200, echo back a + // content_url and content_hash for the *old* bytes, and change nothing — + // indistinguishable from a successful write unless the caller re-read the + // document. Content is written by PUT, which takes a lease and If-Match. + dec := json.NewDecoder(r.Body) + dec.DisallowUnknownFields() + if err := dec.Decode(&in); err != nil { + if strings.Contains(err.Error(), "unknown field \"content\"") { + badRequest(w, "PATCH does not write content — use PUT /docs/{id} with If-Match "+ + "and a lease. PATCH takes tags / add_tags / remove_tags / metadata / title / kind.") + return + } + badRequest(w, "json body required (tags / add_tags / remove_tags / metadata / title / kind): "+err.Error()) + return + } + // A decoder reads one JSON value and stops, so `{"add_tags":[..]} {"content":..}` + // would otherwise apply the tags, discard the content and answer 200 — the + // very no-op this endpoint is being made to refuse. + if err := dec.Decode(new(json.RawMessage)); err != io.EOF { + badRequest(w, "body must be a single JSON object") return } doc, err := s.store.PatchDocument(r.Context(), docID(r), DocPatch{ @@ -523,7 +566,11 @@ func (s *Server) streamContent(w http.ResponseWriter, r *http.Request, idOrSlug // listRevisions returns a document's version history (newest first). func (s *Server) listRevisions(w http.ResponseWriter, r *http.Request) { - revs, err := s.store.DocRevisions(r.Context(), docID(r)) + s.listRevisionsID(w, r, docID(r)) +} + +func (s *Server) listRevisionsID(w http.ResponseWriter, r *http.Request, id string) { + revs, err := s.store.DocRevisions(r.Context(), id) if err != nil { writeErr(w, err) return @@ -531,6 +578,26 @@ func (s *Server) listRevisions(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, map[string]any{"revisions": revs, "count": len(revs)}) } +// cutRevisionRaw splits "myfolio/file.md/revisions/3/raw" into its slug and +// version. A {rest...} wildcard has to end its pattern, so multi-segment slugs +// cannot reach the {id} sub-routes and are dispatched by suffix instead — the +// same shape lockDocRest already uses for POST and DELETE. +func cutRevisionRaw(rest string) (string, int, bool) { + base, ok := strings.CutSuffix(rest, "/raw") + if !ok { + return "", 0, false + } + i := strings.LastIndex(base, "/revisions/") + if i < 0 { + return "", 0, false + } + version, err := strconv.Atoi(base[i+len("/revisions/"):]) + if err != nil { + return "", 0, false + } + return base[:i], version, true +} + // rawRevision streams the content bytes of a specific past version. func (s *Server) rawRevision(w http.ResponseWriter, r *http.Request) { version, err := strconv.Atoi(r.PathValue("version")) @@ -538,7 +605,11 @@ func (s *Server) rawRevision(w http.ResponseWriter, r *http.Request) { badRequest(w, "version must be an integer") return } - key, ctype, err := s.store.RevisionContent(r.Context(), docID(r), version) + s.rawRevisionID(w, r, docID(r), version) +} + +func (s *Server) rawRevisionID(w http.ResponseWriter, r *http.Request, id string, version int) { + key, ctype, err := s.store.RevisionContent(r.Context(), id, version) if err != nil { writeErr(w, err) return diff --git a/openapi.yaml b/openapi.yaml index 31d1bc9..a783490 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -288,8 +288,10 @@ paths: metadata: { type: object, description: "shallow-merged into existing metadata" } title: { type: string } kind: { type: string } + additionalProperties: false responses: "200": { description: relabelled, content: { application/json: { schema: { type: object, properties: { document: { $ref: '#/components/schemas/Document' } } } } } } + "400": { description: "unknown field (e.g. content — use PUT), or more than one JSON value in the body" } "404": { $ref: '#/components/responses/NotFound' } delete: summary: Hard-delete a document (irreversible) diff --git a/silent_writes_db_test.go b/silent_writes_db_test.go new file mode 100644 index 0000000..e1195ab --- /dev/null +++ b/silent_writes_db_test.go @@ -0,0 +1,195 @@ +package main + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// End-to-end coverage for the three "answered 200, did nothing" defects. These +// go through the real mux and the auth middleware, so they exercise routing, +// workspace scoping and content delivery rather than the helpers in isolation. + +func silentWritesMux(srv *Server) *http.ServeMux { + mux := http.NewServeMux() + mux.HandleFunc("GET /docs", srv.auth(srv.listDocs)) + mux.HandleFunc("GET /docs/{id}", srv.auth(srv.getDoc)) + mux.HandleFunc("GET /docs/{rest...}", srv.auth(srv.getDoc)) + mux.HandleFunc("GET /docs/{id}/revisions", srv.auth(srv.listRevisions)) + mux.HandleFunc("GET /docs/{id}/revisions/{version}/raw", srv.auth(srv.rawRevision)) + mux.HandleFunc("PATCH /docs/{id}", srv.auth(srv.patchDoc)) + mux.HandleFunc("PATCH /docs/{rest...}", srv.auth(srv.patchDoc)) + return mux +} + +func do(t *testing.T, mux *http.ServeMux, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + var r *http.Request + if body == "" { + r = httptest.NewRequest(method, path, nil) + } else { + r = httptest.NewRequest(method, path, strings.NewReader(body)) + r.Header.Set("Content-Type", "application/json") + } + r.Header.Set("X-Actor", "tester") + w := httptest.NewRecorder() + mux.ServeHTTP(w, r) + return w +} + +// PATCH must refuse a content field instead of applying the tags, dropping the +// content and answering 200 with a content hash for the unchanged bytes. +func TestPatchRefusesContent(t *testing.T) { + s := testStore(t) + srv := &Server{store: s, cfg: Config{DefaultWorkspace: "default"}} + mux := silentWritesMux(srv) + + ctx, release, err := s.Scoped(context.Background(), "default") + if err != nil { + t.Fatalf("scope: %v", err) + } + ctx = context.WithValue(ctx, reqWSKey{}, reqWS{name: "default"}) + if _, err := s.CreateDocument(ctx, "patch-probe", "probe", "note", nil, nil, + []byte("ORIGINAL"), "text/markdown", "tester"); err != nil { + release() + t.Fatalf("create: %v", err) + } + release() + + for _, body := range []string{ + `{"content":"REPLACED","tags":["probe"]}`, + `{"content_type":"text/plain"}`, + // One decode reads one value; a trailing object must not slip past. + `{"add_tags":["x"]} {"content":"REPLACED"}`, + } { + if w := do(t, mux, "PATCH", "/docs/patch-probe", body); w.Code != http.StatusBadRequest { + t.Fatalf("PATCH %s = %d, want 400 (body %s)", body, w.Code, w.Body.String()) + } + } + + // The document must be untouched by every rejected call. + w := do(t, mux, "GET", "/docs/patch-probe", "") + var env struct { + Document struct { + Version int `json:"version"` + Tags []string `json:"tags"` + } `json:"document"` + } + if err := json.Unmarshal(w.Body.Bytes(), &env); err != nil { + t.Fatalf("decode: %v", err) + } + if env.Document.Version != 1 { + t.Fatalf("version = %d, want 1 — a refused PATCH changed the document", env.Document.Version) + } + if len(env.Document.Tags) != 0 { + t.Fatalf("tags = %v, want none — a refused PATCH applied its tags anyway", env.Document.Tags) + } + + // A legitimate relabel still works. + if w := do(t, mux, "PATCH", "/docs/patch-probe", `{"add_tags":["kept"]}`); w.Code != http.StatusOK { + t.Fatalf("legitimate relabel = %d, want 200 (body %s)", w.Code, w.Body.String()) + } +} + +// ?workspace= must select, not be accepted and ignored. +func TestListDocsHonoursWorkspaceQuery(t *testing.T) { + s := testStore(t) + base := context.Background() + if _, err := s.CreateWorkspace(base, "wsq", "test workspace", "tester"); err != nil && + err.Error() != ErrAlreadyExists.Error() { + t.Fatalf("CreateWorkspace: %v", err) + } + for ws, slug := range map[string]string{"default": "in-default", "wsq": "in-wsq"} { + ctx, release, err := s.Scoped(base, ws) + if err != nil { + t.Fatalf("scope %s: %v", ws, err) + } + ctx = context.WithValue(ctx, reqWSKey{}, reqWS{name: ws}) + if _, err := s.CreateDocument(ctx, slug, slug, "note", nil, nil, nil, "", "tester"); err != nil { + release() + t.Fatalf("create %s: %v", slug, err) + } + release() + } + + srv := &Server{store: s, cfg: Config{DefaultWorkspace: "default"}} + mux := silentWritesMux(srv) + + var out struct { + Documents []struct { + Slug string `json:"slug"` + } `json:"documents"` + } + w := do(t, mux, "GET", "/docs?workspace=wsq", "") + if err := json.Unmarshal(w.Body.Bytes(), &out); err != nil { + t.Fatalf("decode: %v", err) + } + if len(out.Documents) != 1 || out.Documents[0].Slug != "in-wsq" { + t.Fatalf("?workspace=wsq returned %+v — the query was ignored and the default "+ + "workspace answered with 200", out.Documents) + } +} + +// A folio-style slug must reach its own revisions and their bytes. +func TestRevisionSubRoutesByMultiSegmentSlug(t *testing.T) { + s := testStore(t) + srv := &Server{store: s, cfg: Config{DefaultWorkspace: "default"}} + mux := silentWritesMux(srv) + + ctx, release, err := s.Scoped(context.Background(), "default") + if err != nil { + t.Fatalf("scope: %v", err) + } + ctx = context.WithValue(ctx, reqWSKey{}, reqWS{name: "default"}) + const slug = "myfolio/file.md" + doc, err := s.CreateDocument(ctx, slug, "file", "note", nil, nil, + []byte("VERSION ONE"), "text/markdown", "tester") + if err != nil { + release() + t.Fatalf("create: %v", err) + } + lease, err := s.AcquireLease(ctx, slug, "tester", "test", 60_000_000_000, "") + if err != nil { + release() + t.Fatalf("lease: %v", err) + } + if _, err := s.WriteContent(ctx, slug, "tester", lease.LeaseToken, doc.Version, + "text/markdown", []byte("VERSION TWO")); err != nil { + release() + t.Fatalf("write v2: %v", err) + } + release() + + w := do(t, mux, "GET", "/docs/"+slug+"/revisions", "") + if w.Code != http.StatusOK { + t.Fatalf("/revisions by slug = %d, want 200 (body %s)", w.Code, w.Body.String()) + } + var revs struct { + Count int `json:"count"` + } + if err := json.Unmarshal(w.Body.Bytes(), &revs); err != nil || revs.Count < 2 { + t.Fatalf("revisions count = %d, want >= 2 (err %v)", revs.Count, err) + } + + // The old bytes, not the current ones — a plain "/raw" fallback would + // silently serve VERSION TWO here. + w = do(t, mux, "GET", "/docs/"+slug+"/revisions/1/raw", "") + if w.Code != http.StatusOK { + t.Fatalf("/revisions/1/raw = %d, want 200 (body %s)", w.Code, w.Body.String()) + } + got, _ := io.ReadAll(w.Body) + if string(got) != "VERSION ONE" { + t.Fatalf("revision 1 raw = %q, want %q", got, "VERSION ONE") + } + + // The plain content route must still serve the current bytes. + w = do(t, mux, "GET", "/docs/"+slug+"/raw", "") + got, _ = io.ReadAll(w.Body) + if string(got) != "VERSION TWO" { + t.Fatalf("current raw = %q, want %q", got, "VERSION TWO") + } +} diff --git a/silent_writes_test.go b/silent_writes_test.go new file mode 100644 index 0000000..8b76061 --- /dev/null +++ b/silent_writes_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "net/http/httptest" + "testing" +) + +// Unit coverage for the two pure helpers only. Each fails against the behaviour +// it replaces, verified by reverting the fix on a copied tree: without the query +// fallback workspaceSelector returns "", and without cutRevisionRaw a revision +// path is swallowed by the plain "/raw" suffix. +// +// The PATCH change and the end-to-end routing are covered by DB-backed handler +// tests in silent_writes_db_test.go, not here. An earlier version of this +// comment claimed the PATCH regression was covered when no PATCH test existed. + +func TestWorkspaceSelector(t *testing.T) { + for _, tc := range []struct { + name, header, query, want string + }{ + {"header only", "alpha", "", "alpha"}, + // The bug: ?workspace= was never read, so a caller asking for another + // workspace got the default one back with a 200. + {"query only", "", "beta", "beta"}, + {"header wins over query", "alpha", "beta", "alpha"}, + {"neither", "", "", ""}, + {"whitespace is not a selection", " ", "", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest("GET", "/docs?workspace="+tc.query, nil) + if tc.header != "" { + r.Header.Set("X-Workspace", tc.header) + } + if got := workspaceSelector(r); got != tc.want { + t.Fatalf("workspaceSelector = %q, want %q", got, tc.want) + } + }) + } +} + +func TestCutRevisionRaw(t *testing.T) { + for _, tc := range []struct { + rest string + base string + version int + ok bool + }{ + {"myfolio/file.md/revisions/3/raw", "myfolio/file.md", 3, true}, + {"fleet/alpha-testpane-codex/revisions/12/raw", "fleet/alpha-testpane-codex", 12, true}, + // A slug may itself contain "/revisions/", so the split takes the last one. + {"a/revisions/b/revisions/2/raw", "a/revisions/b", 2, true}, + // Must not steal the plain content route. + {"myfolio/file.md/raw", "", 0, false}, + {"myfolio/file.md/revisions", "", 0, false}, + {"myfolio/file.md/revisions/notanumber/raw", "", 0, false}, + {"myfolio/file.md", "", 0, false}, + } { + t.Run(tc.rest, func(t *testing.T) { + base, version, ok := cutRevisionRaw(tc.rest) + if ok != tc.ok || base != tc.base || version != tc.version { + t.Fatalf("cutRevisionRaw(%q) = (%q, %d, %v), want (%q, %d, %v)", + tc.rest, base, version, ok, tc.base, tc.version, tc.ok) + } + }) + } +} diff --git a/web/usage.md b/web/usage.md index 15d2699..3adc06c 100644 --- a/web/usage.md +++ b/web/usage.md @@ -59,7 +59,9 @@ metadata, title}` changes labels **without** a lease, content rewrite, or versio ### Write API `POST /docs` (`{slug, title, kind, tags, metadata, content, content_type}`) creates; -`PATCH /docs/{id}` (`{title, kind, tags, metadata, content, content_type}`) updates; +`PATCH /docs/{id}` (`{title, kind, tags, metadata}`) relabels — it does **not** +write content, and a body carrying `content` is refused with 400; use +`PUT /docs/{id}` with `If-Match` and a lease for that; `DELETE /docs/{id}` soft-deletes (restore with `POST /docs/{id}/restore`); `DELETE /docs/{id}?confirm={slug}` hard-deletes.