Refuse writes PATCH cannot perform; honour ?workspace=; reach sub-routes by full slug

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,22 +566,50 @@ 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
 	}
 	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"))
 	if err != nil {
 		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

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)

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")
+	}
+}