From d1ce80341270216db5fd0bdaffd0f9465fd3017a Mon Sep 17 00:00:00 2001
From: code <code@4lt.ca>
Date: Thu, 16 Jul 2026 05:30:10 +0000
Subject: [PATCH] Fix review findings; consolidate pre-release migrations

Bug fixes:
- Deleting a user with comments no longer 500s: remove the user's
  comments first (covers old DBs) and add ON DELETE CASCADE to the FK.
- Drop 'svg' from the default media upload allowlist (stored-XSS vector
  for files served directly from public/uploads/).
- Content index now reindexes on page deletion: track routable page
  count in content_index_meta and treat a count change as stale, since
  the newest-mtime check alone can't see a removal.
- Dev router (public/router.php) preserves the query string across the
  canonical trailing-slash redirect, matching .htaccess.
- Fix stale worked-example reference in the comments thread partial.

Migrations: since v2 is unreleased with no live databases, fold the
incremental ALTERs into the base migrations rather than shipping them
separately: verification columns into 0002_create_users.sql, source_count
into 0001_create_content_index.sql; renumber comments to 0003. Update all
code/doc references to the removed/renamed files.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 novaconium/config.php                         |  9 +++++--
 .../migrations/0001_create_content_index.sql  |  1 +
 novaconium/migrations/0002_create_users.sql   |  5 +++-
 .../0003_add_users_verification.sql           |  4 ---
 ..._comments.sql => 0003_create_comments.sql} |  2 +-
 .../pages/_partials/comments/thread.twig      |  2 +-
 .../pages/admin/docs/admin-auth/index.twig    |  2 +-
 .../pages/admin/docs/comments/index.twig      |  2 +-
 novaconium/pages/admin/users/index.php        |  9 +++++++
 novaconium/src/AdminAuth.php                  |  9 ++++---
 novaconium/src/ContentIndexer.php             | 26 ++++++++++++++++---
 public/router.php                             |  6 ++++-
 12 files changed, 57 insertions(+), 20 deletions(-)
 delete mode 100644 novaconium/migrations/0003_add_users_verification.sql
 rename novaconium/migrations/{0004_create_comments.sql => 0003_create_comments.sql} (78%)

