From 454faea429ac4b954972f6d328cdf0fcd087f777 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Fri, 2 Oct 2026 22:16:08 -0400 Subject: [PATCH] Address second review findings F1-F3 for 404, public URL and listen host - F1: return the JSON API error shape for unknown paths (404 not_found) - F2: require a non-empty hostname in MEALQUEST_PUBLIC_URL - F3: validate the MEALQUEST_HTTP_ADDR host as an IP address or hostname Co-Authored-By: Claude Opus 5.5 --- internal/config/config.go | 28 +++++++++++++++++++++++-- internal/config/config_test.go | 37 ++++++++++++++++++++++++++++++++++ internal/server/server.go | 14 +++++++------ internal/server/server_test.go | 19 +++++++++++++++++ 4 files changed, 90 insertions(+), 8 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 1e84b99..5673595 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -119,11 +119,16 @@ func (r *reader) listenAddr(name string) string { if !ok { return "" } - _, port, err := net.SplitHostPort(v) + host, port, err := net.SplitHostPort(v) if err != nil { r.fail(name, "invalid listen address %q (want host:port or :port)", v) return "" } + // An empty host listens on all interfaces. + if host != "" && net.ParseIP(host) == nil && !validHostname(host) { + r.fail(name, "invalid host %q in listen address (want IP address or hostname)", host) + return "" + } if _, err := strconv.ParseUint(port, 10, 16); err != nil { r.fail(name, "invalid port %q in listen address (want 0-65535)", port) return "" @@ -131,6 +136,25 @@ func (r *reader) listenAddr(name string) string { return v } +// validHostname reports whether h is a DNS hostname: dot-separated labels of +// 1-63 letters, digits and hyphens, not starting or ending with a hyphen. +func validHostname(h string) bool { + if len(h) > 253 { + return false + } + for _, label := range strings.Split(strings.TrimSuffix(h, "."), ".") { + if len(label) < 1 || len(label) > 63 || label[0] == '-' || label[len(label)-1] == '-' { + return false + } + for _, c := range label { + if !(c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-') { + return false + } + } + } + return true +} + func (r *reader) httpURL(name string) *url.URL { v, ok := r.required(name) if !ok { @@ -138,7 +162,7 @@ func (r *reader) httpURL(name string) *url.URL { } // The value is never echoed: it may carry credentials. u, err := url.Parse(v) - if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { + if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Hostname() == "" { r.fail(name, "invalid URL (want absolute http:// or https:// URL)") return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ebaafa2..01a184f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -102,6 +102,43 @@ func TestLoad(t *testing.T) { env: with(map[string]string{EnvHTTPAddr: ":65536"}), badVars: []string{EnvHTTPAddr}, }, + { + name: "listen address with IP host", + env: with(map[string]string{EnvHTTPAddr: "127.0.0.1:8080"}), + }, + { + name: "listen address with IPv6 host", + env: with(map[string]string{EnvHTTPAddr: "[::1]:8080"}), + }, + { + name: "listen address with hostname", + env: with(map[string]string{EnvHTTPAddr: "localhost:8080"}), + }, + { + name: "listen host with space", + env: with(map[string]string{EnvHTTPAddr: "bad host:8080"}), + badVars: []string{EnvHTTPAddr}, + }, + { + name: "listen host with leading hyphen label", + env: with(map[string]string{EnvHTTPAddr: "-bad.example.com:8080"}), + badVars: []string{EnvHTTPAddr}, + }, + { + name: "listen host with empty label", + env: with(map[string]string{EnvHTTPAddr: "bad..example.com:8080"}), + badVars: []string{EnvHTTPAddr}, + }, + { + name: "listen host with label over 63 chars", + env: with(map[string]string{EnvHTTPAddr: strings.Repeat("a", 64) + ".example.com:8080"}), + badVars: []string{EnvHTTPAddr}, + }, + { + name: "public URL without hostname", + env: with(map[string]string{EnvPublicURL: "http://:8080"}), + badVars: []string{EnvPublicURL}, + }, { name: "relative public URL", env: with(map[string]string{EnvPublicURL: "meals.example.com"}), diff --git a/internal/server/server.go b/internal/server/server.go index 597e456..33b0e38 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -14,7 +14,8 @@ import ( func NewRouter(logger *slog.Logger, version string) http.Handler { r := chi.NewRouter() r.Get("/health", health(logger, version)) - r.MethodNotAllowed(methodNotAllowed(logger)) + r.MethodNotAllowed(apiErrorHandler(logger, http.StatusMethodNotAllowed, "method_not_allowed", "method not allowed")) + r.NotFound(apiErrorHandler(logger, http.StatusNotFound, "not_found", "not found")) return r } @@ -28,19 +29,20 @@ type apiError struct { Retryable bool `json:"retryable"` } -func methodNotAllowed(logger *slog.Logger) http.HandlerFunc { +// apiErrorHandler responds with status and a fixed, non-retryable API error. +func apiErrorHandler(logger *slog.Logger, status int, code, message string) http.HandlerFunc { body, err := json.Marshal(errorBody{Error: apiError{ - Code: "method_not_allowed", - Message: "method not allowed", + Code: code, + Message: message, }}) if err != nil { panic(err) // fixed string and bool fields cannot fail to marshal } return func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusMethodNotAllowed) + w.WriteHeader(status) if _, err := w.Write(body); err != nil { - logger.WarnContext(r.Context(), "write method not allowed response", "error", err) + logger.WarnContext(r.Context(), "write error response", "code", code, "error", err) } } } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index b79856e..f8e3cc5 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -67,3 +67,22 @@ func TestHealthRejectsOtherMethods(t *testing.T) { t.Errorf("error body = %v, want %v", got.Error, want) } } + +func TestUnknownPathNotFound(t *testing.T) { + t.Parallel() + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + rec := httptest.NewRecorder() + NewRouter(logger, "dev").ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/unknown", nil)) + + if rec.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404", rec.Code) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + const want = `{"error":{"code":"not_found","message":"not found","retryable":false}}` + if got := rec.Body.String(); got != want { + t.Errorf("body = %s, want %s", got, want) + } +}