diff --git a/cmd/server/main.go b/cmd/server/main.go index b9e99bd..e0d649b 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -46,12 +46,7 @@ func run(cfg config.Config, logger *slog.Logger) error { errc := make(chan error, 1) go func() { - logger.Info("listening", - "addr", cfg.HTTPAddr, - "public_url", cfg.PublicURL.String(), - "data_dir", cfg.DataDir, - "version", version, - ) + logListening(logger, cfg) errc <- srv.ListenAndServe() }() @@ -72,3 +67,14 @@ func run(cfg config.Config, logger *slog.Logger) error { } return nil } + +// logListening logs the startup line. Only the public URL's origin is logged +// so no credential-bearing URL component can reach the log. +func logListening(logger *slog.Logger, cfg config.Config) { + logger.Info("listening", + "addr", cfg.HTTPAddr, + "public_origin", cfg.PublicOrigin(), + "data_dir", cfg.DataDir, + "version", version, + ) +} diff --git a/cmd/server/main_test.go b/cmd/server/main_test.go new file mode 100644 index 0000000..02748e8 --- /dev/null +++ b/cmd/server/main_test.go @@ -0,0 +1,34 @@ +package main + +import ( + "bytes" + "log/slog" + "net/url" + "strings" + "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/config" +) + +func TestLogListeningOmitsURLCredentials(t *testing.T) { + t.Parallel() + + // Load rejects such URLs; this proves the log stays safe even if one + // reached the Config some other way. + u, err := url.Parse("https://alice:hunter2@meals.example.com/app?token=t0ken") + if err != nil { + t.Fatal(err) + } + var buf bytes.Buffer + logListening(slog.New(slog.NewJSONHandler(&buf, nil)), config.Config{HTTPAddr: ":8080", PublicURL: u}) + + out := buf.String() + for _, leak := range []string{"alice", "hunter2", "t0ken"} { + if strings.Contains(out, leak) { + t.Errorf("startup log leaks %q: %s", leak, out) + } + } + if !strings.Contains(out, `"public_origin":"https://meals.example.com"`) { + t.Errorf("startup log missing public origin: %s", out) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index dd45f72..1e84b99 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,17 +8,18 @@ import ( "net" "net/url" "os" + "strconv" "strings" "time" ) // Environment variable names. const ( - EnvHTTPAddr = "MEALQUEST_HTTP_ADDR" - EnvPublicURL = "MEALQUEST_PUBLIC_URL" - EnvDataDir = "MEALQUEST_DATA_DIR" - EnvShutdownTimeout = "MEALQUEST_SHUTDOWN_TIMEOUT" - EnvSessionSecretFile = "MEALQUEST_SESSION_SECRET_FILE" + EnvHTTPAddr = "MEALQUEST_HTTP_ADDR" + EnvPublicURL = "MEALQUEST_PUBLIC_URL" + EnvDataDir = "MEALQUEST_DATA_DIR" + EnvShutdownTimeout = "MEALQUEST_SHUTDOWN_TIMEOUT" + EnvSecretKeyFile = "MEALQUEST_SECRET_KEY_FILE" ) // Config is the validated service configuration. @@ -27,7 +28,13 @@ type Config struct { PublicURL *url.URL DataDir string ShutdownTimeout time.Duration - SessionSecret Secret + SecretKey Secret +} + +// PublicOrigin returns the scheme and host of PublicURL, the only parts of it +// that are safe to log. +func (c Config) PublicOrigin() string { + return c.PublicURL.Scheme + "://" + c.PublicURL.Host } // Load reads and validates configuration from the process environment. @@ -43,7 +50,7 @@ func load(lookup func(string) (string, bool)) (Config, error) { PublicURL: r.httpURL(EnvPublicURL), DataDir: r.dir(EnvDataDir), ShutdownTimeout: r.duration(EnvShutdownTimeout), - SessionSecret: r.secretFile(EnvSessionSecretFile), + SecretKey: r.secretFile(EnvSecretKeyFile), } if len(r.problems) > 0 { return Config{}, &Error{Problems: r.problems} @@ -112,10 +119,15 @@ func (r *reader) listenAddr(name string) string { if !ok { return "" } - if _, _, err := net.SplitHostPort(v); err != nil { + _, port, err := net.SplitHostPort(v) + if err != nil { r.fail(name, "invalid listen address %q (want host:port or :port)", v) return "" } + if _, err := strconv.ParseUint(port, 10, 16); err != nil { + r.fail(name, "invalid port %q in listen address (want 0-65535)", port) + return "" + } return v } @@ -124,9 +136,14 @@ func (r *reader) httpURL(name string) *url.URL { if !ok { return nil } + // 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 == "" { - r.fail(name, "invalid URL %q (want absolute http:// or https:// URL)", v) + r.fail(name, "invalid URL (want absolute http:// or https:// URL)") + return nil + } + if u.User != nil { + r.fail(name, "URL must not contain user credentials") return nil } return u diff --git a/internal/config/config_test.go b/internal/config/config_test.go index b617872..ebaafa2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "errors" "fmt" + "net/url" "os" "path/filepath" "slices" @@ -15,7 +16,7 @@ func TestLoad(t *testing.T) { t.Parallel() dir := t.TempDir() - secretPath := filepath.Join(dir, "session_secret") + secretPath := filepath.Join(dir, "secret_key") if err := os.WriteFile(secretPath, []byte("s3cret-value\n"), 0o600); err != nil { t.Fatal(err) } @@ -27,11 +28,11 @@ func TestLoad(t *testing.T) { valid := func() map[string]string { return map[string]string{ - EnvHTTPAddr: ":8080", - EnvPublicURL: "https://meals.example.com", - EnvDataDir: dir, - EnvShutdownTimeout: "15s", - EnvSessionSecretFile: secretPath, + EnvHTTPAddr: ":8080", + EnvPublicURL: "https://meals.example.com", + EnvDataDir: dir, + EnvShutdownTimeout: "15s", + EnvSecretKeyFile: secretPath, } } with := func(changes map[string]string, unset ...string) map[string]string { @@ -68,7 +69,7 @@ func TestLoad(t *testing.T) { name: "all missing are collected", env: map[string]string{}, badVars: []string{ - EnvHTTPAddr, EnvPublicURL, EnvDataDir, EnvShutdownTimeout, EnvSessionSecretFile, + EnvHTTPAddr, EnvPublicURL, EnvDataDir, EnvShutdownTimeout, EnvSecretKeyFile, }, }, { @@ -86,6 +87,21 @@ func TestLoad(t *testing.T) { env: with(map[string]string{EnvHTTPAddr: "8080"}), badVars: []string{EnvHTTPAddr}, }, + { + name: "non-numeric port", + env: with(map[string]string{EnvHTTPAddr: ":notaport"}), + badVars: []string{EnvHTTPAddr}, + }, + { + name: "empty port", + env: with(map[string]string{EnvHTTPAddr: "127.0.0.1:"}), + badVars: []string{EnvHTTPAddr}, + }, + { + name: "port out of range", + env: with(map[string]string{EnvHTTPAddr: ":65536"}), + badVars: []string{EnvHTTPAddr}, + }, { name: "relative public URL", env: with(map[string]string{EnvPublicURL: "meals.example.com"}), @@ -98,31 +114,31 @@ func TestLoad(t *testing.T) { }, { name: "unreadable secret file", - env: with(map[string]string{EnvSessionSecretFile: unreadablePath}), - badVars: []string{EnvSessionSecretFile}, + env: with(map[string]string{EnvSecretKeyFile: unreadablePath}), + badVars: []string{EnvSecretKeyFile}, }, { name: "secret file path is a directory", - env: with(map[string]string{EnvSessionSecretFile: dir}), - badVars: []string{EnvSessionSecretFile}, + env: with(map[string]string{EnvSecretKeyFile: dir}), + badVars: []string{EnvSecretKeyFile}, }, { name: "empty secret file", - env: with(map[string]string{EnvSessionSecretFile: emptyPath}), - badVars: []string{EnvSessionSecretFile}, + env: with(map[string]string{EnvSecretKeyFile: emptyPath}), + badVars: []string{EnvSecretKeyFile}, }, { name: "inline secret rejected", - env: with(map[string]string{"MEALQUEST_SESSION_SECRET": "inline"}), - badVars: []string{"MEALQUEST_SESSION_SECRET"}, + env: with(map[string]string{"MEALQUEST_SECRET_KEY": "inline"}), + badVars: []string{"MEALQUEST_SECRET_KEY"}, }, { name: "several invalid values are all named", env: with(map[string]string{ - EnvShutdownTimeout: "soon", - EnvSessionSecretFile: unreadablePath, + EnvShutdownTimeout: "soon", + EnvSecretKeyFile: unreadablePath, }, EnvPublicURL), - badVars: []string{EnvPublicURL, EnvShutdownTimeout, EnvSessionSecretFile}, + badVars: []string{EnvPublicURL, EnvShutdownTimeout, EnvSecretKeyFile}, }, } @@ -169,16 +185,16 @@ func TestLoadValues(t *testing.T) { t.Parallel() dir := t.TempDir() - secretPath := filepath.Join(dir, "session_secret") + secretPath := filepath.Join(dir, "secret_key") if err := os.WriteFile(secretPath, []byte("s3cret-value\r\n"), 0o600); err != nil { t.Fatal(err) } env := map[string]string{ - EnvHTTPAddr: "127.0.0.1:9000", - EnvPublicURL: "http://localhost:9000", - EnvDataDir: dir, - EnvShutdownTimeout: "1m30s", - EnvSessionSecretFile: secretPath, + EnvHTTPAddr: "127.0.0.1:9000", + EnvPublicURL: "http://localhost:9000", + EnvDataDir: dir, + EnvShutdownTimeout: "1m30s", + EnvSecretKeyFile: secretPath, } cfg, err := load(func(k string) (string, bool) { v, ok := env[k]; return v, ok }) if err != nil { @@ -197,8 +213,8 @@ func TestLoadValues(t *testing.T) { if cfg.ShutdownTimeout != 90*time.Second { t.Errorf("ShutdownTimeout = %v, want 1m30s", cfg.ShutdownTimeout) } - if cfg.SessionSecret.Reveal() != "s3cret-value" { - t.Errorf("SessionSecret.Reveal() = %q, want trailing newline trimmed", cfg.SessionSecret.Reveal()) + if cfg.SecretKey.Reveal() != "s3cret-value" { + t.Errorf("SecretKey.Reveal() = %q, want trailing newline trimmed", cfg.SecretKey.Reveal()) } } @@ -207,7 +223,7 @@ func TestSecretRedacted(t *testing.T) { s := Secret{value: "s3cret-value"} for _, format := range []string{"%v", "%s", "%+v", "%#v"} { - cfg := Config{SessionSecret: s} + cfg := Config{SecretKey: s} for _, out := range []string{fmt.Sprintf(format, s), fmt.Sprintf(format, cfg)} { if strings.Contains(out, "s3cret-value") { t.Errorf("format %s leaked secret: %s", format, out) @@ -215,3 +231,36 @@ func TestSecretRedacted(t *testing.T) { } } } + +func TestPublicURLWithCredentialsRejected(t *testing.T) { + t.Parallel() + + for _, raw := range []string{ + "https://alice:hunter2@example.com", + "https://alice@example.com", + "https://alice:hunter2@[bad", + } { + env := map[string]string{EnvPublicURL: raw} + _, err := load(func(k string) (string, bool) { v, ok := env[k]; return v, ok }) + + var cerr *Error + if !errors.As(err, &cerr) || !slices.ContainsFunc(cerr.Problems, func(p Problem) bool { return p.Var == EnvPublicURL }) { + t.Errorf("load(%q) error = %v, want problem for %s", raw, err, EnvPublicURL) + } + if err != nil && (strings.Contains(err.Error(), "hunter2") || strings.Contains(err.Error(), "alice")) { + t.Errorf("load(%q) error leaks credentials: %v", raw, err) + } + } +} + +func TestPublicOrigin(t *testing.T) { + t.Parallel() + + u, err := url.Parse("https://alice:hunter2@meals.example.com:8443/app?token=x#frag") + if err != nil { + t.Fatal(err) + } + if got, want := (Config{PublicURL: u}).PublicOrigin(), "https://meals.example.com:8443"; got != want { + t.Errorf("PublicOrigin() = %q, want %q", got, want) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index fa4f203..597e456 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -14,9 +14,37 @@ import ( func NewRouter(logger *slog.Logger, version string) http.Handler { r := chi.NewRouter() r.Get("/health", health(logger, version)) + r.MethodNotAllowed(methodNotAllowed(logger)) return r } +type errorBody struct { + Error apiError `json:"error"` +} + +type apiError struct { + Code string `json:"code"` + Message string `json:"message"` + Retryable bool `json:"retryable"` +} + +func methodNotAllowed(logger *slog.Logger) http.HandlerFunc { + body, err := json.Marshal(errorBody{Error: apiError{ + Code: "method_not_allowed", + Message: "method not allowed", + }}) + 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) + if _, err := w.Write(body); err != nil { + logger.WarnContext(r.Context(), "write method not allowed response", "error", err) + } + } +} + type healthResponse struct { Status string `json:"status"` Version string `json:"version"` diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 0e7e9f9..b79856e 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "io" "log/slog" + "maps" "net/http" "net/http/httptest" "testing" @@ -48,4 +49,21 @@ func TestHealthRejectsOtherMethods(t *testing.T) { if rec.Code != http.StatusMethodNotAllowed { t.Errorf("status = %d, want 405", rec.Code) } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + var got struct { + Error map[string]any `json:"error"` + } + if err := json.NewDecoder(rec.Body).Decode(&got); err != nil { + t.Fatal(err) + } + want := map[string]any{ + "code": "method_not_allowed", + "message": "method not allowed", + "retryable": false, + } + if !maps.Equal(got.Error, want) { + t.Errorf("error body = %v, want %v", got.Error, want) + } }