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