From 7a4f99132ed2c632027a1a7bde02b2b9c3a33d83 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 17:57:47 +0200 Subject: [PATCH 1/2] fix(api): return session authz denials when no email token is present approvePurchase, cancelPurchase, revokeViaEmailToken and approveRIExchange handed any session 403 to the email-token branch, even with no token. With no token that branch re-entered the session path, which re-resolves the caller via requireSession (context principal first), so the outer denial was discarded and the outcome rested on the inner path resolving the same principal and re-running the same checks. fallsThroughToToken and the RI exchange pre-check now fall through only when a token is present; otherwise the denial is returned. The token contact_email flow for a logged-in user without approve-*/cancel-* is unchanged. The tokenless revoke path now returns the session's 403 rather than a 401 "sign in" prompt to a signed-in user. Existing tests that only passed through the fall-through are updated: the cancel status-guard tests now grant cancel-any, and the approve CSRF test uses an approver that passes RBAC so it still reaches the CSRF guard. Closes #173 --- internal/api/handler_purchases.go | 33 +++-- internal/api/handler_purchases_test.go | 12 +- internal/api/handler_ri_exchange.go | 6 +- internal/api/middleware_test.go | 14 ++- internal/api/session_denial_final_test.go | 140 ++++++++++++++++++++++ 5 files changed, 173 insertions(+), 32 deletions(-) create mode 100644 internal/api/session_denial_final_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 0a87277b..4f34b3cc 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -612,15 +612,14 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio // session. authorizeApprovalAction enforces the per-account // contact_email gate from PR #101; the purchase service // validates the token itself before mutating state. - // 3. token == "" → session-authed dashboard Approve button. - // approvePurchaseViaSession runs the approve-any / - // approve-own RBAC matrix and rejects sessions without it. + // 3. token == "" → a session denial above is final (issue #173); + // with no session, approvePurchaseViaSession returns 401. if session := h.tryGetSession(ctx, req); session != nil { switch err := h.authorizeScopedSessionApprove(ctx, session, execution); { case err == nil: return h.approvePurchaseViaSession(ctx, req, execution) case fallsThroughToToken(err, token): - // Explicit 403, or an out-of-scope 404 with a token → fall + // Explicit 403 or out-of-scope 404, with a token → fall // through to the token branch so the contact_email gate gets a // chance (a logged-in user without approve-* or outside the // account scope may still be the per-account contact recipient). @@ -1215,18 +1214,16 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // preserves the security model for forwarded email / shared // inbox / stolen link cases — a non-privileged session falls // through to this branch. - // 3. token == "" → session-authed dashboard Cancel button (issue - // #46). Same path as branch (1) but reached without a URL - // token. cancelPurchaseViaSession runs the cancel-any / - // cancel-own RBAC matrix and rejects sessions without it. + // 3. token == "" → a session denial above is final (issue #173); + // with no session, cancelPurchaseViaSession returns 401. if session := h.tryGetSession(ctx, req); session != nil { switch err := h.authorizeCancelSession(ctx, session, execution); { case err == nil: // Session is RBAC-authorized → run the session-authed cancel. return h.cancelPurchaseViaSession(ctx, req, execution) case fallsThroughToToken(err, token): - // Explicit "permission denied" (403), or an out-of-scope 404 - // with a token → fall through to the token branch so the + // Explicit "permission denied" (403) or out-of-scope 404, with + // a token → fall through to the token branch so the // contact_email gate still gets a chance (a logged-in user // without admin / cancel-* may still be the per-account contact // email recipient). @@ -2264,15 +2261,15 @@ func wrapConstraintDenied(err error) error { } // fallsThroughToToken reports whether a session-authorization failure should -// hand the request to the email-token branch: an RBAC denial (403), or an -// account-scope miss (404) when a token is present. The token branch's -// contact_email gate is deliberately not account-scoped (issue #92). Without -// a token the 404 is returned as-is, so the token branch's status guards -// cannot reveal an out-of-scope execution's existence or status. Both -// matches are strict, like isPermissionDenied, so a wrapped error from -// deeper in the chain propagates. +// hand the request to the email-token branch: an RBAC denial (403) or an +// account-scope miss (404), and only when a token is present. The token +// branch's contact_email gate is deliberately not account-scoped (issue #92). +// Without a token the denial is final (issue #173): nothing else can authorize +// the request, and the 404 must not let later status guards reveal an +// out-of-scope execution. Both matches are strict, like isPermissionDenied, +// so a wrapped error from deeper in the chain propagates. func fallsThroughToToken(err error, token string) bool { - return isPermissionDenied(err) || (token != "" && err == errNotFound) //nolint:errorlint // strict sentinel identity is deliberate + return token != "" && (isPermissionDenied(err) || err == errNotFound) //nolint:errorlint // strict sentinel identity is deliberate } // isPermissionDenied reports whether err is *directly* a 403 ClientError diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index fa4f6f00..49f3d4cf 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3245,7 +3245,7 @@ func TestHandler_cancelPurchase_Session_RejectsTerminalStatus(t *testing.T) { } session := &Session{UserID: cancelCallerID} - handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, true, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) @@ -3274,7 +3274,7 @@ func TestHandler_cancelPurchase_Session_RejectsEachNonCancelableStatus(t *testin } session := &Session{UserID: cancelCallerID} - handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, true, false) _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "") require.Error(t, err) @@ -5869,9 +5869,9 @@ func TestHandler_revokePurchase_SessionOwnerCancelOwn(t *testing.T) { } // TestHandler_revokePurchase_SessionNoPermissionNoToken verifies that a session -// lacking both cancel-any and cancel-own, with no token supplied, is denied: -// the session branch returns permission-denied and falls through to the -// tokenless 401. Covers the session-auth revoke branch CR finding (PR #889). +// lacking both cancel-any and cancel-own, with no token supplied, gets the +// session branch's 403: without a token the denial is final (issue #173). +// Covers the session-auth revoke branch CR finding (PR #889). func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { ctx := context.Background() execID := "88888888-8888-8888-8888-888888888888" @@ -5899,7 +5899,7 @@ func TestHandler_revokePurchase_SessionNoPermissionNoToken(t *testing.T) { 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, 401, ce.code) + assert.Equal(t, 403, ce.code) mockStore.AssertExpectations(t) mockAuth.AssertExpectations(t) } diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 2185af80..bb994faf 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -2166,8 +2166,8 @@ func (h *Handler) getRIExchangeHistory(ctx context.Context, req *events.LambdaFu // 2. token != "" -> legacy email-link flow. validateExchangeApproval enforces // the token-equality check; the permission-denied fall-through ensures a // logged-in user without approve-* can still use an email link they hold. -// 3. token == "" AND no qualifying session -> 403 via -// approveRIExchangeViaSession's requireSession gate. +// 3. token == "": a session denial is returned as-is (issue #173); with no +// session, approveRIExchangeViaSession's requireSession gate returns 401. func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunctionURLRequest, id, token string) (any, error) { if session := h.tryGetSession(ctx, req); session != nil { // Quick RBAC pre-check (no record fetch needed): does this session hold @@ -2193,7 +2193,7 @@ func (h *Handler) approveRIExchange(ctx context.Context, req *events.LambdaFunct if errors.Is(sessErr, errCSRFRejected) || token == "" || !isPermissionDenied(sessErr) { return nil, sessErr } - case isPermissionDenied(err): + case token != "" && isPermissionDenied(err): // Logged-in user without approve-* may still hold a valid email token. default: return nil, err diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index 2273a0b1..8d816da8 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -306,10 +306,14 @@ func TestApproveViaSession_RequiresCSRF(t *testing.T) { mockConfig := new(MockConfigStore) exec := &config.PurchaseExecution{ - ExecutionID: execID, - ApprovalToken: "email-tok", - Status: "pending", - Recommendations: []config.RecommendationRecord{{ID: "r1"}}, + ExecutionID: execID, + ApprovalToken: "email-tok", + Status: "pending", + // Non-zero UpfrontCost so the approve-any Constraints check passes and + // the dispatcher hands off to approvePurchaseViaSession (issue #173). + Recommendations: []config.RecommendationRecord{ + {ID: "r1", Provider: "aws", Service: "ec2", Region: "us-east-1", UpfrontCost: 100}, + }, } mockConfig.On("GetExecutionByID", ctx, execID).Return(exec, nil) @@ -319,7 +323,7 @@ func TestApproveViaSession_RequiresCSRF(t *testing.T) { // Authorization is group-membership-only after issue #907: the session must // pass the approve-* HasPermissionAPI check to reach the CSRF guard, since // the dispatcher authorizes before invoking approvePurchaseViaSession. - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // CSRF token is empty → ValidateCSRFToken must return an error so the // request is rejected. This is the critical regression assertion for #404. mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(errors.New("csrf mismatch")) diff --git a/internal/api/session_denial_final_test.go b/internal/api/session_denial_final_test.go new file mode 100644 index 00000000..4e2286e8 --- /dev/null +++ b/internal/api/session_denial_final_test.go @@ -0,0 +1,140 @@ +package api + +import ( + "context" + "errors" + "testing" + + "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" +) + +// Issue #173: without a token a session denial is final. The context principal is an authorized +// admin distinct from the denied bearer session, so a fall-through reaches the mutation. + +const ( + denialTestExecID = "17317317-3173-1731-7317-317317317317" + denialTestUserID = "denied-uid" +) + +var errDenialTestReached = errors.New("mutation reached") + +func newDenialTestHandler(t *testing.T) (*Handler, *MockConfigStore, *MockPurchaseManager, context.Context) { + t.Helper() + store := new(MockConfigStore) + mockAuth := new(MockAuthService) + mockPurchase := new(MockPurchaseManager) + + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{UserID: denialTestUserID, Email: "denied@example.com"}, nil) + mockAuth.On("HasPermissionAPI", mock.Anything, denialTestUserID, mock.Anything, mock.Anything).Return(false, nil) + mockAuth.On("GetAllowedAccountsAPI", mock.Anything, denialTestUserID).Return(nil, nil).Maybe() + mockAuth.On("ValidateCSRFToken", mock.Anything, "sess-tok", "csrf-ok").Return(nil).Maybe() + + store.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil).Maybe() + store.On("CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(false, "", errDenialTestReached).Maybe() + store.On("TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errDenialTestReached).Maybe() + mockPurchase.On("ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return("", errDenialTestReached).Maybe() + + ctx := contextWithPrincipal(context.Background(), &Principal{ + Kind: PrincipalSession, + Session: &Session{UserID: apiKeyAdminUserID, Email: "admin@example.com"}, + UserID: apiKeyAdminUserID, + }) + return &Handler{auth: mockAuth, config: store, purchase: mockPurchase}, store, mockPurchase, ctx +} + +func denialTestRequest() *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok", "x-csrf-token": "csrf-ok"}, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: "POST"}, + }, + } +} + +func requireForbidden(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a ClientError, got: %v", err) + assert.Equal(t, 403, ce.code, "got: %v", err) +} + +func TestApprovePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { + h, store, mockPurchase, ctx := newDenialTestHandler(t) + store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: denialTestExecID, + Status: "pending", + Recommendations: []config.RecommendationRecord{{ID: "r1", Provider: "aws", UpfrontCost: 100}}, + }, nil) + + _, err := h.approvePurchase(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires approve-any or approve-own") + mockPurchase.AssertNotCalled(t, "ApproveAndExecute", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestCancelPurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { + h, store, _, ctx := newDenialTestHandler(t) + store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: denialTestExecID, + Status: "pending", + }, nil) + + _, err := h.cancelPurchase(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires cancel-any or cancel-own") + store.AssertNotCalled(t, "CancelExecutionAtomic", mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestRevokePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { + h, store, _, ctx := newDenialTestHandler(t) + store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ + ExecutionID: denialTestExecID, + Status: "completed", + }, nil) + + _, err := h.revokeViaEmailToken(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires cancel-any or cancel-own") +} + +func TestApproveRIExchange_SessionDenialWithoutTokenIsFinal(t *testing.T) { + h, store, _, ctx := newDenialTestHandler(t) + store.On("GetRIExchangeRecord", mock.Anything, denialTestExecID).Return(&config.RIExchangeRecord{ + ID: denialTestExecID, + Status: "pending", + ApprovalToken: config.HashApprovalToken("tok"), + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "10.00", + }, nil).Maybe() + + _, err := h.approveRIExchange(ctx, denialTestRequest(), denialTestExecID, "") + + requireForbidden(t, err) + assert.Contains(t, err.Error(), "requires approve-any or approve-own") + store.AssertNotCalled(t, "TransitionRIExchangeStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestApproveRIExchange_SessionDenialWithTokenUsesToken(t *testing.T) { + h, store, _, ctx := newDenialTestHandler(t) + store.On("GetRIExchangeRecord", mock.Anything, denialTestExecID).Return(&config.RIExchangeRecord{ + ID: denialTestExecID, + Status: "pending", + ApprovalToken: config.HashApprovalToken("tok"), + SourceRIIDs: []string{"ri-1"}, + PaymentDue: "10.00", + }, nil) + + _, err := h.approveRIExchange(ctx, denialTestRequest(), denialTestExecID, "tok") + + require.ErrorIs(t, err, errDenialTestReached) + store.AssertCalled(t, "TransitionRIExchangeStatus", mock.Anything, denialTestExecID, "pending", "processing", (*string)(nil)) +} From 9a6038c02f3b976448de600e9e134faea0d0d42e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 5 Oct 2026 18:07:15 +0200 Subject: [PATCH 2/2] test(api): declare denial-test mocks at the assertion site The vacuous-assertion guard in internal/mocks resolves a mock receiver's type from its declaration and cannot see through a multi-value helper return, so it flagged the ApproveAndExecute assertion as unreviewable. The helper now takes the mocks as parameters. Refs #173 --- internal/api/session_denial_final_test.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/internal/api/session_denial_final_test.go b/internal/api/session_denial_final_test.go index 4e2286e8..ec2f09cf 100644 --- a/internal/api/session_denial_final_test.go +++ b/internal/api/session_denial_final_test.go @@ -23,11 +23,9 @@ const ( var errDenialTestReached = errors.New("mutation reached") -func newDenialTestHandler(t *testing.T) (*Handler, *MockConfigStore, *MockPurchaseManager, context.Context) { +func newDenialTestHandler(t *testing.T, store *MockConfigStore, mockPurchase *MockPurchaseManager) (*Handler, context.Context) { t.Helper() - store := new(MockConfigStore) mockAuth := new(MockAuthService) - mockPurchase := new(MockPurchaseManager) mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{UserID: denialTestUserID, Email: "denied@example.com"}, nil) mockAuth.On("HasPermissionAPI", mock.Anything, denialTestUserID, mock.Anything, mock.Anything).Return(false, nil) @@ -44,7 +42,7 @@ func newDenialTestHandler(t *testing.T) (*Handler, *MockConfigStore, *MockPurcha Session: &Session{UserID: apiKeyAdminUserID, Email: "admin@example.com"}, UserID: apiKeyAdminUserID, }) - return &Handler{auth: mockAuth, config: store, purchase: mockPurchase}, store, mockPurchase, ctx + return &Handler{auth: mockAuth, config: store, purchase: mockPurchase}, ctx } func denialTestRequest() *events.LambdaFunctionURLRequest { @@ -65,7 +63,8 @@ func requireForbidden(t *testing.T, err error) { } func TestApprovePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { - h, store, mockPurchase, ctx := newDenialTestHandler(t) + store, mockPurchase := new(MockConfigStore), new(MockPurchaseManager) + h, ctx := newDenialTestHandler(t, store, mockPurchase) store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ ExecutionID: denialTestExecID, Status: "pending", @@ -80,7 +79,8 @@ func TestApprovePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { } func TestCancelPurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { - h, store, _, ctx := newDenialTestHandler(t) + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ ExecutionID: denialTestExecID, Status: "pending", @@ -94,7 +94,8 @@ func TestCancelPurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { } func TestRevokePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { - h, store, _, ctx := newDenialTestHandler(t) + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) store.On("GetExecutionByID", mock.Anything, denialTestExecID).Return(&config.PurchaseExecution{ ExecutionID: denialTestExecID, Status: "completed", @@ -107,7 +108,8 @@ func TestRevokePurchase_SessionDenialWithoutTokenIsFinal(t *testing.T) { } func TestApproveRIExchange_SessionDenialWithoutTokenIsFinal(t *testing.T) { - h, store, _, ctx := newDenialTestHandler(t) + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) store.On("GetRIExchangeRecord", mock.Anything, denialTestExecID).Return(&config.RIExchangeRecord{ ID: denialTestExecID, Status: "pending", @@ -124,7 +126,8 @@ func TestApproveRIExchange_SessionDenialWithoutTokenIsFinal(t *testing.T) { } func TestApproveRIExchange_SessionDenialWithTokenUsesToken(t *testing.T) { - h, store, _, ctx := newDenialTestHandler(t) + store := new(MockConfigStore) + h, ctx := newDenialTestHandler(t, store, new(MockPurchaseManager)) store.On("GetRIExchangeRecord", mock.Anything, denialTestExecID).Return(&config.RIExchangeRecord{ ID: denialTestExecID, Status: "pending",