From 5f0ea598ecc3172f19871f70a6881b11c9df9030 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:54:17 +0000 Subject: [PATCH] fix(server): stop reporting fabricated success on three operator paths - analytics_refresh reports status "skipped" when the analytics store is absent and no longer returns partition counters it never writes; cleanup_expired_records drops the always-0 sessions_deleted counter. - The HTTP transport reads request bodies through http.MaxBytesReader at the API router's own 1 MB cap: an oversize body is a 413 and a body read error (client disconnect mid-upload) is a 400, instead of a truncated or empty body being routed. - resolveAccountID returns an error instead of "unknown", and the RI exchange reshape run aborts before touching AWS or the store when no account ID is resolved, so no audit row carries a fabricated account. The fourth finding (empty-name guard in force-delete-owned-ecr-repo.sh) was already fixed on main by #489. Closes #326 Co-Authored-By: Claude Opus 5.5 --- internal/server/app_test.go | 6 +- internal/server/handler.go | 16 ++- internal/server/handler_coverage_test.go | 24 +++- internal/server/handler_ladder.go | 5 +- internal/server/handler_ri_exchange.go | 40 ++++-- .../handler_ri_exchange_account_test.go | 101 +++++++++++++++ internal/server/handler_ri_exchange_test.go | 2 + internal/server/http.go | 41 +++--- internal/server/http_body.go | 47 +++++++ internal/server/http_body_test.go | 121 ++++++++++++++++++ internal/server/http_test.go | 3 +- 11 files changed, 364 insertions(+), 42 deletions(-) create mode 100644 internal/server/handler_ri_exchange_account_test.go create mode 100644 internal/server/http_body.go create mode 100644 internal/server/http_body_test.go diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 0eabfbe22..285eb867e 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -78,7 +78,8 @@ func TestHttpToLambdaRequest_XForwardedFor(t *testing.T) { req.Header.Set("X-Forwarded-For", "1.2.3.4, 5.6.7.8") req.Header.Set("User-Agent", "TestAgent/1.0") - lambdaReq := httpToLambdaRequest(req) + lambdaReq, err := httpToLambdaRequest(httptest.NewRecorder(), req) + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "5.6.7.8", lambdaReq.RequestContext.HTTP.SourceIP) testutil.AssertEqual(t, "TestAgent/1.0", lambdaReq.RequestContext.HTTP.UserAgent) @@ -88,7 +89,8 @@ func TestHttpToLambdaRequest_NilBody(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), "GET", "/api/test", nil) req.Body = nil - lambdaReq := httpToLambdaRequest(req) + lambdaReq, err := httpToLambdaRequest(httptest.NewRecorder(), req) + testutil.AssertNoError(t, err) testutil.AssertEqual(t, "", lambdaReq.Body) } diff --git a/internal/server/handler.go b/internal/server/handler.go index 1d0d3ee5a..a24dc176d 100644 --- a/internal/server/handler.go +++ b/internal/server/handler.go @@ -287,8 +287,9 @@ func (app *Application) handleSendNotifications(ctx context.Context) (*purchase. func (app *Application) handleCleanupExpiredRecords(ctx context.Context) (map[string]int64, error) { log.Println("Cleaning up expired records...") + // No sessions_deleted counter: CleanupExpiredSessions reports no count, + // so it could only ever be a fabricated 0 (#326). result := map[string]int64{ - "sessions_deleted": 0, "executions_deleted": 0, } @@ -316,7 +317,7 @@ func (app *Application) handleCleanupExpiredRecords(ctx context.Context) (map[st } } - log.Printf("Cleanup complete: %d sessions, %d executions deleted", result["sessions_deleted"], result["executions_deleted"]) + log.Printf("Cleanup complete: %d executions deleted", result["executions_deleted"]) return result, nil } @@ -385,11 +386,11 @@ func (app *Application) handleFinalizeRevocations(ctx context.Context) (*purchas func (app *Application) handleRefreshAnalytics(ctx context.Context) (map[string]any, error) { log.Println("Refreshing analytics...") + // No partition counters: this task never creates or drops partitions + // (handleCollectAnalytics does), so they could only be fabricated zeros. result := map[string]any{ - "status": "success", - "views_refreshed": 0, - "partitions_created": 0, - "partitions_dropped": 0, + "status": "success", + "views_refreshed": 0, } // Refresh materialized views if analytics store is available. @@ -406,6 +407,9 @@ func (app *Application) handleRefreshAnalytics(ctx context.Context) (map[string] log.Println("Materialized views refreshed successfully") } } else { + // Nothing ran, so this is not a success; handleCollectAnalytics + // reports the same condition as "skipped" (#326). + result["status"] = "skipped" log.Println("Analytics store not available, skipping materialized view refresh") } diff --git a/internal/server/handler_coverage_test.go b/internal/server/handler_coverage_test.go index 7e900c1d2..8bf1446b5 100644 --- a/internal/server/handler_coverage_test.go +++ b/internal/server/handler_coverage_test.go @@ -31,8 +31,12 @@ func TestHandleCleanupExpiredRecords_WithAuthAndConfig(t *testing.T) { result, err := app.handleCleanupExpiredRecords(ctx) testutil.AssertNoError(t, err) testutil.AssertTrue(t, result != nil, "expected non-nil result map") + // CleanupExpiredSessions reports no count, so the result must not carry a + // sessions_deleted counter that is always 0 (#326, A06-018). _, hasSessionsKey := result["sessions_deleted"] - testutil.AssertTrue(t, hasSessionsKey, "expected sessions_deleted key") + testutil.AssertTrue(t, !hasSessionsKey, "sessions_deleted is never populated and must not be reported") + _, hasExecutionsKey := result["executions_deleted"] + testutil.AssertTrue(t, hasExecutionsKey, "expected executions_deleted key") } func TestHandleCleanupExpiredRecords_NilAuthAndConfig(t *testing.T) { @@ -97,6 +101,19 @@ func TestHandleRefreshAnalytics_Success(t *testing.T) { result, err := app.handleRefreshAnalytics(ctx) testutil.AssertNoError(t, err) testutil.AssertEqual(t, "success", result["status"]) + testutil.AssertEqual(t, 1, result["views_refreshed"]) + assertNoPartitionCounters(t, result) +} + +// assertNoPartitionCounters checks that handleRefreshAnalytics does not report +// partition counters: it never creates or drops partitions, so they could only +// ever be fabricated zeros (#326, A06-018). +func assertNoPartitionCounters(t *testing.T, result map[string]any) { + t.Helper() + for _, key := range []string{"partitions_created", "partitions_dropped"} { + _, ok := result[key] + testutil.AssertTrue(t, !ok, key+" is never populated and must not be reported") + } } func TestHandleRefreshAnalytics_RefreshError(t *testing.T) { @@ -117,6 +134,11 @@ func TestHandleRefreshAnalytics_NilAnalytics(t *testing.T) { result, err := app.handleRefreshAnalytics(ctx) testutil.AssertNoError(t, err) testutil.AssertTrue(t, result != nil, "expected non-nil result") + // Nothing was refreshed, so the run must not report success (#326, + // A06-018); handleCollectAnalytics reports "skipped" for the same case. + testutil.AssertEqual(t, "skipped", result["status"]) + testutil.AssertEqual(t, 0, result["views_refreshed"]) + assertNoPartitionCounters(t, result) } // ----- configExchangeStoreAdapter — CompleteRIExchange / FailRIExchange ----- diff --git a/internal/server/handler_ladder.go b/internal/server/handler_ladder.go index 163381334..6cc1897cb 100644 --- a/internal/server/handler_ladder.go +++ b/internal/server/handler_ladder.go @@ -142,9 +142,8 @@ func (app *Application) resolveLadderAccount(ctx context.Context) (accountID, re } // defaultLadderAccountResolver resolves the Lambda's own AWS account ID via STS -// STRICTLY: unlike resolveAccountID (which returns the "unknown" sentinel for -// audit-only use in the RI-exchange path), this returns an error when the -// account cannot be determined. The ladder path uses the account ID to SCOPE +// STRICTLY, like resolveAccountID in the RI-exchange path: this returns an +// error when the account cannot be determined. The ladder path uses the account ID to SCOPE // which configs run, so a fabricated/"unknown" value is unsafe: it would skip // every config as multi-account and report a false success. func defaultLadderAccountResolver(ctx context.Context) (accountID, region string, err error) { diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index e6b2c0b8e..a911dda37 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -2,11 +2,11 @@ package server import ( "context" + "errors" "fmt" "log" "time" - "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/sts" @@ -52,6 +52,10 @@ func (app *Application) handleRIExchangeReshape(ctx context.Context) (*exchange. if err != nil { return nil, fmt.Errorf("failed to load AWS config: %w", err) } + accountID, err := resolveAccountID(ctx, sts.NewFromConfig(awsCfg)) + if err != nil { + return nil, fmt.Errorf("RI exchange reshape aborted: %w", err) + } ec2Client := awsprovider.NewEC2ClientDirect(awsCfg) clients := riExchangeClients{ @@ -64,7 +68,7 @@ func (app *Application) handleRIExchangeReshape(ctx context.Context) (*exchange. }, exchangeClient: exchange.NewExchangeClient(awsCfg), lookupOffering: newConvertibleOfferingLookup(ec2Client), - accountID: resolveAccountID(ctx, awsCfg), + accountID: accountID, region: awsCfg.Region, } @@ -127,6 +131,11 @@ func newConvertibleOfferingLookup(finder convertibleOfferingFinder) exchange.Loo // executeRIExchangeReshape contains the core exchange logic, separated from AWS // client construction for testability. func (app *Application) executeRIExchangeReshape(ctx context.Context, cfg *config.GlobalConfig, clients riExchangeClients) (*exchange.AutoExchangeResult, error) { + // Every record the run creates carries this account ID for the audit + // trail, so a run without one stops before touching AWS or the store. + if clients.accountID == "" { + return nil, errors.New("no AWS account ID resolved for the RI exchange audit trail") + } instances, err := clients.listConvertibleRIs(ctx) if err != nil { return nil, fmt.Errorf("failed to list convertible RIs: %w", err) @@ -174,19 +183,26 @@ func (app *Application) executeRIExchangeReshape(ctx context.Context, cfg *confi return result, nil } -// resolveAccountID fetches the AWS account ID via STS. Returns "unknown" on failure -// since account_id is stored for audit trail only and is not used to scope queries. -func resolveAccountID(ctx context.Context, awsCfg aws.Config) string { - stsClient := sts.NewFromConfig(awsCfg) - identity, err := stsClient.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{}) +// callerIdentityGetter is the slice of the STS client resolveAccountID needs. +// Satisfied by *sts.Client. +type callerIdentityGetter interface { + GetCallerIdentity(ctx context.Context, params *sts.GetCallerIdentityInput, optFns ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) +} + +// resolveAccountID fetches the caller's AWS account ID via STS and fails when +// it cannot be determined. The ID is written onto every RI exchange record the +// run creates, so a placeholder such as "unknown" would put a fabricated +// account on the audit trail of a money action (#326); the run is aborted +// instead, as defaultLadderAccountResolver does. +func resolveAccountID(ctx context.Context, client callerIdentityGetter) (string, error) { + identity, err := client.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{}) if err != nil { - log.Printf("Warning: failed to get AWS account ID via STS: %v (using 'unknown')", err) - return "unknown" + return "", fmt.Errorf("resolve caller AWS account via STS: %w", err) } - if identity.Account != nil { - return *identity.Account + if identity == nil || identity.Account == nil || *identity.Account == "" { + return "", errors.New("STS GetCallerIdentity returned no account ID") } - return "unknown" + return *identity.Account, nil } // sendExchangeNotification sends email notifications based on the exchange result. diff --git a/internal/server/handler_ri_exchange_account_test.go b/internal/server/handler_ri_exchange_account_test.go new file mode 100644 index 000000000..22d354416 --- /dev/null +++ b/internal/server/handler_ri_exchange_account_test.go @@ -0,0 +1,101 @@ +package server + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/sts" + + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" + ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" +) + +// fakeCallerIdentity is a canned STS GetCallerIdentity response. +type fakeCallerIdentity struct { + out *sts.GetCallerIdentityOutput + err error +} + +func (f fakeCallerIdentity) GetCallerIdentity(context.Context, *sts.GetCallerIdentityInput, ...func(*sts.Options)) (*sts.GetCallerIdentityOutput, error) { + return f.out, f.err +} + +// resolveAccountID must fail instead of inventing an "unknown" account ID, +// which used to be written onto every RI exchange audit row the run created +// (#326, A06-025). +func TestResolveAccountID_RefusesToFabricateAnAccount(t *testing.T) { + cases := map[string]fakeCallerIdentity{ + "STS error": {err: errors.New("ExpiredToken: the security token included in the request is expired")}, + "nil output": {}, + "nil account": {out: &sts.GetCallerIdentityOutput{}}, + "empty account": {out: &sts.GetCallerIdentityOutput{Account: aws.String("")}}, + } + for name, fake := range cases { + t.Run(name, func(t *testing.T) { + accountID, err := resolveAccountID(context.Background(), fake) + + testutil.AssertError(t, err) + testutil.AssertEqual(t, "", accountID) + }) + } +} + +func TestResolveAccountID_ReturnsTheCallerAccount(t *testing.T) { + fake := fakeCallerIdentity{out: &sts.GetCallerIdentityOutput{Account: aws.String("123456789012")}} + + accountID, err := resolveAccountID(context.Background(), fake) + + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, "123456789012", accountID) +} + +// A reshape run without a resolved account ID must stop before it lists RIs +// or writes any exchange record, so no audit row carries a blank account. +func TestExecuteRIExchangeReshape_RefusesEmptyAccountID(t *testing.T) { + ctx := testutil.TestContext(t) + writes := 0 + store := &mockConfigStoreForExchange{ + saveRIExchangeRecordFunc: func(context.Context, *config.RIExchangeRecord) error { + writes++ + return nil + }, + reserveRIExchangeFunc: func(context.Context, *config.RIExchangeRecord, string, string) (string, error) { + writes++ + return "", errors.New("unexpected reservation") + }, + } + app := &Application{Config: store} + cfg := &config.GlobalConfig{ + RIExchangeEnabled: true, + RIExchangeMode: "auto", + RIExchangeUtilizationThreshold: 95.0, + RIExchangeLookbackDays: 30, + } + + listed := false + clients := riExchangeClients{ + listConvertibleRIs: func(context.Context) ([]ec2svc.ConvertibleRI, error) { + listed = true + return nil, nil + }, + getRIUtilization: func(context.Context, int) ([]recommendations.RIUtilization, error) { + return nil, nil + }, + exchangeClient: &mockExchangeClient{}, + accountID: "", + region: "us-east-1", + } + + result, err := app.executeRIExchangeReshape(ctx, cfg, clients) + + testutil.AssertError(t, err) + testutil.AssertTrue(t, strings.Contains(err.Error(), "account ID"), "expected an account ID error, got: "+err.Error()) + testutil.AssertTrue(t, result == nil, "expected no result") + testutil.AssertTrue(t, !listed, "convertible RIs must not be listed without an account ID") + testutil.AssertEqual(t, 0, writes) +} diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 6d4d998bc..55f1085f4 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -489,6 +489,7 @@ func TestExecuteRIExchangeReshape_ListRIsFailure(t *testing.T) { listConvertibleRIs: func(ctx context.Context) ([]ec2svc.ConvertibleRI, error) { return nil, errors.New("EC2 API throttled") }, + accountID: "123456789", } _, err := app.executeRIExchangeReshape(ctx, cfg, clients) @@ -517,6 +518,7 @@ func TestExecuteRIExchangeReshape_UtilizationFailure(t *testing.T) { getRIUtilization: func(ctx context.Context, lookbackDays int) ([]recommendations.RIUtilization, error) { return nil, errors.New("Cost Explorer unavailable") }, + accountID: "123456789", } _, err := app.executeRIExchangeReshape(ctx, cfg, clients) diff --git a/internal/server/http.go b/internal/server/http.go index 8bbf2261c..c562e5cea 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "log" "net/http" "os/signal" @@ -231,7 +230,11 @@ func (app *Application) handleOIDCHTTP(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) defer cancel() - lambdaReq := httpToLambdaRequest(r) + lambdaReq, err := httpToLambdaRequest(w, r) + if err != nil { + writeBodyReadError(w, err) + return + } resp, handled := app.API.HandleOIDC(ctx, lambdaReq) if !handled { // Path matched /oidc/ prefix but is not a recognized OIDC endpoint. @@ -279,7 +282,11 @@ func (app *Application) handleHTTPRequest(w http.ResponseWriter, r *http.Request } // Convert HTTP request to Lambda Function URL request format - lambdaReq := httpToLambdaRequest(r) + lambdaReq, err := httpToLambdaRequest(w, r) + if err != nil { + writeBodyReadError(w, err) + return + } // Call the API handler lambdaResp, err := app.API.HandleRequest(ctx, lambdaReq) @@ -303,7 +310,12 @@ func (app *Application) handleVersion(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 5*time.Second) defer cancel() - lambdaResp, err := app.API.HandleRequest(ctx, httpToLambdaRequest(r)) + lambdaReq, err := httpToLambdaRequest(w, r) + if err != nil { + writeBodyReadError(w, err) + return + } + lambdaResp, err := app.API.HandleRequest(ctx, lambdaReq) if err != nil { log.Printf("Version handler error: %v", err) http.Error(w, "Internal server error", http.StatusInternalServerError) @@ -393,18 +405,13 @@ func (app *Application) handleScheduledHTTP(w http.ResponseWriter, r *http.Reque } } -// httpToLambdaRequest converts a standard HTTP request to Lambda Function URL request format. -func httpToLambdaRequest(r *http.Request) *events.LambdaFunctionURLRequest { - // Read body with size limit to prevent memory exhaustion - body := "" - if r.Body != nil { - defer r.Body.Close() - const maxBodySize = 10 << 20 // 10 MB - limited := io.LimitReader(r.Body, maxBodySize) - bodyBytes, err := io.ReadAll(limited) - if err == nil && len(bodyBytes) > 0 { - body = string(bodyBytes) - } +// httpToLambdaRequest converts a standard HTTP request to Lambda Function URL +// request format. It fails when the body cannot be read whole; answer that +// with writeBodyReadError. +func httpToLambdaRequest(w http.ResponseWriter, r *http.Request) (*events.LambdaFunctionURLRequest, error) { + body, err := readRequestBody(w, r) + if err != nil { + return nil, err } // Convert headers - lowercase all keys to match Lambda Function URL behavior. @@ -451,7 +458,7 @@ func httpToLambdaRequest(r *http.Request) *events.LambdaFunctionURLRequest { QueryStringParameters: queryParams, Body: body, IsBase64Encoded: false, - } + }, nil } // safeHeaderNames is a whitelist of headers that are safe to pass through from Lambda responses. diff --git a/internal/server/http_body.go b/internal/server/http_body.go new file mode 100644 index 000000000..db19fba4a --- /dev/null +++ b/internal/server/http_body.go @@ -0,0 +1,47 @@ +package server + +import ( + "encoding/json" + "errors" + "io" + "log" + "net/http" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/api" +) + +// readRequestBody reads the whole request body, capped at the API router's +// own limit (api.MaxRequestBodySize) so the transport never buffers a body +// the router would reject anyway. http.MaxBytesReader makes an oversize body +// an error rather than truncating it, and a read error (a client that +// disconnects mid-upload) is returned rather than read as an empty body +// (#326). +func readRequestBody(w http.ResponseWriter, r *http.Request) (string, error) { + if r.Body == nil { + return "", nil + } + defer r.Body.Close() + bodyBytes, err := io.ReadAll(http.MaxBytesReader(w, r.Body, api.MaxRequestBodySize)) + if err != nil { + return "", err + } + return string(bodyBytes), nil +} + +// writeBodyReadError answers a failed body read: 413 for a body over the cap, +// with the same JSON error the API router returns for one, and 400 for any +// other read failure. +func writeBodyReadError(w http.ResponseWriter, err error) { + status, msg := http.StatusBadRequest, "Failed to read request body" + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + status, msg = http.StatusRequestEntityTooLarge, "Request body too large" + } else { + log.Printf("Failed to read request body: %v", err) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + if encErr := json.NewEncoder(w).Encode(map[string]string{"error": msg}); encErr != nil { + log.Printf("Failed to encode body-read error response: %v", encErr) + } +} diff --git a/internal/server/http_body_test.go b/internal/server/http_body_test.go new file mode 100644 index 000000000..25aee6152 --- /dev/null +++ b/internal/server/http_body_test.go @@ -0,0 +1,121 @@ +package server + +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/api" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil" +) + +// requestBodyLimit is the cap the HTTP transport applies to request bodies: +// the API router's own limit, so the two cannot disagree. +const requestBodyLimit = api.MaxRequestBodySize + +// failingBody yields some bytes and then a read error, like a client that +// disconnects mid-upload. +type failingBody struct { + sent bool +} + +func (b *failingBody) Read(p []byte) (int, error) { + if !b.sent { + b.sent = true + return copy(p, `{"partial":`), nil + } + return 0, errors.New("connection reset by peer") +} + +func (b *failingBody) Close() error { return nil } + +// bodyHandlers lists every HTTP handler that converts the request through +// httpToLambdaRequest, so each one is checked for the same body handling. +func bodyHandlers(app *Application) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + "handleHTTPRequest": app.handleHTTPRequest, + "handleVersion": app.handleVersion, + "handleOIDCHTTP": app.handleOIDCHTTP, + } +} + +// An oversize body must be rejected with 413, not truncated at the cap and +// handed to the API router cut mid-JSON (#326, A06-024). +func TestHTTPHandlers_RejectOversizeBodyWith413(t *testing.T) { + app := &Application{API: api.NewHandler(api.HandlerConfig{})} + for name, h := range bodyHandlers(app) { + t.Run(name, func(t *testing.T) { + body := bytes.Repeat([]byte("a"), requestBodyLimit+1) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/recommendations", bytes.NewReader(body)) + rec := httptest.NewRecorder() + + h(rec, req) + + testutil.AssertEqual(t, http.StatusRequestEntityTooLarge, rec.Code) + testutil.AssertEqual(t, "application/json", rec.Header().Get("Content-Type")) + testutil.AssertTrue(t, strings.Contains(rec.Body.String(), `"error":"Request body too large"`), + "expected the router's JSON 413 body, got: "+rec.Body.String()) + }) + } +} + +// A body read error must be answered with 400, not read as an empty body and +// routed (#326, A06-024). +func TestHTTPHandlers_RejectBodyReadErrorWith400(t *testing.T) { + app := &Application{API: api.NewHandler(api.HandlerConfig{})} + for name, h := range bodyHandlers(app) { + t.Run(name, func(t *testing.T) { + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/recommendations", nil) + req.Body = &failingBody{} + rec := httptest.NewRecorder() + + h(rec, req) + + testutil.AssertEqual(t, http.StatusBadRequest, rec.Code) + testutil.AssertTrue(t, strings.Contains(rec.Body.String(), "Failed to read request body"), + "expected the body-read error message, got: "+rec.Body.String()) + }) + } +} + +// A body of exactly the cap is still accepted whole. +func TestHttpToLambdaRequest_BodyAtLimitIsKeptWhole(t *testing.T) { + body := bytes.Repeat([]byte("a"), requestBodyLimit) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/test", bytes.NewReader(body)) + + lambdaReq, err := httpToLambdaRequest(httptest.NewRecorder(), req) + + testutil.AssertNoError(t, err) + testutil.AssertEqual(t, requestBodyLimit, len(lambdaReq.Body)) +} + +// One byte over the cap is an error carrying *http.MaxBytesError, not a body +// silently truncated to the cap. +func TestHttpToLambdaRequest_OversizeBodyIsAnError(t *testing.T) { + body := bytes.Repeat([]byte("a"), requestBodyLimit+1) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/test", bytes.NewReader(body)) + + lambdaReq, err := httpToLambdaRequest(httptest.NewRecorder(), req) + + testutil.AssertError(t, err) + var tooLarge *http.MaxBytesError + testutil.AssertTrue(t, errors.As(err, &tooLarge), "expected *http.MaxBytesError, got: "+err.Error()) + testutil.AssertTrue(t, lambdaReq == nil, "expected no request on an oversize body") +} + +// A read error is returned, not swallowed into an empty body. +func TestHttpToLambdaRequest_ReadErrorIsReturned(t *testing.T) { + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/api/test", nil) + req.Body = &failingBody{} + + lambdaReq, err := httpToLambdaRequest(httptest.NewRecorder(), req) + + testutil.AssertError(t, err) + testutil.AssertTrue(t, !errors.Is(err, io.EOF), "a read error must not look like a clean EOF") + testutil.AssertTrue(t, lambdaReq == nil, "expected no request on a read error") +} diff --git a/internal/server/http_test.go b/internal/server/http_test.go index b24d32e28..2a2dafe2f 100644 --- a/internal/server/http_test.go +++ b/internal/server/http_test.go @@ -85,7 +85,8 @@ func TestHttpToLambdaRequest(t *testing.T) { } // Convert to Lambda request - lambdaReq := httpToLambdaRequest(req) + lambdaReq, err := httpToLambdaRequest(httptest.NewRecorder(), req) + testutil.AssertNoError(t, err) // Verify conversion testutil.AssertEqual(t, tt.expectedMethod, lambdaReq.RequestContext.HTTP.Method)