From 9e01651086ca2b75bece8131da0a4873813bd297 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:08:14 +0000 Subject: [PATCH] fix(api): check approve-own ownership before existence and status on RI exchange approve A session holding only approve-own on purchases reached fetchAndAuthorizeRIExchange, which answered a missing exchange id with 404 and another user's non-pending exchange with 409 (naming its status) before running the ownership check. That let the caller learn, for any id, whether the exchange exists and read its status, even for exchanges it did not create: the session-path half of the oracle #1175 closed for token-only callers. Run authorizeSessionApproveRIExchange right after the lookup and before the 404/409 answers. For an approve-own session a missing record now gets the same 403 as another user's record, so it also falls through to the email token like any other ownership refusal. The admin key and approve-any sessions may approve any exchange, so they keep 404/409, as does approve-own on an exchange the caller created. A table test through HandleRequest pins one answer for a missing id and for another user's pending, completed, failed and expired exchanges, with and without a token. openapi.yaml documents the change. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 32 +++++--- internal/api/openapi.yaml | 18 +++-- .../ri_exchange_approve_reject_status_test.go | 79 +++++++++++++++++-- 3 files changed, 106 insertions(+), 23 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index e49fe13af..d4d8f26e2 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2537,10 +2537,17 @@ func (h *Handler) approveRIExchangeViaSession(ctx context.Context, req *events.L return result, execErr } -// fetchAndAuthorizeRIExchange looks up the pending exchange record by id, checks -// that it is in "pending" state, and then verifies that session is authorized to -// approve it. Extracted from approveRIExchangeViaSession to keep that function -// under the cyclomatic-complexity limit. +// fetchAndAuthorizeRIExchange looks up the exchange record by id, verifies that +// session is authorized to approve it, and then checks that it is in "pending" +// state. Extracted from approveRIExchangeViaSession to keep that function under +// the cyclomatic-complexity limit. +// +// Ownership is checked before existence and status (issue #1428): an +// approve-own session gets the same 403 for a missing id and for another +// user's exchange, pending or not, so it cannot learn which ids exist or read +// their status. The 403 also falls through to the email token like any other +// ownership refusal. Approve-any sessions and the admin key may approve any +// exchange, so they keep the 404/409 distinction. func (h *Handler) fetchAndAuthorizeRIExchange(ctx context.Context, session *Session, id string) (*config.RIExchangeRecord, error) { if err := validateUUID(id); err != nil { return nil, err @@ -2560,18 +2567,16 @@ func (h *Handler) fetchAndAuthorizeRIExchange(ctx context.Context, session *Sess if err != nil { return nil, err } + if err := h.authorizeSessionApproveRIExchange(ctx, session, record); err != nil { + return nil, err + } if record == nil { return nil, NewClientError(404, "exchange record not found") } - if record.Status != "pending" { return nil, NewClientError(409, fmt.Sprintf("exchange %s cannot be approved (status=%s)", id, record.Status)) } - if err := h.authorizeSessionApproveRIExchange(ctx, session, record); err != nil { - return nil, err - } - return record, nil } @@ -2612,6 +2617,10 @@ func (h *Handler) sessionHasApproveRight(ctx context.Context, session *Session) // The RI exchange shares ResourcePurchases because approval is conceptually // "approving a purchase action on a different resource type" per the issue spec, // which prefers reusing the existing verbs to keep the matrix small. +// +// record may be nil (no such exchange). An approve-own session is refused for +// it exactly as for another user's record (issue #1428); the admin key and +// approve-any pass, and the caller answers 404. func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session *Session, record *config.RIExchangeRecord) error { // Stateless admin API key: full access, no user row. Administrators-group // users pass via the approve-any check below. @@ -2638,8 +2647,9 @@ func (h *Handler) authorizeSessionApproveRIExchange(ctx context.Context, session return NewClientError(403, "permission denied: requires approve-any or approve-own on purchases") } - // approve-own: only allow if the session user created this exchange. - if record.CreatedByUserID == nil || *record.CreatedByUserID != session.UserID { + // approve-own: only allow if the session user created this exchange. A + // missing record answers the same, so the refusal is no existence oracle. + if record == nil || record.CreatedByUserID == nil || *record.CreatedByUserID != session.UserID { return NewClientError(403, "permission denied: cannot approve another user's pending exchange") } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 51a497744..e0b81509e 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2493,7 +2493,10 @@ paths: fails CSRF validation with 403. The token alone authorizes, so the route is open to callers without a session; token mode answers an unknown exchange id with the same 403 as a wrong token, so it does - not reveal which ids exist. + not reveal which ids exist. Likewise a session holding only + approve-own gets the same 403 for an unknown id as for another + user's exchange in any status, and falls through to token mode for + both. security: - bearerAuth: [] - {} @@ -2525,15 +2528,19 @@ paths: token is invalid, or, when a session exists, 4-eyes mode rejects the approver (for example the creator approving their own exchange). In token mode an unknown exchange id, an exchange with - no approval token and a wrong token get the same 403 and body. + no approval token and a wrong token get the same 403 and body. A + session holding only approve-own gets the same 403 and body for an + unknown id as for another user's exchange, pending or not. content: application/json: schema: $ref: '#/components/schemas/Error' '404': description: > - Session mode only, the exchange does not exist. Token mode answers - an unknown id with 403, like a wrong token. + Session mode with approve-any (or the admin key) only, the + exchange does not exist. Token mode answers an unknown id with + 403, like a wrong token, and an approve-own session answers it + like another user's exchange. content: application/json: schema: @@ -2542,7 +2549,8 @@ paths: description: > The exchange is no longer pending (already processed, or canceled by a newer analysis run), including when a concurrent approve or - reject changed it first. + reject changed it first. An approve-own session gets this only + for an exchange it created. content: application/json: schema: diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index 8c2d15881..e07e53831 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -65,12 +65,14 @@ func riStatusTransition(rec riStatusRecord) (*config.RIExchangeRecord, error) { // riStatusOpts tweaks the fixture for the less common cases. type riStatusOpts struct { - fourEyes bool // global 4-eyes mode on; the admin session is the creator - noCreator bool // with fourEyes: the record has no creator, as every scheduler/ladder row does - noApprove bool // the session holds no approve permission - approveOwn bool // the session holds approve-own only (not the creator) - dbError bool // the record lookup fails - rateLimited bool // the public approve/cancel rate limit rejects the request + fourEyes bool // global 4-eyes mode on; the admin session is the creator + noCreator bool // with fourEyes: the record has no creator, as every scheduler/ladder row does + noApprove bool // the session holds no approve permission + approveOwn bool // the session holds approve-own only (not the creator) + ownsRecord bool // with approveOwn: the session created the record + status string // stored record status; "" means pending + dbError bool // the record lookup fails + rateLimited bool // the public approve/cancel rate limit rejects the request } func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { @@ -87,9 +89,12 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { past := time.Now().Add(-time.Hour) record.ExpiresAt = &past } + if o.status != "" { + record.Status = o.status + } if (o.fourEyes && !o.noCreator) || o.approveOwn { creator := "admin-uuid" - if o.approveOwn { + if o.approveOwn && !o.ownsRecord { creator = "someone-else" } record.CreatedByUserID = &creator @@ -234,6 +239,11 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "403 missing record reported as ErrNotFound with token", rec: riRecMissingNotFound, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid approval token"}, {name: "404 missing record with session", rec: riRecMissing, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, {name: "404 missing record reported as ErrNotFound with session", rec: riRecMissingNotFound, sess: riSessCSRF, want: 404, wantError: "exchange record not found"}, + // Issue #1428: approve-any keeps the 404/409 distinction, and so + // does an approve-own session on its own exchange. + {name: "409 approve-any session on an exchange no longer pending", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{status: "completed"}, want: 409, wantError: "exchange " + riStatusID + " cannot be approved (status=completed)"}, + {name: "409 approve-own creator on an exchange no longer pending", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{approveOwn: true, ownsRecord: true, status: "completed"}, want: 409, wantError: "exchange " + riStatusID + " cannot be approved (status=completed)"}, + {name: "200 approve-own creator approves own pending exchange", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{approveOwn: true, ownsRecord: true}, want: 200, wantBody: `"status"`}, {name: "409 token approve of an exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled by a newer analysis run"}, // Issue #587: the same invalid/expired/wrong-status mapping as the // purchase approve, cancel and revoke links (403/410/409). @@ -330,3 +340,58 @@ func TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken(t *tes } } } + +// TestHandleRequest_ApproveRIExchange_ApproveOwnSessionNoOracle is the issue +// #1428 regression. A session holding only approve-own on purchases used to +// get 404 for a missing exchange id and 409 (with the status) for another +// user's exchange that is no longer pending, both before the ownership check, +// so it could learn which ids exist and read their status. It must now get +// one answer for a missing id and for another user's exchange in any status, +// with and without a token. +func TestHandleRequest_ApproveRIExchange_ApproveOwnSessionNoOracle(t *testing.T) { + type probe struct { + name string + rec riStatusRecord + opts riStatusOpts + } + other := riStatusOpts{approveOwn: true} + // The baseline is another user's pending exchange; every other probe + // must answer exactly like it. + baseline := probe{"another user's pending exchange", riRecPending, other} + probes := []probe{ + {"missing record", riRecMissing, other}, + {"missing record reported as ErrNotFound", riRecMissingNotFound, other}, + {"another user's completed exchange", riRecPending, riStatusOpts{approveOwn: true, status: "completed"}}, + {"another user's failed exchange", riRecPending, riStatusOpts{approveOwn: true, status: "failed"}}, + {"another user's expired exchange", riRecExpired, other}, + } + for _, tc := range []struct { + name string + token string + wantError string + }{ + {"no token", "", "permission denied: cannot approve another user's pending exchange"}, + {"wrong token", "wrong", "invalid approval token"}, + } { + do := func(p probe) *events.LambdaFunctionURLResponse { + resp, err := riStatusHandler(p.rec, p.opts).HandleRequest(context.Background(), riStatusRequest("approve", riSessCSRF, tc.token)) + require.NoError(t, err) + return resp + } + want := do(baseline) + t.Run(tc.name+"/baseline answers 403", func(t *testing.T) { + assert.Equal(t, 403, want.StatusCode, want.Body) + var body map[string]string + require.NoError(t, json.Unmarshal([]byte(want.Body), &body)) + assert.Equal(t, tc.wantError, body["error"]) + }) + for _, p := range probes { + t.Run(tc.name+"/"+p.name, func(t *testing.T) { + got := do(p) + assert.Equal(t, want.StatusCode, got.StatusCode, got.Body) + assert.Equal(t, want.Body, got.Body) + assert.Equal(t, want.Headers, got.Headers) + }) + } + } +}