fix(api): check approve-own ownership before existence and status on RI exchange approve

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

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:

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