From f3be5c9f589becb2f2d1bb17acefdcc7cfbe54d5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 08:55:31 +0200 Subject: [PATCH] fix(api): validate the emailed approval token on the delayed-approval path approveViaToken went straight to approveWithDelay when a purchase delay was configured, so only the immediate path (ApproveExecution) checked the token. Export purchase.ValidateApprovalToken, call it before scheduling and map errors through tokenActionError (403 invalid, 410 expired). Closes #586. --- internal/api/handler_purchases.go | 5 + .../api/handler_purchases_delay_token_test.go | 96 +++++++++++++++++++ internal/api/openapi.yaml | 12 +-- internal/purchase/approvals.go | 14 +-- 4 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 internal/api/handler_purchases_delay_token_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index d62232a6d..dd4a79b7a 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -695,6 +695,11 @@ func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctio return nil, fmt.Errorf("failed to read global config for purchase delay check: %w", cfgErr) } if globalCfg.GetPurchaseDelay() > 0 { + // The delayed branch bypasses ApproveExecution, which is where the + // token is otherwise validated (issue #586). + if err := purchase.ValidateApprovalToken(execution, token); err != nil { + return nil, tokenActionError(err) + } return h.approveWithDelay(ctx, execution, globalCfg.GetPurchaseDelay(), actor, nil) } // ApproveExecution now runs the purchase synchronously inside the diff --git a/internal/api/handler_purchases_delay_token_test.go b/internal/api/handler_purchases_delay_token_test.go new file mode 100644 index 000000000..8946e141f --- /dev/null +++ b/internal/api/handler_purchases_delay_token_test.go @@ -0,0 +1,96 @@ +package api + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// The delayed-approval branch of the email-link approve must validate the +// token exactly like the immediate branch (issue #586), reusing the harness +// from handler_purchases_token_errors_test.go. + +func delayTokenHandler(t *testing.T, exec *config.PurchaseExecution, delayHours int) *Handler { + t.Helper() + h := tokenErrHandler(t, exec, nil) + store := h.config.(*MockConfigStore) + kept := store.ExpectedCalls[:0] + for _, c := range store.ExpectedCalls { + if c.Method != "GetGlobalConfig" { + kept = append(kept, c) + } + } + store.ExpectedCalls = kept + approver := "admin@example.com" + store.On("GetGlobalConfig", mock.Anything). + Return(&config.GlobalConfig{NotificationEmail: &approver, PurchaseDelayHours: delayHours}, nil) + return h +} + +func TestHandleRequest_ApproveViaToken_DelayConfigured_ValidatesToken(t *testing.T) { + past := time.Now().Add(-time.Hour) + future := time.Now().Add(time.Hour) + hash := config.HashApprovalToken(tokenErrRawToken) + + cases := []struct { + name string + exec config.PurchaseExecution + token string + want int + }{ + {"wrong token", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}, "WRONG", 403}, + {"expired token", config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &past}, tokenErrRawToken, 410}, + {"execution without a stored token", config.PurchaseExecution{Status: "pending", ApprovalTokenExpiresAt: &future}, tokenErrRawToken, 403}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exec := tc.exec + h := delayTokenHandler(t, &exec, 1) + store := h.config.(*MockConfigStore) + // Registered so a regression answers 200 instead of panicking on an unmocked call. + store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, mock.Anything, "scheduled", mock.Anything). + Return(&config.PurchaseExecution{ExecutionID: tokenErrExecID, Status: "scheduled"}, nil).Maybe() + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil).Maybe() + resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tc.token)) + require.NoError(t, err) + assert.Equal(t, tc.want, resp.StatusCode, resp.Body) + store.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +func TestHandleRequest_ApproveViaToken_DelayConfigured_ValidTokenSchedules(t *testing.T) { + future := time.Now().Add(time.Hour) + exec := &config.PurchaseExecution{Status: "pending", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future} + h := delayTokenHandler(t, exec, 1) + store := h.config.(*MockConfigStore) + scheduled := &config.PurchaseExecution{ExecutionID: tokenErrExecID, Status: "scheduled"} + store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything). + Return(scheduled, nil) + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken)) + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode, resp.Body) + assert.Contains(t, resp.Body, "scheduled") +} + +func TestHandleRequest_ApproveViaToken_DelayConfigured_ReplayOfScheduledIs409(t *testing.T) { + future := time.Now().Add(time.Hour) + exec := &config.PurchaseExecution{Status: "scheduled", ApprovalToken: config.HashApprovalToken(tokenErrRawToken), ApprovalTokenExpiresAt: &future} + h := delayTokenHandler(t, exec, 1) + store := h.config.(*MockConfigStore) + store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, []string{"pending", "notified"}, "scheduled", mock.Anything). + Return(nil, config.ErrExecutionNotInExpectedStatus) + + resp, err := h.HandleRequest(context.Background(), tokenErrRequest("approve", tokenErrRawToken)) + require.NoError(t, err) + assert.Equal(t, 409, resp.StatusCode, resp.Body) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 2095152d2..aae2eeb4f 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -559,10 +559,9 @@ paths: exceeds its Constraints, the caller is not the account's contact email, no contact email is configured on the execution's accounts, 4-eyes mode rejects the approver, or (token mode) the - approval token is invalid. Token validation applies on the - immediate-approval path only; the delayed-approval path (purchase - delay configured) does not validate the token yet (tracked - separately). + approval token is invalid. Token mode validates the token on + both the immediate and the delayed-approval (purchase delay + configured) paths. content: application/json: schema: @@ -588,9 +587,8 @@ paths: $ref: '#/components/schemas/Error' '410': description: > - Token mode only, the approval token has expired (immediate-approval - path only; the delayed-approval path does not validate the token yet, - tracked separately). + Token mode only, the approval token has expired (checked on both + the immediate and the delayed-approval paths). content: application/json: schema: diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index ace1206fc..bcdb48c15 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -58,7 +58,7 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto } // Validate token and TTL (Finding #4 + issue #397). - if tokErr := validateApprovalToken(execution, token); tokErr != nil { + if tokErr := ValidateApprovalToken(execution, token); tokErr != nil { return "", tokErr } @@ -105,17 +105,19 @@ func maskActor(actor string) string { return "****" } -// validateApprovalToken checks that the execution carries a non-empty token, +// ValidateApprovalToken checks that the execution carries a non-empty token, // that the supplied token matches the stored one using constant-time comparison // (Finding #4 -- prevents timing attacks), and that the token has not expired // (issue #397). Legacy rows with a nil ApprovalTokenExpiresAt pass the TTL // check for backward compatibility. Extracted from ApproveExecution to keep -// that function under the gocyclo threshold. +// that function under the gocyclo threshold. Exported so the HTTP layer's +// delayed-approval branch (which never calls ApproveExecution) validates the +// token the same way (issue #586). // // execution.ApprovalToken is the SHA-256 hex digest stored at rest // (issue #103), never the raw secret; config.ApprovalTokenMatches hashes the // supplied token and compares digests in constant time. -func validateApprovalToken(execution *config.PurchaseExecution, token string) error { +func ValidateApprovalToken(execution *config.PurchaseExecution, token string) error { if execution.ApprovalToken == "" || token == "" { return ErrInvalidApprovalToken } @@ -563,10 +565,10 @@ func (m *Manager) loadCancelableExecution(ctx context.Context, executionID, toke return nil, fmt.Errorf("failed to get execution: %w", err) } // execution.ApprovalToken is the SHA-256 hex digest stored at rest - // (issue #103); validateApprovalToken hashes the supplied token and + // (issue #103); ValidateApprovalToken hashes the supplied token and // compares digests in constant time, and enforces the TTL (issue #397; // legacy rows without ApprovalTokenExpiresAt pass). - if err := validateApprovalToken(execution, token); err != nil { + if err := ValidateApprovalToken(execution, token); err != nil { return nil, err }