fix(costmodel): return after writing 400 for invalid window and aggregate

ComputeAllocationHandler and ComputeAllocationHandlerSummary wrote a 400
response for an invalid or missing 'window' parameter (and an invalid
'aggregate' parameter) but did not return, so request processing
continued with a zero-value Window. In the summary handler this
dereferenced the window's nil start time and panicked; in the allocation
handler it proceeded into QueryAllocation with an invalid window after
the error response had already been written.

Add the missing return statements, matching the error handling in every
other branch of these handlers, and add regression tests that fail with
a panic if the handlers keep executing past the 400.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>

pkg/costmodel/aggregation.go

@@ -212,6 +212,7 @@ func (a *Accesses) ComputeAllocationHandlerSummary(w http.ResponseWriter, r *htt
 	window, err := opencost.ParseWindowWithOffset(qp.Get("window", ""), env.GetParsedUTCOffset())
 	if err != nil {
 		http.Error(w, fmt.Sprintf("Invalid 'window' parameter: %s", err), http.StatusBadRequest)
+		return
 	}
 
 	// Step is an optional parameter that defines the duration per-set, i.e.
@@ -225,6 +226,7 @@ func (a *Accesses) ComputeAllocationHandlerSummary(w http.ResponseWriter, r *htt
 	aggregateBy, err := ParseAggregationProperties(aggregations)
 	if err != nil {
 		http.Error(w, fmt.Sprintf("Invalid 'aggregate' parameter: %s", err), http.StatusBadRequest)
+		return
 	}
 
 	// Accumulate is an optional parameter that accepts bool-style values (e.g.
@@ -337,6 +339,7 @@ func (a *Accesses) ComputeAllocationHandler(w http.ResponseWriter, r *http.Reque
 	window, err := opencost.ParseWindowWithOffset(qp.Get("window", ""), env.GetParsedUTCOffset())
 	if err != nil {
 		http.Error(w, fmt.Sprintf("Invalid 'window' parameter: %s", err), http.StatusBadRequest)
+		return
 	}
 
 	// Step is an optional parameter that defines the duration per-set, i.e.
@@ -350,6 +353,7 @@ func (a *Accesses) ComputeAllocationHandler(w http.ResponseWriter, r *http.Reque
 	aggregateBy, err := ParseAggregationProperties(aggregations)
 	if err != nil {
 		http.Error(w, fmt.Sprintf("Invalid 'aggregate' parameter: %s", err), http.StatusBadRequest)
+		return
 	}
 
 	// IncludeIdle, if true, uses Asset data to incorporate Idle Allocation

pkg/costmodel/aggregation_test.go

@@ -1,10 +1,14 @@
 package costmodel
 
 import (
+	"net/http"
+	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/julienschmidt/httprouter"
 	"github.com/opencost/opencost/core/pkg/opencost"
 	"github.com/opencost/opencost/core/pkg/util/httputil"
 )
@@ -445,3 +449,65 @@ func TestTrimAllocationSetRangeToRequestWindow(t *testing.T) {
 		t.Fatalf("expected FromStore to be preserved")
 	}
 }
+
+// invalidWindowRequest builds a GET request to the given path with an
+// unparseable window parameter.
+func invalidWindowRequest(path string) *http.Request {
+	r, _ := http.NewRequest(http.MethodGet, path+"?window=notawindow", nil)
+	return r
+}
+
+// The handlers must return immediately after writing the 400 for an invalid
+// window. Before the fix, execution continued with a zero-value Window: the
+// nil Model here would have caused a panic instead of a clean 400.
+func TestComputeAllocationHandler_InvalidWindow_Returns400(t *testing.T) {
+	a := &Accesses{}
+
+	w := httptest.NewRecorder()
+	a.ComputeAllocationHandler(w, invalidWindowRequest("/allocation"), httprouter.Params{})
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected status %d, got %d", http.StatusBadRequest, w.Code)
+	}
+	if !strings.Contains(w.Body.String(), "Invalid 'window' parameter") {
+		t.Fatalf("expected invalid window error in body, got: %q", w.Body.String())
+	}
+}
+
+func TestComputeAllocationHandler_MissingWindow_Returns400(t *testing.T) {
+	a := &Accesses{}
+
+	r, _ := http.NewRequest(http.MethodGet, "/allocation", nil)
+	w := httptest.NewRecorder()
+	a.ComputeAllocationHandler(w, r, httprouter.Params{})
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected status %d, got %d", http.StatusBadRequest, w.Code)
+	}
+}
+
+func TestComputeAllocationHandlerSummary_InvalidWindow_Returns400(t *testing.T) {
+	a := &Accesses{}
+
+	w := httptest.NewRecorder()
+	a.ComputeAllocationHandlerSummary(w, invalidWindowRequest("/allocation/summary"), httprouter.Params{})
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected status %d, got %d", http.StatusBadRequest, w.Code)
+	}
+	if !strings.Contains(w.Body.String(), "Invalid 'window' parameter") {
+		t.Fatalf("expected invalid window error in body, got: %q", w.Body.String())
+	}
+}
+
+func TestComputeAllocationHandlerSummary_MissingWindow_Returns400(t *testing.T) {
+	a := &Accesses{}
+
+	r, _ := http.NewRequest(http.MethodGet, "/allocation/summary", nil)
+	w := httptest.NewRecorder()
+	a.ComputeAllocationHandlerSummary(w, r, httprouter.Params{})
+
+	if w.Code != http.StatusBadRequest {
+		t.Fatalf("expected status %d, got %d", http.StatusBadRequest, w.Code)
+	}
+}