From 449b550eda99833f032b6f94c78b99afb08b2046 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 13:35:30 +0100 Subject: [PATCH] fix(purchase): enforce approval-token TTL in the async approve/cancel check loadAsyncExecutionForApproval, which authenticates SQS approve and cancel messages in verifyAsyncApprovalActor, was a third hand-rolled copy of the approval-token check. It did the empty check and the constant-time hash compare but never looked at ApprovalTokenExpiresAt, so an expired token passed it and reached the approver-list lookup. It was only rejected because ApproveExecution / CancelExecution re-validated the token one frame later. Route it through ValidateApprovalToken, the helper ApproveExecution and loadCancelableExecution already use, so all three sites share the empty check, config.ApprovalTokenMatches and the TTL guard. Closes #268 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/messages.go | 25 ++-- internal/purchase/messages_token_ttl_test.go | 120 +++++++++++++++++++ 2 files changed, 132 insertions(+), 13 deletions(-) create mode 100644 internal/purchase/messages_token_ttl_test.go diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index eaf051854..7d0ca6b24 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -205,10 +205,11 @@ func (m *Manager) handleCancelMessage(ctx context.Context, msg *AsyncMessage) er // 1. Reject the message outright if actor_email is empty — legacy / // replayed payloads land here and must NOT fall through to a // tokenless approval. -// 2. Load the execution by ID and validate the approval token using -// constant-time compare, mirroring ApproveExecution / CancelExecution -// so a token mismatch fails fast with the same error shape and we -// don't leak whether the actor or the token was wrong. +// 2. Load the execution by ID and validate the approval token (constant- +// time compare plus TTL) through ValidateApprovalToken, the same +// helper ApproveExecution / CancelExecution use, so a bad or expired +// token fails fast with the same error shape and we don't leak +// whether the actor or the token was wrong. // 3. Resolve the per-account contact_email approver list from the // execution's recommendations and assert actor_email matches one // of them (case-insensitive, trimmed). Empty approver list is a @@ -229,7 +230,8 @@ func (m *Manager) verifyAsyncApprovalActor(ctx context.Context, msg *AsyncMessag } // loadAsyncExecutionForApproval fetches the execution targeted by an -// async approve/cancel message and validates the approval token. Split +// async approve/cancel message and validates the approval token, +// including its expiry, via ValidateApprovalToken. Split // out from verifyAsyncApprovalActor so the parent stays under the // repo's gocyclo threshold. func (m *Manager) loadAsyncExecutionForApproval(ctx context.Context, msg *AsyncMessage) (*config.PurchaseExecution, error) { @@ -240,14 +242,11 @@ func (m *Manager) loadAsyncExecutionForApproval(ctx context.Context, msg *AsyncM if err != nil { return nil, fmt.Errorf("failed to get execution: %w", err) } - if execution.ApprovalToken == "" || msg.Token == "" { - return nil, fmt.Errorf("invalid approval token") - } - // execution.ApprovalToken is the SHA-256 hex digest stored at rest - // (issue #103); config.ApprovalTokenMatches hashes msg.Token and - // compares digests in constant time. - if !config.ApprovalTokenMatches(execution.ApprovalToken, msg.Token) { - return nil, fmt.Errorf("invalid approval token") + // Same check as ApproveExecution and loadCancelableExecution (issue + // #268): non-empty token, constant-time digest compare against the + // SHA-256 hash stored at rest (issue #103), and the TTL (issue #397). + if err := ValidateApprovalToken(execution, msg.Token); err != nil { + return nil, err } return execution, nil } diff --git a/internal/purchase/messages_token_ttl_test.go b/internal/purchase/messages_token_ttl_test.go new file mode 100644 index 000000000..c8ea984b1 --- /dev/null +++ b/internal/purchase/messages_token_ttl_test.go @@ -0,0 +1,120 @@ +package purchase + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #268: loadAsyncExecutionForApproval was a third copy of the +// approval-token check (alongside ValidateApprovalToken and +// loadCancelableExecution) that compared the token but never enforced +// ApprovalTokenExpiresAt. An expired token on an SQS approve/cancel message +// was only rejected because ApproveExecution / CancelExecution re-validated +// it one frame later. These tests pin the TTL guard on the helper itself and +// on the SQS entry points, before approver resolution runs. + +func asyncTTLExecution(id string, expiresAt *time.Time) *config.PurchaseExecution { + accountID := "acct-1" + return &config.PurchaseExecution{ + ExecutionID: id, + Status: "pending", + ApprovalToken: config.HashApprovalToken("correct-token"), + ApprovalTokenExpiresAt: expiresAt, + Recommendations: []config.RecommendationRecord{ + {CloudAccountID: &accountID}, + }, + } +} + +func TestLoadAsyncExecutionForApproval_TokenValidation(t *testing.T) { + past := time.Now().Add(-time.Hour) + future := time.Now().Add(time.Hour) + + tests := []struct { + name string + expiresAt *time.Time + token string + wantErr error + }{ + {name: "expired token is rejected", expiresAt: &past, token: "correct-token", wantErr: ErrApprovalTokenExpired}, + {name: "unexpired token is accepted", expiresAt: &future, token: "correct-token"}, + {name: "legacy row without expiry is accepted", expiresAt: nil, token: "correct-token"}, + {name: "wrong token is rejected", expiresAt: &future, token: "wrong-token", wantErr: ErrInvalidApprovalToken}, + {name: "wrong token on expired row reports invalid, not expired", expiresAt: &past, token: "wrong-token", wantErr: ErrInvalidApprovalToken}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + exec := asyncTTLExecution("exec-ttl", tt.expiresAt) + mockStore.On("GetExecutionByID", ctx, "exec-ttl").Return(exec, nil).Once() + + manager := &Manager{config: mockStore} + got, err := manager.loadAsyncExecutionForApproval(ctx, &AsyncMessage{ + ExecutionID: "exec-ttl", + Token: tt.token, + }) + + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + assert.Nil(t, got) + } else { + require.NoError(t, err) + assert.Same(t, exec, got) + } + mockStore.AssertExpectations(t) + }) + } +} + +// TestProcessMessage_ExpiredTokenRejectedBeforeApproverLookup drives the +// real SQS entry point for both approve and cancel with an expired token +// from an actor who IS on the approver list. The rejection must come from +// verifyAsyncApprovalActor's own token check: the execution is loaded once +// and approver resolution (GetCloudAccount) never runs. Before the fix the +// approver lookup succeeded and the message was only rejected by the second +// load inside ApproveExecution / CancelExecution. +func TestProcessMessage_ExpiredTokenRejectedBeforeApproverLookup(t *testing.T) { + for _, msgType := range []string{"approve", "cancel"} { + t.Run(msgType, func(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + + past := time.Now().Add(-24 * time.Hour) + exec := asyncTTLExecution("exec-expired", &past) + // GetExecutionByID and GetCloudAccount are registered without a + // call limit so a pre-fix run reaches the approver match and the + // second load inside ApproveExecution / CancelExecution instead + // of panicking on an unexpected call; the call-count and + // AssertNotCalled checks below are the actual assertions. + mockStore.On("GetExecutionByID", ctx, "exec-expired").Return(exec, nil) + account := &config.CloudAccount{ID: "acct-1", ContactEmail: "owner@example.com"} + mockStore.On("GetCloudAccount", ctx, "acct-1").Return(account, nil).Maybe() + + manager := &Manager{ + config: mockStore, + email: mockEmail, + dashboardURL: "https://dashboard.example.com", + } + + err := manager.ProcessMessage(ctx, `{"type":"`+msgType+`","execution_id":"exec-expired","token":"correct-token","actor_email":"owner@example.com"}`) + require.ErrorIs(t, err, ErrApprovalTokenExpired) + mockStore.AssertNumberOfCalls(t, "GetExecutionByID", 1) + mockStore.AssertNotCalled(t, "GetCloudAccount", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "TransitionExecutionStatus", + mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "CancelExecutionAtomic", + mock.Anything, mock.Anything, mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) + mockStore.AssertExpectations(t) + }) + } +}