From 01999c70f928a0963f60dc1670f68e7201576b1b Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 16 Aug 2026 12:07:41 +0300 Subject: [PATCH] graph: keep webhook management with the owner, not with any agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving /query to the anonymous router admitted tokens.sr.ht working tokens, and webhookAuthorized asked only whether the caller was AUTH_INTERNAL. coreauth maps the owner and its agents alike to INTERNAL — deliberately, because INTERNAL is what core-go's NewAuthConfig and FilterWebhooks demand of anyone at all — so a token carrying nothing but spec:read could create and delete subscriptions and read every subscription's URL, stored query and delivery bodies. A read grant was buying a mutation, which is the one thing a grant vocabulary exists to prevent. Measured before the fix: a spec:read token created two subscriptions against a live daemon. The guard now asks spec's own principal for IsOwner, which is the only value left that still tells the owner from an agent. That restores exactly what the daemon's ownerOnly wrapper enforced before the conversion, and it needs no new grant string: naming a third grant beside spec:read and spec:propose is a vocabulary decision for tokens.sr.ht to mint, not something to invent at a call site. One guard covers the whole webhook chapter. createUserWebhook, deleteUserWebhook, userWebhooks and userWebhook all call it, and the two field resolvers that carry webhook data are reachable only through an object one of those four returned. Query.webhook has no ACL and needs none — outside a delivery there is no payload, so it answers an error; asserted rather than assumed. The consequence, and it is not small: webhook management is now unreachable over /query, because this endpoint accepts no credential that resolves to the owner — authn produces KindOwner from the unified-login cookie alone and this endpoint reads no cookie. The check is written against the right predicate anyway, so a plane that does yield the owner works the moment it exists. Delivery is untouched, and was verified end to end against a subscription inserted directly. Also stop returning coreerrors.ErrAccessDenied itself. It is a package-level *gqlerror.Error and gqlgen assigns the field path onto the error it is handed, so the shared object carried the previous refusal's path: against a live daemon a refused deleteUserWebhook, userWebhooks and userWebhook all reported "path":["createUserWebhook"]. A wrong answer, a cross-request leak of which field somebody else asked for, and a data race on a global. The bug predates this endpoint but was latent while ownerOnly refused non-owners before any resolver ran; refusing agents here makes it the common path. --- graph/schema.resolvers.go | 81 ++++++++++++--- graph/webhook_authz_test.go | 201 ++++++++++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 17 deletions(-) create mode 100644 graph/webhook_authz_test.go diff --git a/graph/schema.resolvers.go b/graph/schema.resolvers.go index e03bbbf69e481903edea6c9370e5662f471683d8..7ddc50e9819d4609eded7a6292a0dfecee5f7d39 100644 --- a/graph/schema.resolvers.go +++ b/graph/schema.resolvers.go @@ -19,6 +19,7 @@ import ( coreerrors "sourcecraft.dev/bigbes/sr-ht-core/errors" model1 "sourcecraft.dev/bigbes/sr-ht-core/model" corewebhooks "sourcecraft.dev/bigbes/sr-ht-core/webhooks" + "sourcecraft.dev/bigbes/sr-ht-spec/authn" "sourcecraft.dev/bigbes/sr-ht-spec/core" "sourcecraft.dev/bigbes/sr-ht-spec/doc" "sourcecraft.dev/bigbes/sr-ht-spec/graph/api" @@ -738,30 +739,76 @@ func projectRef(owner, name string) (core.ProjectRef, error) { return ref, nil } -// webhookAuthorized permits only the instance owner, and the agents acting for -// it, to manage webhooks. +// webhookAuthorized permits only the instance owner to touch webhooks. It is the +// whole ACL of the webhook chapter of this schema: createUserWebhook, +// deleteUserWebhook, userWebhooks and userWebhook all call it, and the two field +// resolvers that carry webhook data (UserWebhookSubscription.deliveries, +// WebhookDelivery.subscription) are reachable only through an object one of +// those four returned. // -// The AuthContext it reads is coreauth's: the graph server derives one from the -// principal /query's credential gate already admitted, and coreauth maps the -// owner and its agents — and nobody else — to AUTH_INTERNAL. So a non-INTERNAL -// method here is a caller with no authority over this instance's webhooks and is -// refused. +// The owner and nobody else, and specifically not an agent. That is the +// pre-conversion behaviour preserved deliberately rather than caution: before +// /query moved to the anonymous router, core-go's auth.Middleware plus the +// daemon's ownerOnly wrapper refused every caller whose username was not +// [sr.ht] owner-name, so an agent could not reach any of these fields at all. +// The move opened the endpoint to tokens.sr.ht working tokens, and checking only +// AUTH_INTERNAL would have let a token carrying nothing but authn.ActionRead +// create and delete subscriptions, and read every subscription's URL, stored +// query and delivery bodies — a read grant buying a mutation, which is the one +// thing a grant vocabulary exists to prevent. // -// What it does NOT check is a grant, and that is a gap rather than a decision: -// the endpoint's gate admits authn.ActionRead, so a working token carrying only -// spec:read reaches these mutations and may manage subscriptions with them. The -// identity requirement is unchanged — the token has to belong to [sr.ht] -// owner-name or authn refuses it at the door — but the grant vocabulary has no -// entry that means "manage this service's webhooks", and inventing one here -// would be a string tokens.sr.ht has never minted and no existing token carries. -// Closing it is a vocabulary change, declared in authn beside ActionRead and -// ActionPropose, and it is deliberately not done in passing. +// The identity is therefore asked first, and asked of spec's own principal, +// because that is the only value left that still tells the owner from an agent: +// coreauth maps both to AUTH_INTERNAL on purpose, since INTERNAL is what +// core-go's NewAuthConfig and FilterWebhooks demand of anyone at all. +// +// Opening this to agents is a grant-vocabulary decision and not a code one. It +// needs a third grant beside authn.ActionRead and authn.ActionPropose, spelled +// once and minted by tokens.sr.ht; inventing that string here would produce a +// grant no existing token carries and no daemon has ever issued. +// +// The consequence today is that webhook management is unreachable over /query, +// because this endpoint accepts no credential that resolves to the owner: authn +// produces authn.KindOwner from the unified-login cookie alone, and this +// endpoint reads no cookie. The check is written against the right predicate +// regardless, so a credential plane that does yield the owner works the moment +// it exists instead of needing this rule rediscovered. func webhookAuthorized(ctx context.Context) error { + if !authn.PrincipalFromContext(ctx).IsOwner() { + return accessDenied() + } + // The shape core-go's webhook engine demands of whoever got this far. + // NewAuthConfig refuses AUTH_COOKIE outright and panics on a method it does + // not recognise, and FilterWebhooks keys its subscription filter off this + // field; coreauth is what makes the owner INTERNAL. Asserting it here is + // what stops a change in that mapping from reaching those two as a panic. if auth.ForContext(ctx).AuthMethod != auth.AUTH_INTERNAL { - return coreerrors.ErrAccessDenied + return accessDenied() } return nil } + +// accessDenied builds a fresh ERR_ACCESS_DENIED, and the freshness is the whole +// point of the function. +// +// coreerrors.ErrAccessDenied is a package-level *gqlerror.Error — one object, +// shared by every caller in the process — and gqlgen writes the field path onto +// the error it is given (graphql/error.go: `gqlErr.Path = GetPath(ctx)`). Return +// the singleton and that assignment lands on the shared object, so the next +// refusal anywhere in the daemon carries the previous one's path: measured +// against a running daemon, a refused `deleteUserWebhook`, `userWebhooks` and +// `userWebhook` all reported `"path":["createUserWebhook"]`, the field some +// earlier request had asked for. It is a data race on a global as well as a +// wrong answer. +// +// The bug is core-go's and predates this endpoint, but it was latent while the +// daemon's ownerOnly middleware refused a non-owner before any resolver ran. +// Refusing agents here makes it the common path, so it is fixed rather than +// inherited. Same message and same code as the singleton; only the identity of +// the object differs. +func accessDenied() error { + return coreerrors.New(coreerrors.AccessDenied, "Access denied") +} func orNull(err error) error { if errors.Is(err, service.ErrNotFound) { return nil diff --git a/graph/webhook_authz_test.go b/graph/webhook_authz_test.go new file mode 100644 index 0000000000000000000000000000000000000000..c08ee84fb44e19a4cfe0fb3af1e811efe9bdc983 --- /dev/null +++ b/graph/webhook_authz_test.go @@ -0,0 +1,201 @@ +package graph + +import ( + "context" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sourcecraft.dev/bigbes/sr-ht-core/auth" + coreerrors "sourcecraft.dev/bigbes/sr-ht-core/errors" + + "sourcecraft.dev/bigbes/sr-ht-spec/authn" + "sourcecraft.dev/bigbes/sr-ht-spec/coreauth" +) + +// The webhook chapter of this schema is owner-only, and an agent token is not +// the owner however wide its grant set. +// +// This is the property the conversion had to preserve and briefly did not. +// Before /query moved to the anonymous router it sat behind core-go's +// auth.Middleware and the daemon's ownerOnly wrapper, which refused every caller +// whose username was not the instance owner. The move admits tokens.sr.ht +// working tokens, and for a while the resolvers asked only whether the caller +// was AUTH_INTERNAL — which coreauth answers "yes" for an agent too. A token +// carrying nothing but spec:read could therefore create and delete +// subscriptions: a read grant buying a mutation. + +// webhookFields is every field guarded by webhookAuthorized. The two mutations +// are the dangerous half, but the queries are listed with them on purpose: they +// serve a subscription's URL, its stored query and its delivery bodies, and they +// were behind the same wall before the conversion. +var webhookFields = map[string]string{ + "createUserWebhook": `mutation { createUserWebhook(config: {url: "https://example.invalid/h", ` + + `events: [PROPOSAL_OPENED], query: "query { webhook { uuid } }"}) { id } }`, + "deleteUserWebhook": `mutation { deleteUserWebhook(id: 1) { id } }`, + "userWebhooks": `{ userWebhooks { results { id url query } } }`, + "userWebhook": `{ userWebhook(id: 1) { id url query } }`, +} + +// An agent reaches /query — that is what the conversion was for — and is refused +// every webhook field, whether its token names the read grant or the universal +// one. +// +// The refusal has to come from webhookAuthorized and not from a missing +// database: the harness wires no core-go database context at all, so a resolver +// that got past the ACL would panic in database.WithTx rather than answer an +// error. That is why the assertions check the message. +func TestAgentTokenIsRefusedEveryWebhookField(t *testing.T) { + for _, grantString := range []string{authn.ActionRead, "*"} { + t.Run("token granting "+grantString, func(t *testing.T) { + for name, q := range webhookFields { + t.Run(name, func(t *testing.T) { + h := newHarness(t, false) + r := post(t, h, q, func(req *http.Request) { + req.Header.Set("Authorization", "Bearer "+agentToken(grantString)) + }) + + // The gate admits the caller: this is an authorization + // failure inside the schema, not a refusal at the door. + require.Equal(t, http.StatusOK, r.status, "body %s", r.body) + assert.Contains(t, r.errText(), "Access denied", + "an agent must not reach the webhook surface") + assert.NotContains(t, r.body, "example.invalid", + "the refusal leaked a subscription URL") + }) + } + }) + } +} + +// The same agent still reads. The point of the fix is that the webhook chapter +// closed, not that the endpoint did — a token carrying spec:read is exactly the +// credential /query was converted to accept. +func TestTheSameAgentTokenStillReads(t *testing.T) { + h := newHarness(t, false) + r := post(t, h, probeQuery, readToken) + + require.Equal(t, http.StatusOK, r.status, "body %s", r.body) + assert.Empty(t, r.errText()) + assert.Contains(t, string(r.Data), "~bigbes/rfcs") +} + +// The owner half of the rule, at the only level where it can be observed. +// +// There is no HTTP test for it and there cannot be one: authn produces +// authn.KindOwner from the unified-login cookie alone, and this endpoint reads +// no cookie, so no request it accepts resolves to the owner. The guard is +// written against the right predicate anyway — see webhookAuthorized — and this +// is what pins it, so that a credential plane which does yield the owner is +// admitted the moment it exists. +func TestWebhookAuthorizedAdmitsTheOwnerAndNobodyElse(t *testing.T) { + const ownerUserID = 42 + + cases := []struct { + name string + principal authn.Principal + wantErr bool + }{ + { + "the owner", + authn.Principal{Kind: authn.KindOwner, Owner: "bigbes"}, + false, + }, + { + // The case the fix is about. coreauth maps this to AUTH_INTERNAL + // exactly like the owner, so a check that read only the AuthContext + // would admit it. + "an agent on a working token with the universal grant", + authn.Principal{ + Kind: authn.KindAgent, Owner: "bigbes", Plane: authn.PlaneInstance, + Grants: mustGrants(t, "*"), UserID: ownerUserID, + }, + true, + }, + { + "an agent a local process asserted", + authn.Principal{Kind: authn.KindAgent, Owner: "bigbes"}, + true, + }, + { + "anonymous", + authn.Anonymous(), + true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := authn.WithPrincipal(context.Background(), tc.principal) + ctx = coreauth.Context(ctx, tc.principal, ownerUserID) + + err := webhookAuthorized(ctx) + if tc.wantErr { + require.Error(t, err) + assert.True(t, coreerrors.Is(err, coreerrors.ErrAccessDenied), + "want ERR_ACCESS_DENIED, got %v", err) + return + } + assert.NoError(t, err) + }) + } +} + +// coreauth maps the owner and an agent to the same AuthMethod, which is why the +// principal and not the AuthContext is what webhookAuthorized asks. Asserted +// rather than described: if this ever stops being true, the comment explaining +// why the guard is written the way it is stops being true with it. +func TestCoreauthCannotTellTheOwnerFromAnAgent(t *testing.T) { + const ownerUserID = 7 + owner := coreauth.Derive(authn.Principal{Kind: authn.KindOwner, Owner: "bigbes"}, ownerUserID) + agent := coreauth.Derive(authn.Principal{Kind: authn.KindAgent, Owner: "bigbes"}, ownerUserID) + + assert.Equal(t, auth.AUTH_INTERNAL, owner.AuthMethod) + assert.Equal(t, owner.AuthMethod, agent.AuthMethod, + "if these ever differ, webhookAuthorized could read the AuthContext again") + assert.Equal(t, owner.UserID, agent.UserID) +} + +// A refusal must name the field it refused, and two refusals in one process must +// not share an error object. +// +// coreerrors.ErrAccessDenied is a package-level *gqlerror.Error and gqlgen +// assigns the field path onto whatever error it is handed, so returning that +// singleton makes every later refusal in the daemon report the path of an +// earlier one — a wrong answer, a cross-request leak of which field somebody +// else asked for, and a data race on a global. Found by driving a live daemon: +// a refused deleteUserWebhook came back as "path":["createUserWebhook"]. +func TestARefusalNamesItsOwnField(t *testing.T) { + h := newHarness(t, false) + + // createUserWebhook first, deliberately: it is the path that used to be + // stamped onto the shared object and inherited by everything after it. + first := post(t, h, webhookFields["createUserWebhook"], readToken) + require.Contains(t, first.errText(), "Access denied", "body %s", first.body) + + for _, field := range []string{"deleteUserWebhook", "userWebhooks", "userWebhook"} { + t.Run(field, func(t *testing.T) { + r := post(t, h, webhookFields[field], readToken) + + require.Contains(t, r.errText(), "Access denied", "body %s", r.body) + assert.Contains(t, r.body, `"path":["`+field+`"]`, + "the refusal reported another field's path") + assert.NotContains(t, r.body, `"path":["createUserWebhook"]`, + "a shared error object carried the first refusal's path") + }) + } +} + +// The one webhook field with no ACL of its own. It reads the payload of the +// delivery being rendered, and outside a webhook context there is none, so it +// answers an error rather than anything about this instance's subscriptions. +// Left ungated deliberately; asserted so that "it leaks nothing" is measured. +func TestWebhookPayloadFieldLeaksNothingOverHTTP(t *testing.T) { + h := newHarness(t, false) + r := post(t, h, `{ webhook { uuid event } }`, readToken) + + require.Equal(t, http.StatusOK, r.status, "body %s", r.body) + assert.Contains(t, r.errText(), "without an active webhook context") +}