Address review findings F1-F4 for config and /health
Independent review Not signed off — 0 high, 3 medium (#2)

- F1: read the secret from MEALQUEST_SECRET_KEY_FILE, the documented name.
- F2: reject public URLs carrying userinfo, never echo the raw URL in
  config errors, and log only the public origin (scheme + host).
- F3: validate the MEALQUEST_HTTP_ADDR port (numeric, 0-65535) during Load.
- F4: return 405 with the API error JSON shape for unsupported methods.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
2026-10-02 22:10:25 -04:00
co-authored by Claude Opus 5.5
parent 5707682018
commit 60555bf8ec
6 changed files with 194 additions and 42 deletions
+76 -27
View File
@@ -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)
}
}