mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 06:19:49 +00:00
fix(mcp): a refused call names its cause on both transports (BUG-3864) (#2218)
* fix(mcp): a refused call names its cause on both transports (BUG-3864) A 403 from one of the shared gates now carries details.reason (role, token_scope, no_access, workspace_not_authorized), with code and message unchanged. MCP builds one permission_denied envelope from it on both transports, with a hint that says what to do next. It used to lead with "Insufficient role" for every 403, so a bot with a read-only API token was told to ask for a role. An unverified Cloud email is reported as the server's own email_not_verified code. Stdio gets the same envelope from a structured marker the CLI root writes for a 403 coded forbidden, permission_denied or email_not_verified. Before it, the token-scope and unverified-email refusals said server_error there. ToolSurfaceVersion 0.99 -> 0.100 (behavior). Claude-Session: https://claude.ai/code/session_01MPeShv3DBfM9C94PTvMnqi * fix(server): the inline workspace-role gates name their reason too (BUG-3864) The bulk endpoint, the owner-only member doors and the owner-only app install and upgrade doors say role, or no_access for a guest. Claude-Session: https://claude.ai/code/session_01MPeShv3DBfM9C94PTvMnqi * fix(mcp): the permission hint is the same on both transports (BUG-3864) The remote hint appended the route, which stdio has no way to know, so one refusal reached the two transports with different hints. The tests compared prefixes only and missed it; both now assert the transports agree exactly. Claude-Session: https://claude.ai/code/session_01MPeShv3DBfM9C94PTvMnqi * fix(server): no role hint for a caller with no role to change (BUG-3864) requireMinRole and the grants list said role for a guest, who holds grants rather than a role, and for a caller with no workspace role at all. Both now go through roleGateReason, which answers no_access for either. Claude-Session: https://claude.ai/code/session_01MPeShv3DBfM9C94PTvMnqi
This commit is contained in:
@@ -402,7 +402,7 @@ directory for `claude-code`, and an `[mcp_servers.pad]` table in
|
||||
project-scoped, it's install-on-request only — `--all` and `pad mcp status` cover
|
||||
the per-user clients (including Codex) and skip it.
|
||||
|
||||
**Tool catalog (v0.99)** — ten resource × action tools plus `pad_set_workspace`. Undeclared input keys are rejected with a structured error rather than silently dropped, and field values are typed against the collection schema on the server:
|
||||
**Tool catalog (v0.100)** — ten resource × action tools plus `pad_set_workspace`. Undeclared input keys are rejected with a structured error rather than silently dropped, and field values are typed against the collection schema on the server:
|
||||
|
||||
| Tool | Actions |
|
||||
|---|---|
|
||||
@@ -426,7 +426,7 @@ Plus resources at `pad://workspaces`, `pad://workspace/{ws}/dashboard`,
|
||||
and `pad://_meta/version`.
|
||||
|
||||
**Stability contract.** The catalog is versioned
|
||||
(`tool_surface_version: "0.99"`) and advertised in the initialize handshake and at
|
||||
(`tool_surface_version: "0.100"`) and advertised in the initialize handshake and at
|
||||
`pad://_meta/version`, so external agents can pin against it; the per-version
|
||||
changelog lives in [`internal/mcp/version.go`](internal/mcp/version.go). Errors
|
||||
come back as structured envelopes (`{error: {code, message, hint, ...}}`) from a
|
||||
|
||||
@@ -0,0 +1,278 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"os"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/mcp"
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3864: a refused MCP call names its cause. Before, every 403 reached a
|
||||
// remote caller as permission_denied with a hint that LED with "Insufficient
|
||||
// role for this operation", whatever the cause, and the stdio transport,
|
||||
// which reads the CLI's stderr, said server_error for any refusal whose
|
||||
// message did not happen to contain "forbidden". The support case was a bot
|
||||
// with a read-only API token: reads worked, writes were refused, and the
|
||||
// agent was told its role was the problem.
|
||||
//
|
||||
// Each row is a real refusal from the real server, reached through the
|
||||
// shipped dispatchers on both transports: wireMCP's /mcp for remote, and the
|
||||
// real command tree run as a subprocess for stdio. Both must report the same
|
||||
// code, details.reason and hint.
|
||||
|
||||
const (
|
||||
bug3864HintRole = "Your role in this workspace does not allow this. Ask a workspace owner to change your role."
|
||||
bug3864HintScope = "This API token's access does not include this action: a 'Read only' token cannot make changes. Create a token with 'Read and write' access under Account settings → API Tokens, and use it instead."
|
||||
bug3864HintNoAccess = "You do not have access to this workspace or collection. Ask a workspace owner for access."
|
||||
bug3864HintEmail = "Verify your account's email address (the link is in your inbox, and the web app can send a new one). Changes are refused until then."
|
||||
)
|
||||
|
||||
// bug3864Op is one refused write, in both transports' terms.
|
||||
type bug3864Op struct {
|
||||
remote map[string]any // pad_item arguments, less workspace
|
||||
cmdPath []string // the CLI command stdio runs
|
||||
argv []string // its flags and, after "--", its positionals
|
||||
}
|
||||
|
||||
var bug3864Create = bug3864Op{
|
||||
remote: map[string]any{"action": "create", "collection": "tasks", "title": bug3864Title},
|
||||
cmdPath: []string{"item", "create"},
|
||||
argv: []string{"--", "tasks", bug3864Title},
|
||||
}
|
||||
|
||||
type bug3864Want struct {
|
||||
code mcp.ErrorCode
|
||||
reason string // details.reason; "" means the field must be absent
|
||||
hint string // the hint must START with this
|
||||
}
|
||||
|
||||
func TestBUG3864_RefusalsNameTheirCauseOnBothTransports(t *testing.T) {
|
||||
f := newCapFixture(t, cfgHTTP, false, nil, "true")
|
||||
if err := f.store.SeedCollectionsFromTemplate(f.ws.ID, "startup"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
viewer := bug3864User(t, f, "viewer@example.com", false)
|
||||
if err := f.store.AddWorkspaceMember(f.ws.ID, viewer.ID, "viewer"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// A guest holds a view grant on one item. Editing it reaches the edit
|
||||
// check, which a role change cannot satisfy: the guest has no role.
|
||||
guest := bug3864User(t, f, "guest@example.com", false)
|
||||
granted, err := f.store.CreateItem(f.ws.ID, bug3864TasksID(t, f), models.ItemCreate{Title: "granted"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if _, err := f.store.CreateItemGrant(f.ws.ID, granted.ID, guest.ID, "view", f.member.ID); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
guestEdit := bug3864Op{
|
||||
remote: map[string]any{"action": "update", "ref": granted.Ref, "title": bug3864Title},
|
||||
cmdPath: []string{"item", "update"},
|
||||
argv: []string{"--title", bug3864Title, "--", granted.Ref},
|
||||
}
|
||||
|
||||
rows := []struct {
|
||||
name string
|
||||
token string
|
||||
op bug3864Op
|
||||
want bug3864Want
|
||||
}{
|
||||
{"role", bug3864Token(t, f, viewer, `["*"]`), bug3864Create, bug3864Want{mcp.ErrPermissionDenied, "role", bug3864HintRole}},
|
||||
{"token_scope", bug3864Token(t, f, f.member, `["read"]`), bug3864Create, bug3864Want{mcp.ErrPermissionDenied, "token_scope", bug3864HintScope}},
|
||||
// A non-member is answered 404, which hides that the workspace exists.
|
||||
// The 403 is a platform admin's bearer token, which reaches only the
|
||||
// workspaces the admin is a member of (BUG-1616).
|
||||
{"no_access", bug3864Token(t, f, f.admin, `["*"]`), bug3864Create, bug3864Want{mcp.ErrPermissionDenied, "no_access", bug3864HintNoAccess}},
|
||||
{"guest edit", bug3864Token(t, f, guest, `["*"]`), guestEdit, bug3864Want{mcp.ErrPermissionDenied, "no_access", bug3864HintNoAccess}},
|
||||
}
|
||||
for _, r := range rows {
|
||||
var remote, stdio mcp.ErrorEnvelope
|
||||
t.Run(r.name+"/remote", func(t *testing.T) {
|
||||
remote = bug3864Remote(t, f, r.token, r.op)
|
||||
bug3864Check(t, remote, r.want)
|
||||
})
|
||||
t.Run(r.name+"/stdio", func(t *testing.T) {
|
||||
stdio = bug3864Stdio(t, f, r.token, r.op)
|
||||
bug3864Check(t, stdio, r.want)
|
||||
})
|
||||
bug3864Same(t, r.name, remote, stdio)
|
||||
}
|
||||
if n := bug3864CountTitled(t, f); n != 0 {
|
||||
t.Errorf("a refused write created %d items", n)
|
||||
}
|
||||
}
|
||||
|
||||
// The Cloud-only cause: an account whose email is unverified may read and may
|
||||
// not write. The remote transport refuses in its own gate before the request
|
||||
// is built; stdio reaches the server's RequireVerifiedEmail middleware.
|
||||
func TestBUG3864_UnverifiedEmailIsItsOwnCodeOnBothTransports(t *testing.T) {
|
||||
f := newCapFixture(t, cfgHTTP, true, nil, "true")
|
||||
if err := f.store.SeedCollectionsFromTemplate(f.ws.ID, "startup"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
unverified := bug3864User(t, f, "unverified@example.com", true)
|
||||
if err := f.store.AddWorkspaceMember(f.ws.ID, unverified.ID, "owner"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
tok := bug3864Token(t, f, unverified, `["*"]`)
|
||||
want := bug3864Want{mcp.ErrorCode("email_not_verified"), "", bug3864HintEmail}
|
||||
var remote, stdio mcp.ErrorEnvelope
|
||||
t.Run("remote", func(t *testing.T) { remote = bug3864Remote(t, f, tok, bug3864Create); bug3864Check(t, remote, want) })
|
||||
t.Run("stdio", func(t *testing.T) { stdio = bug3864Stdio(t, f, tok, bug3864Create); bug3864Check(t, stdio, want) })
|
||||
bug3864Same(t, "unverified email", remote, stdio)
|
||||
}
|
||||
|
||||
func bug3864User(t *testing.T, f *capFixture, email string, unverified bool) *models.User {
|
||||
t.Helper()
|
||||
u, err := f.store.CreateUser(models.UserCreate{Email: email, Name: email, Password: "correct-horse-battery-staple", Unverified: unverified})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
return u
|
||||
}
|
||||
|
||||
func bug3864Token(t *testing.T, f *capFixture, u *models.User, scopes string) string {
|
||||
t.Helper()
|
||||
tok, err := f.store.CreateAPIToken(u.ID, models.APITokenCreate{Name: "bot", Scopes: scopes}, 30, 0)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
return tok.Token
|
||||
}
|
||||
|
||||
const bug3864Title = "bug3864 refused write"
|
||||
|
||||
func bug3864Remote(t *testing.T, f *capFixture, token string, op bug3864Op) mcp.ErrorEnvelope {
|
||||
t.Helper()
|
||||
const proto = "2025-11-25"
|
||||
call := func(id int, method string, params map[string]any) []byte {
|
||||
t.Helper()
|
||||
body, _ := json.Marshal(map[string]any{"jsonrpc": "2.0", "id": id, "method": method, "params": params})
|
||||
req := httptest.NewRequest("POST", "/mcp", bytes.NewReader(body))
|
||||
req.Host = f.host
|
||||
req.RemoteAddr = "192.0.2.10:4242"
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set("Accept", "application/json, text/event-stream")
|
||||
req.Header.Set("MCP-Protocol-Version", proto)
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
rr := newMCPRecorder()
|
||||
f.srv.ServeHTTP(rr, req)
|
||||
if rr.Code != http.StatusOK {
|
||||
t.Fatalf("%s: %d %s", method, rr.Code, rr.Body.String())
|
||||
}
|
||||
return rr.Body.Bytes()
|
||||
}
|
||||
call(1, "initialize", map[string]any{
|
||||
"protocolVersion": proto, "capabilities": map[string]any{},
|
||||
"clientInfo": map[string]any{"name": "bug3864", "version": "1"},
|
||||
})
|
||||
args := map[string]any{"workspace": f.ws.Slug}
|
||||
for k, v := range op.remote {
|
||||
args[k] = v
|
||||
}
|
||||
out := call(2, "tools/call", map[string]any{"name": "pad_item", "arguments": args})
|
||||
var rpc struct {
|
||||
Result struct {
|
||||
IsError bool `json:"isError"`
|
||||
StructuredContent mcp.ErrorEnvelope `json:"structuredContent"`
|
||||
} `json:"result"`
|
||||
}
|
||||
if err := json.Unmarshal(out, &rpc); err != nil {
|
||||
t.Fatalf("decode tools/call: %v: %s", err, out)
|
||||
}
|
||||
if !rpc.Result.IsError {
|
||||
t.Fatalf("PRECONDITION: the write should have been refused: %s", out)
|
||||
}
|
||||
return rpc.Result.StructuredContent
|
||||
}
|
||||
|
||||
func bug3864Stdio(t *testing.T, f *capFixture, token string, op bug3864Op) mcp.ErrorEnvelope {
|
||||
t.Helper()
|
||||
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
r.Host = f.host
|
||||
r.RemoteAddr = "192.0.2.10:4242"
|
||||
f.srv.ServeHTTP(w, r)
|
||||
}))
|
||||
defer ts.Close()
|
||||
|
||||
t.Setenv(padHelperEnv, "1")
|
||||
t.Setenv("HOME", t.TempDir())
|
||||
t.Setenv("PAD_TOKEN", token)
|
||||
bin, err := os.Executable()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
d := &mcp.ExecDispatcher{Binary: bin}
|
||||
argv := append([]string{"--url", ts.URL, "--workspace", f.ws.Slug, "--format", "json"}, op.argv...)
|
||||
res, err := d.Dispatch(context.Background(), op.cmdPath, argv)
|
||||
if err != nil {
|
||||
t.Fatalf("Dispatch: %v", err)
|
||||
}
|
||||
env, ok := res.StructuredContent.(mcp.ErrorEnvelope)
|
||||
if !ok {
|
||||
t.Fatalf("PRECONDITION: the write should have been refused; got %T %+v", res.StructuredContent, res.Content)
|
||||
}
|
||||
return env
|
||||
}
|
||||
|
||||
func bug3864Check(t *testing.T, env mcp.ErrorEnvelope, want bug3864Want) {
|
||||
t.Helper()
|
||||
got := ""
|
||||
if len(env.Error.Details) > 0 {
|
||||
var d struct {
|
||||
Reason string `json:"reason"`
|
||||
}
|
||||
_ = json.Unmarshal(env.Error.Details, &d)
|
||||
got = d.Reason
|
||||
}
|
||||
if env.Error.Code != want.code || got != want.reason || !strings.HasPrefix(env.Error.Hint, want.hint) {
|
||||
t.Errorf("got code=%q reason=%q hint=%q (message %q)\nwant code=%q reason=%q hint starting %q",
|
||||
env.Error.Code, got, env.Error.Hint, env.Error.Message, want.code, want.reason, want.hint)
|
||||
}
|
||||
if want.reason != "role" && strings.Contains(strings.ToLower(env.Error.Hint), "role") {
|
||||
t.Errorf("a %q refusal must not blame a role: hint %q", want.reason, env.Error.Hint)
|
||||
}
|
||||
}
|
||||
|
||||
func bug3864CountTitled(t *testing.T, f *capFixture) int {
|
||||
t.Helper()
|
||||
items, err := f.store.ListItems(f.ws.ID, models.ItemListParams{})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
n := 0
|
||||
for _, it := range items {
|
||||
if it.Title == bug3864Title {
|
||||
n++
|
||||
}
|
||||
}
|
||||
return n
|
||||
}
|
||||
|
||||
func bug3864TasksID(t *testing.T, f *capFixture) string {
|
||||
t.Helper()
|
||||
c, err := f.store.GetCollectionBySlug(f.ws.ID, "tasks")
|
||||
if err != nil || c == nil {
|
||||
t.Fatalf("tasks collection: %v", err)
|
||||
}
|
||||
return c.ID
|
||||
}
|
||||
|
||||
// bug3864Same: one refusal, one answer. The message may differ (the remote
|
||||
// dispatcher refuses some writes before the server sees them), but the code,
|
||||
// details and hint an agent acts on may not.
|
||||
func bug3864Same(t *testing.T, name string, remote, stdio mcp.ErrorEnvelope) {
|
||||
t.Helper()
|
||||
if remote.Error.Code != stdio.Error.Code || remote.Error.Hint != stdio.Error.Hint || string(remote.Error.Details) != string(stdio.Error.Details) {
|
||||
t.Errorf("%s: the transports differ:\n remote code=%q details=%s hint=%q\n stdio code=%q details=%s hint=%q", name,
|
||||
remote.Error.Code, remote.Error.Details, remote.Error.Hint, stdio.Error.Code, stdio.Error.Details, stdio.Error.Hint)
|
||||
}
|
||||
}
|
||||
@@ -95,6 +95,11 @@ func writeRootStructuredError(w io.Writer, err error) {
|
||||
// PLAN-3578: a patch whose anchor no longer matches, or an open item.
|
||||
if apiErr, ok := cli.IsPatchRefusal(err); ok {
|
||||
cli.WritePatchRefusalError(w, apiErr)
|
||||
return
|
||||
}
|
||||
// BUG-3864: a 403 refusing the caller, with the server's details.reason.
|
||||
if apiErr, ok := cli.IsPermissionDenied(err); ok {
|
||||
cli.WritePermissionDeniedError(w, apiErr)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+2
-2
File diff suppressed because one or more lines are too long
+2
-2
File diff suppressed because one or more lines are too long
@@ -7,6 +7,7 @@ Moved verbatim from CLAUDE.md (TASK-3485), where it had pushed the file past Cla
|
||||
REST API at `/api/v1/`. Key endpoints:
|
||||
|
||||
- **A workspace that does not resolve for the caller answers `404 not_found` with `details: {"scope": "workspace"}`** (BUG-3069) — a non-member and a deleted workspace alike, so it is not an existence oracle. A 404 about something INSIDE a workspace the caller can read (an item, a collection, a guest's ungranted item) carries no such marker. Code, status and message are unchanged from before the marker, so a consumer keyed on `not_found` is unaffected; key on `details.scope` to tell the two apart. A workspace addressed by UUID that the caller is not a member of answers `403 forbidden` instead, a separate refusal
|
||||
- **A 403 from one of the shared gates names the gate in `details.reason`** (BUG-3864): `role` (the caller's workspace role is too low: `requireMinRole`, the edit check for a viewer, the grants list), `token_scope` (the API token's access does not include the action, i.e. a "Read only" token writing), `no_access` (not a member, a platform admin's bearer token on a workspace they are not in, an editor outside their collection access, a guest without the grant, or the cross-workspace check), and `workspace_not_authorized` (an OAuth connection's consent allow-list excludes the workspace; code `permission_denied`). Code and message are unchanged, so a consumer keyed on `forbidden` is unaffected. Any other 403 carries no reason, and its message is the only account of the cause. MCP reads the reason to tell the caller what to fix, on both transports; before this it said "Insufficient role" for every 403. An unverified Cloud email keeps its own code, `403 email_not_verified`, which MCP now reports as that code instead of `permission_denied`
|
||||
- `GET/POST /workspaces/{ws}/collections` — collection CRUD. `DELETE /collections/{slug}` ARCHIVES: a soft delete of the collection row, its items untouched and only hidden with it; nothing purges an archived collection (TASK-2189). A create or update whose `schema` has a field with no key, two fields with one key, or a select or multi_select field with an empty option or the same option twice answers **400 `validation_error`** naming it (`field key "status" is defined more than once`, `field "status" lists the option "open" more than once`, …), and nothing is written (TASK-3539): every client renders these as lists keyed by the value. It is a write check only: a schema already stored with a repeat still reads, an update that sends no `schema` still applies, and the web renders such a schema with each field and option once
|
||||
- **Work or reference: `settings.tracks_work`** (PLAN-3535). `false` makes a collection REFERENCE material (docs, conventions, playbooks): its items do not count toward parent progress (every progress endpoint and the dashboard's active plans), do not block a parent from closing (`open_children`), stay out of a default report (`?collections` unset; naming the collection still includes it), and stay out of open-work counts (dashboard active items, attention, suggestions, `by_role`, `pad project next|ready|stale|standup|changelog`, the attention decision set, the admin `items_open`). They still count in the sidebar and dashboard summary totals, item lists, search and links. A workspace bundle written before the key existed imports its system collections as reference, as the migration marked them. **Absent means work**, today's behaviour, so old rows and writers that never send it are unchanged. A collection CREATE that does not set it gets `false` for a system collection and otherwise `true` iff its items can finish: its done field is a select and one of its options is terminal (its `terminal_options`, or the default terminal list when it declares none, so `open, done` counts). A NEW template Docs collection is created as reference, existing collections keep work except Conventions and Playbooks (migration 135 / PG 107), and owners flip any of them. Because settings are written WHOLESALE, an update whose `settings` omit the key KEEPS the stored value, never resetting it to work. A typed top-level `tracks_work` boolean on POST and PATCH sets that one key without touching the rest of settings, and is what the CLI (`pad collection create|update --tracks-work=true|false`) and MCP (`pad_collection` `tracks_work`, v0.81) send. Any non-boolean value is refused 400. An older server ignores the member and answers 200, so the CLI checks the response
|
||||
- `GET /workspaces/{ws}/collections/{coll}/field-usage` — per field key, how many live items hold a value, and per string value how many hold it (TASK-2188): what the web schema editor shows before a field or option is removed. Owner-only plus full visibility, the PATCH's gates. The PATCH itself answers with an additive `omitempty` `warnings.orphaned` [{field, option?, items}] for each removed field, or option removed from a field that stays a select/multi_select, that live items still hold, read after the commit (a renamed option's migrated values are not listed). The values stay stored: a removed field's come back if a field with that key is declared again; a removed option's stay on their items, survive a `fields_patch` of other keys, and are refused on a write that sets them or a full `fields` blob that carries them. The CLI prints the list to stderr
|
||||
|
||||
+1
-1
File diff suppressed because one or more lines are too long
@@ -3014,6 +3014,44 @@ func WriteTooLargeError(w io.Writer, apiErr *APIError) {
|
||||
}
|
||||
}
|
||||
|
||||
// IsPermissionDenied reports whether err is a server 403 refusing the caller:
|
||||
// code forbidden or permission_denied, or an unverified Cloud email
|
||||
// (BUG-3864). Other 403 codes (plan_limit_exceeded and the like) have writers
|
||||
// of their own, and a marker from this one would replace theirs.
|
||||
func IsPermissionDenied(err error) (*APIError, bool) {
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) || apiErr.Status != http.StatusForbidden {
|
||||
return nil, false
|
||||
}
|
||||
switch apiErr.Code {
|
||||
case "forbidden", "permission_denied", "email_not_verified":
|
||||
return apiErr, true
|
||||
}
|
||||
return nil, false
|
||||
}
|
||||
|
||||
// WritePermissionDeniedError writes the structured marker line for a refused
|
||||
// caller (BUG-3864), carrying the server's details.reason, so the stdio MCP
|
||||
// transport reports which gate refused instead of inferring permission_denied,
|
||||
// or server_error, from the message's wording. No hint: the MCP side builds it
|
||||
// from the reason, the same way for both transports.
|
||||
func WritePermissionDeniedError(w io.Writer, apiErr *APIError) {
|
||||
code := "permission_denied"
|
||||
if apiErr.Code == "email_not_verified" {
|
||||
code = apiErr.Code
|
||||
}
|
||||
body := map[string]any{"code": code, "message": apiErr.Message}
|
||||
var details struct {
|
||||
Reason string `json:"reason"`
|
||||
}
|
||||
if len(apiErr.Details) > 0 && json.Unmarshal(apiErr.Details, &details) == nil && details.Reason != "" {
|
||||
body["details"] = map[string]any{"reason": details.Reason}
|
||||
}
|
||||
if data, err := json.Marshal(map[string]any{"error": body}); err == nil {
|
||||
fmt.Fprintln(w, StructuredErrorMarker+string(data))
|
||||
}
|
||||
}
|
||||
|
||||
// WriteRateLimitedError writes the structured marker line for a 429
|
||||
// (BUG-3147), so the stdio MCP transport reports rate_limited — with the
|
||||
// server's wait — instead of inferring server_error from prose that matches
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
package mcp
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/cli"
|
||||
)
|
||||
|
||||
// BUG-3864, the classifier half: the same 403 body reaches the same envelope
|
||||
// on both transports. Remote reads the body; stdio reads the marker the CLI
|
||||
// root writes from the same body. cmd/pad's end-to-end test drives the rows a
|
||||
// fixture can reach; these are the ones it cannot (an OAuth allow-list, an
|
||||
// older server, a newer server's reason).
|
||||
func TestBUG3864_A403ReadsTheSameOnBothTransports(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name, code, message, reason string
|
||||
wantCode ErrorCode
|
||||
wantReason string
|
||||
wantHint string
|
||||
}{
|
||||
{"workspace allow-list", "permission_denied", "Token is not authorized for this workspace", "workspace_not_authorized",
|
||||
ErrPermissionDenied, "workspace_not_authorized", PermissionWorkspaceNotAuthorizedHint},
|
||||
{"role", "forbidden", "Insufficient permissions", "role", ErrPermissionDenied, "role", PermissionRoleHint},
|
||||
// An older server says no reason. The hint must not guess one.
|
||||
{"no reason", "forbidden", "Only the comment author can edit it.", "",
|
||||
ErrPermissionDenied, "", "The server refused this operation: Only the comment author can edit it."},
|
||||
// A newer server's reason this build has no words for: dropped, so
|
||||
// details.reason stays a closed vocabulary, and the message speaks.
|
||||
{"unknown reason", "forbidden", "Refused by a gate from the future", "future_gate",
|
||||
ErrPermissionDenied, "", "The server refused this operation: Refused by a gate from the future."},
|
||||
{"unverified email", "email_not_verified", "Verify your email address to perform this action.", "",
|
||||
ErrEmailNotVerified, "", EmailNotVerifiedHint},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
api := &cli.APIError{Code: tc.code, Message: tc.message, Status: http.StatusForbidden}
|
||||
if tc.reason != "" {
|
||||
api.Details, _ = json.Marshal(map[string]string{"reason": tc.reason})
|
||||
}
|
||||
body, _ := json.Marshal(map[string]any{"error": api})
|
||||
|
||||
remote := decodeEnvelope(t, classifyHTTPStatus(context.Background(), "item create", http.StatusForbidden, body, nil))
|
||||
|
||||
var stderr bytes.Buffer
|
||||
stderr.WriteString("Error: " + tc.message + "\n")
|
||||
if _, ok := cli.IsPermissionDenied(api); !ok {
|
||||
t.Fatalf("the CLI root would write no marker for code %q", tc.code)
|
||||
}
|
||||
cli.WritePermissionDeniedError(&stderr, api)
|
||||
lifted := extractStructuredCLIError(stderr.String())
|
||||
if lifted == nil {
|
||||
t.Fatalf("stdio did not lift the marker: %s", stderr.String())
|
||||
}
|
||||
stdio := decodeEnvelope(t, lifted)
|
||||
|
||||
if remote.Error.Hint != stdio.Error.Hint || remote.Error.Code != stdio.Error.Code || string(remote.Error.Details) != string(stdio.Error.Details) {
|
||||
t.Errorf("the transports differ:\n remote %+v\n stdio %+v", remote.Error, stdio.Error)
|
||||
}
|
||||
for door, env := range map[string]ErrorEnvelope{"remote": remote, "stdio": stdio} {
|
||||
if env.Error.Code != tc.wantCode || reasonOf(t, env) != tc.wantReason || !strings.HasPrefix(env.Error.Hint, tc.wantHint) {
|
||||
t.Errorf("%s: code=%q reason=%q hint=%q, want %q %q hint starting %q",
|
||||
door, env.Error.Code, reasonOf(t, env), env.Error.Hint, tc.wantCode, tc.wantReason, tc.wantHint)
|
||||
}
|
||||
if tc.wantReason != "role" && strings.Contains(strings.ToLower(env.Error.Hint), "role") {
|
||||
t.Errorf("%s: hint blames a role: %q", door, env.Error.Hint)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Other 403 codes keep their own writers. A permission_denied marker from the
|
||||
// root would come after theirs and, last marker winning, replace it.
|
||||
func TestBUG3864_TheRootWriterLeavesOtherRefusalsAlone(t *testing.T) {
|
||||
for _, code := range []string{"plan_limit_exceeded", "workspace_member_limit", "insufficient_scope", "account_disabled"} {
|
||||
if _, ok := cli.IsPermissionDenied(&cli.APIError{Code: code, Status: http.StatusForbidden}); ok {
|
||||
t.Errorf("IsPermissionDenied claims %q", code)
|
||||
}
|
||||
}
|
||||
if _, ok := cli.IsPermissionDenied(&cli.APIError{Code: "forbidden", Status: http.StatusBadRequest}); ok {
|
||||
t.Error("IsPermissionDenied claims a non-403")
|
||||
}
|
||||
}
|
||||
@@ -544,22 +544,14 @@ func buildRequestErrorResult(cmdKey string, err error) *CallToolResult {
|
||||
// agents see the same code as a backend 403, not a
|
||||
// generic server_error.
|
||||
if strings.HasPrefix(err.Error(), "permission_denied:") {
|
||||
return NewErrorResult(ErrorPayload{
|
||||
Code: ErrPermissionDenied,
|
||||
Message: fmt.Sprintf("%s: %s", cmdKey, err.Error()),
|
||||
Hint: "Token scope does not permit this operation. Re-issue with the required scopes (read for GET; write for POST/PATCH; admin for workspace settings).",
|
||||
})
|
||||
return NewErrorResult(tokenScopeDeniedPayload(cmdKey, err))
|
||||
}
|
||||
// PLAN-1933 DR-4: the RequireVerifiedEmail gate rejected this
|
||||
// write because the cloud user hasn't verified their email.
|
||||
// Surface as permission_denied (the closest closed-set code) so
|
||||
// agents branch consistently with a backend 403.
|
||||
// PLAN-1933 DR-4: the RequireVerifiedEmail gate rejected this write
|
||||
// because the cloud user hasn't verified their email. The server's own
|
||||
// code for the same refusal (BUG-3864), so stdio, which reaches the
|
||||
// server's gate instead of this one, reports the same thing.
|
||||
if strings.HasPrefix(err.Error(), errEmailNotVerifiedPrefix+":") {
|
||||
return NewErrorResult(ErrorPayload{
|
||||
Code: ErrPermissionDenied,
|
||||
Message: fmt.Sprintf("%s: %s", cmdKey, err.Error()),
|
||||
Hint: "Verify your email address (check your inbox for the verification link) before creating or editing content.",
|
||||
})
|
||||
return NewErrorResult(emailNotVerifiedPayload(fmt.Sprintf("%s: %s", cmdKey, err.Error())))
|
||||
}
|
||||
return dispatcherErrorResult(cmdKey, "build request", err)
|
||||
}
|
||||
@@ -596,6 +588,13 @@ func buildRequestErrorResult(cmdKey string, err error) *CallToolResult {
|
||||
// dispatch_http_project.go's bulk-update path (PATCH issued
|
||||
// directly via buildAuthedRequest + ServeHTTP, bypassing
|
||||
// executeRequest's previous scope check).
|
||||
// tokenScopeDeniedPayload is buildAuthedRequest's scope refusal. That check
|
||||
// mirrors the server's own token-scope gate, so it reports what the server
|
||||
// would have (BUG-3864): permission_denied with reason token_scope.
|
||||
func tokenScopeDeniedPayload(cmdKey string, err error) ErrorPayload {
|
||||
return permissionDeniedPayload(cmdKey+": "+err.Error(), "token_scope")
|
||||
}
|
||||
|
||||
// errEmailNotVerifiedPrefix tags the build-request error the
|
||||
// RequireVerifiedEmail gate returns so executeRequest can map it to a
|
||||
// clean permission_denied envelope (mirrors the "permission_denied:"
|
||||
|
||||
@@ -63,11 +63,7 @@ func (d *HTTPHandlerDispatcher) dispatchItemImport(
|
||||
req, err := d.buildAuthedRequest(ctx, http.MethodPost, urlPath, []byte(artifact), user)
|
||||
if err != nil {
|
||||
if strings.HasPrefix(err.Error(), "permission_denied:") {
|
||||
return NewErrorResult(ErrorPayload{
|
||||
Code: ErrPermissionDenied,
|
||||
Message: cmdKey + ": " + err.Error(),
|
||||
Hint: "Token scope does not permit this operation. Re-issue with a write scope (POST).",
|
||||
}), nil
|
||||
return NewErrorResult(tokenScopeDeniedPayload(cmdKey, err)), nil
|
||||
}
|
||||
return dispatcherErrorResult(cmdKey, "build request", err), nil
|
||||
}
|
||||
|
||||
@@ -3,7 +3,6 @@ package mcp
|
||||
import (
|
||||
"context"
|
||||
"net/http"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/mark3labs/mcp-go/mcp"
|
||||
@@ -57,13 +56,11 @@ func TestHTTPHandlerDispatcher_VerifiedEmail_BlocksUnverifiedWrite(t *testing.T)
|
||||
msg = tc.Text
|
||||
}
|
||||
}
|
||||
// Surfaced as permission_denied (closed-set code) with the
|
||||
// email_not_verified marker in the message.
|
||||
if !strings.Contains(msg, "permission_denied") {
|
||||
t.Errorf("error envelope missing permission_denied marker: %q", msg)
|
||||
}
|
||||
if !strings.Contains(msg, "email_not_verified") {
|
||||
t.Errorf("error envelope missing email_not_verified marker: %q", msg)
|
||||
// The server's own code for the same refusal (BUG-3864), not
|
||||
// permission_denied: no role or token fixes it.
|
||||
env, ok := res.StructuredContent.(ErrorEnvelope)
|
||||
if !ok || env.Error.Code != ErrEmailNotVerified || env.Error.Hint != EmailNotVerifiedHint {
|
||||
t.Errorf("want code %q with EmailNotVerifiedHint, got %q", ErrEmailNotVerified, msg)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+97
-17
@@ -56,10 +56,19 @@ const (
|
||||
// CLI: ~/.pad/credentials.json missing or expired; HTTP: 401.
|
||||
ErrAuthRequired ErrorCode = "auth_required"
|
||||
|
||||
// ErrPermissionDenied fires when authentication succeeds but role
|
||||
// is insufficient for the operation. HTTP: 403.
|
||||
// ErrPermissionDenied fires when authentication succeeds but the
|
||||
// operation is refused. HTTP: 403. details.reason names the gate when the
|
||||
// server said which (BUG-3864): role, token_scope, no_access or
|
||||
// workspace_not_authorized. With no reason, the message is the only
|
||||
// account of the cause, and the hint claims none.
|
||||
ErrPermissionDenied ErrorCode = "permission_denied"
|
||||
|
||||
// ErrEmailNotVerified fires when a Cloud account whose email address is
|
||||
// unverified attempts a write (BUG-3864). The server's own code on every
|
||||
// door that enforces it. Not permission_denied: no role, token or grant
|
||||
// fixes it, and retrying never helps until the address is verified.
|
||||
ErrEmailNotVerified ErrorCode = "email_not_verified"
|
||||
|
||||
// ErrItemNotFound fires when an item ref / slug doesn't resolve.
|
||||
// Distinct from ErrNotFound: this code is reserved for the
|
||||
// specific case where a tool was looking up an item by ref
|
||||
@@ -534,6 +543,15 @@ var allowedStructuredErrorCodes = map[string]struct{}{
|
||||
// (cli.WriteTooLargeError), with the server's own code in details.reason.
|
||||
// Before this both transports said server_error for a deliberate cap.
|
||||
"too_large": {},
|
||||
// BUG-3864. Written at the CLI's root for a 403 whose code is forbidden or
|
||||
// permission_denied (cli.WritePermissionDeniedError), with the server's
|
||||
// details.reason, and for email_not_verified. Before this stdio said
|
||||
// server_error for any refusal whose message lacked "forbidden", which
|
||||
// included the token-scope and unverified-email refusals. Both lifts
|
||||
// rebuild the hint from the reason (permissionDeniedPayload), so the two
|
||||
// transports cannot word the same refusal differently.
|
||||
"permission_denied": {},
|
||||
"email_not_verified": {},
|
||||
}
|
||||
|
||||
// extractStructuredCLIError scans stderr for the
|
||||
@@ -587,6 +605,12 @@ func extractStructuredCLIError(stderr string) *CallToolResult {
|
||||
// Unknown code. Don't forward — regex classifier handles it.
|
||||
return nil
|
||||
}
|
||||
switch ErrorCode(env.Error.Code) {
|
||||
case ErrPermissionDenied:
|
||||
return NewErrorResult(permissionDeniedPayload(env.Error.Message, permissionReasonIn(env.Error.Details)))
|
||||
case ErrEmailNotVerified:
|
||||
return NewErrorResult(emailNotVerifiedPayload(env.Error.Message))
|
||||
}
|
||||
// Hint is forwarded when the writer set one (BUG-2675, Codex round 2).
|
||||
// Without this the two transports delivered the same CODE and different
|
||||
// guidance: remote MCP told the agent retrying is pointless and how to
|
||||
@@ -1002,6 +1026,12 @@ func classifyHTTPStatusKind(
|
||||
// open_children on 409) so unknown 403 codes still collapse to
|
||||
// ErrPermissionDenied.
|
||||
upstream403 := extractUpstreamErrorEnvelope(bodyText)
|
||||
switch ErrorCode(upstream403.Code) {
|
||||
case ErrEmailNotVerified:
|
||||
return NewErrorResult(emailNotVerifiedPayload(upstream403.Message))
|
||||
case ErrPermissionDenied:
|
||||
return NewErrorResult(permissionDeniedPayload(bodyMessage, permissionReasonIn(upstream403.Details)))
|
||||
}
|
||||
if _, allowed := allowedStructuredErrorCodes[upstream403.Code]; allowed {
|
||||
return NewErrorResult(ErrorPayload{
|
||||
Code: ErrorCode(upstream403.Code),
|
||||
@@ -1010,11 +1040,7 @@ func classifyHTTPStatusKind(
|
||||
Details: upstream403.Details,
|
||||
})
|
||||
}
|
||||
return NewErrorResult(ErrorPayload{
|
||||
Code: ErrPermissionDenied,
|
||||
Message: "Permission denied for this operation.",
|
||||
Hint: permissionHintFor(bodyMessage, route),
|
||||
})
|
||||
return NewErrorResult(permissionDeniedPayload(bodyMessage, permissionReasonIn(upstream403.Details)))
|
||||
case http.StatusNotFound:
|
||||
// PLAN-3578 step 3: an undo naming a patch this item does not have is
|
||||
// a 404 about the PATCH, not the item; the generic classifier would
|
||||
@@ -1416,18 +1442,72 @@ func authHintFor(bodyMsg, route string) string {
|
||||
return strings.Join(parts, " ")
|
||||
}
|
||||
|
||||
// permissionHintFor generates the actionable hint for
|
||||
// ErrPermissionDenied. References the route + the inner backend
|
||||
// message (which often names the missing role / workspace).
|
||||
func permissionHintFor(bodyMsg, route string) string {
|
||||
parts := []string{"Insufficient role for this operation."}
|
||||
if route != "" {
|
||||
parts = append(parts, fmt.Sprintf("Route: %s.", route))
|
||||
// The permission_denied hints, by the server's details.reason (BUG-3864).
|
||||
// Each says what the caller does next, in the words the web app uses. A
|
||||
// reason outside this table, or none, gets permissionUnknownHintPrefix and
|
||||
// the server's message instead: the old hint led with "Insufficient role"
|
||||
// for every 403, which sent a bot with a read-only token to ask for a role
|
||||
// it already had.
|
||||
const (
|
||||
PermissionRoleHint = "Your role in this workspace does not allow this. Ask a workspace owner to change your role."
|
||||
PermissionTokenScopeHint = "This API token's access does not include this action: a 'Read only' token cannot make changes. " +
|
||||
"Create a token with 'Read and write' access under Account settings → API Tokens, and use it instead."
|
||||
PermissionNoAccessHint = "You do not have access to this workspace or collection. Ask a workspace owner for access."
|
||||
PermissionWorkspaceNotAuthorizedHint = "This connection was not granted access to this workspace. " +
|
||||
"Reconnect and include it, or use a workspace it was granted."
|
||||
permissionUnknownHintPrefix = "The server refused this operation"
|
||||
|
||||
EmailNotVerifiedHint = "Verify your account's email address (the link is in your inbox, and the web app can send a new one). " +
|
||||
"Changes are refused until then."
|
||||
)
|
||||
|
||||
var permissionReasonHints = map[string]string{
|
||||
"role": PermissionRoleHint,
|
||||
"token_scope": PermissionTokenScopeHint,
|
||||
"no_access": PermissionNoAccessHint,
|
||||
"workspace_not_authorized": PermissionWorkspaceNotAuthorizedHint,
|
||||
}
|
||||
|
||||
// permissionReasonIn reads details.reason from an upstream error's details.
|
||||
func permissionReasonIn(details json.RawMessage) string {
|
||||
var d struct {
|
||||
Reason string `json:"reason"`
|
||||
}
|
||||
if bodyMsg != "" {
|
||||
parts = append(parts, fmt.Sprintf("Backend: %s", bodyMsg))
|
||||
if len(details) == 0 || json.Unmarshal(details, &d) != nil {
|
||||
return ""
|
||||
}
|
||||
return strings.Join(parts, " ")
|
||||
return d.Reason
|
||||
}
|
||||
|
||||
// permissionDeniedPayload is the one permission_denied envelope, for every
|
||||
// door that produces one: the remote 403, the remote dispatcher's own scope
|
||||
// gate, and the stdio marker. details.reason is forwarded only when it is a
|
||||
// reason this build knows, so the field stays a closed vocabulary. No route in
|
||||
// the hint: stdio has none, and the two transports must say the same thing.
|
||||
func permissionDeniedPayload(message, reason string) ErrorPayload {
|
||||
p := ErrorPayload{Code: ErrPermissionDenied, Message: message}
|
||||
if p.Message == "" {
|
||||
p.Message = "Permission denied for this operation."
|
||||
}
|
||||
hint, known := permissionReasonHints[reason]
|
||||
if known {
|
||||
p.Details, _ = json.Marshal(map[string]string{"reason": reason})
|
||||
} else {
|
||||
hint = permissionUnknownHintPrefix
|
||||
if message != "" {
|
||||
hint += ": " + strings.TrimRight(message, ".")
|
||||
}
|
||||
hint += "."
|
||||
}
|
||||
p.Hint = hint
|
||||
return p
|
||||
}
|
||||
|
||||
func emailNotVerifiedPayload(message string) ErrorPayload {
|
||||
if message == "" {
|
||||
message = "Verify your email address to perform this action."
|
||||
}
|
||||
return ErrorPayload{Code: ErrEmailNotVerified, Message: message, Hint: EmailNotVerifiedHint}
|
||||
}
|
||||
|
||||
// CommentHasRepliesHint is the recovery guidance for ErrCommentHasReplies on
|
||||
|
||||
@@ -6,7 +6,7 @@ Pad is a project tracker for developers and AI agents — issues (TASK, BUG), pl
|
||||
|
||||
If the user is asking general code questions with no project-management thread, you don't need this server.
|
||||
|
||||
## Tool surface (v0.99)
|
||||
## Tool surface (v0.100)
|
||||
|
||||
Ten resource × action tools, plus `pad_set_workspace`. Eleven tools total.
|
||||
|
||||
|
||||
@@ -205,6 +205,9 @@ func TestEveryCLIMarkerCodeIsInTheStdioVocabulary(t *testing.T) {
|
||||
"WritePatchRefusalError": func(w *bytes.Buffer) {
|
||||
cli.WritePatchRefusalError(w, api("patch_anchor_missing"))
|
||||
},
|
||||
"WritePermissionDeniedError": func(w *bytes.Buffer) {
|
||||
cli.WritePermissionDeniedError(w, api("forbidden"))
|
||||
},
|
||||
}
|
||||
|
||||
for name, write := range writers {
|
||||
|
||||
+21
-1
@@ -1738,6 +1738,26 @@ const CmdhelpVersion = "0.1"
|
||||
// caller cannot read embeds nothing; a failed fetch stays non-fatal,
|
||||
// as on stdio. Stdio is unchanged.
|
||||
//
|
||||
// 0.100 — BUG-3864. BEHAVIOR bump on the 0.45 grounds; no name, enum
|
||||
// or param shape changed. A refused call names its cause, on BOTH
|
||||
// transports. `permission_denied` gains `details.reason` when the server
|
||||
// says which gate refused (`role`, `token_scope`, `no_access`,
|
||||
// `workspace_not_authorized`), and a hint per reason that says what to
|
||||
// do next. With no reason, or one this build does not know, the hint
|
||||
// leads with the server's message and claims no cause. The old hint led
|
||||
// with "Insufficient role" for every 403, so a bot with a read-only API
|
||||
// token was told to ask for a role. New code `email_not_verified` (the
|
||||
// server's own, on every door that enforces it) replaces the
|
||||
// `permission_denied` the remote gate reported for an unverified Cloud
|
||||
// email. REMOTE: classifyHTTPStatus reads the body's reason, and the
|
||||
// dispatcher's own scope and email gates build the same envelopes.
|
||||
// STDIO: the CLI root writes a structured marker for a 403 coded
|
||||
// forbidden, permission_denied or email_not_verified
|
||||
// (cli.WritePermissionDeniedError); before it, a refusal whose message
|
||||
// lacked "forbidden" (token scope, unverified email) said server_error.
|
||||
// Both lifts build the hint in permissionDeniedPayload. "0.100" follows
|
||||
// "0.99": the version is an opaque dotted string, compared by no code.
|
||||
//
|
||||
// 0.99 — BUG-3790, starred truncation. ADDITIVE bump on the v0.53
|
||||
// grounds: `pad_item.action=starred` returns at most 300 items, most
|
||||
// recently starred first, and when more exist says `has_more: true`
|
||||
@@ -2187,7 +2207,7 @@ const CmdhelpVersion = "0.1"
|
||||
// this surface can receive it; the entry exists so a future action does
|
||||
// not collapse it to permission_denied. When an action that can reach
|
||||
// it is added, that addition is the contract change and owns the bump.
|
||||
const ToolSurfaceVersion = "0.99"
|
||||
const ToolSurfaceVersion = "0.100"
|
||||
|
||||
// MetaVersionURI is the canonical URI of the queryable version document.
|
||||
// Lives outside the pad://workspace/{ws}/... namespace because it's a
|
||||
|
||||
@@ -235,11 +235,11 @@ func (a CrossWorkspaceAccess) WriteDenied(w http.ResponseWriter) {
|
||||
case CrossWorkspaceLookupFailed:
|
||||
writeInternalError(w, a.Err)
|
||||
case CrossWorkspaceTokenNotAllowed:
|
||||
writeError(w, http.StatusForbidden, "permission_denied",
|
||||
"Token is not authorized for this workspace")
|
||||
writeForbidden(w, "permission_denied",
|
||||
"Token is not authorized for this workspace", forbiddenReasonWorkspaceNotAuthorized)
|
||||
default:
|
||||
writeError(w, http.StatusForbidden, "forbidden",
|
||||
"You do not have access to the requested workspace")
|
||||
writeForbidden(w, "forbidden",
|
||||
"You do not have access to the requested workspace", forbiddenReasonNoAccess)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
package server
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/PerpetualSoftware/pad/internal/models"
|
||||
)
|
||||
|
||||
// BUG-3864: the shared 403 gates name which gate refused, in details.reason,
|
||||
// with code and message unchanged. cmd/pad's end-to-end test drives the role,
|
||||
// token-scope and not-a-member gates through /mcp and the CLI; these are the
|
||||
// gates a PAT fixture cannot reach (an OAuth consent allow-list) and the
|
||||
// cross-workspace writer's two arms.
|
||||
|
||||
func bug3864Refusal(t *testing.T, rr *httptest.ResponseRecorder) (code, message, reason string) {
|
||||
t.Helper()
|
||||
if rr.Code != http.StatusForbidden {
|
||||
t.Fatalf("status %d, want 403: %s", rr.Code, rr.Body.String())
|
||||
}
|
||||
var env struct {
|
||||
Error struct {
|
||||
Code string `json:"code"`
|
||||
Message string `json:"message"`
|
||||
Details struct {
|
||||
Reason string `json:"reason"`
|
||||
} `json:"details"`
|
||||
} `json:"error"`
|
||||
}
|
||||
if err := json.Unmarshal(rr.Body.Bytes(), &env); err != nil {
|
||||
t.Fatalf("decode: %v: %s", err, rr.Body.String())
|
||||
}
|
||||
return env.Error.Code, env.Error.Message, env.Error.Details.Reason
|
||||
}
|
||||
|
||||
func TestBUG3864_TheConsentAllowListNamesItself(t *testing.T) {
|
||||
srv := testServer(t)
|
||||
owner := mustCreateUser(t, srv, "owner@example.com", "Owner", "")
|
||||
ws := mustCreateOwnedWorkspace(t, srv, "Refused", owner)
|
||||
|
||||
rr := httptest.NewRecorder()
|
||||
req := consentScopedRequest(http.MethodGet, "/api/v1/workspaces/"+ws.Slug, owner, []string{"some-other-workspace"}, map[string]string{"slug": ws.Slug})
|
||||
srv.RequireWorkspaceAccess(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
|
||||
t.Fatal("the allow-list gate let the request through")
|
||||
})).ServeHTTP(rr, req)
|
||||
|
||||
code, message, reason := bug3864Refusal(t, rr)
|
||||
if code != "permission_denied" || message != "Token is not authorized for this workspace" || reason != forbiddenReasonWorkspaceNotAuthorized {
|
||||
t.Errorf("got %q %q reason %q", code, message, reason)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBUG3864_CrossWorkspaceRefusalsNameTheirGate(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
access CrossWorkspaceAccess
|
||||
code string
|
||||
wantReason string
|
||||
}{
|
||||
{CrossWorkspaceAccess{Reason: CrossWorkspaceTokenNotAllowed}, "permission_denied", forbiddenReasonWorkspaceNotAuthorized},
|
||||
{CrossWorkspaceAccess{}, "forbidden", forbiddenReasonNoAccess},
|
||||
} {
|
||||
rr := httptest.NewRecorder()
|
||||
tc.access.WriteDenied(rr)
|
||||
code, _, reason := bug3864Refusal(t, rr)
|
||||
if code != tc.code || reason != tc.wantReason {
|
||||
t.Errorf("%+v: got %q reason %q, want %q reason %q", tc.access, code, reason, tc.code, tc.wantReason)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// A viewer can be given a higher role. A guest holds grants, not a role, and
|
||||
// a caller with no workspace role (a platform admin who is not a member) has
|
||||
// no role to change, so neither is told to ask for one.
|
||||
func TestBUG3864_RequireMinRoleSaysRoleOnlyWhenARoleCanFixIt(t *testing.T) {
|
||||
for role, want := range map[string]string{
|
||||
"viewer": forbiddenReasonRole,
|
||||
"guest": forbiddenReasonNoAccess,
|
||||
"": forbiddenReasonNoAccess,
|
||||
} {
|
||||
req := httptest.NewRequest(http.MethodPost, "/", nil)
|
||||
if role != "" {
|
||||
req = req.WithContext(context.WithValue(req.Context(), ctxWorkspaceRole, role))
|
||||
}
|
||||
rr := httptest.NewRecorder()
|
||||
if requireMinRole(rr, req, "editor") {
|
||||
t.Fatalf("role %q passed requireMinRole(editor)", role)
|
||||
}
|
||||
code, message, reason := bug3864Refusal(t, rr)
|
||||
if code != "forbidden" || message != "Insufficient permissions" || reason != want {
|
||||
t.Errorf("role %q: got %q %q reason %q, want reason %q", role, code, message, reason, want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// The workspace-role gates that check the role inline rather than through
|
||||
// requireMinRole (the lead's Codex round found the bulk endpoint). Driven
|
||||
// through the router with real tokens, so the role on context is the one the
|
||||
// auth middleware put there. A guest holds grants, not a role.
|
||||
func TestBUG3864_InlineRoleGatesSayRoleOrNoAccess(t *testing.T) {
|
||||
srv := testServer(t)
|
||||
owner := mustCreateUser(t, srv, "owner@example.com", "Owner", "")
|
||||
ws := mustCreateOwnedWorkspace(t, srv, "Gated", owner)
|
||||
member := func(email, role string) string {
|
||||
u := mustCreateUser(t, srv, email, email, "")
|
||||
if role != "" {
|
||||
if err := srv.store.AddWorkspaceMember(ws.ID, u.ID, role); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
tok, err := srv.store.CreateAPIToken(u.ID, models.APITokenCreate{Name: "t"}, 30, 0)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if role == "" { // a guest: one grant, no membership
|
||||
coll, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{Name: "Notes"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
item, err := srv.store.CreateItem(ws.ID, coll.ID, models.ItemCreate{Title: "granted"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if _, err := srv.store.CreateItemGrant(ws.ID, item.ID, u.ID, "view", owner.ID); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
return tok.Token
|
||||
}
|
||||
viewer, editor, guest := member("viewer@example.com", "viewer"), member("editor@example.com", "editor"), member("guest@example.com", "")
|
||||
|
||||
for _, tc := range []struct {
|
||||
name, token, method, path, want string
|
||||
}{
|
||||
{"bulk by a viewer", viewer, http.MethodPost, "/items/bulk", forbiddenReasonRole},
|
||||
{"bulk by a guest", guest, http.MethodPost, "/items/bulk", forbiddenReasonNoAccess},
|
||||
{"invite by an editor", editor, http.MethodPost, "/members/invite", forbiddenReasonRole},
|
||||
{"another member's grants, by a guest", guest, http.MethodGet, "/users/" + owner.ID + "/grants", forbiddenReasonNoAccess},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
req := httptest.NewRequest(tc.method, "/api/v1/workspaces/"+ws.Slug+tc.path, strings.NewReader(`{}`))
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
req.Header.Set("Authorization", "Bearer "+tc.token)
|
||||
rr := httptest.NewRecorder()
|
||||
srv.ServeHTTP(rr, req)
|
||||
if _, _, reason := bug3864Refusal(t, rr); reason != tc.want {
|
||||
t.Errorf("reason %q, want %q: %s", reason, tc.want, rr.Body.String())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -101,7 +101,7 @@ func (s *Server) handleConfirmAppInstall(w http.ResponseWriter, r *http.Request)
|
||||
case errors.As(err, &ae):
|
||||
ae.write(w)
|
||||
case errors.Is(err, store.ErrNotWorkspaceOwner):
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only a workspace owner can install apps")
|
||||
writeForbidden(w, "forbidden", "Only a workspace owner can install apps", roleGateReason(r))
|
||||
case errors.As(err, &pc):
|
||||
details := map[string]interface{}{}
|
||||
if pc.Field != "" {
|
||||
|
||||
@@ -237,7 +237,7 @@ func (s *Server) handleAppUpgradeConfirm(w http.ResponseWriter, r *http.Request)
|
||||
case errors.Is(err, store.ErrPendingManifestChanged):
|
||||
writeError(w, http.StatusConflict, "install_review_mismatch", "This pending upgrade is not the manifest you reviewed; preview it again")
|
||||
case errors.Is(err, store.ErrNotWorkspaceOwner):
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only a workspace owner can upgrade apps")
|
||||
writeForbidden(w, "forbidden", "Only a workspace owner can upgrade apps", roleGateReason(r))
|
||||
default:
|
||||
s.writeInstallLifecycleError(w, err)
|
||||
}
|
||||
|
||||
@@ -351,7 +351,7 @@ func (s *Server) handleListUserGrants(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
// Only workspace owners or the user themselves can view grants
|
||||
if !requireRole(r, "owner") && currentUserID(r) != userID {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Insufficient permissions")
|
||||
writeForbidden(w, "forbidden", "Insufficient permissions", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -122,7 +122,7 @@ func (s *Server) handleBulkItems(w http.ResponseWriter, r *http.Request) {
|
||||
// Owner/editor gated. Bulk lane actions are canEdit-only; viewers
|
||||
// and guests (grant-based access) cannot bulk-mutate.
|
||||
if !requireRole(r, "editor") {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Bulk mutations require editor or owner role")
|
||||
writeForbidden(w, "forbidden", "Bulk mutations require editor or owner role", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -104,7 +104,7 @@ func (s *Server) handleInviteMember(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
// Only owners can invite
|
||||
if !requireRole(r, "owner") {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only workspace owners can invite members")
|
||||
writeForbidden(w, "forbidden", "Only workspace owners can invite members", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
// An invitation carries no scope, so the member it makes sees every
|
||||
@@ -264,7 +264,7 @@ func (s *Server) handleRemoveMember(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
|
||||
if !requireRole(r, "owner") {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only workspace owners can remove members")
|
||||
writeForbidden(w, "forbidden", "Only workspace owners can remove members", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -311,7 +311,7 @@ func (s *Server) handleUpdateMemberRole(w http.ResponseWriter, r *http.Request)
|
||||
}
|
||||
|
||||
if !requireRole(r, "owner") {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only workspace owners can change roles")
|
||||
writeForbidden(w, "forbidden", "Only workspace owners can change roles", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -416,7 +416,7 @@ func (s *Server) handleCancelInvitation(w http.ResponseWriter, r *http.Request)
|
||||
}
|
||||
|
||||
if !requireRole(r, "owner") {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only workspace owners can cancel invitations")
|
||||
writeForbidden(w, "forbidden", "Only workspace owners can cancel invitations", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -776,7 +776,7 @@ func (s *Server) handleGetMemberCollectionAccess(w http.ResponseWriter, r *http.
|
||||
// Only workspace owners or the user themselves can view collection access
|
||||
userID := chi.URLParam(r, "userID")
|
||||
if !requireRole(r, "owner") && currentUserID(r) != userID {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only workspace owners can view other members' collection access")
|
||||
writeForbidden(w, "forbidden", "Only workspace owners can view other members' collection access", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
member, err := s.store.GetWorkspaceMember(workspaceID, userID)
|
||||
@@ -810,7 +810,7 @@ func (s *Server) handleSetMemberCollectionAccess(w http.ResponseWriter, r *http.
|
||||
}
|
||||
|
||||
if !requireRole(r, "owner") {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Only workspace owners can manage collection access")
|
||||
writeForbidden(w, "forbidden", "Only workspace owners can manage collection access", roleGateReason(r))
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -225,7 +225,7 @@ func (s *Server) TokenAuth(next http.Handler) http.Handler {
|
||||
|
||||
// Enforce token scopes
|
||||
if !tokenScopeAllows(apiToken.Scopes, r.Method, r.URL.Path) {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Token scope does not permit this action")
|
||||
writeForbidden(w, "forbidden", "Token scope does not permit this action", forbiddenReasonTokenScope)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -732,8 +732,8 @@ func (s *Server) RequireWorkspaceAccess(next http.Handler) http.Handler {
|
||||
// today, but might in a future PAT-with-allow-list world
|
||||
// — doesn't pollute the MCP-specific counter.
|
||||
s.recordMCPAuthzDenial(r, "workspace_not_in_allowlist")
|
||||
writeError(w, http.StatusForbidden, "permission_denied",
|
||||
"Token is not authorized for this workspace")
|
||||
writeForbidden(w, "permission_denied",
|
||||
"Token is not authorized for this workspace", forbiddenReasonWorkspaceNotAuthorized)
|
||||
return
|
||||
}
|
||||
|
||||
@@ -821,7 +821,7 @@ func (s *Server) RequireWorkspaceAccess(next http.Handler) http.Handler {
|
||||
// non-members hit.
|
||||
if user.Role == "admin" && isBearer {
|
||||
s.recordMCPAuthzDenial(r, "not_a_member")
|
||||
writeError(w, http.StatusForbidden, "forbidden", "You are not a member of this workspace")
|
||||
writeForbidden(w, "forbidden", "You are not a member of this workspace", forbiddenReasonNoAccess)
|
||||
return
|
||||
}
|
||||
// Not a member — check for guest access via grants
|
||||
@@ -837,7 +837,7 @@ func (s *Server) RequireWorkspaceAccess(next http.Handler) http.Handler {
|
||||
// the regular /api/v1 403s would inflate the MCP-
|
||||
// specific counter).
|
||||
s.recordMCPAuthzDenial(r, "not_a_member")
|
||||
writeError(w, http.StatusForbidden, "forbidden", "You are not a member of this workspace")
|
||||
writeForbidden(w, "forbidden", "You are not a member of this workspace", forbiddenReasonNoAccess)
|
||||
return
|
||||
}
|
||||
ctx = context.WithValue(ctx, ctxWorkspaceRole, "guest")
|
||||
@@ -922,7 +922,7 @@ func requireMinRole(w http.ResponseWriter, r *http.Request, minRole string) bool
|
||||
if requireRole(r, minRole) {
|
||||
return true
|
||||
}
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Insufficient permissions")
|
||||
writeForbidden(w, "forbidden", "Insufficient permissions", roleGateReason(r))
|
||||
return false
|
||||
}
|
||||
|
||||
|
||||
@@ -2777,6 +2777,33 @@ func writeError(w http.ResponseWriter, status int, code, message string) {
|
||||
})
|
||||
}
|
||||
|
||||
// Values of details.reason on a 403, naming which gate refused (BUG-3864).
|
||||
// Code and message are unchanged, so the web app and CLI read what they always
|
||||
// did; MCP reads the reason to tell a caller what to fix, which used to be
|
||||
// "Insufficient role" whatever the cause. A 403 with no reason is any other
|
||||
// refusal, and its message is the only account of it.
|
||||
const (
|
||||
forbiddenReasonRole = "role"
|
||||
forbiddenReasonTokenScope = "token_scope"
|
||||
forbiddenReasonNoAccess = "no_access"
|
||||
forbiddenReasonWorkspaceNotAuthorized = "workspace_not_authorized"
|
||||
)
|
||||
|
||||
// roleGateReason is the reason for a refusal by a workspace-role gate. A
|
||||
// guest holds grants, not a role, and a caller with no workspace role at all
|
||||
// (a platform admin who is not a member) has none to change: no role change
|
||||
// admits either.
|
||||
func roleGateReason(r *http.Request) string {
|
||||
if role := workspaceRole(r); role == "guest" || role == "" {
|
||||
return forbiddenReasonNoAccess
|
||||
}
|
||||
return forbiddenReasonRole
|
||||
}
|
||||
|
||||
func writeForbidden(w http.ResponseWriter, code, message, reason string) {
|
||||
writeError2(w, http.StatusForbidden, code, message, map[string]interface{}{"reason": reason})
|
||||
}
|
||||
|
||||
// workspaceNotFoundDetails marks a 404 as the WORKSPACE failing to resolve
|
||||
// for this caller (a non-member, or a deleted workspace), as opposed to a
|
||||
// sub-resource inside a workspace the caller can read (BUG-3069). The two
|
||||
@@ -3757,7 +3784,13 @@ func (s *Server) requireEditPermission(w http.ResponseWriter, r *http.Request, w
|
||||
return false
|
||||
}
|
||||
if !ok {
|
||||
writeError(w, http.StatusForbidden, "forbidden", "Insufficient permissions")
|
||||
// An editor or owner refused here is outside their collection access,
|
||||
// and a guest holds grants, not a role: neither is fixed by a role change.
|
||||
reason := roleGateReason(r)
|
||||
if requireRole(r, "editor") {
|
||||
reason = forbiddenReasonNoAccess
|
||||
}
|
||||
writeForbidden(w, "forbidden", "Insufficient permissions", reason)
|
||||
return false
|
||||
}
|
||||
return true
|
||||
|
||||
Reference in New Issue
Block a user