Fix handler early returns on invalid window parameter (#3936)

Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>

docs/swagger.json

@@ -78,6 +78,12 @@
                 }
               }
             }
+          },
+          "400": {
+            "description": "Bad request — missing or invalid parameters (e.g. `window`, `step`, `accumulateBy`)"
+          },
+          "500": {
+            "description": "Internal server error — the allocation query failed"
           }
         }
       }
@@ -106,6 +112,12 @@
                 }
               }
             }
+          },
+          "400": {
+            "description": "Bad request — missing or invalid `window` parameter"
+          },
+          "500": {
+            "description": "Internal server error — computing or filtering assets failed"
           }
         }
       }
@@ -113,7 +125,7 @@
     "/cloudCost": {
       "get": {
         "summary": "query for cloud provider billing data",
-        "description": "Retrieves non-K8s cloud provider costs via cloud integration.",
+        "description": "Retrieves non-K8s cloud provider costs via cloud integration. Requires `CLOUD_COST_ENABLED=true`; when disabled this route is not registered and requests return 404.",
         "parameters": [
           {
             "name": "window",
@@ -134,6 +146,12 @@
                 }
               }
             }
+          },
+          "400": {
+            "description": "Bad request — missing or invalid parameters"
+          },
+          "500": {
+            "description": "Internal server error — the cloud cost query failed"
           }
         }
       }
@@ -215,6 +233,9 @@
           "400": {
             "description": "Bad request — missing or invalid parameters"
           },
+          "500": {
+            "description": "Internal server error — the inference cost query failed"
+          },
           "501": {
             "description": "Inference cost tracking is not enabled (`INFERENCE_COST_ENABLED` is not set to `true`)"
           }
@@ -296,6 +317,9 @@
           "400": {
             "description": "Bad request — missing or invalid parameters (including missing `accumulate`)"
           },
+          "500": {
+            "description": "Internal server error — the inference cost query failed"
+          },
           "501": {
             "description": "Inference cost tracking is not enabled (`INFERENCE_COST_ENABLED` is not set to `true`)"
           }
@@ -362,17 +386,7 @@
             "type": "number"
           },
           "window": {
-            "type": "object",
-            "properties": {
-              "start": {
-                "type": "string",
-                "format": "date-time"
-              },
-              "end": {
-                "type": "string",
-                "format": "date-time"
-              }
-            }
+            "$ref": "#/components/schemas/Window"
           },
           "properties": {
             "type": "object",
@@ -418,17 +432,7 @@
             "type": "string"
           },
           "window": {
-            "type": "object",
-            "properties": {
-              "start": {
-                "type": "string",
-                "format": "date-time"
-              },
-              "end": {
-                "type": "string",
-                "format": "date-time"
-              }
-            }
+            "$ref": "#/components/schemas/Window"
           }
         }
       },

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)
+	}
+}