diff --git a/internal/api/coverage_extras_test.go b/internal/api/coverage_extras_test.go index f12a4c784..2373d40f4 100644 --- a/internal/api/coverage_extras_test.go +++ b/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) { diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index dd4a79b7a..8ffe19031 100644 --- a/internal/api/handler_purchases.go +++ b/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 diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index b16c30889..cc965b3f5 100644 --- a/internal/api/handler_purchases_test.go +++ b/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) diff --git a/internal/api/handler_purchases_token_oracle_test.go b/internal/api/handler_purchases_token_oracle_test.go new file mode 100644 index 000000000..1cd37bdcb --- /dev/null +++ b/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) + }) + } + } + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index aae2eeb4f..6ef383d41 100644 --- a/internal/api/openapi.yaml +++ b/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: diff --git a/internal/server/app_rate_limiter_integration_test.go b/internal/server/app_rate_limiter_integration_test.go index 1943170f3..ab7e4dc67 100644 --- a/internal/server/app_rate_limiter_integration_test.go +++ b/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 } diff --git a/internal/server/lambda_coverage_test.go b/internal/server/lambda_coverage_test.go index 2d172c1e3..4fbdb11d3 100644 --- a/internal/server/lambda_coverage_test.go +++ b/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 +}