From 0f3dedab73223517fb7af188d66a167192f565f2 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 15:19:30 -0400 Subject: [PATCH] Address Gadfly review findings on Phase 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applied the fixes warranted by the adversarial review of PR #21 (all findings graded in the gadfly store): Store (highest impact): - buildDSN always merges busy_timeout/journal_mode/foreign_keys pragmas into the DSN, even for file: URIs or paths with a query string — the prior passthrough silently disabled FK enforcement for those PANSY_DB values. Also escape '#' in the path and tighten in-memory detection to an exact ':memory:' token or mode=memory param (no substring misfire). - migrate.go: detect duplicate migration versions with a clear error; wrap appliedVersions' rows.Err() with the store: prefix. API: - SetTrustedProxies failure now falls back to trust-none instead of leaving gin's trust-everyone default (X-Forwarded-For spoofing). - SPA: 404 embedded directories (was a directory listing), 404 bare /api, add X-Content-Type-Options: nosniff, cache index.html bytes once at startup, single fs.Stat existence check, shared writeAPIError helper. - Move gin.SetMode out of package init() into New(). Config: validate PANSY_PORT range (fall back to 8080 with a warning). Web: - api.ts: serialize the request body before the fetch try so a JSON.stringify failure isn't misreported as a network error. - AppShell: move state-specific/conflicting utilities into active/inactiveProps so concatenated Tailwind classes don't collide. Tests: added store DSN-pragma/memory-detection cases and SPA directory-404 case. go build/vet/test clean (CGO off); web tsc + build clean; re-verified in a browser (SPA, deep links, /assets 404, nav). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- internal/api/api.go | 13 ++--- internal/api/spa.go | 57 +++++++++++++-------- internal/api/spa_test.go | 10 ++++ internal/config/config.go | 5 ++ internal/store/migrate.go | 10 +++- internal/store/sqlite.go | 70 ++++++++++++++++++++------ internal/store/store_test.go | 58 +++++++++++++++++++++ web/src/components/layout/AppShell.tsx | 15 ++++-- web/src/lib/api.ts | 6 ++- 9 files changed, 193 insertions(+), 51 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index 48a7b3f..721dcac 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -15,21 +15,22 @@ import ( "gitea.stevedudenhoeffer.com/steve/pansy/internal/config" ) -func init() { - // Quiet gin's default debug output; we log via slog-gin instead. - gin.SetMode(gin.ReleaseMode) -} - // New builds the gin engine with the standard middleware stack and registers the // API routes. The embedded SPA fallback is registered separately by the caller // via RegisterSPA (see spa.go) so the API can be built and tested without a web // build present. func New(cfg *config.Config) *gin.Engine { + gin.SetMode(gin.ReleaseMode) + r := gin.New() r.Use(sloggin.New(slog.Default()), gin.Recovery()) if err := r.SetTrustedProxies(cfg.TrustedProxies); err != nil { - slog.Warn("api: failed to set trusted proxies", "error", err) + // Do not leave gin's trust-everyone default active on a parse failure — + // that would let any client spoof X-Forwarded-For. Fall back to trusting + // no proxies, which is also the behavior when none are configured. + slog.Error("api: invalid trusted proxies, trusting none", "error", err) + _ = r.SetTrustedProxies(nil) } v1 := r.Group("/api/v1") diff --git a/internal/api/spa.go b/internal/api/spa.go index d0197f2..a8be3c6 100644 --- a/internal/api/spa.go +++ b/internal/api/spa.go @@ -17,30 +17,44 @@ import ( func RegisterSPA(r *gin.Engine, dist fs.FS) { fileServer := http.FileServer(http.FS(dist)) + // index.html is served on every client-route deep link; read it once at + // startup rather than re-reading the embedded FS per request. A build without + // it is a packaging bug, so fail loudly. + index, err := fs.ReadFile(dist, "index.html") + if err != nil { + panic("api: embedded web build missing index.html: " + err.Error()) + } + r.NoRoute(func(c *gin.Context) { reqPath := c.Request.URL.Path - if strings.HasPrefix(reqPath, "/api/") { - c.JSON(http.StatusNotFound, gin.H{"error": gin.H{ - "code": "NOT_FOUND", "message": "no such endpoint", - }}) + // API paths (including a bare "/api") are never the SPA: a miss is a real + // JSON 404, not the app shell. + if reqPath == "/api" || strings.HasPrefix(reqPath, "/api/") { + writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "no such endpoint") return } if c.Request.Method != http.MethodGet && c.Request.Method != http.MethodHead { - c.JSON(http.StatusMethodNotAllowed, gin.H{"error": gin.H{ - "code": "METHOD_NOT_ALLOWED", "message": "method not allowed", - }}) + writeAPIError(c, http.StatusMethodNotAllowed, "METHOD_NOT_ALLOWED", "method not allowed") return } + c.Header("X-Content-Type-Options", "nosniff") + name := strings.TrimPrefix(path.Clean(reqPath), "/") if name == "" { - name = "index.html" + serveIndex(c, index) + return } - if f, err := dist.Open(name); err == nil { - f.Close() + if info, statErr := fs.Stat(dist, name); statErr == nil { + // An embedded directory is neither a client route nor a servable file; + // serving it would leak a directory listing, so it is a 404. + if info.IsDir() { + writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "not found") + return + } // Vite emits content-hashed filenames under assets/, so those are safe // to cache forever; index.html must always revalidate to pick up new builds. if strings.HasPrefix(name, "assets/") { @@ -56,23 +70,22 @@ func RegisterSPA(r *gin.Engine, dist fs.FS) { // falling back to index.html would return HTML with a 200 for a .js/.css // request, which the browser rejects with a confusing MIME-type error. if strings.HasPrefix(name, "assets/") { - c.JSON(http.StatusNotFound, gin.H{"error": gin.H{ - "code": "NOT_FOUND", "message": "asset not found", - }}) + writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "asset not found") return } - serveIndex(c, dist) + serveIndex(c, index) }) } -// serveIndex writes index.html as the SPA fallback for an unmatched client route. -func serveIndex(c *gin.Context, dist fs.FS) { - data, err := fs.ReadFile(dist, "index.html") - if err != nil { - c.String(http.StatusInternalServerError, "index.html missing from build") - return - } +// serveIndex writes the cached index.html as the SPA fallback (no-cache so new +// builds are picked up). net/http suppresses the body for HEAD requests. +func serveIndex(c *gin.Context, index []byte) { c.Header("Cache-Control", "no-cache") - c.Data(http.StatusOK, "text/html; charset=utf-8", data) + c.Data(http.StatusOK, "text/html; charset=utf-8", index) +} + +// writeAPIError writes pansy's standard JSON error envelope. +func writeAPIError(c *gin.Context, status int, code, message string) { + c.JSON(status, gin.H{"error": gin.H{"code": code, "message": message}}) } diff --git a/internal/api/spa_test.go b/internal/api/spa_test.go index 03e911f..94b7862 100644 --- a/internal/api/spa_test.go +++ b/internal/api/spa_test.go @@ -77,6 +77,16 @@ func TestSPAMissingAssetIs404(t *testing.T) { } } +func TestSPADirectoryIs404NotListing(t *testing.T) { + // A request that resolves to an embedded directory must 404, never a listing. + for _, p := range []string{"/assets", "/assets/"} { + w := do(t, testEngine(), http.MethodGet, p) + if w.Code != http.StatusNotFound { + t.Errorf("GET %s status = %d, want 404", p, w.Code) + } + } +} + func TestSPAUnmatchedAPIIs404JSON(t *testing.T) { w := do(t, testEngine(), http.MethodGet, "/api/v1/nope") if w.Code != http.StatusNotFound { diff --git a/internal/config/config.go b/internal/config/config.go index f07f049..4347d21 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -84,6 +84,11 @@ func Load() *Config { cfg.Registration = RegistrationOpen } + if cfg.Port < 1 || cfg.Port > 65535 { + slog.Warn("config: PANSY_PORT out of range, using default", "value", cfg.Port, "default", 8080) + cfg.Port = 8080 + } + return cfg } diff --git a/internal/store/migrate.go b/internal/store/migrate.go index 16ff9dc..1c291bb 100644 --- a/internal/store/migrate.go +++ b/internal/store/migrate.go @@ -69,7 +69,10 @@ func (d *DB) appliedVersions(ctx context.Context) (map[int]bool, error) { } applied[v] = true } - return applied, rows.Err() + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("store: iterate schema_migrations: %w", err) + } + return applied, nil } func (d *DB) applyMigration(ctx context.Context, m migration) error { @@ -99,6 +102,7 @@ func loadMigrations() ([]migration, error) { } var migrations []migration + seen := map[int]string{} for _, e := range entries { if e.IsDir() || !strings.HasSuffix(e.Name(), ".sql") { continue @@ -107,6 +111,10 @@ func loadMigrations() ([]migration, error) { if err != nil { return nil, err } + if prev, dup := seen[version]; dup { + return nil, fmt.Errorf("store: duplicate migration version %d: %q and %q", version, prev, e.Name()) + } + seen[version] = e.Name() body, err := fs.ReadFile(migrationsFS, "migrations/"+e.Name()) if err != nil { return nil, fmt.Errorf("store: read migration %s: %w", e.Name(), err) diff --git a/internal/store/sqlite.go b/internal/store/sqlite.go index de9505a..ef76b1f 100644 --- a/internal/store/sqlite.go +++ b/internal/store/sqlite.go @@ -13,6 +13,16 @@ import ( _ "modernc.org/sqlite" // pure-Go driver, registered as "sqlite" ) +// requiredPragmas are applied to every connection (they are per-connection in +// SQLite): wait rather than fail on a contended write, use WAL journaling, and +// enforce foreign keys. They are attached to the DSN so the whole pool inherits +// them. +var requiredPragmas = []string{ + "busy_timeout(5000)", + "journal_mode(WAL)", + "foreign_keys(1)", +} + // DB is a handle to the SQLite database backing pansy. Safe for concurrent use: // WAL mode allows many readers with one writer, and busy_timeout makes a // contended writer wait rather than fail with SQLITE_BUSY. @@ -22,10 +32,11 @@ type DB struct { // Open opens (creating if absent) the SQLite database at path and returns a DB. // The pragmas — busy_timeout, WAL journal, and foreign-key enforcement — are -// applied per connection via the DSN so every pooled connection inherits them. -// A path of ":memory:" yields an ephemeral in-memory database (useful in tests); -// the pool is pinned to a single connection so all queries see the same DB. -// The caller owns the returned DB and must Close it. +// applied per connection via the DSN so every pooled connection inherits them, +// regardless of whether path is a bare filename, a path with query parameters, +// or a full file: URI. A path of ":memory:" yields an ephemeral in-memory +// database (useful in tests); the pool is pinned to a single connection so all +// queries see the same DB. The caller owns the returned DB and must Close it. func Open(path string) (*DB, error) { dsn, memory := buildDSN(path) sqldb, err := sql.Open("sqlite", dsn) @@ -45,20 +56,47 @@ func Open(path string) (*DB, error) { return &DB{sql: sqldb}, nil } -// buildDSN turns a filesystem path (or ":memory:") into a modernc DSN carrying -// the pragmas we want on every connection, and reports whether it is in-memory. -// A path that already looks like a DSN (has a scheme or query) is passed through -// untouched. +// buildDSN converts the configured path into a modernc DSN that always carries +// the required pragmas, and reports whether it is an in-memory database. It +// accepts a bare filename, a path with existing query parameters, or a full +// file: URI, and merges the pragmas without clobbering any the user supplied. func buildDSN(path string) (dsn string, memory bool) { - memory = path == ":memory:" || strings.Contains(path, ":memory:") || strings.Contains(path, "mode=memory") - if strings.HasPrefix(path, "file:") || strings.Contains(path, "?") { - return path, memory + body := strings.TrimPrefix(path, "file:") + + filePart, rawQuery, _ := strings.Cut(body, "?") + query := url.Values{} + if q, err := url.ParseQuery(rawQuery); err == nil { + query = q } - pragmas := url.Values{} - pragmas.Add("_pragma", "busy_timeout(5000)") - pragmas.Add("_pragma", "journal_mode(WAL)") - pragmas.Add("_pragma", "foreign_keys(1)") - return "file:" + path + "?" + pragmas.Encode(), memory + + // Detect in-memory precisely: the exact ":memory:" token, or an explicit + // mode=memory parameter. A substring match would misfire on real file paths. + memory = filePart == ":memory:" || query.Get("mode") == "memory" + + have := make(map[string]bool, len(query["_pragma"])) + for _, p := range query["_pragma"] { + have[pragmaName(p)] = true + } + for _, p := range requiredPragmas { + if !have[pragmaName(p)] { + query.Add("_pragma", p) + } + } + + // A '#' in a filesystem path would otherwise start a URI fragment and drop + // the query string that carries the pragmas. + filePart = strings.ReplaceAll(filePart, "#", "%23") + return "file:" + filePart + "?" + query.Encode(), memory +} + +// pragmaName extracts the pragma's name from a "_pragma" value such as +// "busy_timeout(5000)" or "foreign_keys=1", lowercased for case-insensitive +// de-duplication against a user-supplied value. +func pragmaName(p string) string { + if i := strings.IndexAny(p, "(="); i >= 0 { + p = p[:i] + } + return strings.ToLower(strings.TrimSpace(p)) } // Close closes the underlying database. diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 10b3fa4..9cd5154 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -3,6 +3,7 @@ package store import ( "context" "path/filepath" + "strings" "testing" ) @@ -102,6 +103,63 @@ func TestCheckConstraintRejectsBadEnum(t *testing.T) { } } +func TestForeignKeysEnforcedForFileURIDSN(t *testing.T) { + // A file: URI (or any path with query params) must still get foreign_keys(1): + // the earlier passthrough silently dropped the pragmas for these DSNs. + p := filepath.Join(t.TempDir(), "uri.db") + db, err := Open("file:" + p + "?_pragma=synchronous(1)") + if err != nil { + t.Fatalf("Open file: URI: %v", err) + } + t.Cleanup(func() { db.Close() }) + if err := db.Migrate(context.Background()); err != nil { + t.Fatalf("Migrate: %v", err) + } + _, err = db.SQL().Exec( + `INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm) VALUES (999, 999, 0, 0, 10)`, + ) + if err == nil { + t.Fatal("expected foreign-key violation for file: URI DSN, got nil") + } +} + +func TestBuildDSNAlwaysIncludesPragmas(t *testing.T) { + for _, in := range []string{ + "./pansy.db", + "/data/pansy.db", + "file:/data/pansy.db", + "file:/data/pansy.db?cache=shared", + "/tmp/weird?name.db", + } { + dsn, _ := buildDSN(in) + for _, want := range []string{"busy_timeout", "journal_mode", "foreign_keys"} { + if !strings.Contains(dsn, want) { + t.Errorf("buildDSN(%q) = %q, missing %s pragma", in, dsn, want) + } + } + } +} + +func TestBuildDSNDoesNotDuplicateUserPragma(t *testing.T) { + // A user-supplied busy_timeout must not be duplicated by our default. + dsn, _ := buildDSN("file:/data/x.db?_pragma=busy_timeout(9000)") + if strings.Count(dsn, "busy_timeout") != 1 { + t.Errorf("buildDSN kept both user and default busy_timeout: %q", dsn) + } +} + +func TestBuildDSNMemoryDetection(t *testing.T) { + if _, mem := buildDSN(":memory:"); !mem { + t.Error(":memory: not detected as in-memory") + } + if _, mem := buildDSN("file:x.db?mode=memory"); !mem { + t.Error("mode=memory not detected as in-memory") + } + if _, mem := buildDSN("/var/lib/pansy/pansy.db"); mem { + t.Error("file path wrongly detected as in-memory") + } +} + func TestInMemoryDBIsCoherentAcrossQueries(t *testing.T) { // A bare ":memory:" DSN gives each connection its own DB; Open must pin the // pool so the migrated schema is visible to subsequent queries. diff --git a/web/src/components/layout/AppShell.tsx b/web/src/components/layout/AppShell.tsx index 5fc81bd..be99caf 100644 --- a/web/src/components/layout/AppShell.tsx +++ b/web/src/components/layout/AppShell.tsx @@ -1,11 +1,17 @@ import { Link, Outlet } from '@tanstack/react-router' -import { cn } from '@/lib/cn' const navLinks = [ { to: '/gardens', label: 'Gardens' }, { to: '/plants', label: 'Plants' }, ] as const +// TanStack Router concatenates the base className with activeProps/inactiveProps, +// so state-specific and conflicting utilities (text-muted vs text-fg) live in the +// state props — never in the base — to avoid ambiguous overrides. +const navLinkBase = 'rounded-md px-3 py-1.5 text-sm font-medium transition-colors' +const navLinkActive = 'bg-border/60 text-fg' +const navLinkInactive = 'text-muted hover:bg-border/60 hover:text-fg' + /** Top-level chrome: a sticky nav bar plus the routed page in an . */ export function AppShell() { return ( @@ -20,10 +26,9 @@ export function AppShell() { {l.label} diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 906d37b..9cefee5 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -84,6 +84,10 @@ function messageFrom(body: unknown, status: number): string { export async function apiFetch(path: string, opts: RequestOptions = {}): Promise { const { method = 'GET', body, params, signal } = opts + // Serialize before the try so a JSON.stringify failure (e.g. a circular value) + // surfaces as itself, not as a misleading "cannot reach the server" error. + const requestBody = body !== undefined ? JSON.stringify(body) : undefined + let res: Response try { res = await fetch(buildUrl(path, params), { @@ -94,7 +98,7 @@ export async function apiFetch(path: string, opts: RequestOptions = {}): Prom accept: 'application/json', ...(body !== undefined ? { 'content-type': 'application/json' } : {}), }, - body: body !== undefined ? JSON.stringify(body) : undefined, + body: requestBody, }) } catch (err) { if ((err as Error)?.name === 'AbortError') throw err