feat(banlist): demo download, comment gates, and in-place comment composer

ARCHITECTURE.md

@@ -361,7 +361,8 @@ honours `config.enablepubliccomments` and `banlist.hideadminname`;
 anonymous GET of comm comments is 404 when `config.enablecomms` is off,
 matching `/comms`; POST /
 PATCH reuse `bans.add_comment` / `bans.edit_comment`, and PATCH is author
-or Owner on both REST and the RPC handler; DELETE is Owner via
+or Owner on both REST and the RPC handler; RPC add/edit of `ctype` S/P
+also requires BanSubmissions / BanProtests; DELETE is Owner via
 `bans.remove_comment`), `/settings` GET+PATCH (dedicated; never
 `smtp.pass` or `telemetry.instance_id`).
 

web/api/handlers/bans.php

@@ -19,6 +19,48 @@ function _api_bans_steam_api_key(): string
     return $key;
 }
 
+/**
+ * Validate the upload callback pair before linking a demo to a new ban.
+ *
+ * @return bool True when a complete, valid attachment should be inserted.
+ */
+function _api_bans_validate_demo_attachment(string $filename, string $originalName): bool
+{
+    // The add-ban form has historically sent numeric `0` when no upload
+    // was selected. Preserve that wire sentinel as the empty state.
+    $hasFilename = $filename !== '' && $filename !== '0';
+    $hasOriginalName = $originalName !== '';
+    if (!$hasFilename && !$hasOriginalName) {
+        return false;
+    }
+    if (!$hasFilename || !$hasOriginalName) {
+        throw new ApiError('validation', 'The demo upload is incomplete.', 'dfile');
+    }
+    if (!preg_match('/^[a-f0-9]{32}$/D', $filename)) {
+        throw new ApiError('validation', 'The demo upload reference is invalid.', 'dfile');
+    }
+    // `:prefix_demos.origname` is VARCHAR(128); reject over-width input
+    // before MariaDB strict mode turns the INSERT into a generic 500.
+    if (!mb_check_encoding($originalName, 'UTF-8') || mb_strlen($originalName, 'UTF-8') > 128) {
+        throw new ApiError('validation', 'The demo filename is invalid.', 'dname');
+    }
+
+    // SECURITY-REVIEW: `$filename` is API input. Require the exact
+    // uploader-generated hash shape, reject links, and contain the resolved
+    // regular file inside SB_DEMOS before persisting a public download row.
+    $demoRoot = realpath(SB_DEMOS);
+    $path = rtrim(SB_DEMOS, '/\\') . DIRECTORY_SEPARATOR . $filename;
+    $resolvedPath = realpath($path);
+    $insideDemoRoot = $demoRoot !== false
+        && $resolvedPath !== false
+        && str_starts_with($resolvedPath, $demoRoot . DIRECTORY_SEPARATOR);
+    if ($resolvedPath === false || !$insideDemoRoot || is_link($path) || !is_file($resolvedPath)) {
+        throw new ApiError('validation', 'The uploaded demo is no longer available.', 'dfile');
+    }
+
+    return true;
+}
+
 function api_bans_add(array $params): array
 {
     global $userbank;
@@ -158,6 +200,8 @@ function api_bans_add(array $params): array
         }
     }
 
+    $attachDemo = _api_bans_validate_demo_attachment($dfile, $dname);
+
     $GLOBALS['PDO']->query(
         "INSERT INTO `:prefix_bans`(created,type,ip,authid,name,ends,length,reason,aid,adminIp,admin_name) VALUES
         (UNIX_TIMESTAMP(),?,?,?,?,(UNIX_TIMESTAMP() + ?),?,?,?,?,?)"
@@ -175,7 +219,7 @@ function api_bans_add(array $params): array
     ]);
     $newId = (int)$GLOBALS['PDO']->lastInsertId();
 
-    if ($dname && $dfile && preg_match('/^[a-z0-9]*$/i', $dfile)) {
+    if ($attachDemo) {
         $GLOBALS['PDO']->query("INSERT INTO `:prefix_demos`(demid,demtype,filename,origname) VALUES(?,'B',?,?)")
             ->execute([$newId, $dfile, $dname]);
     }
@@ -288,6 +332,52 @@ function api_bans_paste(array $params): array
     throw new ApiError('player_not_found', "Can't get player info for " . htmlspecialchars($name) . '. Player is not on the server anymore!');
 }
 
