fix(purchases): admin / cancel-* sessions bypass contact_email gate on email-link cancel#216
Conversation
…n email-link cancel 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHandler cancel flow changed to prefer session-authorized cancellation: session pre-check → session cancel on allowed, fall through to legacy token/email-link when pre-check unwrapped 403, and propagate other auth errors. Tests expanded to cover these session vs token branch behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Handler
participant Auth as AuthService
participant TokenSvc as Token/EmailGate
participant Purchase as PurchaseService
participant Config as ConfigService
Client->>Handler: POST /cancel (token?, session?)
Handler->>Handler: tryGetSession()
alt Session found
Handler->>Auth: authorizeSessionCancel(session, purchase)
alt Auth allows (non-403)
Handler->>Purchase: cancelPurchaseViaSession(actorEmail)
Purchase-->>Handler: success
Handler-->>Client: 200 OK (cancelled)
else Auth returns 403 (permission denied)
Handler->>TokenSvc: validateTokenOrContactEmail(token, purchase)
alt Token/Contact allowed
Handler->>Purchase: CancelExecution(via token)
Purchase-->>Handler: success
Handler-->>Client: 200 OK
else contact-email missing/error
Handler-->>Client: 400/422 contact-email missing
end
else Auth returns non-403 error
Auth-->>Handler: error
Handler-->>Client: propagate error
end
else No session
Handler->>TokenSvc: validateTokenOrContactEmail(token, purchase)
TokenSvc-->>Handler: allowed or contact-email error
Handler->>Purchase: CancelExecution(via token/sessionless)
Purchase-->>Handler: result
Handler-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 1/5 review remaining, refill in 43 minutes and 28 seconds. Comment |
cristim
added
type/bug
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 333-337: The current session-path silently falls through on any
error from authorizeSessionCancel and treats it like a token-fallback; change it
so only explicit “permission denied” errors trigger the fallback to
cancelPurchaseViaSession. In the block that calls
tryGetSession/authorizeSessionCancel, if authorizeSessionCancel returns nil call
cancelPurchaseViaSession as before; if it returns an error, detect whether that
error is an explicit authorization denial (e.g., error value/type or status code
== 403 using the same permission-error type or helper used elsewhere in the
codebase); if it is a denial then proceed with the existing fallback to
cancelPurchaseViaSession, otherwise immediately return the
authorizeSessionCancel error to the caller. Ensure you reference and use the
same permission error sentinel/helper used by authorizeSessionCancel to identify
a 403 denial.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 356c33ec-3379-4a2e-b95a-8accbb202c83
📒 Files selected for processing (4)
internal/api/coverage_extras_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/handler_test.go
…lback (CR #216) 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.
|
Addressed @coderabbitai feedback in daf13d9: switched the session-authed pre-check from |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/handler_purchases.go`:
- Around line 807-810: isPermissionDenied currently uses IsClientError (which
unwraps via errors.As) and thus treats wrapped 403 errors as permission denials;
change isPermissionDenied to detect an unwrapped client 403 only by doing a
direct type assertion on the incoming error (not using IsClientError/errors.As)
or otherwise verifying the error is the exact client error value (no wrapping),
so that wrapped 403s from an auth-backend surface as real backend errors instead
of being treated as authorization denials; update references to IsClientError
inside isPermissionDenied accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa96fa5d-b021-4e79-9e4d-b801e20233ec
📒 Files selected for processing (2)
internal/api/handler_purchases.gointernal/api/handler_purchases_test.go
…s 2 #216) 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.
|
Addressed CR pass 2 in 979437c: switched |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
1 participant
Symptom
Clicking the Cancel purchase link in a notification email surfaces:
…even when the user is an admin and could already cancel the same execution from the History page Cancel button.
Root cause
cancelPurchase(handler_purchases.go) takes the token branch unconditionally when a URL token is present:The deep-link cancel flow (
frontend/src/purchases-deeplink.ts) always appends?token=…, so an authenticated admin clicking the email link hits the token branch even though their session already establishes their identity.authorizeApprovalActionthen 403s when the execution's recs have no per-accountcontact_emailavailable — most commonly:CloudAccountID == nilon every rec).contact_emailempty / not yet configured.The same admin can already cancel from the History page button (
cancelPurchaseViaSession→authorizeSessionCancelRBAC), so blocking them in the email-link flow is gratuitous.Fix
Pre-check the session in
cancelPurchasebefore falling into the token branch:Tokenless callers (no session) still hit the per-account contact_email gate from PR #101 — the fix is strictly additive and only widens for sessions that pass the existing RBAC matrix (
admin/cancel-any:purchases/cancel-own:purchasesmatchingcreated_by_user_id).A small helper
tryGetSessionis extracted fromtryResolveActorEmail(which collapses to a one-line wrapper).Out of scope (deliberate)
contact_emaileven though the global notification email no longer participates in approval post-fix(security): approval-flow hardening — crypto/rand tokens + per-account contact gating #101). Worth tightening in a follow-up.Test plan
Three new regression tests:
TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate— admin session + token + ambient-credentials execution → 200, status flips,CancelledBystamped. AssertsGetGlobalConfig(the token branch's signature call) is NOT consulted.TestHandler_cancelPurchase_DeepLink_CancelOwnBypassesContactEmailGate— non-admin withcancel-own+ matching creator → 200.TestHandler_cancelPurchase_DeepLink_NonPrivilegedSessionStillHitsContactGate— pins the security invariant: a logged-in user without admin / cancel-* MUST still go through the contact_email gate.Existing tests with sessions but no permission stubs (TestHandler_cancelPurchase, TestHandler_cancelPurchase_PurchaseError, TestHandler_HandleRequest_CancelPurchase) get
HasPermissionAPIstubs returningfalseso the new pre-check falls through cleanly to the token branch they were always exercising.go build ./...clean.go test ./...— every package green.Triage
type/bug,severity/high(production deep-link cancel locked out for admins),urgency/now,impact/many,effort/s,priority/p1,triaged.Summary by CodeRabbit
Tests
Refactor