Address second review findings F1-F3 for 404, public URL and listen host
Independent review Approved — 0 high findings (#2)

- 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 <noreply@anthropic.com>
This commit is contained in:
2026-10-02 22:16:08 -04:00
co-authored by Claude Opus 5.5
parent 60555bf8ec
commit 454faea429
4 changed files with 90 additions and 8 deletions
+26 -2
View File
@@ -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
}
+37
View File
@@ -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"}),
+8 -6
View File
@@ -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)
}
}
}
+19
View File
@@ -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)
}
}