diff --git a/novaconium/config.php b/novaconium/config.php
index 1890309..7f0580e 100644
--- a/novaconium/config.php
+++ b/novaconium/config.php
@@ -111,8 +111,13 @@ return [
     // against the uploaded filename's extension; media_upload_max_bytes
     // caps a single file's size (checked against both $_FILES' reported
     // size and PHP's own upload_max_filesize/post_max_size ini limits,
-    // see /admin/docs/media-manager).
-    'media_upload_extensions' => ['jpg', 'jpeg', 'png', 'gif', 'webp', 'svg', 'pdf', 'txt', 'zip'],
+    // see /admin/docs/media-manager). 'svg' is deliberately NOT in this
+    // default list: files under public/uploads/ are served directly from
+    // this origin, and an SVG can carry inline <script>, making it a
+    // stored-XSS vector. Add 'svg' back via App/config.php only if you
+    // serve uploads with a restrictive CSP or Content-Disposition:
+    // attachment.
+    'media_upload_extensions' => ['jpg', 'jpeg', 'png', 'gif', 'webp', 'pdf', 'txt', 'zip'],
     'media_upload_max_bytes'  => 10 * 1024 * 1024,
 
     // Lib\Mailer's transactional-mail driver (see /admin/docs/admin-auth's
diff --git a/novaconium/migrations/0001_create_content_index.sql b/novaconium/migrations/0001_create_content_index.sql
index d556e04..d9446ca 100644
--- a/novaconium/migrations/0001_create_content_index.sql
+++ b/novaconium/migrations/0001_create_content_index.sql
@@ -21,5 +21,6 @@ CREATE VIRTUAL TABLE IF NOT EXISTS content_search USING fts5(route UNINDEXED, ti
 CREATE TABLE IF NOT EXISTS content_index_meta (
     id INTEGER PRIMARY KEY CHECK (id = 1),
     newest_source_mtime INTEGER NOT NULL,
+    source_count INTEGER NOT NULL DEFAULT 0,
     indexed_at TEXT NOT NULL
 );
diff --git a/novaconium/migrations/0002_create_users.sql b/novaconium/migrations/0002_create_users.sql
index e24be32..e9a1f25 100644
--- a/novaconium/migrations/0002_create_users.sql
+++ b/novaconium/migrations/0002_create_users.sql
@@ -6,5 +6,8 @@ CREATE TABLE IF NOT EXISTS users (
     role TEXT NOT NULL DEFAULT 'registered',
     user_group TEXT NOT NULL DEFAULT '',
     is_disabled INTEGER NOT NULL DEFAULT 0,
-    created_at TEXT NOT NULL
+    created_at TEXT NOT NULL,
+    verified_at TEXT,
+    verification_token TEXT,
+    verification_token_expires_at TEXT
 );
diff --git a/novaconium/migrations/0003_add_users_verification.sql b/novaconium/migrations/0003_add_users_verification.sql
deleted file mode 100644
index 2132147..0000000
--- a/novaconium/migrations/0003_add_users_verification.sql
+++ /dev/null
@@ -1,4 +0,0 @@
-ALTER TABLE users ADD COLUMN verified_at TEXT;
-ALTER TABLE users ADD COLUMN verification_token TEXT;
-ALTER TABLE users ADD COLUMN verification_token_expires_at TEXT;
-UPDATE users SET verified_at = created_at WHERE verified_at IS NULL;
diff --git a/novaconium/migrations/0004_create_comments.sql b/novaconium/migrations/0003_create_comments.sql
similarity index 78%
rename from novaconium/migrations/0004_create_comments.sql
rename to novaconium/migrations/0003_create_comments.sql
index 53df21a..1d4bc24 100644
--- a/novaconium/migrations/0004_create_comments.sql
+++ b/novaconium/migrations/0003_create_comments.sql
@@ -1,7 +1,7 @@
 CREATE TABLE comments (
     id INTEGER PRIMARY KEY AUTOINCREMENT,
     page_path TEXT NOT NULL,
-    user_id INTEGER NOT NULL REFERENCES users(id),
+    user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
     body TEXT NOT NULL,
     is_hidden INTEGER NOT NULL DEFAULT 0,
     created_at TEXT NOT NULL
diff --git a/novaconium/pages/_partials/comments/thread.twig b/novaconium/pages/_partials/comments/thread.twig
index 431fcff..884ee59 100644
--- a/novaconium/pages/_partials/comments/thread.twig
+++ b/novaconium/pages/_partials/comments/thread.twig
@@ -4,7 +4,7 @@
     (App\AdminAuth::currentUser()), 'csrfField'/'csrfToken', and
     'renderedAt' (Lib\SpamGuard::renderedAt()) in its context. See
     /admin/docs/comments for the full sidecar contract this expects, and
-    App/pages/blog/hello-world/index.php for a worked example. A page
+    App/pages/blog/comments-demo/index.php for a worked example. A page
     with no 'comments' key never includes this at all — see the
     surrounding {% if comments is defined %} in blog/_layout/layout.twig.
 #}
diff --git a/novaconium/pages/admin/docs/admin-auth/index.twig b/novaconium/pages/admin/docs/admin-auth/index.twig
index 0501c44..5468dd5 100644
--- a/novaconium/pages/admin/docs/admin-auth/index.twig
+++ b/novaconium/pages/admin/docs/admin-auth/index.twig
@@ -41,7 +41,7 @@ return [
 
     <h2>Email verification</h2>
 
-    <p>Every user created after the first must click a link emailed to their address before they can log in at all — <code>AdminAuth::attempt()</code> fails a login the same generic way it fails a disabled account or a wrong password, with no distinction made to an anonymous caller between "wrong password", "disabled", and "unverified" (novaconium/migrations/0003_add_users_verification.sql's <code>verified_at</code> column). Two accounts are exempt by design, both following the same reasoning as the last-active-admin guard elsewhere on this page — verification can't depend on a mail transport actually being configured:</p>
+    <p>Every user created after the first must click a link emailed to their address before they can log in at all — <code>AdminAuth::attempt()</code> fails a login the same generic way it fails a disabled account or a wrong password, with no distinction made to an anonymous caller between "wrong password", "disabled", and "unverified" (the <code>verified_at</code> column in <code>novaconium/migrations/0002_create_users.sql</code>). Two accounts are exempt by design, both following the same reasoning as the last-active-admin guard elsewhere on this page — verification can't depend on a mail transport actually being configured:</p>
 
     <ul>
         <li><strong>The very first user</strong> (created at <a href="/admin/users">/admin/users</a> or via the CLI below) is auto-verified at creation — there's no other admin to have vouched for them, and they're logged in immediately afterward regardless.</li>
diff --git a/novaconium/pages/admin/docs/comments/index.twig b/novaconium/pages/admin/docs/comments/index.twig
index 176542b..a519778 100644
--- a/novaconium/pages/admin/docs/comments/index.twig
+++ b/novaconium/pages/admin/docs/comments/index.twig
@@ -48,5 +48,5 @@ return [
 
     <h2>Schema</h2>
 
-    <p>Ships as a framework migration, <code>novaconium/migrations/0004_create_comments.sql</code> — a <code>comments</code> table (<code>id</code>, <code>page_path</code>, <code>user_id</code>, <code>body</code>, <code>is_hidden</code>, <code>created_at</code>) on the same <code>default</code> connection as <code>users</code>, applied automatically the first time anything touches it, no manual step. <code>page_path</code> is whatever <code>Comments::currentPagePath()</code> derives from the request (e.g. <code>/blog/comments-demo</code>) — free-text, not a foreign key, so a thread survives even if the page it was attached to is later restructured.</p>
+    <p>Ships as a framework migration, <code>novaconium/migrations/0003_create_comments.sql</code> — a <code>comments</code> table (<code>id</code>, <code>page_path</code>, <code>user_id</code>, <code>body</code>, <code>is_hidden</code>, <code>created_at</code>) on the same <code>default</code> connection as <code>users</code>, applied automatically the first time anything touches it, no manual step. <code>page_path</code> is whatever <code>Comments::currentPagePath()</code> derives from the request (e.g. <code>/blog/comments-demo</code>) — free-text, not a foreign key, so a thread survives even if the page it was attached to is later restructured.</p>
 {% endblock %}
diff --git a/novaconium/pages/admin/users/index.php b/novaconium/pages/admin/users/index.php
index d3eb20c..26c46ec 100644
--- a/novaconium/pages/admin/users/index.php
+++ b/novaconium/pages/admin/users/index.php
@@ -175,6 +175,15 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') {
             // "keep the account but shut it out", so delete is for
             // accounts that shouldn't exist at all. Any live session dies
             // on its next request (currentUser() re-checks the row).
+            //
+            // Remove the user's comments first: comments.user_id has a
+            // NOT NULL foreign key to users(id) (0003_create_comments.sql)
+            // and Lib\Db runs PRAGMA foreign_keys = ON, so deleting a user
+            // who has ever commented would otherwise raise a FOREIGN KEY
+            // constraint violation and 500. Fresh installs also get
+            // ON DELETE CASCADE on the FK, but this keeps already-migrated
+            // databases (whose FK predates that) working too.
+            Db::query('DELETE FROM comments WHERE user_id = ?', [$id]);
             Db::query('DELETE FROM users WHERE id = ?', [$id]);
             Session::flash('users_notice', "User \u{201C}{$target['username']}\u{201D} deleted.");
         }
diff --git a/novaconium/src/AdminAuth.php b/novaconium/src/AdminAuth.php
index f27f07c..61006a0 100644
--- a/novaconium/src/AdminAuth.php
+++ b/novaconium/src/AdminAuth.php
@@ -114,10 +114,11 @@ final class AdminAuth
      * exactly like a wrong password — the response never distinguishes
      * "no such user", "disabled", "unverified", and "bad password" (see
      * /admin/docs/admin-auth's email verification section). Unset
-     * verified_at means never verified; the first user ever created and
-     * every row that existed before this check shipped are backfilled to a
-     * non-null value (novaconium/migrations/0003_add_users_verification.sql)
-     * so verification only actually gates accounts created after it.
+     * verified_at means never verified (the verified_at/verification_token
+     * columns are part of novaconium/migrations/0002_create_users.sql); the
+     * first user ever created is inserted already-verified (see
+     * /admin/users and bin/create-admin-user.php), so verification only
+     * actually gates accounts created after it.
      */
     public static function attempt(string $username, string $password): bool
     {
diff --git a/novaconium/src/ContentIndexer.php b/novaconium/src/ContentIndexer.php
index cecf617..40a49ff 100644
--- a/novaconium/src/ContentIndexer.php
+++ b/novaconium/src/ContentIndexer.php
@@ -104,12 +104,21 @@ final class ContentIndexer
             $insertSearch = $pdo->prepare('INSERT INTO content_search (route, title, body) VALUES (?, ?, ?)');
 
             $newestMtime = 0;
+            $sourceCount = 0;
 
             foreach ($routes as $dir) {
                 if (in_array($dir, $config['draft_routes'], true)) {
                     continue;
                 }
 
+                // Count every non-draft routable page, regardless of
+                // whether it ends up indexed (a noindex or Response-only
+                // page still counts) — this is a stable fingerprint of the
+                // routable page *set*, so isStale() can notice a deletion,
+                // which the newest-mtime check alone can't (deleting a page
+                // only ever lowers the max mtime, never raises it).
+                $sourceCount++;
+
                 $mtime = self::sourceMtime($config['pages_dirs'], $dir);
                 $newestMtime = max($newestMtime, $mtime);
 
@@ -140,8 +149,8 @@ final class ContentIndexer
             }
 
             $pdo->prepare('DELETE FROM content_index_meta')->execute();
-            $pdo->prepare('INSERT INTO content_index_meta (id, newest_source_mtime, indexed_at) VALUES (1, ?, ?)')
-                ->execute([$newestMtime, gmdate('Y-m-d\TH:i:s\Z')]);
+            $pdo->prepare('INSERT INTO content_index_meta (id, newest_source_mtime, source_count, indexed_at) VALUES (1, ?, ?, ?)')
+                ->execute([$newestMtime, $sourceCount, gmdate('Y-m-d\TH:i:s\Z')]);
 
             $pdo->commit();
         } catch (\Throwable $e) {
@@ -166,17 +175,26 @@ final class ContentIndexer
     {
         $pdo = Db::connection();
 
-        $meta = $pdo->query('SELECT newest_source_mtime FROM content_index_meta WHERE id = 1')->fetch();
+        $meta = $pdo->query('SELECT newest_source_mtime, source_count FROM content_index_meta WHERE id = 1')->fetch();
         if ($meta === false) {
             return true;
         }
 
         $newest = 0;
+        $count = 0;
         foreach (Overlay::listPageDirs($config['pages_dirs']) as $dir) {
+            if (in_array($dir, $config['draft_routes'], true)) {
+                continue;
+            }
+            $count++;
             $newest = max($newest, self::sourceMtime($config['pages_dirs'], $dir));
         }
 
-        return $newest > (int) $meta['newest_source_mtime'];
+        // A newer source file means an edit/addition; a changed page count
+        // means a page was deleted (or a draft toggled) — the mtime check
+        // alone can't see a deletion, since removing a page only lowers the
+        // max mtime. Either signal means the index is stale.
+        return $newest > (int) $meta['newest_source_mtime'] || $count !== (int) $meta['source_count'];
     }
 
     /**
diff --git a/public/router.php b/public/router.php
index f26506b..2d61743 100644
--- a/public/router.php
+++ b/public/router.php
@@ -5,7 +5,11 @@
 $uri = urldecode(parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH));
 
 if ($uri !== '/' && str_ends_with($uri, '/')) {
-    header('Location: ' . rtrim($uri, '/'), true, 301);
+    // Preserve the query string across the canonical redirect — Apache's
+    // .htaccess does this automatically, so dev must too or post/redirect
+    // flows like /contact/?sent=1 lose their flags under `php -S`.
+    $query = parse_url($_SERVER['REQUEST_URI'], PHP_URL_QUERY);
+    header('Location: ' . rtrim($uri, '/') . ($query !== null ? '?' . $query : ''), true, 301);
     exit;
 }