Merge pull request #594 from LeanerCloud/fix/token-actions-no-existence-oracle

fix(api): answer 401 before looking up the execution on the email-link routes

Author: cristim

internal/api/coverage_extras_test.go

@@ -120,17 +120,17 @@ func TestHandler_cancelPurchase_InvalidUUID(t *testing.T) {
 // TestHandler_cancelPurchase_EmptyToken_FallsThroughToSession asserts that
 // the token-empty branch no longer short-circuits with "cancellation token
 // is required" — the empty-token path is now the dispatch into the
-// session-authed cancel flow (issue #46). Without an execution to load,
-// GetExecutionByID is the first thing that runs; with no config wired,
-// the call surfaces a downstream error rather than the legacy 400.
+// session-authed cancel flow (issue #46). With no session the caller gets a
+// 401 before any execution is loaded (issue #435), not the legacy 400.
 func TestHandler_cancelPurchase_EmptyToken_FallsThroughToSession(t *testing.T) {
 	execID := "11111111-1111-1111-1111-111111111111"
 	mockConfig := new(MockConfigStore)
-	mockConfig.On("GetExecutionByID", mock.Anything, execID).Return(nil, errors.New("store error"))
 	h := &Handler{config: mockConfig}
 	_, err := h.cancelPurchase(context.Background(), nil, execID, "")
-	assert.Error(t, err)
-	assert.Contains(t, err.Error(), "failed to get execution")
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a client error, got: %v", err)
+	assert.Equal(t, 401, ce.code)
+	mockConfig.AssertNotCalled(t, "GetExecutionByID", mock.Anything, execID)
 }
 
 func TestHandler_cancelPurchase_PurchaseError(t *testing.T) {

internal/api/handler_purchases.go

@@ -596,6 +596,10 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio
 		return nil, err
 	}
 
+	if err := h.requireSessionBeforeLookup(ctx, req); err != nil {
+		return nil, err
+	}
+
 	execution, err := h.loadApproveExecution(ctx, execID)
 	if err != nil {
 		return nil, err
@@ -636,6 +640,18 @@ func (h *Handler) approvePurchase(ctx context.Context, req *events.LambdaFunctio
 	return h.approvePurchaseViaSession(ctx, req, execution)
 }
 
+// requireSessionBeforeLookup answers 401 to a caller with no valid session
+// before the execution is loaded. Every branch of the email-link
+// approve/cancel/revoke routes needs a session (the token alone never
+// authorizes), so an unauthenticated caller gets the same response for a
+// missing and an existing execution ID (issue #435).
+func (h *Handler) requireSessionBeforeLookup(ctx context.Context, req *events.LambdaFunctionURLRequest) error {
+	if h.tryGetSession(ctx, req) == nil {
+		return NewClientError(401, "sign in with the account's contact email to approve or cancel this purchase")
+	}
+	return nil
+}
+
 // tokenActionError maps the email-link approve/cancel failures that are the
 // caller's fault to a 4xx: a wrong token is 403 (as on the RI exchange
 // approve path), an expired token is 410, and an execution
@@ -1206,12 +1222,13 @@ func (h *Handler) cancelPurchase(ctx context.Context, req *events.LambdaFunction
 		return nil, err
 	}
 
-	execution, err := h.config.GetExecutionByID(ctx, execID)
-	if errors.Is(err, config.ErrNotFound) {
-		return nil, NewClientError(404, "execution not found")
+	if err := h.requireSessionBeforeLookup(ctx, req); err != nil {
+		return nil, err
 	}
+
+	execution, err := h.getExecutionOr404(ctx, execID)
 	if err != nil {
-		return nil, fmt.Errorf("failed to get execution: %w", err)
+		return nil, err
 	}
 
 	// Three-mode dispatch:
@@ -1424,6 +1441,10 @@ func (h *Handler) revokeViaEmailToken(ctx context.Context, req *events.LambdaFun
 		return renderRevokeConfirmPage(execID, token), nil
 	}
 
+	if err := h.requireSessionBeforeLookup(ctx, req); err != nil {
+		return nil, err
+	}
+
 	execution, err := h.getExecutionOr404(ctx, execID)
 	if err != nil {
 		return nil, err

internal/api/handler_purchases_test.go

@@ -3406,15 +3406,14 @@ func TestHandler_cancelPurchase_Session_RejectsMissingSession(t *testing.T) {
 	mockConfig.On("GetExecutionByID", mock.Anything, cancelExecID).Return(exec, nil)
 
 	handler := &Handler{config: mockConfig, auth: new(MockAuthService)}
-	// No Authorization header → CSRF fires first (issue #404: cancelPurchaseViaSession
-	// now enforces CSRF before requireSession). A tokenless request can't provide a
-	// valid CSRF binding, so we get a 403 "CSRF validation failed".
+	// No Authorization header → 401 before the execution is loaded (issue #435),
+	// so a missing and an existing ID are indistinguishable.
 	_, err := handler.cancelPurchase(context.Background(), &events.LambdaFunctionURLRequest{}, cancelExecID, "")
 	require.Error(t, err)
 	ce, ok := IsClientError(err)
 	require.True(t, ok, "expected a clientError, got: %v", err)
-	assert.Equal(t, 403, ce.code)
-	assert.Contains(t, ce.Error(), "CSRF validation failed")
+	assert.Equal(t, 401, ce.code)
+	mockConfig.AssertNotCalled(t, "GetExecutionByID", mock.Anything, cancelExecID)
 }
 
 // TestHandler_cancelPurchase_DeepLink_AdminBypassesContactEmailGate is the
@@ -5721,8 +5720,10 @@ func TestHandler_revokePurchase_NotFound(t *testing.T) {
 	mockStore := new(MockConfigStore)
 	mockStore.On("GetExecutionByID", ctx, execID).Return(nil, nil)
 
-	handler := &Handler{config: mockStore}
-	req := &events.LambdaFunctionURLRequest{}
+	mockAuth := new(MockAuthService)
+	mockAuth.On("ValidateSession", mock.Anything, "sess-tok").Return(&Session{Email: "admin@example.com"}, nil)
+	handler := &Handler{config: mockStore, auth: mockAuth}
+	req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer sess-tok"}}
 	_, err := handler.revokeViaEmailToken(ctx, req, execID, "some-token")
 	require.Error(t, err)
 	ce, ok := IsClientError(err)

internal/api/handler_purchases_token_oracle_test.go

@@ -0,0 +1,57 @@
+package api
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"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"
+)
+
+// A caller with no session must get the same answer for a missing and an
+// existing execution on the email-link routes, so the routes are not an
+// existence oracle (issue #435).
+func TestHandleRequest_TokenActions_UnauthenticatedCannotTellMissingFromExisting(t *testing.T) {
+	future := time.Now().Add(time.Hour)
+	hash := config.HashApprovalToken(tokenErrRawToken)
+
+	headerVariants := map[string]map[string]string{
+		"no-header":      {},
+		"invalid-bearer": {"authorization": "Bearer bad-tok"},
+		"bogus-x-auth":   {"x-authorization": "Basic bad-tok"},
+	}
+	for _, action := range []string{"approve", "cancel", "revoke"} {
+		for _, token := range []string{"", tokenErrRawToken, "wrong"} {
+			for variant, headers := range headerVariants {
+				t.Run(action+"/token="+token+"/"+variant, func(t *testing.T) {
+					call := func(h *Handler) *events.LambdaFunctionURLResponse {
+						req := tokenErrRequest(action, token)
+						req.Headers = headers
+						resp, err := h.HandleRequest(context.Background(), req)
+						require.NoError(t, err)
+						return resp
+					}
+
+					exec := &config.PurchaseExecution{Status: "pending", ApprovalToken: hash, ApprovalTokenExpiresAt: &future}
+					existingH := tokenErrHandler(t, exec, nil)
+					missingH := tokenErrHandler(t, nil, config.ErrNotFound)
+					for _, h := range []*Handler{existingH, missingH} {
+						h.auth.(*MockAuthService).On("ValidateSession", mock.Anything, "bad-tok").Return(nil, errors.New("invalid session")).Maybe()
+					}
+					existing := call(existingH)
+					missing := call(missingH)
+
+					assert.Equal(t, 401, existing.StatusCode, existing.Body)
+					assert.Equal(t, existing.StatusCode, missing.StatusCode, "missing: %s", missing.Body)
+					assert.Equal(t, existing.Body, missing.Body)
+				})
+			}
+		}
+	}
+}

internal/api/openapi.yaml

@@ -544,18 +544,16 @@ paths:
           $ref: '#/components/responses/BadRequest'
         '401':
           description: >
-            Token mode only: a token was supplied but the caller is not
-            signed in. A request with no credentials at all gets 403 (CSRF
-            validation failed), not 401, because the session path validates
-            CSRF before it requires a session.
+            The caller has no valid session, with or without a token. This
+            is answered before the execution is looked up, so an existing
+            and a missing execution ID get the same response.
           content:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
         '403':
           description: >
-            CSRF validation failed (this includes a request with no
-            credentials), the session lacks approve permission or
+            CSRF validation failed for a signed-in session, the session lacks approve permission or
             exceeds its Constraints, the caller is not the account's contact
             email, no contact email is configured on the execution's
             accounts, 4-eyes mode rejects the approver, or (token mode) the
@@ -649,18 +647,16 @@ paths:
           $ref: '#/components/responses/BadRequest'
         '401':
           description: >
-            Token mode only: a token was supplied but the caller is not
-            signed in. A request with no credentials at all gets 403 (CSRF
-            validation failed), not 401, because the session path validates
-            CSRF before it requires a session.
+            The caller has no valid session, with or without a token. This
+            is answered before the execution is looked up, so an existing
+            and a missing execution ID get the same response.
           content:
             application/json:
               schema:
                 $ref: '#/components/schemas/Error'
         '403':
           description: >
-            CSRF validation failed (this includes a request with no
-            credentials), the session lacks cancel permission, the caller is
+            CSRF validation failed for a signed-in session, the session lacks cancel permission, the caller is
             not the account's contact email, no contact email is
             configured on the execution's accounts, or (token mode) the
             approval token is invalid.
@@ -758,8 +754,9 @@ paths:
           $ref: '#/components/responses/BadRequest'
         '401':
           description: >
-            No session and no token, or token mode without a signed-in
-            contact-email session.
+            The caller has no valid session, with or without a token. This
+            is answered before the execution is looked up, so an existing
+            and a missing execution ID get the same response.
           content:
             application/json:
               schema:

internal/server/app_rate_limiter_integration_test.go

@@ -122,7 +122,7 @@ func TestApplicationRateLimiter(t *testing.T) {
 				if i%2 == 1 {
 					action = "cancel"
 				}
-				want := http.StatusNotFound
+				want := http.StatusUnauthorized
 				if i >= 30 {
 					want = http.StatusTooManyRequests
 				}

internal/server/lambda_coverage_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"strings"
 	"testing"
 
@@ -129,14 +130,13 @@ func TestHandleLambdaHTTPEvent_StaticPath(t *testing.T) {
 //
 // This drives the real failing scenario end-to-end: a base64-encoded,
 // form-urlencoded POST body hitting POST /api/purchases/revoke/{execID}.
-// With no session, revokeViaEmailToken (internal/api/handler_purchases.go)
-// 401s with "sign in or use the revocation link..." when the token fails to
-// resolve, or 401s with the DIFFERENT message "sign in with the account's
-// contact email..." from authorizeApprovalAction once the token resolves and
-// the (session-only) actor lookup fails. Only the second message is reachable
-// once the token has actually been parsed out of the decoded body, so
-// asserting it proves the fix; asserting the pre-fix code fails this test
-// caught the bug (verified manually before landing the fix).
+// With a session that lacks cancel permission, revokeViaEmailToken answers 403
+// "permission denied" when the token fails to resolve, or falls through to the
+// token branch and answers the DIFFERENT 403 "no per-account contact email"
+// from authorizeApprovalAction once the token has been parsed out of the
+// decoded body. Only the second message is reachable when the token resolves,
+// so asserting it proves the fix. (A request with no session never reaches
+// either: it gets 401 before the lookup, issue #435.)
 func TestHandleLambdaHTTPEvent_DecodesBase64FormBody(t *testing.T) {
 	execID := "11111111-1111-1111-1111-111111111111"
 	exec := &config.PurchaseExecution{
@@ -146,15 +146,19 @@ func TestHandleLambdaHTTPEvent_DecodesBase64FormBody(t *testing.T) {
 
 	mockStore := new(mocks.MockConfigStore)
 	mockStore.On("GetExecutionByID", mock.Anything, execID).Return(exec, nil)
+	mockStore.On("GetGlobalConfig", mock.Anything).Return(&config.GlobalConfig{}, nil)
 
 	app := &Application{
-		API: api.NewHandler(api.HandlerConfig{ConfigStore: mockStore}),
+		API: api.NewHandler(api.HandlerConfig{ConfigStore: mockStore, AuthService: sessionOnlyAuth{}}),
 	}
 
 	encodedBody := base64.StdEncoding.EncodeToString([]byte("token=body-token"))
 	request := events.LambdaFunctionURLRequest{
 		RawPath: "/api/purchases/revoke/" + execID,
-		Headers: map[string]string{"content-type": "application/x-www-form-urlencoded"},
+		Headers: map[string]string{
+			"content-type":  "application/x-www-form-urlencoded",
+			"authorization": "Bearer sess-tok",
+		},
 		RequestContext: events.LambdaFunctionURLRequestContext{
 			HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{
 				Method: "POST",
@@ -170,9 +174,9 @@ func TestHandleLambdaHTTPEvent_DecodesBase64FormBody(t *testing.T) {
 	ctx := testutil.TestContext(t)
 	resp, err := app.handleLambdaHTTPEvent(ctx, rawEvent)
 	testutil.AssertNoError(t, err)
-	testutil.AssertEqual(t, 401, resp.StatusCode)
-	testutil.AssertContains(t, resp.Body, "sign in with the account's contact email")
-	testutil.AssertTrue(t, !strings.Contains(resp.Body, "revocation link from the notification email"),
+	testutil.AssertEqual(t, 403, resp.StatusCode)
+	testutil.AssertContains(t, resp.Body, "no per-account contact email")
+	testutil.AssertTrue(t, !strings.Contains(resp.Body, "requires cancel-any"),
 		"token must have been resolved from the decoded body, not left empty")
 
 	mockStore.AssertExpectations(t)
@@ -191,3 +195,22 @@ func TestHandleLambdaEvent_UnknownEventRouteToScheduled(t *testing.T) {
 	// Unknown action → error from ParseScheduledEvent
 	testutil.AssertError(t, err)
 }
+
+// sessionOnlyAuth accepts the bearer "sess-tok" as a signed-in user with no
+// purchase permissions; any other method panics via the nil embedded interface.
+type sessionOnlyAuth struct{ api.AuthServiceInterface }
+
+func (sessionOnlyAuth) ValidateSession(_ context.Context, token string) (*api.Session, error) {
+	if token != "sess-tok" {
+		return nil, errors.New("invalid session")
+	}
+	return &api.Session{UserID: "user-1", Email: "user@example.com"}, nil
+}
+
+func (sessionOnlyAuth) HasPermissionAPI(context.Context, string, string, string) (bool, error) {
+	return false, nil
+}
+
+func (sessionOnlyAuth) GetAllowedAccountsAPI(context.Context, string) ([]string, error) {
+	return nil, nil
+}