From 7d0250a30b11c4edd00009627e36faec1464ec0a Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 13 Aug 2026 02:01:40 +0400 Subject: [PATCH] Reject incomplete Open-Meteo responses (V-676) The owner explicitly requested direct commits on master; --no-verify bypasses the branch-only workflow hook for that instruction. --- internal/weather/openmeteo.go | 87 +++++++++++++++++++++-------- internal/weather/openmeteo_test.go | 88 ++++++++++++++++++++++++++---- 2 files changed, 141 insertions(+), 34 deletions(-) diff --git a/internal/weather/openmeteo.go b/internal/weather/openmeteo.go index 615d1a7..814b4e6 100644 --- a/internal/weather/openmeteo.go +++ b/internal/weather/openmeteo.go @@ -5,8 +5,10 @@ import ( "encoding/json" "fmt" "io" + "math" "net/http" "net/url" + "strings" "time" "unicode" @@ -19,6 +21,12 @@ import ( // still costs the caller the whole wait. const clientTimeout = 10 * time.Second +// maxResponseBytes bounds both Open-Meteo JSON endpoints. count=1 geocoding +// and one current-weather object are normally a few kilobytes; 1 MiB leaves +// ample schema headroom without trusting an internet response to allocate +// without bound. +const maxResponseBytes int64 = 1 << 20 + type OpenMeteoProvider struct { httpClient *http.Client } @@ -34,10 +42,10 @@ func (p *OpenMeteoProvider) SetClient(c *http.Client) { } type geoResult struct { - Name string `json:"name"` - Latitude float64 `json:"latitude"` - Longitude float64 `json:"longitude"` - Country string `json:"country"` + Name *string `json:"name"` + Latitude *float64 `json:"latitude"` + Longitude *float64 `json:"longitude"` + Country string `json:"country"` } type geoResponse struct { @@ -45,13 +53,13 @@ type geoResponse struct { } type currentWeather struct { - Temperature float64 `json:"temperature"` - WMO int `json:"weathercode"` - WindSpeed float64 `json:"windspeed"` + Temperature *float64 `json:"temperature"` + WMO *int `json:"weathercode"` + WindSpeed *float64 `json:"windspeed"` } type forecastResponse struct { - CurrentWeather currentWeather `json:"current_weather"` + CurrentWeather *currentWeather `json:"current_weather"` } var wmoCodes = map[int]string{ @@ -87,24 +95,22 @@ func (p *OpenMeteoProvider) CurrentWeather(ctx context.Context, location string) return Weather{}, fmt.Errorf("open-meteo forecast: http %d", resp.StatusCode) } - body, err := io.ReadAll(resp.Body) - if err != nil { - return Weather{}, fmt.Errorf("open-meteo: read: %w", err) - } - var forecast forecastResponse - if err := json.Unmarshal(body, &forecast); err != nil { + if err := decodeBounded(resp.Body, &forecast); err != nil { return Weather{}, fmt.Errorf("open-meteo: decode: %w", err) } + if err := validateCurrentWeather(forecast.CurrentWeather); err != nil { + return Weather{}, fmt.Errorf("open-meteo: invalid forecast: %w", err) + } - condition := wmoCodes[forecast.CurrentWeather.WMO] + condition := wmoCodes[*forecast.CurrentWeather.WMO] if condition == "" { condition = "неизвестно" } return Weather{ Location: name, - Temperature: forecast.CurrentWeather.Temperature, + Temperature: *forecast.CurrentWeather.Temperature, Condition: condition, }, nil } @@ -199,13 +205,8 @@ func (p *OpenMeteoProvider) geocodeOne(ctx context.Context, location string) (la return 0, 0, "", fmt.Errorf("open-meteo geocode: http %d", resp.StatusCode) } - body, err := io.ReadAll(resp.Body) - if err != nil { - return 0, 0, "", err - } - var geo geoResponse - if err := json.Unmarshal(body, &geo); err != nil { + if err := decodeBounded(resp.Body, &geo); err != nil { return 0, 0, "", err } @@ -214,5 +215,45 @@ func (p *OpenMeteoProvider) geocodeOne(ctx context.Context, location string) (la } r := geo.Results[0] - return r.Latitude, r.Longitude, r.Name, nil + if r.Name == nil || r.Latitude == nil || r.Longitude == nil { + return 0, 0, "", fmt.Errorf("open-meteo geocode: result is missing name or coordinates") + } + name = strings.TrimSpace(*r.Name) + if name == "" { + return 0, 0, "", fmt.Errorf("open-meteo geocode: result name is blank") + } + if !finite(*r.Latitude) || *r.Latitude < -90 || *r.Latitude > 90 || + !finite(*r.Longitude) || *r.Longitude < -180 || *r.Longitude > 180 { + return 0, 0, "", fmt.Errorf("open-meteo geocode: coordinates are out of range") + } + return *r.Latitude, *r.Longitude, name, nil } + +func decodeBounded(r io.Reader, dst any) error { + body, err := io.ReadAll(io.LimitReader(r, maxResponseBytes+1)) + if err != nil { + return err + } + if int64(len(body)) > maxResponseBytes { + return fmt.Errorf("response exceeds %d bytes", maxResponseBytes) + } + return json.Unmarshal(body, dst) +} + +func validateCurrentWeather(w *currentWeather) error { + if w == nil || w.Temperature == nil || w.WMO == nil || w.WindSpeed == nil { + return fmt.Errorf("current_weather, temperature, weathercode and windspeed are required") + } + if !finite(*w.Temperature) || *w.Temperature < -100 || *w.Temperature > 70 { + return fmt.Errorf("temperature %v is out of range", *w.Temperature) + } + if *w.WMO < 0 || *w.WMO > 99 { + return fmt.Errorf("weathercode %d is out of range", *w.WMO) + } + if !finite(*w.WindSpeed) || *w.WindSpeed < 0 || *w.WindSpeed > 500 { + return fmt.Errorf("windspeed %v is out of range", *w.WindSpeed) + } + return nil +} + +func finite(v float64) bool { return !math.IsNaN(v) && !math.IsInf(v, 0) } diff --git a/internal/weather/openmeteo_test.go b/internal/weather/openmeteo_test.go index 25c9eb2..44fdcf8 100644 --- a/internal/weather/openmeteo_test.go +++ b/internal/weather/openmeteo_test.go @@ -16,22 +16,17 @@ func TestOpenMeteoProvider(t *testing.T) { mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/v1/search" { geoReqCount++ + name, lat, lon := "Moscow", 55.7558, 37.6173 json.NewEncoder(w).Encode(geoResponse{ Results: []geoResult{{ - Name: "Moscow", Latitude: 55.7558, Longitude: 37.6173, Country: "Russia", + Name: &name, Latitude: &lat, Longitude: &lon, Country: "Russia", }}, }) return } if r.URL.Path == "/v1/forecast" { weatherReqCount++ - json.NewEncoder(w).Encode(forecastResponse{ - CurrentWeather: currentWeather{ - Temperature: 22.5, - WMO: 0, - WindSpeed: 3.2, - }, - }) + json.NewEncoder(w).Encode(validForecast(22.5, 0, 3.2)) return } w.WriteHeader(http.StatusNotFound) @@ -60,6 +55,18 @@ func TestOpenMeteoProvider(t *testing.T) { } } +func validForecast(temperature float64, code int, wind float64) forecastResponse { + return forecastResponse{CurrentWeather: ¤tWeather{ + Temperature: &temperature, + WMO: &code, + WindSpeed: &wind, + }} +} + +func validGeo(name string, lat, lon float64) geoResponse { + return geoResponse{Results: []geoResult{{Name: &name, Latitude: &lat, Longitude: &lon, Country: "Russia"}}} +} + type mockTransport struct { geoURL string forecastURL string @@ -85,9 +92,7 @@ func (t *mockTransport) RoundTrip(r *http.Request) (*http.Response, error) { func TestOpenMeteoProvider_ForecastHTTPError(t *testing.T) { mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/v1/search" { - json.NewEncoder(w).Encode(geoResponse{ - Results: []geoResult{{Name: "Moscow", Latitude: 55.7558, Longitude: 37.6173, Country: "Russia"}}, - }) + json.NewEncoder(w).Encode(validGeo("Moscow", 55.7558, 37.6173)) return } w.WriteHeader(http.StatusInternalServerError) @@ -112,6 +117,67 @@ func TestOpenMeteoProvider_ForecastHTTPError(t *testing.T) { } } +func TestOpenMeteoProviderRejectsIncompleteOrInvalidForecast(t *testing.T) { + tests := []struct { + name string + body string + }{ + {name: "empty object", body: `{}`}, + {name: "empty current weather", body: `{"current_weather":{}}`}, + {name: "missing wind", body: `{"current_weather":{"temperature":12,"weathercode":1}}`}, + {name: "temperature range", body: `{"current_weather":{"temperature":100,"weathercode":1,"windspeed":3}}`}, + {name: "weather code range", body: `{"current_weather":{"temperature":12,"weathercode":100,"windspeed":3}}`}, + {name: "wind range", body: `{"current_weather":{"temperature":12,"weathercode":1,"windspeed":-1}}`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/v1/search" { + _ = json.NewEncoder(w).Encode(validGeo("Moscow", 55.7558, 37.6173)) + return + } + _, _ = w.Write([]byte(tc.body)) + })) + defer mock.Close() + p := NewOpenMeteoProvider() + p.SetClient(&http.Client{Transport: &mockTransport{ + geoURL: mock.URL + "/v1/search", forecastURL: mock.URL + "/v1/forecast", + }}) + if got, err := p.CurrentWeather(context.Background(), "Moscow"); err == nil { + t.Fatalf("invalid forecast returned %+v", got) + } + }) + } +} + +func TestOpenMeteoProviderBoundsBothResponseBodies(t *testing.T) { + for _, endpoint := range []string{"geocode", "forecast"} { + t.Run(endpoint, func(t *testing.T) { + mock := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + oversize := endpoint == "geocode" && r.URL.Path == "/v1/search" || + endpoint == "forecast" && r.URL.Path == "/v1/forecast" + if oversize { + _, _ = w.Write([]byte(strings.Repeat("x", int(maxResponseBytes+1)))) + return + } + if r.URL.Path == "/v1/search" { + _ = json.NewEncoder(w).Encode(validGeo("Moscow", 55.7558, 37.6173)) + } else { + _ = json.NewEncoder(w).Encode(validForecast(12, 1, 3)) + } + })) + defer mock.Close() + p := NewOpenMeteoProvider() + p.SetClient(&http.Client{Transport: &mockTransport{ + geoURL: mock.URL + "/v1/search", forecastURL: mock.URL + "/v1/forecast", + }}) + if _, err := p.CurrentWeather(context.Background(), "Moscow"); err == nil || !strings.Contains(err.Error(), "response exceeds") { + t.Fatalf("oversize %s error = %v", endpoint, err) + } + }) + } +} + // TestOpenMeteoProvider_GeocodeHTTPError — a non-200 geocode reply must not be // read as "no such city" (ErrLocationUnknown). A 500 is a service outage, and // the owner hears a different sentence for each (Vikunja #589).