diff --git a/.claude/skills/beacon-data-privacy/SKILL.md b/.claude/skills/beacon-data-privacy/SKILL.md index edd7bde..515200d 100644 --- a/.claude/skills/beacon-data-privacy/SKILL.md +++ b/.claude/skills/beacon-data-privacy/SKILL.md @@ -66,3 +66,12 @@ lines already include `chat=` for observability and that is fine. Do no `@username`, message body content, or any other off-limits field. The access-log format `middleware [200] GET /api/v1/me/subscriptions` intentionally omits the `X-Telegram-Init-Data` header for the same reason. + +**An outbound URL is a log field too.** Open-Meteo requests carry a user-selected city's +coordinates, and geocoding requests carry the search term the user typed. Both are +pre-approved data, so this is hygiene rather than policy — but neither belongs in a log line +by accident. `internal/infrastructure/weather/openmeteo.go` composes its status-code errors +from host and path only, and runs transport errors through `redactURLError`, because +`net/http` returns a `*url.Error` whose `Error()` embeds the request URL verbatim and any +caller printing it with `%v` prints the query string with it. A new outbound client wraps its +own transport errors the same way. diff --git a/internal/infrastructure/weather/openmeteo.go b/internal/infrastructure/weather/openmeteo.go index 7a16611..9590a58 100644 --- a/internal/infrastructure/weather/openmeteo.go +++ b/internal/infrastructure/weather/openmeteo.go @@ -272,6 +272,31 @@ func (o *OpenMeteo) get(ctx context.Context, rawURL string) ([]byte, error) { ) } +// redactURLError rebuilds a *url.Error with the query string stripped from its URL. +// +// The status-code branch below composes its own message from host and path deliberately, to +// keep coordinates and search terms out of the logs. A transport failure defeats that on its +// own: net/http returns a *url.Error whose Error() embeds the URL verbatim, so a plain +// `dial tcp: i/o timeout` arrives carrying every latitude, longitude and query term the +// request was built with, and any caller formatting it with %v prints them. +// +// It is called on the error http.Client.Do returns, where the *url.Error is the whole chain. +// Anything else is handed back untouched. +func redactURLError(err error) error { + var urlErr *url.Error + if !errors.As(err, &urlErr) { + return err + } + + // A URL that will not parse yields the empty string rather than the original: the point + // is that nothing unexamined reaches the log. + redacted := "" + if parsed, parseErr := url.Parse(urlErr.URL); parseErr == nil { + redacted = parsed.Host + parsed.Path + } + return &url.Error{Op: urlErr.Op, URL: redacted, Err: urlErr.Err} +} + // attempt performs exactly one request. Its errors are classified by retryableError so // get can tell an upstream hiccup from an answer that will not change. func (o *OpenMeteo) attempt(ctx context.Context, rawURL string) ([]byte, error) { @@ -286,6 +311,7 @@ func (o *OpenMeteo) attempt(ctx context.Context, rawURL string) ([]byte, error) // Transport-level failures — timeout, reset, refused — are indistinguishable // from a 5xx from here and just as transient. A cancelled context is not: the // caller asked to stop, and re-sending would ignore that. + err = redactURLError(err) if ctx.Err() != nil { return nil, fmt.Errorf("open-meteo: do request: %w", err) } diff --git a/internal/infrastructure/weather/openmeteo_test.go b/internal/infrastructure/weather/openmeteo_test.go index 2257835..0117983 100644 --- a/internal/infrastructure/weather/openmeteo_test.go +++ b/internal/infrastructure/weather/openmeteo_test.go @@ -2,6 +2,7 @@ package weather import ( "context" + "errors" "fmt" "io" "net/http" @@ -665,6 +666,56 @@ func loggedDuration(t *testing.T, text, pattern string) time.Duration { return d } +func TestOpenMeteoTransportErrorRedaction(t *testing.T) { + t.Parallel() + + // A dead listener forces http.Client.Do to fail at the transport, which is the branch + // that returns a *url.Error carrying the whole request URL. + deadServer := func(t *testing.T) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + addr := srv.URL + srv.Close() + return addr + } + + t.Run("a forecast transport failure carries no coordinates", func(t *testing.T) { + t.Parallel() + addr := deadServer(t) + om := newTestOpenMeteo(t, addr, addr) + + _, err := om.Forecast(t.Context(), 51.169392, 71.449074) + require.Error(t, err) + assert.NotContains(t, err.Error(), "51.169392") + assert.NotContains(t, err.Error(), "71.449074") + assert.NotContains(t, err.Error(), "latitude") + assert.Contains(t, err.Error(), "/v1/forecast", "the path still has to say what failed") + }) + + t.Run("a geocode transport failure carries no search term", func(t *testing.T) { + t.Parallel() + addr := deadServer(t) + om := newTestOpenMeteo(t, addr, addr) + + _, err := om.Geocode(t.Context(), "Karagandy", 3) + require.Error(t, err) + assert.NotContains(t, err.Error(), "Karagandy") + assert.Contains(t, err.Error(), "/v1/search") + }) + + t.Run("an error that is not a *url.Error is untouched", func(t *testing.T) { + t.Parallel() + sentinel := errors.New("nothing to redact") + assert.Equal(t, sentinel, redactURLError(sentinel)) + }) + + t.Run("a URL that will not parse redacts to nothing, not to itself", func(t *testing.T) { + t.Parallel() + got := redactURLError(&url.Error{Op: "Get", URL: "://%zz", Err: errors.New("boom")}) + assert.NotContains(t, got.Error(), "%zz") + }) +} + func TestOpenMeteo_ForecastRange(t *testing.T) { t.Parallel()