From 8441e6f42d658730c969dfa7c11472e2f33dd0b5 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 03:37:15 +0100 Subject: [PATCH] fix(api): answer a hidden execution like a missing one on approve, cancel and revoke A signed-in caller could still tell a missing execution from one they may not act on. A missing ID got 404 "execution not found". An existing one got a 403 RBAC or contact_email denial, a 404 "Not found" from the account-scope gate, a 409 from the approve orphan preflight, or 403 "CSRF validation failed" when the CSRF header was missing. The routes now validate CSRF before the lookup for every session caller, token branch included. Final authorization denials (scope miss, RBAC 403, contact_email 401/403) map to the same 404 a missing ID gets. The orphan 409 only runs once the session is authorized. Constraint denials and 4-eyes still answer 403, since they reach only callers who already hold approve-* for the execution. Closes #597 Co-Authored-By: Claude Opus 5.5 --- internal/api/coverage_extras_test.go | 4 + .../api/executed_notification_flow_test.go | 8 +- internal/api/execution_scope_test.go | 16 ++- internal/api/handler_purchases.go | 105 ++++++++++---- internal/api/handler_purchases_test.go | 90 ++++++++---- .../handler_purchases_token_errors_test.go | 2 + .../handler_purchases_token_oracle_test.go | 136 ++++++++++++++++++ internal/api/handler_test.go | 4 + internal/api/middleware_test.go | 115 ++++++++------- internal/api/session_denial_final_test.go | 10 +- internal/server/lambda_coverage_test.go | 39 +++-- 11 files changed, 402 insertions(+), 127 deletions(-) diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 2373d40f4..3e0d9842f 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -87,6 +87,8 @@ func TestHandler_approvePurchase_PurchaseError(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // After issue #286, approvePurchase is session-first: with a Bearer // header present the dispatch consults the approve-{any,own} RBAC // matrix BEFORE falling through to the token branch. The session @@ -160,6 +162,8 @@ func TestHandler_cancelPurchase_PurchaseError(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // Session has no admin role / cancel permissions → cancelPurchase's // session-authed pre-check falls through to authorizeApprovalAction → // purchase manager invocation, which is what this test's diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 6f89a9826..fb15aa3ef 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -94,7 +94,7 @@ func TestExecutedNotification_TokenApprovePath(t *testing.T) { }, } - // First call: loadApproveExecution fetches the pending execution. + // First call: getExecutionOr404 fetches the pending execution. // Second call: approveViaToken re-fetches after ApproveExecution returns, // to pick up the FINAL execution state (status, completed_at) for the // email body -- not the token, which comes from the return value below. @@ -108,6 +108,8 @@ func TestExecutedNotification_TokenApprovePath(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() @@ -164,7 +166,7 @@ func TestExecutedNotification_TokenApprovePath_RefetchFailureStillEmailsToken(t mockConfig := new(MockConfigStore) exec := approvalTestExec(execID, contact, mockConfig) - // First call: loadApproveExecution fetches the pending execution. + // First call: getExecutionOr404 fetches the pending execution. // Second call: approveViaToken re-fetches for the final email state, // but this time the store errors -- the handler must fall back to the // pre-approve snapshot for those fields, and still emails the token. @@ -179,6 +181,8 @@ func TestExecutedNotification_TokenApprovePath_RefetchFailureStillEmailsToken(t mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contact}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() diff --git a/internal/api/execution_scope_test.go b/internal/api/execution_scope_test.go index 4842539d0..9495bc493 100644 --- a/internal/api/execution_scope_test.go +++ b/internal/api/execution_scope_test.go @@ -246,7 +246,7 @@ func TestExecutionEndpoints_ScopeMatrix(t *testing.T) { if !sc.allow { require.Error(t, err) - assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + assert.True(t, isNotFound404(err), "expected 404 not-found, got %v", err) // A denied call reaches no mutation of any endpoint. store.AssertNotCalled(t, "SavePurchaseExecution") store.AssertNotCalled(t, "TransitionExecutionStatus") @@ -301,11 +301,23 @@ func TestExecutionEndpoints_OutOfScopeNoTokenIs404(t *testing.T) { t.Run(tc.name, func(t *testing.T) { h := newScopeTestHandler(t, scopeTestExecution(adhoc, tc.status), []string{"Other"}, new(MockConfigStore), new(MockPurchaseManager)) _, err := tc.call(h, scopeTestRequest()) - assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + assert.True(t, isNotFound404(err), "expected 404 not-found, got %v", err) }) } } +// isNotFound404 accepts either form of the enumeration-safe 404: errNotFound +// from the scope gate, or the "execution not found" ClientError the approve, +// cancel and revoke routes answer for both a missing and a hidden execution +// (issue #597). +func isNotFound404(err error) bool { + if IsNotFoundError(err) { + return true + } + ce, ok := IsClientError(err) + return ok && ce.code == 404 && ce.message == "execution not found" +} + // TestRevoke_AuthorizedNonRevocableStatusIs409: moving the status check // behind authorization must not drop it. An authorized caller, via the // session or the email token, still gets the 409 for a status with nothing diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index f3d4977d9..c651150ca 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -616,25 +616,20 @@ func (h *Handler) disablePlan(ctx context.Context, planID string) error { return nil } -// loadApproveExecution fetches an execution by ID, returns a 404 ClientError -// when not found, and then runs the issue-#609 orphan preflight check. -// Extracted from approvePurchase to keep that function below the gocyclo -// threshold — same pattern as loadCancelableExecution in the purchase package. -func (h *Handler) loadApproveExecution(ctx context.Context, execID string) (*config.PurchaseExecution, error) { - execution, err := h.config.GetExecutionByID(ctx, execID) - if errors.Is(err, config.ErrNotFound) { - return nil, NewClientError(404, "execution not found") - } - if err != nil { - return nil, fmt.Errorf("failed to get execution: %w", err) - } - // Preflight (issue #609): reject non-AWS orphan executions before the - // cloud SDK is reached. Delegates to the centralized predicate in the - // purchase package so the logic is maintained in one place. +// rejectOrphanExecution runs the issue-#609 orphan preflight: non-AWS +// executions whose account no longer exists are rejected with a 409 before +// the cloud SDK is reached. Delegates to the centralized predicate in the +// purchase package so the logic is maintained in one place. It runs only +// once the session is authorized for the execution, so the 409 cannot tell +// an unauthorized caller that the execution exists (issue #597). The token +// branch needs no copy: an orphan has no per-account contact_email, so +// authorizeApprovalAction never authorizes it, and ApproveExecution runs +// the same predicate itself. +func rejectOrphanExecution(execution *config.PurchaseExecution) error { if err := purchase.OrphanExecutionError(execution); err != nil { - return nil, NewClientError(409, err.Error()) + return NewClientError(409, err.Error()) } - return execution, nil + return nil } // Purchase action handlers. @@ -647,7 +642,7 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio return nil, err } - execution, err := h.loadApproveExecution(ctx, execID) + execution, err := h.getExecutionOr404(ctx, execID) if err != nil { return nil, err } @@ -665,6 +660,7 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // validates the token itself before mutating state. // 3. token == "" → a session denial above is final (issue #173); // with no session, approvePurchaseViaSession returns 401. + // A final denial answers exactly like a missing execution (issue #597). if session := h.tryGetSession(ctx, req); session != nil { switch err := h.authorizeScopedSessionApprove(ctx, session, execution); { case err == nil: @@ -675,8 +671,9 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // chance (a logged-in user without approve-* or outside the // account scope may still be the per-account contact recipient). default: - // Transient failure — propagate instead of silently widening. - return nil, err + // A final denial (no token) or a transient failure — propagate + // instead of silently widening; the denial is concealed. + return nil, concealExecutionDenial(err) } } @@ -692,13 +689,59 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // approve/cancel/revoke routes needs a session (the token alone never // authorizes), so an unauthenticated caller gets the same response for a // missing and an existing execution ID (issue #435). +// +// CSRF is validated here too, before the lookup (issue #597). Every +// remaining caller is session-authenticated, and the session bearer token +// is cookie-equivalent, so the token branch needs CSRF as much as the +// session branch does. Checking it only on the session branch, after the +// lookup, answered "CSRF validation failed" for an existing execution and +// 404 for a missing one. func (h *Handler) requireSessionBeforeLookup(ctx context.Context, req *events.LambdaFunctionURLRequest) error { if h.tryGetSession(ctx, req) == nil { return NewClientError(401, "sign in with the account's contact email to approve or cancel this purchase") } + if err := h.validateCSRF(ctx, req); err != nil { + return errCSRFRejected + } return nil } +// executionNotFound is the answer for an execution ID that does not exist +// on the approve, cancel and revoke routes. concealExecutionDenial returns +// the same error for an execution the caller may not act on, so the two +// cases have the same status, headers and body (issue #597). +func executionNotFound() *clientError { + return &clientError{code: 404, message: "execution not found"} +} + +// concealedDenialError is the "execution not found" 404 returned in place of +// a final authorization denial. It responds exactly like executionNotFound +// (IsClientError resolves it through Unwrap) and keeps the denial it hides +// in cause, for logs and tests only. +type concealedDenialError struct { + *clientError + cause error +} + +func (e *concealedDenialError) Unwrap() error { return e.clientError } + +// concealExecutionDenial maps a final authorization denial on an existing +// execution (an account-scope miss, an RBAC or contact_email 403, or a 401 +// from the contact_email gate) to the 404 a missing execution gets, so a +// signed-in caller cannot use the approve, cancel and revoke routes to +// learn which execution IDs exist (issue #597). Only direct *clientError +// values are mapped: a Constraints denial (constraintDeniedError) goes to a +// caller who already holds approve-* for the execution, and wrapped +// backend failures must keep surfacing as 500s. +func concealExecutionDenial(err error) error { + ce, ok := err.(*clientError) //nolint:errorlint // strict (unwrapped) assertion is deliberate; see isPermissionDenied + if !errors.Is(err, errNotFound) && (!ok || (ce.code != 401 && ce.code != 403)) { + return err + } + logging.Infof("purchase action denied, answered as execution not found: %v", err) + return &concealedDenialError{clientError: executionNotFound(), cause: err} +} + // tokenActionError maps the email-link approve/cancel failures that are the // caller's fault to a 4xx: a wrong token is 403 (as on the RI exchange // approve path), an expired token is 410, and an execution @@ -725,7 +768,7 @@ func tokenActionError(err error) error { func (h *Handler) approveViaToken(ctx context.Context, req *events.LambdaFunctionURLRequest, execution *config.PurchaseExecution, token string) (any, error) { actor, err := h.authorizeApprovalAction(ctx, req, execution) if err != nil { - return nil, err + return nil, concealExecutionDenial(err) } // 4-eyes mode (issue #1005): resolve the session again for identity // comparison. On the email-token deep-link flow the user is logged in @@ -832,6 +875,12 @@ func (h *Handler) approvePurchaseViaSession(ctx context.Context, req *events.Lam return nil, err } + // approvePurchase only calls this with a session it has authorized, so + // the orphan 409 reveals nothing to an unauthorized caller (issue #597). + if err := rejectOrphanExecution(execution); err != nil { + return nil, err + } + if execution.Status != "pending" && execution.Status != "notified" { return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be approved (status=%s)", execution.ExecutionID, execution.Status)) } @@ -1351,15 +1400,16 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // returning a wrapped error, h.auth==nil 500). Propagate // instead of silently widening to the contact_email gate — // a stale auth backend should not mask itself as a 403 about - // missing contact emails. CR feedback on PR #216. - return nil, err + // missing contact emails. CR feedback on PR #216. A final + // denial answers like a missing execution (issue #597). + return nil, concealExecutionDenial(err) } } if token != "" { actor, err := h.authorizeApprovalAction(ctx, req, execution) if err != nil { - return nil, err + return nil, concealExecutionDenial(err) } if err := h.purchase.CancelExecution(ctx, execID, token, actor); err != nil { return nil, tokenActionError(err) @@ -1554,7 +1604,7 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun // this iteration) and confirm the caller is the authorized contact email. actor, err := h.authorizeApprovalAction(ctx, req, execution) if err != nil { - return nil, err + return nil, concealExecutionDenial(err) } if err := validateRevokeToken(execution, token); err != nil { return nil, err @@ -1570,7 +1620,7 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun func (h *Handler) getExecutionOr404(ctx context.Context, execID string) (*config.PurchaseExecution, error) { execution, err := h.config.GetExecutionByID(ctx, execID) if errors.Is(err, config.ErrNotFound) || (err == nil && execution == nil) { - return nil, NewClientError(404, "execution not found") + return nil, executionNotFound() } if err != nil { return nil, fmt.Errorf("failed to get execution: %w", err) @@ -1633,7 +1683,8 @@ func (h *Handler) tryRevokeViaSession(ctx context.Context, req *events.LambdaFun // Fall through to the token branch. return nil, false, nil default: - return nil, true, sessErr + // A final denial answers like a missing execution (issue #597). + return nil, true, concealExecutionDenial(sessErr) } } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 9ceee183c..1cd8819b5 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -62,6 +62,8 @@ func TestHandler_approvePurchase(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // After issue #286 the session-authed approve dispatch consults the // approve-{any,own} verb matrix BEFORE falling through to the token // branch. The session here is the approver's mailbox (no role / no @@ -105,6 +107,8 @@ func TestHandler_cancelPurchase(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // Session has no admin role and no cancel permissions, so cancelPurchase's // session-authed pre-check (added to fix the deep-link contact_email gate) // falls through to authorizeApprovalAction. The contact_email + globalNotify @@ -144,6 +148,8 @@ func TestHandler_approvePurchase_RejectsMismatchedSession(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Session belongs to someone who is NOT the authorized approver. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: "wrong@example.com"}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // After issue #286 the dispatch consults approve-{any,own} BEFORE // the contact_email gate. The wrong@example.com session has neither // verb, so the dispatch returns 403 from authorizeSessionApprove, @@ -161,8 +167,8 @@ func TestHandler_approvePurchase_RejectsMismatchedSession(t *testing.T) { Headers: map[string]string{"authorization": "Bearer sess-tok"}, } _, err := handler.approvePurchase(ctx, req, execID, "valid-token") - require.Error(t, err) - assert.Contains(t, err.Error(), "not the authorized approver") + // The contact_email denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "not the authorized approver") // ApproveExecution must not have been called — purchase manager mock // asserts nothing by construction; a .On(...) entry above would create // a false positive, so we pin the negative by confirming the error is @@ -203,6 +209,8 @@ func TestHandler_approvePurchase_RejectsMissingContactEmail(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: globalNotify}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // Issue #286 dispatch consults approve-{any,own} BEFORE the contact_email // gate. globalNotify session has neither verb → 403 → fall through to // token branch → authorizeApprovalAction surfaces the "no per-account @@ -218,8 +226,8 @@ func TestHandler_approvePurchase_RejectsMissingContactEmail(t *testing.T) { Headers: map[string]string{"authorization": "Bearer sess-tok"}, } _, err := handler.approvePurchase(ctx, req, execID, "valid-token") - require.Error(t, err) - assert.Contains(t, err.Error(), "no per-account contact email") + // The contact_email denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "no per-account contact email") mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } @@ -273,6 +281,8 @@ func TestHandler_approvePurchase_AcceptsContactEmailSession(t *testing.T) { // Session email matches the account contact email — global notify is // NOT enough here because a contact email exists for the account. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the contact_email // gate. The contact-email session has neither verb (no Role / no UserID) // → 403 → fall through to token branch → authorizeApprovalAction matches @@ -455,6 +465,8 @@ func TestHandler_approvePurchase_SessionApproveAny_PermissionConstraintsDenied(t t.Cleanup(func() { mockAuth.AssertExpectations(t) }) userSession := &Session{UserID: "dddddddd-dddd-dddd-dddd-dddddddddddd", Email: "capped@example.com"} mockAuth.On("ValidateSession", ctx, "sess-tok").Return(userSession, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // The bare verb/resource gate passes - this is exactly what happens for // a permission that carries Constraints, because HasPermissionAPI checks // with nil request-side constraints. @@ -532,6 +544,8 @@ func TestHandler_approvePurchase_SessionApproveAny_TokenPresent_ConstraintsDenie mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() t.Cleanup(func() { mockAuth.AssertExpectations(t) }) mockAuth.On("ValidateSession", ctx, "capped-token").Return(capped, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "capped-token", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, capped.UserID, "approve-any", "purchases").Return(true, nil) mockAuth.On("HasPermissionForConstraintsAPI", ctx, capped.UserID, "approve-any", "purchases", purchaseConstraintSets(exec.Recommendations)).Return(false, nil) @@ -614,6 +628,8 @@ func TestHandler_approveViaToken_GlobalConfigError_FailsClosed(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // No approve-any / approve-own permissions — dispatch falls to token path. mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() @@ -763,13 +779,17 @@ func TestHandler_approvePurchase_AzureOrphanRejects409(t *testing.T) { ApprovalToken: "valid-token", Status: "pending", // CloudAccountID intentionally nil — account was deleted. - Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "azure"}}, + Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "azure", UpfrontCost: 100}}, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: "admin@example.com"}, nil) - mockAuth.grantAdmin() + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + // The orphan guard runs once the session is authorized to approve + // (issue #597), so the caller holds approve-any, not just admin:*. + mockAuth.grantAdminPurchaser() mockPurchase := new(MockPurchaseManager) @@ -800,13 +820,17 @@ func TestHandler_approvePurchase_GCPOrphanRejects409(t *testing.T) { ExecutionID: execID, ApprovalToken: "valid-token", Status: "pending", - Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "gcp"}}, + Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "gcp", UpfrontCost: 100}}, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: "admin@example.com"}, nil) - mockAuth.grantAdmin() + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) + // The orphan guard runs once the session is authorized to approve + // (issue #597), so the caller holds approve-any, not just admin:*. + mockAuth.grantAdminPurchaser() mockPurchase := new(MockPurchaseManager) @@ -942,6 +966,8 @@ func TestHandler_approvePurchase_RejectsGlobalNotifyWhenContactSet(t *testing.T) mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: globalNotify}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the // contact_email gate. Returning false for both verbs lets the // dispatch fall through to the token branch where the @@ -957,8 +983,8 @@ func TestHandler_approvePurchase_RejectsGlobalNotifyWhenContactSet(t *testing.T) Headers: map[string]string{"authorization": "Bearer sess-tok"}, } _, err := handler.approvePurchase(ctx, req, execID, "valid-token") - require.Error(t, err) - assert.Contains(t, err.Error(), "not the authorized approver") + // The contact_email denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "not the authorized approver") mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } @@ -3023,8 +3049,8 @@ func TestHandler_approvePurchase_OutOfScope(t *testing.T) { Headers: map[string]string{"Authorization": "Bearer operator-token"}, } _, err := handler.approvePurchase(ctx, req, executionID, "") - require.Error(t, err) - assert.True(t, IsNotFoundError(err), "expected 404 not-found, got %v", err) + // The scope miss answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "not found") mockStore.AssertNotCalled(t, "GetGlobalConfig") } @@ -3225,8 +3251,8 @@ func TestHandler_cancelPurchase_Session_CancelOwn_RejectsNonCreator(t *testing.T handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, true) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") - require.Error(t, err) - assert.Contains(t, err.Error(), "another user's pending purchase") + // The denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "another user's pending purchase") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") mockAuth.AssertExpectations(t) @@ -3244,8 +3270,8 @@ func TestHandler_cancelPurchase_Session_NoVerb_Rejects(t *testing.T) { handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") - require.Error(t, err) - assert.Contains(t, err.Error(), "cancel-any or cancel-own") + // The denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "cancel-any or cancel-own") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") mockAuth.AssertExpectations(t) @@ -3408,8 +3434,8 @@ func TestHandler_cancelPurchase_Session_LegacyNullCreator_NonAdminRejected(t *te handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, true) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") - require.Error(t, err) - assert.Contains(t, err.Error(), "another user's pending purchase") + // The denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "another user's pending purchase") mockConfig.AssertNotCalled(t, "WithTx") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") mockAuth.AssertExpectations(t) @@ -3584,6 +3610,8 @@ func TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates(t *testing mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil) // Simulate a transient auth-backend failure on the cancel-any check. // authorizeSessionCancel wraps this as "permission check failed: …" // — NOT a 403 ClientError. The pre-check must propagate. @@ -3629,9 +3657,9 @@ func TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGat mockConfig.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") - require.Error(t, err) - assert.Contains(t, err.Error(), "no per-account contact email configured for this execution", - "non-privileged session must fall through to the contact_email gate from PR #101") + // The non-privileged session falls through to the contact_email gate + // from PR #101, whose denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "no per-account contact email configured for this execution") mockConfig.AssertNotCalled(t, "SavePurchaseExecution") mockAuth.AssertExpectations(t) } @@ -5736,6 +5764,8 @@ func TestHandler_revokePurchase_ValidToken(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() // Provide a session for the revoker so authorizeApprovalAction resolves actor. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) // RBAC: revoker has no cancel-any or cancel-own, so falls through to token path. mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -5782,6 +5812,8 @@ func TestHandler_revokePurchase_InvalidToken(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -5836,6 +5868,8 @@ func TestHandler_revokePurchase_NotFound(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: "admin@example.com"}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil) handler := &Handler{config: mockStore, auth: mockAuth} req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}} _, err := handler.revokeViaEmailToken(ctx, req, execID, "some-token") @@ -5878,6 +5912,8 @@ func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -6004,6 +6040,8 @@ func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "user-token"). Return(&Session{UserID: userID, Email: userEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "user-token", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, userID, "cancel-own", "purchases").Return(false, nil) @@ -6012,10 +6050,8 @@ func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { Headers: map[string]string{"authorization": "Bearer user-token"}, } _, err := handler.revokeViaEmailToken(ctx, req, execID, "") - require.Error(t, err, "no permission and no token must be denied") - ce, ok := IsClientError(err) - require.True(t, ok, "expected a client error") - assert.Equal(t, 403, ce.code) + // No permission and no token: denied, answered like a missing execution (issue #597). + requireConcealedDenial(t, err, "requires cancel-any or cancel-own") mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) } @@ -6372,6 +6408,8 @@ func TestRevokePurchase_POSTPerformsRevoke(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: revokerEmail}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() @@ -6566,6 +6604,8 @@ func TestRequireDifferentApprover_EmailTokenPath_ModeOn(t *testing.T) { // through to the token branch. The 4-eyes check then uses the session // identity against CreatedByUserID and must deny. mockAuth.On("ValidateSession", ctx, "sess-tok").Return(creatorSession, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockAuth.On("HasPermissionAPI", ctx, creatorID, "approve-any", "purchases").Return(false, nil).Maybe() mockAuth.On("HasPermissionAPI", ctx, creatorID, "approve-own", "purchases").Return(false, nil).Maybe() diff --git a/internal/api/handler_purchases_token_errors_test.go b/internal/api/handler_purchases_token_errors_test.go index 7993072f8..91d471794 100644 --- a/internal/api/handler_purchases_token_errors_test.go +++ b/internal/api/handler_purchases_token_errors_test.go @@ -42,6 +42,8 @@ func tokenErrHandler(t *testing.T, exec *config.PurchaseExecution, getErr error) auth := new(MockAuthService) auth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() auth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + auth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil).Maybe() for _, perm := range []string{"approve-any", "approve-own", "cancel-any", "cancel-own", "revoke-any", "revoke-own"} { auth.On("HasPermissionAPI", mock.Anything, "", perm, "purchases").Return(false, nil).Maybe() } diff --git a/internal/api/handler_purchases_token_oracle_test.go b/internal/api/handler_purchases_token_oracle_test.go index 1cd37bdcb..358b3b9ac 100644 --- a/internal/api/handler_purchases_token_oracle_test.go +++ b/internal/api/handler_purchases_token_oracle_test.go @@ -11,7 +11,9 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" ) // A caller with no session must get the same answer for a missing and an @@ -55,3 +57,137 @@ func TestHandleRequest_TokenActions_UnauthenticatedCannotTellMissingFromExisting } } } + +// oracleCSRF is the CSRF header value the signed-in callers below send when +// the variant says they send a valid one. +const oracleCSRF = "csrf-ok" + +// oracleCaller is a signed-in caller who may not act on the execution, or +// who may but forgot the CSRF header. +type oracleCaller struct { + grant func(*MockAuthService) + email string + sendCSRF bool +} + +func oracleCallers() map[string]oracleCaller { + none := func(m *MockAuthService) { m.grantPermissions(nil) } + own := func(m *MockAuthService) { + m.grantPermissions([]auth.Permission{ + {Action: auth.ActionApproveOwn, Resource: auth.ResourcePurchases}, + {Action: auth.ActionCancelOwn, Resource: auth.ResourcePurchases}, + }) + } + return map[string]oracleCaller{ + "no-rbac/csrf": {grant: none, email: "outsider@example.com", sendCSRF: true}, + "no-rbac/no-csrf": {grant: none, email: "outsider@example.com"}, + "own-not-creator/csrf": {grant: own, email: "outsider@example.com", sendCSRF: true}, + "scoped-out/csrf": {grant: func(m *MockAuthService) { m.grantScoped("other-acct") }, email: "outsider@example.com", sendCSRF: true}, + "admin/no-csrf": {grant: func(m *MockAuthService) { m.grantAdmin() }, email: "admin@example.com"}, + "contact/no-csrf": {grant: none, email: "contact@example.com"}, + } +} + +// oracleHandler wires a handler whose execution belongs to acct-1 (contact +// contact@example.com) and was created by another user; an orphan is an +// Azure execution whose account is gone (issue #609). exec == nil makes +// the store answer ErrNotFound. Every status write loses its CAS, so a caller +// that wrongly got past authorization sees a 409 and no purchase runs. +func oracleHandler(t *testing.T, caller oracleCaller, exec *config.PurchaseExecution, orphan bool) *Handler { + t.Helper() + accountID := "acct-1" + creator := "creator-user-id" + store := new(MockConfigStore) + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: id, ContactEmail: "contact@example.com"}, nil + } + if exec == nil { + store.On("GetExecutionByID", mock.Anything, tokenErrExecID).Return(nil, config.ErrNotFound) + } else { + exec.ExecutionID = tokenErrExecID + exec.CloudAccountID = &accountID + exec.CreatedByUserID = &creator + exec.Recommendations = []config.RecommendationRecord{{ID: "r1", Provider: "aws", CloudAccountID: &accountID, UpfrontCost: 100}} + if orphan { + exec.CloudAccountID = nil + exec.Recommendations = []config.RecommendationRecord{{ID: "r1", Provider: "azure", UpfrontCost: 100}} + } + store.On("GetExecutionByID", mock.Anything, tokenErrExecID).Return(exec, nil) + store.On("TransitionExecutionStatus", mock.Anything, tokenErrExecID, mock.Anything, mock.Anything, mock.Anything). + Return(nil, config.ErrExecutionNotInExpectedStatus).Maybe() + store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, tokenErrExecID, mock.Anything). + Return(false, exec.Status, nil).Maybe() + } + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + + m := new(MockAuthService) + m.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{UserID: "caller-user-id", Email: caller.email}, nil) + m.On("ValidateCSRFToken", mock.Anything, "sess-tok", oracleCSRF).Return(nil).Maybe() + m.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(errors.New("missing CSRF token")).Maybe() + caller.grant(m) + + return &Handler{ + config: store, + auth: m, + purchase: purchase.NewManager(purchase.ManagerConfig{ConfigStore: store}), + } +} + +// A signed-in caller who may not act on an execution, or who omits the CSRF +// header, must get the same status, headers and body for an existing and a +// missing execution ID on approve, cancel and revoke (issue #597). +func TestHandleRequest_TokenActions_SignedInCannotTellMissingFromExisting(t *testing.T) { + future := time.Now().Add(time.Hour) + hash := config.HashApprovalToken(tokenErrRawToken) + + for _, action := range []string{"approve", "cancel", "revoke"} { + for _, status := range []string{"pending", "completed", "orphan"} { + for _, token := range []string{"", tokenErrRawToken, "wrong"} { + for name, caller := range oracleCallers() { + if name == "contact/no-csrf" && token == "" { + continue // without a token the contact gate never runs + } + t.Run(action+"/"+status+"/token="+token+"/"+name, func(t *testing.T) { + call := func(h *Handler) *events.LambdaFunctionURLResponse { + req := tokenErrRequest(action, token) + if caller.sendCSRF { + req.Headers["x-csrf-token"] = oracleCSRF + } + resp, err := h.HandleRequest(context.Background(), req) + require.NoError(t, err) + return resp + } + orphan := status == "orphan" + execStatus := status + if orphan { + execStatus = "pending" + } + exec := &config.PurchaseExecution{Status: execStatus, ApprovalToken: hash, ApprovalTokenExpiresAt: &future} + existing := call(oracleHandler(t, caller, exec, orphan)) + missing := call(oracleHandler(t, caller, nil, false)) + + assert.Contains(t, []int{403, 404}, existing.StatusCode, existing.Body) + assert.Equal(t, missing.StatusCode, existing.StatusCode, "existing: %s / missing: %s", existing.Body, missing.Body) + assert.Equal(t, missing.Body, existing.Body) + assert.Equal(t, missing.Headers, existing.Headers) + }) + } + } + } + } +} + +// requireConcealedDenial asserts err answers like a missing execution (404 +// "execution not found") and that the denial it hides contains wantCause, +// so a test still pins which gate denied the caller (issue #597). +func requireConcealedDenial(t *testing.T, err error, wantCause string) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got %v", err) + assert.Equal(t, 404, ce.code) + assert.Equal(t, "execution not found", ce.message) + var cd *concealedDenialError + require.ErrorAs(t, err, &cd) + assert.Contains(t, cd.cause.Error(), wantCause) +} diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 85a83bad6..73d253f51 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -867,6 +867,8 @@ func TestHandler_HandleRequest_ApprovePurchase(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil) // Issue #286: dispatch consults approve-{any,own} BEFORE the // contact_email gate. Returning false for both verbs lets the // dispatch fall through to the legacy token branch this test @@ -954,6 +956,8 @@ func TestHandler_HandleRequest_CancelPurchase(t *testing.T) { mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) + // CSRF is validated before the execution lookup (issue #597). + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "").Return(nil) // Session has no admin role / cancel permissions → cancelPurchase's // session-authed pre-check falls through to authorizeApprovalAction → // MockPurchaseManager.CancelExecution, exercising the legacy diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 11d376095..0a7b5bcb8 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -669,64 +669,71 @@ func TestApproveRIExchangeViaSession_PassesCSRF(t *testing.T) { mockStore.AssertCalled(t, "TransitionRIExchangeStatus", ctx, id, "pending", "processing", mock.Anything) } -// TestTokenOnlyApprove_BypassesCSRF asserts that the email-link token path -// does NOT invoke approvePurchaseViaSession, so CSRF is not checked on that -// code path. The token path uses authorizeApprovalAction which validates the -// signed token directly -- it has no session cookie to forge. -// -// The caller must supply a session whose email matches the contact_email (the -// approval gate added in PR #101 ensures only the intended inbox can confirm -// via the email link). Because the session has no approve-* RBAC, the handler -// falls through from the session-authed branch to the token branch. -func TestTokenOnlyApprove_BypassesCSRF(t *testing.T) { +// TestTokenApprove_SessionRequiresCSRF covers the email-link token branch, +// which every caller now reaches with a session (issue #435). The session +// bearer token is cookie-equivalent, so CSRF is validated before the +// execution is even looked up (issue #597): without the header the caller +// gets 403 and the execution is never read; with it, the contact_email +// recipient (no approve-* RBAC, so the dispatch falls through to the token +// branch) still approves. +func TestTokenApprove_SessionRequiresCSRF(t *testing.T) { ctx := context.Background() execID := "12345678-1234-1234-1234-123456789abc" contactEmail := "contact@example.com" - mockConfig := new(MockConfigStore) - accountID := "acct-1" - exec := &config.PurchaseExecution{ - ExecutionID: execID, - ApprovalToken: "email-token", - Status: "pending", - Recommendations: []config.RecommendationRecord{ - {ID: "r1", CloudAccountID: &accountID}, - }, - } - mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) - mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { - return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil - } - mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ - NotificationEmail: &contactEmail, - }, nil) - - // Session is present but has no approve-* permissions → RBAC denies, - // falls through to the token branch. CSRF is NOT called on the token - // branch (approvePurchaseViaSession is never reached). - mockAuth := new(MockAuthService) - mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() - mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) - mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() - mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() - t.Cleanup(func() { mockAuth.AssertExpectations(t) }) - - mockPurchase := new(MockPurchaseManager) - mockPurchase.On("ApproveExecution", ctx, execID, "email-token", contactEmail).Return("", nil) - - handler := &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase} - - // Session header present (so tryGetSession succeeds) but no CSRF header. - // The session has no approve-* permission → falls to token branch. - // No ValidateCSRFToken mock is registered here, so if the code ever - // called ValidateCSRFToken, the test would panic with "unexpected call". - req := &events.LambdaFunctionURLRequest{ - Headers: map[string]string{"authorization": "Bearer sess-tok"}, - // Intentionally no x-csrf-token header. - } - result, err := handler.approvePurchase(ctx, req, execID, "email-token") - require.NoError(t, err) - assert.Equal(t, "completed", result.(map[string]string)["status"]) + newHandler := func(mockConfig *MockConfigStore, mockPurchase *MockPurchaseManager) *Handler { + accountID := "acct-1" + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: "email-token", + Status: "pending", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil).Maybe() + mockConfig.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + mockConfig.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + NotificationEmail: &contactEmail, + }, nil).Maybe() + + mockAuth := new(MockAuthService) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything).Return(nil, nil).Maybe() + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("missing CSRF token")).Maybe() + mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "csrf-ok").Return(nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "approve-own", "purchases").Return(false, nil).Maybe() + + return &Handler{config: mockConfig, auth: mockAuth, purchase: mockPurchase} + } + + t.Run("no CSRF header", func(t *testing.T) { + mockConfig, mockPurchase := new(MockConfigStore), new(MockPurchaseManager) + handler := newHandler(mockConfig, mockPurchase) + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + } + _, err := handler.approvePurchase(ctx, req, execID, "email-token") + require.ErrorIs(t, err, errCSRFRejected) + mockConfig.AssertNotCalled(t, "GetExecutionByID", mock.Anything, mock.Anything) + mockPurchase.AssertNotCalled(t, "ApproveExecution", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("valid CSRF header", func(t *testing.T) { + mockPurchase := new(MockPurchaseManager) + handler := newHandler(new(MockConfigStore), mockPurchase) + mockPurchase.On("ApproveExecution", ctx, execID, "email-token", contactEmail).Return("", nil) + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok", "x-csrf-token": "csrf-ok"}, + } + result, err := handler.approvePurchase(ctx, req, execID, "email-token") + require.NoError(t, err) + assert.Equal(t, "completed", result.(map[string]string)["status"]) + mockPurchase.AssertExpectations(t) + }) } // Regression test for #404: pins requiresCSRFValidation's own per-path diff --git a/internal/api/session_denial_final_test.go b/internal/api/session_denial_final_test.go index 0f619702f..8339641d5 100644 --- a/internal/api/session_denial_final_test.go +++ b/internal/api/session_denial_final_test.go @@ -73,8 +73,8 @@ func TestApprovePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { _, err := h.approvePurchase(ctx, denialTestRequest(), denialTestExecID, "") - requireForbidden(t, err) - assert.Contains(t, err.Error(), "requires approve-any or approve-own") + // The denial answers like a missing execution (issue #597). + requireConcealedDenial(t, err, "requires approve-any or approve-own") mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } @@ -88,8 +88,7 @@ func TestCancelPurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { _, err := h.cancelPurchase(ctx, denialTestRequest(), denialTestExecID, "") - requireForbidden(t, err) - assert.Contains(t, err.Error(), "requires cancel-any or cancel-own") + requireConcealedDenial(t, err, "requires cancel-any or cancel-own") store.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } @@ -103,8 +102,7 @@ func TestRevokePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { _, err := h.revokeViaEmailToken(ctx, denialTestRequest(), denialTestExecID, "") - requireForbidden(t, err) - assert.Contains(t, err.Error(), "requires cancel-any or cancel-own") + requireConcealedDenial(t, err, "requires cancel-any or cancel-own") } func TestApproveRIExchange_SessionDenialWithoutTokenIsFinal(t *testing.T) { diff --git a/internal/server/lambda_coverage_test.go b/internal/server/lambda_coverage_test.go index 4fbdb11d3..d1349c4e2 100644 --- a/internal/server/lambda_coverage_test.go +++ b/internal/server/lambda_coverage_test.go @@ -130,23 +130,30 @@ func TestHandleLambdaHTTPEvent_StaticPath(t *testing.T) { // // This drives the real failing scenario end-to-end: a base64-encoded, // form-urlencoded POST body hitting POST /api/purchases/revoke/{execID}. -// With a session that lacks cancel permission, revokeViaEmailToken answers 403 -// "permission denied" when the token fails to resolve, or falls through to the -// token branch and answers the DIFFERENT 403 "no per-account contact email" -// from authorizeApprovalAction once the token has been parsed out of the -// decoded body. Only the second message is reachable when the token resolves, -// so asserting it proves the fix. (A request with no session never reaches -// either: it gets 401 before the lookup, issue #435.) +// The session lacks cancel permission but is the account's contact email. +// When the token fails to resolve, revokeViaEmailToken answers the 404 a +// missing execution gets (issue #597). Once the token has been parsed out of +// the decoded body, it falls through to the token branch, passes the +// contact_email gate, and answers 403 "invalid revocation token" because the +// token does not match. Only the second answer is reachable when the token +// resolves, so asserting it proves the fix. (A request with no session never +// reaches either: it gets 401 before the lookup, issue #435.) func TestHandleLambdaHTTPEvent_DecodesBase64FormBody(t *testing.T) { execID := "11111111-1111-1111-1111-111111111111" + accountID := "acct-1" exec := &config.PurchaseExecution{ - ExecutionID: execID, - Status: "completed", + ExecutionID: execID, + Status: "completed", + ApprovalToken: config.HashApprovalToken("a-different-token"), + Recommendations: []config.RecommendationRecord{{ID: "r1", CloudAccountID: &accountID}}, } mockStore := new(mocks.MockConfigStore) mockStore.On("GetExecutionByID", mock.Anything, execID).Return(exec, nil) mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: "user@example.com"}, nil + } app := &Application{ API: api.NewHandler(api.HandlerConfig{ConfigStore: mockStore, AuthService: sessionOnlyAuth{}}), @@ -158,6 +165,7 @@ func TestHandleLambdaHTTPEvent_DecodesBase64FormBody(t *testing.T) { Headers: map[string]string{ "content-type": "application/x-www-form-urlencoded", "authorization": "Bearer sess-tok", + "x-csrf-token": "csrf-ok", }, RequestContext: events.LambdaFunctionURLRequestContext{ HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ @@ -175,8 +183,8 @@ func TestHandleLambdaHTTPEvent_DecodesBase64FormBody(t *testing.T) { resp, err := app.handleLambdaHTTPEvent(ctx, rawEvent) testutil.AssertNoError(t, err) testutil.AssertEqual(t, 403, resp.StatusCode) - testutil.AssertContains(t, resp.Body, "no per-account contact email") - testutil.AssertTrue(t, !strings.Contains(resp.Body, "requires cancel-any"), + testutil.AssertContains(t, resp.Body, "invalid revocation token") + testutil.AssertTrue(t, !strings.Contains(resp.Body, "execution not found"), "token must have been resolved from the decoded body, not left empty") mockStore.AssertExpectations(t) @@ -214,3 +222,12 @@ func (sessionOnlyAuth) HasPermissionAPI(context.Context, string, string, string) func (sessionOnlyAuth) GetAllowedAccountsAPI(context.Context, string) ([]string, error) { return nil, nil } + +// ValidateCSRFToken accepts "csrf-ok" for "sess-tok": the email-link routes +// validate CSRF before the execution lookup (issue #597). +func (sessionOnlyAuth) ValidateCSRFToken(_ context.Context, sessionToken, csrfToken string) error { + if sessionToken != "sess-tok" || csrfToken != "csrf-ok" { + return errors.New("invalid CSRF token") + } + return nil +}