feat(email): post-execution notification with one-click revoke (closes #291) - #889
Merged
Conversation
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds post-execution purchase notifications (emails with optional one‑click revoke link), a token/session revocation handler and public routes, extends the email sender contract and templates, integrates best-effort sends into approval flows, and updates tests/mocks accordingly. ChangesPurchase Revocation and Post-Execution Notifications
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant Handler
participant ExecutionStore
participant EmailSender
Client->>Router: GET/POST /api/purchases/revoke/:id?token=...
Router->>Handler: revokePurchase(req, id, token)
Handler->>ExecutionStore: checkRevokableStatus(id)
alt token path
Handler->>ExecutionStore: validateRevokeToken(token)
else session path
Handler->>ExecutionStore: revokeViaSession(session)
end
Handler->>EmailSender: (best-effort) SendPurchaseExecutedNotification(NotificationData)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/purchase/mocks_test.go (1)
654-656: ⚡ Quick winUse testify call recording for the new mock method.
This method bypasses
m.Called, so tests can’t assert invocation or force error paths forSendPurchaseExecutedNotification.Proposed fix
func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { - return nil + args := m.Called() + return args.Error(0) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/purchase/mocks_test.go` around lines 654 - 656, The mock method MockEmailSender.SendPurchaseExecutedNotification currently returns nil directly and skips testify's call recorder, preventing tests from asserting or simulating errors; modify this method to call and return m.Called(...) results (use m.Called(ctx, notificationData) and convert the first return to error) so tests can assert invocation and set return values via m.On/Assert; update the method in the MockEmailSender type to delegate to m.Called instead of returning nil.internal/scheduler/scheduler_test.go (1)
534-536: ⚡ Quick winAlign this mock method with the rest of the testify-backed sender.
Returning
nilunconditionally blocks expectation/assertion coverage forSendPurchaseExecutedNotification.Proposed fix
-func (m *MockEmailSender) SendPurchaseExecutedNotification(_ context.Context, _ email.NotificationData) error { - return nil +func (m *MockEmailSender) SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error { + args := m.Called(ctx, data) + return args.Error(0) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scheduler/scheduler_test.go` around lines 534 - 536, The mock method MockEmailSender.SendPurchaseExecutedNotification currently always returns nil which prevents testify mock expectations from being asserted; modify this method to use the testify mock call pattern (invoke m.Called with the context and notification args or placeholders) and return the error extracted from the call result (e.g., args.Error(0)) so tests can set expectations and control the returned error via mock.On for SendPurchaseExecutedNotification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 2850-2987: Add tests that exercise the session-auth revoke branch
by invoking Handler.revokePurchase without a token in the request so the path
goes through tryRevokeViaSession -> revokeViaSession; create cases for an admin
with HasPermissionAPI returning true for "cancel-any" and a non-admin owner with
"cancel-own" true (and corresponding ValidateSession returning a Session with
matching Email), plus a failure case where neither permission is present to
confirm denial. Ensure the tests mock MockAuth.ValidateSession,
MockAuth.HasPermissionAPI, and
MockConfigStore.GetExecutionByID/GetCloudAccount/SavePurchaseExecution to assert
the execution status transitions (e.g., to "revocation_requested") or the
expected client errors, and reference the functions tryRevokeViaSession,
revokeViaSession and revokePurchase in test names/comments so reviewers can
locate them.
In `@internal/api/handler_purchases.go`:
- Around line 681-685: The current revoke path calls
validateRevokeToken(execution, token) but never verifies the token/window
expiry, allowing old executions to be marked revocation_requested; update the
revoke flow to enforce expiry by adding an expiry/window check before calling
revokeViaSession: either extend validateRevokeToken to also validate token
expiration and execution window timestamps (e.g., compare
execution.RevokeTokenExpiry or execution.WindowEnd against time.Now()) or call a
new helper (e.g., validateRevokeTokenExpiry or isRevokeWindowValid) after
validateRevokeToken and before revokeViaSession(ctx, execution, actor), and
return an error if the token/window is expired; apply the same change to the
other revoke path handling lines ~733-741 so both code paths enforce expiry.
- Line 757: The log call currently prints the raw actor email via revokedBy in
logging.Infof("Revocation requested for execution %s by %s",
execution.ExecutionID, revokedBy); change this to emit a PII-safe identifier
instead (e.g., mask or hash the email or log the actor's internal ID). Update
the call to use a helper like redactEmail(revokedBy) or hashActor(revokedBy)
(create such a helper if missing) so the message becomes
logging.Infof("Revocation requested for execution %s by %s",
execution.ExecutionID, redactEmail(revokedBy)); ensure redactEmail/hashActor is
deterministic and irreversible to avoid leaking the actual email.
- Around line 748-756: revokeViaSession currently mutates execution.Status and
calls h.config.SavePurchaseExecution, which can cause lost-updates; change it to
perform an atomic/conditional status transition that only sets Status =
"revocation_requested" when the current status is one of the allowed
predecessors ("completed" or "partially_completed"). Locate revokeViaSession and
replace the blind mutation+SavePurchaseExecution with a single conditional
update call (or add a new config method) that issues an UPDATE ... WHERE
execution_id = ? AND status IN ('completed','partially_completed') (or
equivalent ORM/Repo call) and returns a clear error when no rows were updated;
ensure CancelledBy is included in the same atomic update and preserve existing
SavePurchaseExecution behavior for other flows.
---
Nitpick comments:
In `@internal/purchase/mocks_test.go`:
- Around line 654-656: The mock method
MockEmailSender.SendPurchaseExecutedNotification currently returns nil directly
and skips testify's call recorder, preventing tests from asserting or simulating
errors; modify this method to call and return m.Called(...) results (use
m.Called(ctx, notificationData) and convert the first return to error) so tests
can assert invocation and set return values via m.On/Assert; update the method
in the MockEmailSender type to delegate to m.Called instead of returning nil.
In `@internal/scheduler/scheduler_test.go`:
- Around line 534-536: The mock method
MockEmailSender.SendPurchaseExecutedNotification currently always returns nil
which prevents testify mock expectations from being asserted; modify this method
to use the testify mock call pattern (invoke m.Called with the context and
notification args or placeholders) and return the error extracted from the call
result (e.g., args.Error(0)) so tests can set expectations and control the
returned error via mock.On for SendPurchaseExecutedNotification.
🪄 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: d8b413f2-fa20-492c-8162-27871ec7dbd4
📒 Files selected for processing (15)
internal/api/coverage_gaps_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/middleware.gointernal/api/router.gointernal/email/interfaces.gointernal/email/nop_sender.gointernal/email/sender.gointernal/email/smtp_sender.gointernal/email/templates.gointernal/mocks/email.gointernal/purchase/mocks_test.gointernal/scheduler/scheduler_test.gointernal/server/app_test.gointernal/server/handler_ri_exchange_test.go
Member
Author
|
Rebased onto fresh Addressed CR pass-1 findings (commit
@coderabbitai review |
Contributor
|
Triggering an incremental review of the new commits now. 🧠 Learnings used✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/middleware.go (1)
16-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse exact matching for fixed public endpoints like
"/version"Line 20 adds
"/version"to a prefix-matched list; that makes any"/version*"path bypassvalidateSecurity(auth + CSRF short-circuit). Keep prefix checks only for intentional dynamic routes and exact-match singleton endpoints.Suggested hardening
func (h *Handler) isPublicEndpoint(path string) bool { - publicEndpoints := []string{ + publicPrefixEndpoints := []string{ "/health", // Root health endpoint (no /api prefix) "/api/health", // API health endpoint "/api/info", - "/version", // Public build-version endpoint (version / git SHA / build time) "/api/purchases/approve/", "/api/purchases/cancel/", "/api/purchases/revoke/", "/api/ri-exchange/approve/", "/api/ri-exchange/reject/", "/api/auth/login", @@ "/docs", "/api/docs", } - for _, ep := range publicEndpoints { + for _, ep := range publicPrefixEndpoints { if strings.HasPrefix(path, ep) { return true } } + // Exact-match singleton public endpoints. + if path == "/version" { + return true + } // Exact match for POST /api/register (no trailing slash). if path == "/api/register" { return true } return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/middleware.go` around lines 16 - 37, The current publicEndpoints slice is checked with strings.HasPrefix in the middleware that implements validateSecurity, which lets fixed endpoints like "/version" match "/version*" unintentionally; update the logic to distinguish exact-match endpoints from prefix-matched dynamic routes: keep entries intended as prefixes with trailing slashes (e.g., "/api/purchases/") and create a separate exactEndpoints set (or check equality) for singleton paths like "/version", "/health", "/api/health", "/api/info", "/docs", "/api/docs", and other non-dynamic routes, then in the loop use exact equality for exactEndpoints and strings.HasPrefix only for prefix-style entries so fixed endpoints no longer bypass auth/CSRF.
🧹 Nitpick comments (1)
internal/api/handler_purchases_test.go (1)
3434-3479: ⚡ Quick winAdd an expired+wrong-token precedence assertion.
This test uses a matching token, so it doesn’t pin the contract that expiry is checked before token comparison. Add a case where the supplied token is wrong but
ApprovalTokenExpiresAtis past, and still expect409 expired.💡 Suggested test addition
func TestHandler_revokePurchase_ExpiredToken(t *testing.T) { @@ assert.Equal(t, 409, ce.code) assert.Contains(t, ce.message, "expired") } + +func TestHandler_revokePurchase_ExpiredToken_WrongTokenStill409(t *testing.T) { + ctx := context.Background() + execID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + realToken := "real-token" + wrongToken := "wrong-token" + contactEmail := "contact@acct.example.com" + accountID := "acct-1" + past := time.Now().Add(-1 * time.Hour) + + exec := &config.PurchaseExecution{ + ExecutionID: execID, + ApprovalToken: realToken, + ApprovalTokenExpiresAt: &past, + Status: "completed", + Recommendations: []config.RecommendationRecord{ + {ID: "r1", CloudAccountID: &accountID}, + }, + } + + mockStore := new(MockConfigStore) + mockStore.On("GetExecutionByID", ctx, execID).Return(exec, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + mockStore.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, ContactEmail: contactEmail}, nil + } + + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: contactEmail}, nil) + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-any", "purchases").Return(false, nil).Maybe() + mockAuth.On("HasPermissionAPI", ctx, "", "cancel-own", "purchases").Return(false, nil).Maybe() + + handler := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer sess-tok"}, + QueryStringParameters: map[string]string{"token": wrongToken}, + } + _, err := handler.revokePurchase(ctx, req, execID, wrongToken) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.message, "expired") +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_purchases_test.go` around lines 3434 - 3479, Add a new assertion in TestHandler_revokePurchase_ExpiredToken to verify expiry takes precedence over token equality: create a second call where the PurchaseExecution returned by mockStore (from GetExecutionByID) has ApprovalTokenExpiresAt in the past but the supplied query token is a non-matching value (e.g., wrongToken); call handler.revokePurchase(ctx, reqWrongToken, execID, wrongToken) and assert the error is a client error with code 409 and message contains "expired". Keep the same mocks (mockStore, mockAuth/GetCloudAccountFn) and request headers as the existing test so only the supplied token differs. Ensure you reference handler.revokePurchase, TestHandler_revokePurchase_ExpiredToken, GetExecutionByID and ApprovalTokenExpiresAt when locating where to add the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/api/middleware.go`:
- Around line 16-37: The current publicEndpoints slice is checked with
strings.HasPrefix in the middleware that implements validateSecurity, which lets
fixed endpoints like "/version" match "/version*" unintentionally; update the
logic to distinguish exact-match endpoints from prefix-matched dynamic routes:
keep entries intended as prefixes with trailing slashes (e.g.,
"/api/purchases/") and create a separate exactEndpoints set (or check equality)
for singleton paths like "/version", "/health", "/api/health", "/api/info",
"/docs", "/api/docs", and other non-dynamic routes, then in the loop use exact
equality for exactEndpoints and strings.HasPrefix only for prefix-style entries
so fixed endpoints no longer bypass auth/CSRF.
---
Nitpick comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 3434-3479: Add a new assertion in
TestHandler_revokePurchase_ExpiredToken to verify expiry takes precedence over
token equality: create a second call where the PurchaseExecution returned by
mockStore (from GetExecutionByID) has ApprovalTokenExpiresAt in the past but the
supplied query token is a non-matching value (e.g., wrongToken); call
handler.revokePurchase(ctx, reqWrongToken, execID, wrongToken) and assert the
error is a client error with code 409 and message contains "expired". Keep the
same mocks (mockStore, mockAuth/GetCloudAccountFn) and request headers as the
existing test so only the supplied token differs. Ensure you reference
handler.revokePurchase, TestHandler_revokePurchase_ExpiredToken,
GetExecutionByID and ApprovalTokenExpiresAt when locating where to add the new
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d7a4e10-10de-4505-a161-499d3819237a
📒 Files selected for processing (15)
internal/api/coverage_gaps_test.gointernal/api/handler_purchases.gointernal/api/handler_purchases_test.gointernal/api/middleware.gointernal/api/router.gointernal/email/interfaces.gointernal/email/nop_sender.gointernal/email/sender.gointernal/email/smtp_sender.gointernal/email/templates.gointernal/mocks/email.gointernal/purchase/mocks_test.gointernal/scheduler/scheduler_test.gointernal/server/app_test.gointernal/server/handler_ri_exchange_test.go
💤 Files with no reviewable changes (10)
- internal/scheduler/scheduler_test.go
- internal/mocks/email.go
- internal/purchase/mocks_test.go
- internal/server/app_test.go
- internal/server/handler_ri_exchange_test.go
- internal/email/nop_sender.go
- internal/email/sender.go
- internal/email/templates.go
- internal/email/smtp_sender.go
- internal/email/interfaces.go
✅ Files skipped from review due to trivial changes (1)
- internal/api/coverage_gaps_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/router.go
- internal/api/handler_purchases.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/api/middleware_test.go (1)
17-34: ⚡ Quick winAdd explicit coverage for the new revoke public path.
The one-click revoke flow now depends on
/api/purchases/revoke/<id>being treated as public by middleware, so this table should assert that contract alongside approve/cancel. A negative case like/api/purchases/revoked/...would also guard against another prefix-overlap regression.Proposed test additions
{"/api/purchases/approve/12345678-1234-1234-1234-123456789abc", true}, {"/api/purchases/cancel/45645645-6456-4564-5645-645645645645", true}, + {"/api/purchases/revoke/12345678-1234-1234-1234-123456789abc", true}, + {"/api/purchases/revoked/12345678-1234-1234-1234-123456789abc", false}, {"/api/config", false},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/middleware_test.go` around lines 17 - 34, Update the tests table in middleware_test.go to cover the new public revoke path: add a positive case for "/api/purchases/revoke/12345678-1234-1234-1234-123456789abc" (expected true) alongside the existing approve/cancel entries, and add a negative prefix-overlap case like "/api/purchases/revoked/12345678-1234-1234-1234-123456789abc" (expected false) to ensure the middleware only exact-matches the revoke pattern; modify the tests slice where paths and expected booleans are defined to include these two entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/middleware_test.go`:
- Around line 28-33: Run gofmt (or `go fmt`) on the test file to apply the
standard formatting changes reported by pre-commit and commit the reformatted
file; specifically reformat internal/api/middleware_test.go (the table of test
paths containing entries like "/version", "/version-evil", "/versionXYZ",
"/api/register", "/api/registrations") so the file matches gofmt output and CI
stops failing.
In `@internal/api/middleware.go`:
- Around line 42-48: The CSRF exemption logic is using strings.HasPrefix for
"/api/register" while isPublicEndpoint() was hardened to exact matches; update
requiresCSRFValidation() to use the same exact-match policy (e.g., compare path
== "/api/register" or delegate to isPublicEndpoint(path)) instead of
strings.HasPrefix so POST requests to "/api/register*" no longer improperly skip
CSRF validation; adjust any related checks to preserve the original
no-trailing-slash semantics for "/api/register".
---
Nitpick comments:
In `@internal/api/middleware_test.go`:
- Around line 17-34: Update the tests table in middleware_test.go to cover the
new public revoke path: add a positive case for
"/api/purchases/revoke/12345678-1234-1234-1234-123456789abc" (expected true)
alongside the existing approve/cancel entries, and add a negative prefix-overlap
case like "/api/purchases/revoked/12345678-1234-1234-1234-123456789abc"
(expected false) to ensure the middleware only exact-matches the revoke pattern;
modify the tests slice where paths and expected booleans are defined to include
these two entries.
🪄 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: 589913bd-170b-495a-90ff-a14877744b52
📒 Files selected for processing (2)
internal/api/middleware.gointernal/api/middleware_test.go
#291) The post-execution notification (#291) had no send-path coverage: every existing approve test built the handler with emailNotifier=nil, so the send was silently skipped, and the direct-execute path had no test at all. Add execution-flow tests with a recording emailNotifier asserting SendPurchaseExecutedNotification fires exactly once with the expected To (per-account contact), executor, and revocation token for all three paths: token-approve, session-approve, and direct-execute. Include a nil-notifier guard test confirming direct-execute still completes the purchase without panicking when no notifier is configured. Add internal/email render tests for RenderPurchaseExecutedNotification Email / ...HTML and buildExecutedNotificationSubject: revoke URL present with the execution route, token url-escaped (raw special chars never leak), revoke panel omitted when the token is absent, the expiry note gated on RevocationWindowClosesAt (deferred -> omitted; populated -> rendered), and the single-vs-multi subject format.
internal/api test package failed to compile: TestHandler_getPurchaseDetails_ NotFound_IsClientError set a nonexistent Session.Role field. The Session struct carries only UserID/Email; admin is resolved via HasPermissionAPI. Drop the bogus field and confer admin through mockAuth.grantAdmin() so the test reaches the GetExecutionByID not-found branch it asserts on (#431). Also apply gofmt to handler_purchases_guards_test.go and middleware_test.go, which carried multi-statement-per-line drift that failed the gofmt hook.
…leware_test Replace strings.HasPrefix loop in requiresCSRFValidation() with a switch statement using exact-match, mirroring the approach already in place for isPublicEndpoint(). Previously a path like /api/register-malicious would be silently exempted from CSRF validation because it shares the /api/register prefix with the legitimate registration endpoint. Add TestHandler_requiresCSRFValidation_ExactMatch to regression-guard the prefix-overlap gap (e.g. POST /api/register-malicious must require CSRF).
The base branch added a req parameter to requiresCSRFValidation; the ExactMatch test in this PR still called the 2-arg form. Pass nil for tests that only exercise the path-matching branches (which return before inspecting the request).
…mutates (CRITICAL: email prefetchers)
GET /api/purchases/revoke/:id now returns a minimal HTML confirmation page
containing a POST form with the token in a hidden input. No DB queries are
made on GET, so email prefetchers and link scanners cannot auto-trigger the
revocation.
The POST form encoding (application/x-www-form-urlencoded) is now also
supported by resolveApprovalToken alongside the existing JSON body and
query-string paths, so the confirmation form's submission is correctly
handled on the POST path.
Tests: TestRevokePurchase_GETRendersConfirmationPage_NoMutation,
TestRevokePurchase_POSTPerformsRevoke
…leaked token cannot revoke later
After a successful ApproveExecution or CancelExecution, a follow-up
SavePurchaseExecution clears ApprovalToken and ApprovalTokenExpiresAt.
This prevents a token that leaked (e.g. via server logs or email
forwarding) from being replayed on the /revoke endpoint after the
primary action has already consumed it.
The rotation is best-effort: if the follow-up save fails the
approve/cancel has already landed, so only a warning is logged.
Tests: TestApproveExecution_RotatesApprovalToken,
TestRevokePurchase_RejectsRotatedToken,
TestRotateApprovalToken_PersistsEmpty
…oke reads token from body
Add redactURL / redactQueryParam helpers to middleware.go that replace any
token= query parameter value with REDACTED before it is written to a log
line. Apply to the debug log in HandleRequest and to logMissingCSRFToken.
resolveApprovalToken now also parses application/x-www-form-urlencoded
bodies (used by the GET /revoke confirmation form) so the token submitted
by the HTML form lands in the POST body rather than the query string.
Tests: TestRedactURL_StripsTokenQueryParam,
TestRevokePOSTReadsTokenFromBody
…il copy
Add revocationWindowDuration = 24h const and enforce it in validateRevokeToken
via a new checkRevocationWindow helper. The reference timestamp is CompletedAt
(falling back to ExecutedAt for direct-execute rows); legacy rows without
either field skip the check for backward compatibility.
sendPurchaseExecutedEmail now populates RevocationWindowClosesAt so the email
body shows the deadline that validateRevokeToken enforces.
Tests: TestRevoke_WithinWindow_Allowed,
TestRevoke_AfterWindow_Denied,
TestRevoke_NoTimestamp_Allowed,
TestRevoke_4EyesMode_TODO (skip placeholder for #1005)
…pdate clobber Introduce SetCancelledBy as a targeted narrow SQL UPDATE (execution_id, cancelled_by) instead of a full-row SavePurchaseExecution overwrite in revokeViaSession. The prior code read the execution, mutated the struct, and wrote the whole row, which could clobber any concurrent update (e.g. a CompletedAt write) that landed between the read and the write. StoreInterface gains SetCancelledBy; PostgresStore, all mocks, and test stubs are updated.
Raw variable-length string comparison via subtle.ConstantTimeCompare leaks token length through the short-circuit path in the Go stdlib implementation when the two slices have different lengths. Pre-hashing both the stored and user-supplied token to fixed-size 32-byte digests eliminates the length side channel while preserving the constant-time property for equal-length inputs. Applied to all three ApprovalToken comparison sites: loadCancelableExecution, ApproveExecution (purchase package), validateRevokeToken (api package), and the SQS worker path in messages.go.
lookupRequesterInfo silently swallowed both GetUser errors and nil-user responses, making it impossible to distinguish a lookup failure from a user that genuinely has no email. Add Warnf log lines on both branches so operators can diagnose missing notification recipients.
…handler; fix mock sigs After rebasing onto origin/main, several callers were broken by signature changes that landed in main: - TransitionExecutionStatus gained an actor *string parameter (PR #xxx): update all mock.On registrations in handler_purchases_test and approvals_test to pass the 4th/5th argument; use mock.MatchedBy for revoke-path callers where the actor is the revoker email. - ApproveAndExecute gained a transitionedBy *string parameter: update executed_notification_flow_test mocks to pass (*string)(nil). - Rename the PR-local revokeViaEmailToken handler (was revokePurchase, now collides with handler_purchases_revoke.go from main); update router registrations and all test call-sites accordingly. - Wire sendPurchaseExecutedEmail in approveViaToken so the token-auth approve path fires the post-execution notification (the session-auth path already called it; this was the missing call). - Extract validateApprovalToken helper from ApproveExecution to bring cyclomatic complexity below the gocyclo threshold. All tests pass after these fixes.
Member
Author
|
Rebased onto origin/main (post-#1208 merge, ed288c4). 2 struct conflicts resolved by intent: sender.go (kept main's NotificationData layout + added PR's new RevocationToken/ExecutedAt/ExecutedBy fields); interfaces.go (added PR's new SetCancelledBy method, kept main's CancelExecutionAtomic comment excluding scheduled status). Azure files no longer appear in diff (correctly inheriting from main now). Gates:
Branch force-pushed: fix/291-wave2 @coderabbitai review |
Contributor
|
✅ Action performedReview finished.
|
Defect: approveViaToken called rotateApprovalToken which CLEARED ApprovalToken in the DB, then passed the stale pre-approve execution struct to sendPurchaseExecutedEmail. The email embedded the now-cleared token as RevocationToken, causing validateRevokeToken to return 403 on every revoke attempt from the executed-notification email. Fix (Option A): after a successful token-authed approve, mintRevocationToken generates a fresh random token with a 24-hour expiry (matching the revocation window) and persists it. approveViaToken re-fetches the execution after ApproveExecution returns so the email carries the valid fresh token. Security: approve-replay is blocked by the status check in TransitionExecutionStatus (execution is "completed" after approve), not by token rotation, so the fresh token cannot be used to re-approve. The fresh token is scoped to revoke-only via checkRevokableStatus. The cancel path is unchanged: clearApprovalToken (renamed from rotateApprovalToken) still clears the token to empty since a canceled execution has no valid follow-up action. Nit #1: added CSRF validation to tryRevokeViaSession for consistency with sibling cancelPurchaseViaSession and approvePurchaseViaSession. Regression test: TestExecutedNotification_TokenApprovePath uses .Once() to return different executions on the pre- and post-approve GetExecutionByID calls and asserts the email RevocationToken matches the fresh token AND passes validateRevokeToken. Confirmed FAILS on pre-fix code (second .Once() unconsumed, token assertion fails) and PASSES after fix.
Member
Author
Blocking defect fixed (adversarial review)Root cause: Two commits in this PR contradicted each other: Fix chosen: Option A (preferred) over Option B. Option A is cleaner because it preserves the security principle of token rotation while solving the email problem. The old approval token is replaced by a fresh revocation-scoped token (not just cleared), so:
Changes
Regression test
It then asserts: (a) the email carries "fresh-revoke-token" and (b) Confirmed: FAILS on pre-fix code (second New tests added
Gate exit codes
Out-of-scope nits (follow-up)Nits #2-#4 from the adversarial review are not addressed here per instructions (skip the other nits, note as follow-ups). @coderabbitai review |
Fold in post-security-review nits for PR #889. 1. approveViaToken: the re-fetch-failure fallback reused the stale pre-approve execution struct, which after a successful mint carries the OLD approval token (already replaced in the DB). Emailing it would make the Revoke button 403 -- the original defect resurrected on the rare degraded path. Now send a COPY with ApprovalToken blanked so the email template's {{if .RevocationToken}} suppresses the Revoke panel entirely (no broken button), and never mutate the caller's struct. 2. Corrected the fallback log line to describe the new behavior (panel suppressed) rather than the inaccurate "sending without valid revocation token". 3. Fixed the mintRevocationToken doc comment: it claimed an "empty RevocationToken" fallback that no longer matches the mint logic; it now accurately describes both degraded paths (mint-write failure vs handler re-fetch failure). Updated the stale TestApproveExecution_RotatesApprovalToken comment above the renamed _MintsRevocationToken test. 4. Shared the 24h window as config.RevocationWindow, replacing the two in-sync-by-comment constants (purchase.revocationTokenWindow and api.revocationWindowDuration). Both packages already import config, so no new import cycle. Regression test: TestExecutedNotification_TokenApprovePath_RefetchFailureSuppressesPanel drives the token-approve flow with a failing post-approve re-fetch and asserts the email carries an EMPTY RevocationToken (panel suppressed) and that the caller's struct is untouched. Confirmed FAILS on pre-fix code ("Should be empty, but was valid-token") and PASSES after the fix.
Member
Author
Folded in the four review nits (commit 52b6393)1 (blocking degraded path fixed) — 2 — Corrected the fallback log line: "suppressing Revoke panel (no valid token available)" instead of the inaccurate "sending without valid revocation token". 3 — Fixed the 4 — Shared the 24h window as Regression test
Gate exit codes
Not merging — leaving for self-merge on green. |
The CI "Lint Code" job runs the FULL golangci-lint (not incremental), which surfaced gocritic/govet/misspell/unparam findings the --new-from-rev check missed. Fix all of them so `golangci-lint run ./...` is clean. gocritic: - tryRevokeViaSession: name the (any, bool, error) results; rename the inner csrf/revoke locals so the named returns are not shadowed. - sendPurchaseExecutedEmail: range over index with a pointer instead of copying 312-byte RecommendationRecord values each iteration. - lookupRequesterInfo: rename the return that shadowed the imported 'email' package. - SetCancelledBy (store_postgres + mock): combine the repeated string param types. govet shadow: - revokeViaEmailToken + ApproveExecution: use distinct error variable names (statusErr, tokErr) instead of re-declaring/reassigning the outer err, avoiding both the shadow warning and gocritic sloppyReassign. unparam: - lookupRequesterInfo always returned "" for the name (api.User has no display-name field). Drop the dead second return and set the email's RequestedByName to "" at the call site until a name source lands. misspell (US locale): behaviour->behavior, authorised->authorized, cancelled->canceled in prose comments. The one comment that referenced the British-spelled cancelled_by DB column is reworded to avoid the token while staying accurate (the SetCancelledBy call directly below keeps the column discoverable). No behavior change; all api/purchase/config tests still pass.
Member
Author
Cleared the full "Lint Code" CI findings (commit a7961a1)The CI Lint job runs the FULL gocritic
govet shadow
unparam
misspell (US locale): Gate exit codes (all 0)
No behavior change. Not merging — leaving for self-merge on green. |
Member
Author
|
Merged to main (rebased onto current main, all CI green). post-execution email notification with a working one-click revoke link (mints a fresh purpose-built 24h revocation token on approve, preserving approve-replay protection); adversarial-reviewed SHIP. |
This was referenced Jul 17, 2026
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
…parsing AWS Lambda Function URLs base64-encode POST bodies whenever the Content-Type isn't recognized as plain text, which is the case for the one-click revoke confirmation form's application/x-www-form-urlencoded POST added in #889. handleLambdaHTTPEvent never checked request.IsBase64Encoded, so resolveApprovalToken received a base64 blob instead of "token=...", and every revoke via the email link 401'd in Lambda mode (the primary deploy mode). The HTTP/Fargate adapter was unaffected since it builds its own request with IsBase64Encoded always false. Decode the body once, centrally, right after the raw event is unmarshaled and before anything downstream (OIDC, static files, the API router) reads it. A decode failure returns the same 400 response already used for a malformed event instead of silently proceeding with garbage. Regression coverage: the base64-encoded form-body test drives the real failing revoke scenario end-to-end. A plain-body companion test proves the decode stays gated on IsBase64Encoded so an unconditional decode can't corrupt plaintext bodies, and a malformed-base64 test proves the handler fails closed with a clean 400 rather than panicking or forwarding a garbage body.
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
…parsing AWS Lambda Function URLs base64-encode POST bodies whenever the Content-Type isn't recognized as plain text, which is the case for the one-click revoke confirmation form's application/x-www-form-urlencoded POST added in #889. handleLambdaHTTPEvent never checked request.IsBase64Encoded, so resolveApprovalToken received a base64 blob instead of "token=...", and every revoke via the email link 401'd in Lambda mode (the primary deploy mode). The HTTP/Fargate adapter was unaffected since it builds its own request with IsBase64Encoded always false. Decode the body once, centrally, right after the raw event is unmarshaled and before anything downstream (OIDC, static files, the API router) reads it. A decode failure returns the same 400 response already used for a malformed event instead of silently proceeding with garbage.
cristim
added a commit
that referenced
this pull request
Jul 22, 2026
…parsing (#1483) AWS Lambda Function URLs base64-encode POST bodies whenever the Content-Type isn't recognized as plain text, which is the case for the one-click revoke confirmation form's application/x-www-form-urlencoded POST added in #889. handleLambdaHTTPEvent never checked request.IsBase64Encoded, so resolveApprovalToken received a base64 blob instead of "token=...", and every revoke via the email link 401'd in Lambda mode (the primary deploy mode). The HTTP/Fargate adapter was unaffected since it builds its own request with IsBase64Encoded always false. Decode the body once, centrally, right after the raw event is unmarshaled and before anything downstream (OIDC, static files, the API router) reads it. A decode failure returns the same 400 response already used for a malformed event instead of silently proceeding with garbage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
notification_email, per-accountcontact_email(s), and the requester's email. Recipients are deduplicated; first contact is To, rest are Cc.approval_token(a dedicated revocation token column is deferred to the sibling AWS RI/SP revocation-via-support-case issue).AuthPublicrouteGET/POST /api/purchases/revoke/{id}?token=...validates the token withcrypto/subtle.ConstantTimeCompare(timing-safe) and recordsrevocation_requestedstatus. Session-authed users withcancel-anyorcancel-ownpermission can also revoke without a token (same three-mode dispatch as approve/cancel).Test plan
TestHandler_revokePurchase_ValidToken- valid token path resolves torevocation_requestedTestHandler_revokePurchase_InvalidToken- wrong token returns 403TestHandler_revokePurchase_PendingExecution- pending purchase returns 409 directing to CancelTestHandler_revokePurchase_NotFound- missing execution returns 404TestResolveExecutedNotificationRecipients_*- four dedup/fallback scenarios coveredSummary by CodeRabbit
New Features
Tests