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