+function _api_bans_comment_parent_exists(string $ctype, int $id): bool
+{
+    return match ($ctype) {
+        'B' => $GLOBALS['PDO']->query('SELECT bid FROM `:prefix_bans` WHERE bid = ?')->single([$id]) !== false,
+        'C' => $GLOBALS['PDO']->query('SELECT bid FROM `:prefix_comms` WHERE bid = ?')->single([$id]) !== false,
+        'S' => $GLOBALS['PDO']->query('SELECT subid FROM `:prefix_submissions` WHERE subid = ?')->single([$id]) !== false,
+        'P' => $GLOBALS['PDO']->query('SELECT pid FROM `:prefix_protests` WHERE pid = ?')->single([$id]) !== false,
+        default => false,
+    };
+}
+
+/**
+ * Enforce the parent surface's authorization before reading or mutating
+ * moderation-queue comments.
+ */
+function _api_bans_can_manage_comment_type(string $ctype): bool
+{
+    global $userbank;
+
+    // SECURITY-REVIEW: submission and protest comments expose moderation
+    // records, so a generic web-admin session is not sufficient.
+    return match ($ctype) {
+        'B', 'C' => $userbank->is_admin(),
+        'S' => $userbank->HasAccess(WebPermission::mask(
+            WebPermission::Owner,
+            WebPermission::BanSubmissions,
+        )),
+        'P' => $userbank->HasAccess(WebPermission::mask(
+            WebPermission::Owner,
+            WebPermission::BanProtests,
+        )),
+        default => false,
+    };
+}
+
+function _api_bans_comment_subject_label(string $ctype): string
+{
+    return match ($ctype) {
+        'B' => 'ban',
+        'C' => 'comm block',
+        'S' => 'submission',
+        'P' => 'protest',
+        default => 'record',
+    };
+}
+
 function api_bans_add_comment(array $params): array
 {
     global $userbank, $username;
@@ -313,13 +403,26 @@ function api_bans_add_comment(array $params): array
     if ($redir === null) {
         throw new ApiError('bad_type', 'Bad comment type.');
     }
+    if (!_api_bans_can_manage_comment_type($ctype)) {
+        throw new ApiError('forbidden', 'You do not have permission to manage comments for this record.');
+    }
+    if ($bid <= 0) {
+        throw new ApiError('validation', 'Missing or invalid record id.', 'bid');
+    }
+    if ($ctext === '') {
+        throw new ApiError('validation', 'Comment is required.', 'ctext');
+    }
+    if (!_api_bans_comment_parent_exists($ctype, $bid)) {
+        throw new ApiError('not_found', 'Record not found.', 'bid', 404);
+    }
 
     $GLOBALS['PDO']->query(
         "INSERT INTO `:prefix_comments`(bid,type,aid,commenttxt,added) VALUES (?,?,?,?,UNIX_TIMESTAMP())"
     )->execute([$bid, $ctype, $userbank->GetAid(), $ctext]);
     $cid = (int) $GLOBALS['PDO']->lastInsertId();
 
-    Log::add(LogType::Message, 'Comment Added', "$username added a comment for ban #$bid");
+    $subject = _api_bans_comment_subject_label($ctype);
+    Log::add(LogType::Message, 'Comment Added', "$username added a comment for $subject #$bid");
 
     return [
         'reload'  => true,
@@ -336,6 +439,8 @@ function api_bans_add_comment(array $params): array
 function api_bans_edit_comment(array $params): array
 {
     global $userbank, $username;
+    $hasBid = array_key_exists('bid', $params);
+    $bid   = (int)($params['bid']   ?? 0);
     $cid   = (int)($params['cid']   ?? 0);
     $ctype = (string)($params['ctype'] ?? '');
     $ctext = trim((string)($params['ctext'] ?? ''));
@@ -354,23 +459,46 @@ function api_bans_edit_comment(array $params): array
     if ($redir === null) {
         throw new ApiError('bad_type', 'Bad comment type.');
     }
-
-    $row = $GLOBALS['PDO']->query(
-        "SELECT cid, aid FROM `:prefix_comments` WHERE cid = ?"
-    )->single([$cid]);
-    if (!$row) {
-        throw new ApiError('not_found', 'Comment not found.', null, 404);
+    if (!_api_bans_can_manage_comment_type($ctype)) {
+        throw new ApiError('forbidden', 'You do not have permission to manage comments for this record.');
+    }
+    if ($cid <= 0) {
+        throw new ApiError('validation', 'Missing or invalid comment id.', 'cid');
+    }
+    if ($hasBid && $bid <= 0) {
+        throw new ApiError('validation', 'Missing or invalid record id.', 'bid');
+    }
+    if ($ctext === '') {
+        throw new ApiError('validation', 'Comment is required.', 'ctext');
     }
 
-    $authorAid = (int) $row['aid'];
-    $canEdit = $authorAid === $userbank->GetAid() || $userbank->HasAccess(WebPermission::Owner);
-    if (!$canEdit) {
-        throw new ApiError('forbidden', 'You can only edit your own comments.', null, 403);
+    $comment = $GLOBALS['PDO']
+        ->query('SELECT aid, bid, type FROM `:prefix_comments` WHERE cid = ?')
+        ->single([$cid]);
+    if (!$comment) {
+        throw new ApiError('not_found', 'Comment not found.', 'cid', 404);
+    }
+    if ((string)$comment['type'] !== $ctype) {
+        throw new ApiError('validation', 'Comment type does not match.', 'ctype');
+    }
+    if ((int)$comment['aid'] !== $userbank->GetAid()
+        && !$userbank->HasAccess(WebPermission::Owner)) {
+        throw new ApiError('forbidden', 'You do not have permission to edit this comment.');
+    }
+    $commentBid = (int)$comment['bid'];
+    if ($hasBid && $commentBid !== $bid) {
+        throw new ApiError('validation', 'Comment does not belong to this record.', 'bid');
+    }
+    $bid = $commentBid;
+    if (!_api_bans_comment_parent_exists($ctype, $bid)) {
+        throw new ApiError('not_found', 'Record not found.', 'bid', 404);
     }
 
     $GLOBALS['PDO']->query(
-        "UPDATE `:prefix_comments` SET commenttxt = ?, editaid = ?, edittime = UNIX_TIMESTAMP() WHERE cid = ?"
-    )->execute([$ctext, $userbank->GetAid(), $cid]);
+        "UPDATE `:prefix_comments`
+         SET commenttxt = ?, editaid = ?, edittime = UNIX_TIMESTAMP()
+         WHERE cid = ? AND bid = ? AND type = ?"
+    )->execute([$ctext, $userbank->GetAid(), $cid, $bid, $ctype]);
 
     Log::add(LogType::Message, 'Comment Edited', "$username edited comment #$cid");
 
@@ -453,19 +581,38 @@ function api_bans_remove_comment(array $params): array
     $ctype = (string)($params['ctype'] ?? '');
     $page  = (int)($params['page']  ?? -1);
 
+    if ($cid <= 0) {
+        throw new ApiError('validation', 'Missing or invalid comment id.', 'cid');
+    }
+    if (!in_array($ctype, ['B', 'C', 'S', 'P'], true)) {
+        throw new ApiError('bad_type', 'Bad comment type.');
+    }
+
+    $comment = $GLOBALS['PDO']
+        ->query('SELECT type FROM `:prefix_comments` WHERE cid = ?')
+        ->single([$cid]);
+    if (!$comment) {
+        throw new ApiError('not_found', 'Comment not found.', 'cid', 404);
+    }
+    if ((string)$comment['type'] !== $ctype) {
+        throw new ApiError('validation', 'Comment type does not match.', 'ctype');
+    }
+
     $pagelink = $page !== -1 ? '&page=' . $page : '';
     // #1275 — match the section-aware redirect shape from
     // api_bans_add_comment / api_bans_edit_comment so a deleted
     // comment lands the admin back on the queue they were on.
-    $redir = match ($ctype) {
+    $redirects = [
         'B' => '?p=banlist' . $pagelink,
         'C' => '?p=commslist' . $pagelink,
         'S' => '?p=admin&c=bans&section=submissions',
         'P' => '?p=admin&c=bans&section=protests',
-        default => '?p=admin&c=bans',
-    };
+    ];
+    $redir = $redirects[$ctype];
 
-    $GLOBALS['PDO']->query("DELETE FROM `:prefix_comments` WHERE cid = ?")->execute([$cid]);
+    $GLOBALS['PDO']->query(
+        'DELETE FROM `:prefix_comments` WHERE cid = ? AND type = ?'
+    )->execute([$cid, $ctype]);
     Log::add(LogType::Message, 'Comment Deleted', "$username deleted comment #$cid");
 
     return [

web/getdemo.php

@@ -12,7 +12,7 @@
  *
  * `:prefix_demos` carries one row per uploaded demo with:
  *   - demtype  enum('B','S')  — B = Ban, S = Submission
- *   - demid    int            — fk to :prefix_bans.bid OR :prefix_submissions.id
+ *   - demid    int            — fk to :prefix_bans.bid OR :prefix_submissions.subid
  *   - filename text           — server-side basename under SB_DEMOS
  *   - origname text           — display name shown to the downloader
  *
@@ -23,10 +23,10 @@
  *      column might carry (tampered DB row, partial migration). We don't
  *      run user-supplied paths through this — but we DO run a row a DB
  *      compromise could have rewritten, so the LFI guard is layered.
- *   2. `in_array(scandir(SB_DEMOS), …, true)` ensures the file we resolve
- *      is actually one of the listed demos in the directory — symlinks
- *      pointing outside SB_DEMOS aren't valid even if `file_exists()`
- *      would have accepted them.
+ *   2. `in_array(scandir(SB_DEMOS), …, true)` ensures the requested
+ *      basename is present in the demo directory.
+ *   3. `is_link()` rejects symlinks, then `realpath()` containment proves
+ *      the canonical file path remains under the canonical demo root.
  *
  * Different from the legacy 1.x entry point (the rewrite does not
  * trace structurally back):
@@ -99,11 +99,31 @@ function getdemo_disposition_header(string $name): string
 $onDisk = basename((string) $row['filename']);
 $origin = (string) ($row['origname'] ?? '') !== '' ? (string) $row['origname'] : $onDisk;
 $path   = SB_DEMOS . '/' . $onDisk;
+if ($onDisk === '' || str_contains($onDisk, "\0")) {
+    getdemo_die(404, 'Demo file is no longer on disk.');
+}
 
+// SECURITY-REVIEW: the filename originates in an upload-backed database
+// row. Reject links and require canonical root containment before reading.
 $listing = is_dir(SB_DEMOS) ? scandir(SB_DEMOS) : false;
-if ($listing === false || !in_array($onDisk, $listing, true) || !is_file($path)) {
+$demoRoot = realpath(SB_DEMOS);
+$resolvedPath = realpath($path);
+$insideDemoRoot = $demoRoot !== false
+    && $resolvedPath !== false
+    && str_starts_with(
+        $resolvedPath,
+        rtrim($demoRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR,
+    );
+if (
+    $listing === false
+    || !in_array($onDisk, $listing, true)
+    || is_link($path)
+    || !$insideDemoRoot
+    || !is_file($resolvedPath)
+) {
     getdemo_die(404, 'Demo file is no longer on disk.');
 }
+$path = $resolvedPath;
 
 $size = filesize($path);
 if ($size === false) {

web/includes/View/BanListView.php

@@ -9,10 +9,9 @@
  * `$ban_list` items expose `bid|name|steam|state|length_human|
  * banned_human|sname|can_edit_ban|can_unban` plus avatar metadata; the
  * page chrome reads `$total_bans`, `$can_export`, `$hidetext`,
- * `$searchlink`, `$ban_nav`, the comment-edit scratch pad
- * (`$comment`, `$commenttype`, `$commenttext`, `$ctype`, `$cid`,
- * `$page`, `$canedit`, `$othercomments`), and the testid hooks reach
- * the per-row `state`.
+ * `$searchlink`, `$ban_nav`, and the testid hooks reach the per-row
+ * `state`. Add / Edit comments open the player drawer; there is no
+ * `?comment=` editor on this page.
  *
  * Each row also carries the legacy keys (`ban_id|player|class|
  * reban_link|edit_link|…`) so any third-party theme that forked the
@@ -32,9 +31,6 @@ final class BanListView extends View
 
     /**
      * @param list<array<string,mixed>>           $ban_list
-     * @param int|false                           $comment        Bid being commented on, or false when not in comment-edit mode.
-     * @param int                                 $page           Active pagination page (or -1 when not paginated).
-     * @param array<int, array<string,mixed>>|string $othercomments  Sibling comments shown beneath the editor; "None" string when the ban has no other comments.
      * @param list<array{sid: int, name: string}> $server_list    Enabled servers for the public filter bar's `<select name="server">` (#1226).
      * @param array{search: string, server: string, time: string, state: string} $filters Current filter state — drives the sticky filter bar's pre-fill + active selected `<option>` (#1226 + #1352).
      */
@@ -44,14 +40,6 @@ public function __construct(
         public readonly int $total_bans,
         public readonly bool $view_bans,
         public readonly bool $view_comments,
-        public readonly int|false $comment,
-        public readonly string $commenttype,
-        public readonly string $commenttext,
-        public readonly string $ctype,
-        public readonly string $cid,
-        public readonly int $page,
-        public readonly bool $canedit,
-        public readonly array|string $othercomments,
         public readonly string $searchlink,
         public readonly string $hidetext,
         public readonly bool $hideadminname,