From a1657b671b9fe2ab59398dac737e9841f62fea61 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 17:23:14 +0200 Subject: [PATCH 1/3] fix(purchases): admin / cancel-* sessions bypass contact_email gate on email-link cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deep-link cancel flow (frontend purchases-deeplink.ts) always POSTs /api/purchases/cancel/:id with both an X-Authorization session AND the URL token from the email link. cancelPurchase took the token branch unconditionally → authorizeApprovalAction → 403 when the execution had no per-account contact_email available (e.g. AWS ambient-credentials recs where CloudAccountID is nil, or any execution whose recommendation accounts simply have an empty contact_email column): Failed to cancel purchase: no per-account contact email configured for this execution; set the cloud account's contact_email before approving The same admin (or any user with cancel-any:purchases / cancel-own matching the creator) could already cancel the same execution from the History page Cancel button — that path goes through cancelPurchaseViaSession → authorizeSessionCancel (RBAC matrix) and never touches contact_email. The deep-link UX was inconsistent. Fix: pre-check the session in cancelPurchase before falling into the token branch. When the caller carries a valid session AND authorizeSessionCancel approves them, take the session-authed path regardless of whether a token is in the URL. Tokenless / no-session callers (forwarded email, shared inbox, scripted flow without auth) still hit the per-account contact_email gate from PR #101. Approve flow stays strict — the dashboard has no admin approve override either, so widening it via the email link would change the security policy. Out of scope for this fix. Helpers: - New tryGetSession returns *Session or nil silently. tryResolveActorEmail collapses to a one-line wrapper. Tests: - New TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate: admin session + token + ambient-credentials execution → 200, status flips, CancelledBy stamped, GetGlobalConfig (the token branch's signature call) is asserted NOT called. - New TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate: non-admin with cancel-own + matching creator → 200. - New TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate: pins the security model — a logged-in user without admin / cancel-* MUST still go through authorizeApprovalAction. - Existing TestHandler_cancelPurchase, TestHandler_cancelPurchase_PurchaseError, and TestHandler_HandleRequest_CancelPurchase get HasPermissionAPI mock stubs returning false so the new pre-check correctly falls through to the token branch (preserving their original assertions). go test ./... — every package green. --- internal/api/coverage_extras_test.go | 6 ++ internal/api/handler_purchases.go | 65 +++++++++---- internal/api/handler_purchases_test.go | 130 +++++++++++++++++++++++++ internal/api/handler_test.go | 6 ++ 4 files changed, 190 insertions(+), 17 deletions(-) diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index 70c932e3..81c8b0a8 100644 --- a/internal/api/coverage_extras_test.go +++ b/internal/api/coverage_extras_test.go @@ -121,6 +121,12 @@ func TestHandler_cancelPurchase_PurchaseError(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, 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 + // "cancel failed" assertion exercises. + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) mockPurchase.On("CancelExecution", ctx, execID, "tok", approver). diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 47af2e60..2c5d7236 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -304,19 +304,38 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction return nil, NewClientError(404, "execution not found") } - // Two-mode dispatch (issue #46): - // * token != "" → legacy email-link flow. Token possession proves - // intent; the handler still requires the session email (when one - // exists) to be on the authorised-approver list, then delegates - // to the purchase service which validates the token itself. - // * token == "" → session-authed dashboard Cancel button. The - // session must carry a permission allowing the cancel: - // - cancel-any:purchases (admin) → any pending execution; or - // - cancel-own:purchases (default user) AND the execution's - // created_by_user_id matches the session UserID. - // Legacy rows with NULL created_by_user_id are reachable only - // via cancel-any (admin) or via the email token already in the - // inbox — they do not become orphaned by this change. + // Three-mode dispatch: + // 1. Session present AND RBAC-authorized (admin / cancel-any / + // cancel-own match) → session-authed cancel, regardless of + // whether a token is in the URL. Restores parity with the + // History page Cancel button: a user who can cancel from the + // dashboard should also be able to cancel from the email-link + // flow, since the same email-link flow already requires a + // logged-in session before reaching this endpoint (see + // frontend purchases-deeplink.ts). Without this branch, an + // admin clicking Cancel from a notification email about an + // ambient-credentials execution (CloudAccountID == nil → no + // per-account contact_email available) is locked out of an + // action they can perform from the dashboard. The token in + // the URL is informational at that point — the session has + // already authenticated the actor. + // 2. token != "" → legacy email-link flow without a qualifying + // session. authorizeApprovalAction enforces the per-account + // contact_email gate from PR #101; the purchase service + // validates the token itself before mutating state. This + // 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. + if session := h.tryGetSession(ctx, req); session != nil { + if err := h.authorizeSessionCancel(ctx, session, execution); err == nil { + return h.cancelPurchaseViaSession(ctx, req, execution) + } + } + if token != "" { actor, err := h.authorizeApprovalAction(ctx, req, execution) if err != nil { @@ -759,18 +778,30 @@ func (h *Handler) authorizeSessionRetry(ctx context.Context, session *Session, e // → session-authed call with ?token=…) can record per-user attribution // without changing the token-only fallback path used by message workers. func (h *Handler) tryResolveActorEmail(ctx context.Context, req *events.LambdaFunctionURLRequest) string { + if s := h.tryGetSession(ctx, req); s != nil { + return s.Email + } + return "" +} + +// tryGetSession returns the validated session for the request, or nil when +// the request carries no Bearer token, the auth service isn't configured, +// or session validation fails. Mirrors tryResolveActorEmail's silent +// best-effort semantics so AuthPublic callers can opt into session-aware +// behaviour without forcing a 401 on tokenless flows. +func (h *Handler) tryGetSession(ctx context.Context, req *events.LambdaFunctionURLRequest) *Session { if req == nil || h.auth == nil { - return "" + return nil } bearer := h.extractBearerToken(req) if bearer == "" { - return "" + return nil } session, err := h.auth.ValidateSession(ctx, bearer) if err != nil || session == nil { - return "" + return nil } - return session.Email + return session } // buildPurchaseDetailsResponse builds the response map for a purchase execution. diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 096bd98c..19213eeb 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -80,6 +80,12 @@ func TestHandler_cancelPurchase(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: approver}, 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 + // fixture above is what proves the legacy email-link path still works. + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) mockPurchase.On("CancelExecution", ctx, execID, "valid-token", approver).Return(nil) @@ -1418,6 +1424,130 @@ func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) { assert.Contains(t, err.Error(), "no authorization token provided") } +// TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate is the +// regression for the cancel-from-email contact_email-gate UX bug: +// +// "Failed to cancel purchase: no per-account contact email configured for +// this execution; set the cloud account's contact_email before approving" +// +// The deep-link cancel flow (frontend purchases-deeplink.ts) always POSTs +// /api/purchases/cancel/:id with both an X-Authorization Bearer session AND +// the email-link's URL token. Before this fix, cancelPurchase always took +// the token branch → authorizeApprovalAction → 403 when the execution's +// recs had no per-account contact_email (e.g. AWS ambient-credentials +// rows where CloudAccountID is nil). The same admin could already cancel +// the same execution from the History page Cancel button (session-authed, +// no contact_email gate); the email-link path was the only place they +// were locked out. +// +// Fix: when the caller has a valid session AND authorizeSessionCancel +// passes (admin / cancel-any / cancel-own match), use the session-authed +// cancel path, regardless of whether a token is present. Token-only +// callers (no session, e.g. forwarded email or shared inbox) still hit +// the contact_email gate from PR #101. +func TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate(t *testing.T) { + // Ambient-credentials execution: CloudAccountID is nil, so + // gatherAccountContactEmails returns []. Pre-fix this would 403 even + // for admin sessions. Post-fix the session-authed pre-check fires and + // the cancel succeeds. + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: nil, // no creator binding either; admin can still cancel + Recommendations: []config.RecommendationRecord{ + {ID: "r-ambient", CloudAccountID: nil}, + }, + } + session := &Session{UserID: cancelCallerID, Role: "admin", Email: "admin@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false, false) + var saved *config.PurchaseExecution + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")). + Run(func(args mock.Arguments) { + saved = args.Get(1).(*config.PurchaseExecution) + }). + Return(nil) + + // Token IS present in the URL — the deep-link flow always sends one. + // The fix's whole point is that the admin session takes the + // session-authed branch instead of routing through the token path. + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.NoError(t, err, "admin clicking Cancel from notification email must succeed even when no contact_email is configured") + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + + require.NotNil(t, saved, "session-authed branch must commit the status flip") + assert.Equal(t, "cancelled", saved.Status) + require.NotNil(t, saved.CancelledBy, "session-authed branch must stamp CancelledBy") + assert.Equal(t, session.Email, *saved.CancelledBy) + + // Critical security assertion: the token branch's contact_email gate + // (authorizeApprovalAction → GetGlobalConfig → resolveApprovalRecipients) + // was NOT consulted. If a regression re-routed admins through the + // token path, GetGlobalConfig would fire because the gate fetches + // the global notification email; asserting it didn't is the cleanest + // way to pin the new branch behaviour. + mockConfig.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockAuth.AssertExpectations(t) +} + +// TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate is +// the cancel-own variant of the regression above: a non-admin user with +// cancel-own on a purchase they themselves created should be able to +// cancel it from the email link, bypassing the contact_email gate that +// would otherwise lock them out for ambient-credentials executions. +func TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate(t *testing.T) { + creator := cancelCallerID // session user is the creator + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{ + {ID: "r-ambient", CloudAccountID: nil}, + }, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false /*hasAny*/, true /*hasOwn*/) + mockConfig.On("SavePurchaseExecution", mock.Anything, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) + + result, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.NoError(t, err) + assert.Equal(t, "cancelled", result.(map[string]string)["status"]) + mockConfig.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockAuth.AssertExpectations(t) +} + +// TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate +// pins the security-model invariant from PR #101: a logged-in user +// without admin / cancel-any / cancel-own permission MUST still go +// through authorizeApprovalAction and hit the contact_email gate when +// clicking a forwarded cancel link. The session-authed pre-check is +// strictly additive — it widens for privileged users only. +func TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate(t *testing.T) { + creator := cancelOtherID // someone else created it; no cancel-own match + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + ApprovalToken: "deep-link-token", + Status: "notified", + CreatedByUserID: &creator, + Recommendations: []config.RecommendationRecord{ + {ID: "r-ambient", CloudAccountID: nil}, + }, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + handler, mockConfig, mockAuth := buildSessionCancelHandler(exec, session, false /*hasAny*/, false /*hasOwn*/) + // Token branch fetches the global config to populate the Cc list. + 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") + mockConfig.AssertNotCalled(t, "SavePurchaseExecution") + mockAuth.AssertExpectations(t) +} + // ─── Session-authed Retry (issue #47) ────────────────────────────────────── // // Mirror image of the cancel matrix above. retryPurchase creates a NEW diff --git a/internal/api/handler_test.go b/internal/api/handler_test.go index 7c78240a..658f753d 100644 --- a/internal/api/handler_test.go +++ b/internal/api/handler_test.go @@ -764,6 +764,12 @@ func TestHandler_HandleRequest_CancelPurchase(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: approver}, nil) + // Session has no admin role / cancel permissions → cancelPurchase's + // session-authed pre-check falls through to authorizeApprovalAction → + // MockPurchaseManager.CancelExecution, exercising the legacy + // email-link path that this end-to-end test was always covering. + mockAuth.On("HasPermissionAPI", mock.Anything, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", mock.Anything, "", "cancel-own", "purchases").Return(false, nil).Maybe() mockPurchase := new(MockPurchaseManager) mockPurchase.On("CancelExecution", mock.Anything, execID, "token456", approver).Return(nil) From daf13d9b7d6ce688a99c23c62549cad2fd629d48 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 17:32:55 +0200 Subject: [PATCH 2/3] fix(purchases): propagate transient auth errors instead of silent fallback (CR #216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit on PR #216 flagged that the new session-authed pre-check in cancelPurchase silently swallowed every error from authorizeSessionCancel and treated them all as "fall through to the contact_email gate". That masks transient auth-service failures (HasPermissionAPI returning a wrapped error, h.auth nil, network blip) behind a misleading "set the contact_email" 403 — exactly the user-facing message #216 was meant to stop conflating with real failures. Distinguish the two cases via a new local helper isPermissionDenied, which uses the existing IsClientError + .code accessor to detect a 403 ClientError specifically. authorizeSessionCancel returns 403 ClientError on legitimate denials (admin role mismatch, missing cancel-* verb, cancel-own creator mismatch) and a wrapped non-ClientError on transient backend failures. case err == nil: → session-authed cancel case isPermissionDenied(): → fall through to contact_email gate default: → return err (propagate) Regression test TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates simulates a HasPermissionAPI failure ("auth backend timeout") and asserts: * the error surfaces with "permission check failed" wrapped, * the contact_email message is NOT in the error, * GetGlobalConfig (the token branch's signature call) is NOT reached. Existing 403-fall-through path covered by TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate. --- internal/api/handler_purchases.go | 27 ++++++++++++++++- internal/api/handler_purchases_test.go | 42 ++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 2c5d7236..06215020 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -331,8 +331,22 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction // token. cancelPurchaseViaSession runs the cancel-any / // cancel-own RBAC matrix and rejects sessions without it. if session := h.tryGetSession(ctx, req); session != nil { - if err := h.authorizeSessionCancel(ctx, session, execution); err == nil { + switch err := h.authorizeSessionCancel(ctx, session, execution); { + case err == nil: + // Session is RBAC-authorized → run the session-authed cancel. return h.cancelPurchaseViaSession(ctx, req, execution) + case isPermissionDenied(err): + // Explicit "permission denied" (403) → 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). + default: + // Transient failure (auth-service down, HasPermissionAPI + // 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 } } @@ -784,6 +798,17 @@ func (h *Handler) tryResolveActorEmail(ctx context.Context, req *events.LambdaFu return "" } +// isPermissionDenied reports whether err is a ClientError with HTTP status +// 403. The cancel-from-email session pre-check uses this to distinguish a +// legitimate "your session lacks cancel-* permission" answer (fall through +// to the token branch's contact_email gate) from a transient auth-service +// failure (propagate so the caller sees the real cause). CR feedback on +// PR #216. +func isPermissionDenied(err error) bool { + ce, ok := IsClientError(err) + return ok && ce.code == 403 +} + // tryGetSession returns the validated session for the request, or nil when // the request carries no Bearer token, the auth service isn't configured, // or session validation fails. Mirrors tryResolveActorEmail's silent diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 19213eeb..8a853483 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1517,6 +1517,48 @@ func TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate(t *te mockAuth.AssertExpectations(t) } +// TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates pins the +// CR-feedback hardening on PR #216: when authorizeSessionCancel returns a +// non-403 error (auth service down, HasPermissionAPI wrapped error, +// h.auth nil), the pre-check MUST surface it instead of silently falling +// through to the contact_email gate. A stale auth backend should not +// disguise itself as a "set the contact_email" 403, which would mislead +// operators investigating the failure. +func TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates(t *testing.T) { + creator := cancelOtherID + exec := &config.PurchaseExecution{ + ExecutionID: cancelExecID, + Status: "notified", + CreatedByUserID: &creator, + } + session := &Session{UserID: cancelCallerID, Role: "user", Email: "u1@example.com"} + + mockConfig := new(MockConfigStore) + mockConfig.On("GetExecutionByID", mock.Anything, exec.ExecutionID).Return(exec, nil) + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(session, 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. + mockAuth.On("HasPermissionAPI", mock.Anything, session.UserID, "cancel-any", "purchases"). + Return(false, errors.New("auth backend timeout")) + + handler := &Handler{config: mockConfig, auth: mockAuth} + + _, err := handler.cancelPurchase(context.Background(), sessionCancelReq(), cancelExecID, "deep-link-token") + require.Error(t, err) + assert.Contains(t, err.Error(), "permission check failed", + "transient auth-service errors must surface, not silently route to the contact_email gate") + assert.NotContains(t, err.Error(), "contact email", + "the contact_email message would mislead operators about the actual failure cause") + + // Token branch must NOT have been reached — GetGlobalConfig is the + // signature first call inside authorizeApprovalAction. + mockConfig.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) + mockAuth.AssertExpectations(t) +} + // TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate // pins the security-model invariant from PR #101: a logged-in user // without admin / cancel-any / cancel-own permission MUST still go From 979437c3bbbc0ac8db620df0f01e9ee68b582347 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 30 Apr 2026 17:42:47 +0200 Subject: [PATCH 3/3] fix(purchases): isPermissionDenied uses strict type assertion (CR pass 2 #216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit pass 2 on PR #216 flagged that the helper introduced in pass 1 relied on IsClientError, which goes through errors.As — that unwraps the error chain. A wrapped 403 ClientError (e.g. fmt.Errorf("permission check failed: %w", NewClientError(403, ...))) would still be classified as permission denied, even though the wrapper changes the failure's outer category and signals a different intent. Switch to a direct *clientError type assertion (no unwrapping). Only an exact, un-wrapped 403 ClientError now triggers the fall-through to the contact_email gate; anything wrapping a 403 propagates as the wrapper's own failure mode. This preserves the propagate-vs-fall-through split's original intent against future code that might decide to wrap auth-layer errors for context. Regression: TestIsPermissionDenied table-driven tests pin all six cases including the new "wrapped 403 is NOT denial" invariant. --- internal/api/handler_purchases.go | 22 ++++++++++++------- internal/api/handler_purchases_test.go | 29 ++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index 06215020..6709c91d 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -798,14 +798,22 @@ func (h *Handler) tryResolveActorEmail(ctx context.Context, req *events.LambdaFu return "" } -// isPermissionDenied reports whether err is a ClientError with HTTP status -// 403. The cancel-from-email session pre-check uses this to distinguish a -// legitimate "your session lacks cancel-* permission" answer (fall through -// to the token branch's contact_email gate) from a transient auth-service -// failure (propagate so the caller sees the real cause). CR feedback on -// PR #216. +// isPermissionDenied reports whether err is *directly* a 403 ClientError +// (not merely something that wraps one). Used by the cancel-from-email +// session pre-check to distinguish a legitimate "your session lacks +// cancel-* permission" answer (fall through to the token branch's +// contact_email gate) from a transient auth-service failure (propagate so +// the caller sees the real cause). CR feedback on PR #216. +// +// Strict (un-wrapped) type assertion is deliberate: if a future caller +// wraps a 403 to add context (fmt.Errorf("permission check failed: %w", +// ...)), that wrapped error represents the *outer* failure mode (the +// wrapper's intent), not "this is still a 403". errors.As-style unwrapping +// would erase that distinction and silently route wrapped backend +// failures into the contact_email gate — exactly the misclassification +// the propagate-vs-fall-through split is meant to prevent. func isPermissionDenied(err error) bool { - ce, ok := IsClientError(err) + ce, ok := err.(*clientError) return ok && ce.code == 403 } diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 8a853483..fdeec1a7 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -1517,6 +1517,35 @@ func TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate(t *te mockAuth.AssertExpectations(t) } +// TestIsPermissionDenied pins the strict (un-wrapped) type-assertion +// invariant from CR pass 2 on PR #216: a wrapped 403 ClientError must +// NOT count as permission-denied because the wrapper changes the +// failure's outer category. Only an *exact* *clientError with code 403 +// triggers the fall-through to the contact_email gate. +func TestIsPermissionDenied(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil error is not denial", nil, false}, + {"plain 403 ClientError is denial", NewClientError(403, "permission denied"), true}, + {"500 ClientError is not denial", NewClientError(500, "auth service down"), false}, + {"401 ClientError is not denial", NewClientError(401, "no session"), false}, + {"non-ClientError is not denial", errors.New("auth backend timeout"), false}, + { + name: "wrapped 403 is NOT denial (errors.As-style unwrap is rejected)", + err: fmt.Errorf("permission check failed: %w", NewClientError(403, "denied")), + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isPermissionDenied(tc.err)) + }) + } +} + // TestHandler_cancelPurchase_DeepLink_TransientAuthErrorPropagates pins the // CR-feedback hardening on PR #216: when authorizeSessionCancel returns a // non-403 error (auth service down, HasPermissionAPI wrapped error,