diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 66852d132..ee9e8f3dc 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -318,8 +318,12 @@ func TestHandler_rejectRIExchange_RecordNotFound(t *testing.T) { h := &Handler{config: mockStore} _, err := h.rejectRIExchange(ctx, "11111111-1111-1111-1111-111111111111", "tok") - assert.Error(t, err) - assert.Contains(t, err.Error(), "not found") + // A missing record answers like a wrong token (issue #601). + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %v", err) + assert.Equal(t, 403, ce.code) + assert.Equal(t, "invalid rejection token", ce.message) } func TestHandler_rejectRIExchange_WrongToken(t *testing.T) { diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 69ff4161e..b14b5f407 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2546,9 +2546,9 @@ func (h *Handler) fetchAndAuthorizeRIExchange(ctx context.Context, session *Sess return nil, errNotFound } - record, err := h.config.GetRIExchangeRecord(ctx, id) + record, err := h.getRIExchangeRecord(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to look up exchange record: %w", err) + return nil, err } if record == nil { return nil, NewClientError(404, "exchange record not found") @@ -2713,6 +2713,22 @@ func (h *Handler) requireDifferentRIExchangeApprover(ctx context.Context, sessio return NewClientError(500, "4-eyes approval mode is enabled but no session could be resolved; sign in before approving") } +// getRIExchangeRecord loads an exchange record for the approve and reject +// routes. A missing record is (nil, nil) whether the store reports it as a +// nil record or as a wrapped config.ErrNotFound (PostgresStore), so callers +// answer it as a client error and not a 500 (issue #601). Any other store +// error stays a server error. +func (h *Handler) getRIExchangeRecord(ctx context.Context, id string) (*config.RIExchangeRecord, error) { + record, err := h.config.GetRIExchangeRecord(ctx, id) + if errors.Is(err, config.ErrNotFound) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to look up exchange record: %w", err) + } + return record, nil +} + // validateExchangeApproval validates ID, token, and record state for an exchange approval. func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string) (*config.RIExchangeRecord, error) { if err := validateUUID(id); err != nil { @@ -2722,14 +2738,10 @@ func (h *Handler) validateExchangeApproval(ctx context.Context, id, token string return nil, NewClientError(400, "approval token is required") } - record, err := h.config.GetRIExchangeRecord(ctx, id) + record, err := h.getRIExchangeRecord(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to look up exchange record: %w", err) - } - if record == nil { - return nil, NewClientError(404, "exchange record not found") + return nil, err } - if err := checkExchangeToken(record, token, "approval"); err != nil { return nil, err } @@ -2743,20 +2755,23 @@ const ( ) // checkExchangeToken applies the emailed-token status mapping shared with the -// purchase approve, cancel and revoke links (issue #587): a record without a -// token, or a token that does not match, is 403, and a matching token on an -// exchange past its expires_at is 410. The match is checked first, so only -// the token holder learns that the exchange expired. kind is "approval" or -// "rejection" and only shapes the 403 text. +// purchase approve, cancel and revoke links (issue #587): a token that does +// not match is 403, and a matching token on an exchange past its expires_at +// is 410. The match is checked first, so only the token holder learns that +// the exchange expired. kind is "approval" or "rejection" and only shapes the +// 403 text. +// +// A missing record (nil) and a record without a token get the same 403 as a +// wrong token (issue #601): the token alone authorizes these routes, so they +// are open to callers with no session, and a distinct answer would tell such +// a caller which exchange ids exist. // // record.ApprovalToken is the SHA-256 hex digest stored at rest (issue #103); // config.ApprovalTokenMatches hashes the supplied token and compares digests -// in constant time, as everywhere else that validates one of these tokens. +// in constant time, as everywhere else that validates one of these tokens, +// and never matches an empty stored digest. func checkExchangeToken(record *config.RIExchangeRecord, token, kind string) error { - if record.ApprovalToken == "" { - return NewClientError(403, "this exchange record does not support "+kind) - } - if !config.ApprovalTokenMatches(record.ApprovalToken, token) { + if record == nil || !config.ApprovalTokenMatches(record.ApprovalToken, token) { return NewClientError(403, "invalid "+kind+" token") } if record.ExpiresAt != nil && time.Now().After(*record.ExpiresAt) { @@ -2933,14 +2948,10 @@ func (h *Handler) rejectRIExchange(ctx context.Context, id, token string) (any, return nil, NewClientError(400, "rejection token is required") } - record, err := h.config.GetRIExchangeRecord(ctx, id) + record, err := h.getRIExchangeRecord(ctx, id) if err != nil { - return nil, fmt.Errorf("failed to look up exchange record: %w", err) - } - if record == nil { - return nil, NewClientError(404, "exchange record not found") + return nil, err } - if tokenErr := checkExchangeToken(record, token, "rejection"); tokenErr != nil { return nil, tokenErr } diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 4357ac275..d3dcab4a3 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -893,7 +893,8 @@ func TestRejectRIExchange_EmptyStoredToken(t *testing.T) { ce, ok := IsClientError(err) assert.True(t, ok) assert.Equal(t, 403, ce.code) - assert.Contains(t, err.Error(), "does not support rejection") + // Same answer as a wrong token or a missing record (issue #601). + assert.Contains(t, err.Error(), "invalid rejection token") mockStore.AssertExpectations(t) } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index d80632770..e828dfd2c 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2483,8 +2483,10 @@ paths: present; a CSRF rejection never falls through. The token is read from the query string only. This route never returns 401: a request with no token and no usable session - fails CSRF validation with 403. A missing record (404) and a wrong - token (403) are distinguishable for unauthenticated callers. + 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. security: - bearerAuth: [] - {} @@ -2513,15 +2515,22 @@ paths: description: > CSRF validation failed (including a request with neither a token nor a valid session), the session lacks approve permission, the - token is invalid, the record does not support approval, or, when a - session exists, 4-eyes mode rejects the approver (for example the - creator approving their own exchange). + 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. content: application/json: schema: $ref: '#/components/schemas/Error' '404': - $ref: '#/components/responses/NotFound' + description: > + Session mode only, the exchange does not exist. Token mode answers + an unknown id with 403, like a wrong token. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' '409': description: > The exchange is no longer pending (already processed, or canceled @@ -2561,9 +2570,9 @@ paths: summary: Reject a pending RI exchange (token-based, no session required) description: > Authorized by the rejection token in the query string alone. A session - is ignored and no CSRF check runs. A missing token is 400; a missing - record (404) and a wrong token (403) are distinguishable for - unauthenticated callers. + is ignored and no CSRF check runs. A missing token is 400. An unknown + exchange id gets the same 403 and body as a wrong token, so the route + does not reveal which ids exist. security: [] parameters: - name: token @@ -2581,13 +2590,13 @@ paths: '400': $ref: '#/components/responses/BadRequest' '403': - description: The rejection token is invalid, or the record does not support rejection. + description: > + The rejection token is invalid. An unknown exchange id and an + exchange with no token get the same answer. content: application/json: schema: $ref: '#/components/schemas/Error' - '404': - $ref: '#/components/responses/NotFound' '409': description: > The exchange is no longer pending (already processed or canceled), diff --git a/internal/api/ri_exchange_approve_reject_status_test.go b/internal/api/ri_exchange_approve_reject_status_test.go index 380b4d1ea..8c2d15881 100644 --- a/internal/api/ri_exchange_approve_reject_status_test.go +++ b/internal/api/ri_exchange_approve_reject_status_test.go @@ -29,6 +29,9 @@ const ( riRecPending riStatusRecord = iota riRecNoToken riRecMissing + // riRecMissingNotFound is missing as PostgresStore reports it: a wrapped + // config.ErrNotFound instead of a nil record (issue #601). + riRecMissingNotFound riRecAlreadyProcessed // riRecExpired is pending but past its expires_at. Its transition is // mocked to succeed, so a 410 proves the token check answered before @@ -72,7 +75,7 @@ type riStatusOpts struct { func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { var record *config.RIExchangeRecord - if rec != riRecMissing { + if rec != riRecMissing && rec != riRecMissingNotFound { record = &config.RIExchangeRecord{ ID: riStatusID, Status: "pending", SourceRIIDs: []string{"ri-1"}, PaymentDue: "0", ApprovalToken: config.HashApprovalToken(riStatusRawToken), @@ -93,9 +96,13 @@ func riStatusHandler(rec riStatusRecord, o riStatusOpts) *Handler { } } store := new(MockConfigStore) - if o.dbError { + switch { + case o.dbError: store.On("GetRIExchangeRecord", mock.Anything, riStatusID).Return(nil, errors.New("db down")).Maybe() - } else { + case rec == riRecMissingNotFound: + store.On("GetRIExchangeRecord", mock.Anything, riStatusID). + Return(nil, fmt.Errorf("ri exchange record %s: %w", riStatusID, config.ErrNotFound)).Maybe() + default: store.On("GetRIExchangeRecord", mock.Anything, riStatusID).Return(record, nil).Maybe() } transitioned, transitionErr := riStatusTransition(rec) @@ -209,7 +216,7 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "403 session without CSRF and no token", rec: riRecPending, sess: riSessNoCSRF, want: 403, wantError: "CSRF validation failed"}, {name: "403 session without CSRF does not fall through to a valid token", rec: riRecPending, sess: riSessNoCSRF, token: riStatusRawToken, want: 403, wantError: "CSRF validation failed"}, {name: "403 wrong token without session", rec: riRecPending, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, - {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "this exchange record does not support approval"}, + {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid approval token"}, {name: "400 invalid UUID", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, id: "not-a-uuid", want: 400, wantError: "invalid ID format: must be a valid UUID"}, {name: "403 session without approve right and no token", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{noApprove: true}, want: 403, wantError: "permission denied: requires approve-any or approve-own on purchases"}, {name: "403 4-eyes creator cannot approve own exchange", rec: riRecPending, sess: riSessCSRF, opts: riStatusOpts{fourEyes: true}, want: 403, wantError: "approval declined: 4-eyes mode requires a different approver than the requester"}, @@ -220,9 +227,13 @@ func TestHandleRequest_ApproveRIExchange_StatusCodes(t *testing.T) { {name: "403 body token is not read", rec: riRecPending, sess: riSessNone, body: `{"token":"` + riStatusRawToken + `"}`, want: 403, wantError: "CSRF validation failed"}, {name: "429 rate limited", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{rateLimited: true}, want: 429, wantError: "too many requests, please try again later"}, {name: "500 record lookup fails", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{dbError: true}, want: 500}, - {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, - {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, + // Issue #601: in token mode a missing record answers like a wrong + // token, so the route does not reveal which ids exist. + {name: "403 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid approval token"}, + {name: "403 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid approval token"}, + {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"}, {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). @@ -250,9 +261,11 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "429 rate limited", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{rateLimited: true}, want: 429, wantError: "too many requests, please try again later"}, {name: "500 record lookup fails", rec: riRecPending, sess: riSessNone, token: riStatusRawToken, opts: riStatusOpts{dbError: true}, want: 500}, {name: "403 wrong token", rec: riRecPending, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, - {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "this exchange record does not support rejection"}, - {name: "404 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 404, wantError: "exchange record not found"}, - {name: "404 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 404, wantError: "exchange record not found"}, + {name: "403 record without approval token", rec: riRecNoToken, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid rejection token"}, + // Issue #601: a missing record answers like a wrong token. + {name: "403 missing record with token", rec: riRecMissing, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid rejection token"}, + {name: "403 missing record with wrong token", rec: riRecMissing, sess: riSessNone, token: "wrong", want: 403, wantError: "invalid rejection token"}, + {name: "403 missing record reported as ErrNotFound with token", rec: riRecMissingNotFound, sess: riSessNone, token: riStatusRawToken, want: 403, wantError: "invalid rejection token"}, {name: "409 exchange no longer pending", rec: riRecAlreadyProcessed, sess: riSessNone, token: riStatusRawToken, want: 409, wantError: "exchange already processed or was canceled"}, // Issue #587: 403 invalid, 410 expired, 409 wrong status. {name: "410 expired exchange", rec: riRecExpired, sess: riSessNone, token: riStatusRawToken, want: 410, wantError: "exchange has expired"}, @@ -262,3 +275,58 @@ func TestHandleRequest_RejectRIExchange_StatusCodes(t *testing.T) { {name: "500 transition store failure", rec: riRecTransitionFails, sess: riSessNone, token: riStatusRawToken, want: 500}, }) } + +// TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken is the +// issue #601 regression: a caller that does not hold the emailed token gets +// the same status, headers and body for a missing exchange id as for an +// existing one, so approve and reject cannot be used to learn which ids +// exist. A 401 before the lookup (the purchase routes' fix, #435) does not +// fit here: the token alone authorizes these routes, with no session. +func TestHandleRequest_RIExchangeTokenRoutes_MissingAnswersLikeWrongToken(t *testing.T) { + type probe struct { + name string + rec riStatusRecord + token string + } + // The baseline is a missing record as the mocks report it; every other + // probe must answer exactly like it. + baseline := probe{"missing record", riRecMissing, "wrong"} + probes := []probe{ + {"missing record reported as ErrNotFound", riRecMissingNotFound, "wrong"}, + {"missing record reported as ErrNotFound with the real token's value", riRecMissingNotFound, riStatusRawToken}, + {"pending record with a wrong token", riRecPending, "wrong"}, + {"record without a token", riRecNoToken, "wrong"}, + {"expired record with a wrong token", riRecExpired, "wrong"}, + } + callers := []struct { + name string + sess riStatusSession + opts riStatusOpts + }{ + {"no session", riSessNone, riStatusOpts{}}, + {"invalid session", riSessInvalid, riStatusOpts{}}, + {"session without approve permission", riSessCSRF, riStatusOpts{noApprove: true}}, + } + for _, action := range []string{"approve", "reject"} { + for _, caller := range callers { + do := func(p probe) *events.LambdaFunctionURLResponse { + resp, err := riStatusHandler(p.rec, caller.opts).HandleRequest(context.Background(), riStatusRequest(action, caller.sess, p.token)) + require.NoError(t, err) + return resp + } + want := do(baseline) + t.Run(action+"/"+caller.name+"/answers 403 invalid token", func(t *testing.T) { + assert.Equal(t, 403, want.StatusCode, want.Body) + assert.Contains(t, want.Body, "invalid "+map[string]string{"approve": "approval", "reject": "rejection"}[action]+" token") + }) + for _, p := range probes { + t.Run(action+"/"+caller.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) + }) + } + } + } +} diff --git a/internal/api/ri_exchange_token_oracle_integration_test.go b/internal/api/ri_exchange_token_oracle_integration_test.go new file mode 100644 index 000000000..4bc95d63e --- /dev/null +++ b/internal/api/ri_exchange_token_oracle_integration_test.go @@ -0,0 +1,136 @@ +//go:build integration + +package api + +// Real-Postgres check for issue #601. PostgresStore reports a missing RI +// exchange as an error, not a nil record, so before the fix the approve and +// reject email links answered a missing id with 500 and an existing one with +// a wrong token with 403: an oracle for which exchange ids exist, open to +// callers with no session. These tests drive HandleRequest against the real +// store and compare the two answers. + +import ( + "context" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +const oracleExchangeRawToken = "oracle-exchange-raw-token" + +// oracleMissingID is a well-formed UUID that no seeded exchange uses. +const oracleMissingID = "550e8400-e29b-41d4-a716-446655440601" + +func setupOracleExchangeStore(ctx context.Context, t *testing.T) *config.PostgresStore { + t.Helper() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { _ = container.Cleanup(context.Background()) }) + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + return config.NewPostgresStore(container.DB) +} + +// seedOracleExchange stores a pending exchange; rawToken "" stores none. +func seedOracleExchange(ctx context.Context, t *testing.T, store *config.PostgresStore, rawToken string, expiresAt time.Time) string { + t.Helper() + rec := &config.RIExchangeRecord{ + AccountID: "123456789012", + Region: "us-east-1", + SourceRIIDs: []string{"ri-601"}, + SourceInstanceType: "m5.large", + SourceCount: 1, + TargetOfferingID: "offering-601", + TargetInstanceType: "c5.large", + TargetCount: 1, + PaymentDue: "10.00", + Status: "pending", + ApprovalToken: rawToken, + Mode: "manual", + ExpiresAt: &expiresAt, + } + require.NoError(t, store.SaveRIExchangeRecord(ctx, rec)) + return rec.ID +} + +func oracleRequest(action, id, token string, headers map[string]string) *events.LambdaFunctionURLRequest { + if headers == nil { + headers = map[string]string{} + } + return &events.LambdaFunctionURLRequest{ + Headers: headers, + QueryStringParameters: map[string]string{"token": token}, + RequestContext: events.LambdaFunctionURLRequestContext{HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "POST", Path: "/api/ri-exchange/" + action + "/" + id}}, + } +} + +func TestRIExchangeTokenRoutes_Integration_MissingAnswersLikeWrongToken(t *testing.T) { + ctx := context.Background() + store := setupOracleExchangeStore(ctx, t) + h := &Handler{config: store} + + _, lookupErr := store.GetRIExchangeRecord(ctx, oracleMissingID) + require.ErrorIs(t, lookupErr, config.ErrNotFound, "fixture: the id must be missing, reported as PostgresStore does") + + pending := seedOracleExchange(ctx, t, store, oracleExchangeRawToken, time.Now().Add(24*time.Hour)) + noToken := seedOracleExchange(ctx, t, store, "", time.Now().Add(24*time.Hour)) + expired := seedOracleExchange(ctx, t, store, oracleExchangeRawToken+"-expired", time.Now().Add(-time.Hour)) // tokens are unique + + for _, action := range []string{"approve", "reject"} { + missing, missErr := h.HandleRequest(ctx, oracleRequest(action, oracleMissingID, "wrong", nil)) + require.NoError(t, missErr) + t.Run(action+"/missing id is a 403, not a 500", func(t *testing.T) { + assert.Equal(t, 403, missing.StatusCode, missing.Body) + }) + for name, id := range map[string]string{"pending": pending, "no token": noToken, "expired": expired} { + t.Run(action+"/wrong token on "+name+" answers like a missing id", func(t *testing.T) { + got, err := h.HandleRequest(ctx, oracleRequest(action, id, "wrong", nil)) + require.NoError(t, err) + assert.Equal(t, missing.StatusCode, got.StatusCode, got.Body) + assert.Equal(t, missing.Body, got.Body) + assert.Equal(t, missing.Headers, got.Headers) + }) + } + } + + // The refused requests must not have touched the rows. + for _, id := range []string{pending, noToken, expired} { + rec, getErr := store.GetRIExchangeRecord(ctx, id) + require.NoError(t, getErr) + assert.Equal(t, "pending", rec.Status) + } + + // Control: the token holder still rejects, so the 403s above come from + // the token check and not from a fixture the routes cannot act on. + resp, err := h.HandleRequest(ctx, oracleRequest("reject", pending, oracleExchangeRawToken, nil)) + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode, resp.Body) +} + +// TestApproveRIExchange_Integration_SessionMissingIs404 covers the +// session-authed approve: a missing exchange is the documented 404, not the +// 500 the bare store error used to produce. +func TestApproveRIExchange_Integration_SessionMissingIs404(t *testing.T) { + ctx := context.Background() + store := setupOracleExchangeStore(ctx, t) + + a := new(MockAuthService) + a.On("ValidateSession", mock.Anything, "admin-bearer").Return(&Session{UserID: "admin-uuid", Email: "admin@example.com"}, nil).Maybe() + a.grantAdminPurchaser() + a.On("ValidateCSRFToken", mock.Anything, "admin-bearer", "csrf-ok").Return(nil).Maybe() + h := &Handler{config: store, auth: a} + + resp, err := h.HandleRequest(ctx, oracleRequest("approve", oracleMissingID, "", + map[string]string{"authorization": "Bearer admin-bearer", "x-csrf-token": "csrf-ok"})) + require.NoError(t, err) + assert.Equal(t, 404, resp.StatusCode, resp.Body) + assert.Contains(t, resp.Body, "exchange record not found") +} diff --git a/internal/api/router_handlers_test.go b/internal/api/router_handlers_test.go index 5d355f825..5670a05d6 100644 --- a/internal/api/router_handlers_test.go +++ b/internal/api/router_handlers_test.go @@ -634,8 +634,12 @@ func TestHandler_validateExchangeApproval_RecordNotFound(t *testing.T) { h := &Handler{config: mockStore} _, err := h.validateExchangeApproval(ctx, "11111111-1111-1111-1111-111111111111", "some-token") - assert.Error(t, err) - assert.Contains(t, err.Error(), "not found") + // A missing record answers like a wrong token (issue #601). + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %v", err) + assert.Equal(t, 403, ce.code) + assert.Equal(t, "invalid approval token", ce.message) } func TestHandler_validateExchangeApproval_NoApprovalToken(t *testing.T) { @@ -647,7 +651,8 @@ func TestHandler_validateExchangeApproval_NoApprovalToken(t *testing.T) { h := &Handler{config: mockStore} _, err := h.validateExchangeApproval(ctx, "11111111-1111-1111-1111-111111111111", "some-token") assert.Error(t, err) - assert.Contains(t, err.Error(), "does not support approval") + // Same answer as a wrong token or a missing record (issue #601). + assert.Contains(t, err.Error(), "invalid approval token") } func TestHandler_validateExchangeApproval_WrongToken(t *testing.T) { diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 9f6ce0bd4..3106fe437 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -507,6 +507,7 @@ type StoreInterface interface { // processing (approved) exchange under the shared daily-cap lock, excluding // the row's own amount, and stores the ceiling as its payment_due. ReserveApprovedRIExchange(ctx context.Context, id, dailyCapUSD, perExchangeCapUSD string) (string, error) + // GetRIExchangeRecord returns a wrapped ErrNotFound when no record has id. GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) GetRIExchangeRecordByToken(ctx context.Context, token string) (*RIExchangeRecord, error) GetRIExchangeHistory(ctx context.Context, since time.Time, limit int) ([]RIExchangeRecord, error) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index b17c5c85c..b94c317d6 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3325,7 +3325,9 @@ func riExchangeUSD(micros *big.Int) string { return fmt.Sprintf("%s.%06d", whole, fractional) } -// GetRIExchangeRecord retrieves an RI exchange record by ID. +// GetRIExchangeRecord retrieves an RI exchange record by ID. A missing record +// is a wrapped ErrNotFound, so callers can tell it from a failed query and +// answer it as a client error rather than a 500 (issue #601). func (s *PostgresStore) GetRIExchangeRecord(ctx context.Context, id string) (*RIExchangeRecord, error) { query := ` SELECT id, account_id, exchange_id, region, source_ri_ids, @@ -3344,7 +3346,7 @@ func (s *PostgresStore) GetRIExchangeRecord(ctx context.Context, id string) (*RI } if len(records) == 0 { - return nil, fmt.Errorf("ri exchange record not found: %s", id) + return nil, fmt.Errorf("ri exchange record %s: %w", id, ErrNotFound) } return &records[0], nil diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 64017f8fb..1ff85637e 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -1418,6 +1418,39 @@ func TestPGXMock_GetRIExchangeRecord_WithTimestamps(t *testing.T) { assert.Equal(t, "some error", rec.Error) } +// TestPGXMock_GetRIExchangeRecord_NotFound pins the missing-record error the +// RI exchange approve and reject routes rely on: a wrapped ErrNotFound, which +// they answer as a 4xx, not a bare error they would answer with 500 (#601). +func TestPGXMock_GetRIExchangeRecord_NotFound(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery("SELECT").WithArgs("ri-id").WillReturnRows(pgxmock.NewRows(riExchangeCols)) + + rec, err := store.GetRIExchangeRecord(ctx, "ri-id") + require.Error(t, err) + assert.Nil(t, rec) + assert.ErrorIs(t, err, ErrNotFound) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestPGXMock_GetRIExchangeRecord_QueryErrorIsNotErrNotFound keeps a failed +// query a server error: only an empty result is ErrNotFound. +func TestPGXMock_GetRIExchangeRecord_QueryErrorIsNotErrNotFound(t *testing.T) { + mock := newMock(t) + store := storeWith(mock) + ctx := context.Background() + + mock.ExpectQuery("SELECT").WithArgs("ri-id").WillReturnError(errors.New("connection reset")) + + rec, err := store.GetRIExchangeRecord(ctx, "ri-id") + require.Error(t, err) + assert.Nil(t, rec) + assert.NotErrorIs(t, err, ErrNotFound) + assert.NoError(t, mock.ExpectationsWereMet()) +} + func TestPGXMock_GetRIExchangeRecordByToken_Success(t *testing.T) { mock := newMock(t) store := storeWith(mock)