Procházet zdrojové kódy

Fix R01-N26: one-shot session flash for post-delete chip

Anyone could craft `/?deleted=foo` and see the green "Sprint foo was
deleted" banner without an actual delete having happened — Twig
auto-escape ruled out XSS, but the spoof was bad UX and a small
forensic-confusion trap.

SprintController::delete now stashes the deleted sprint's name in
$_SESSION['flash_deleted_sprint_name'] and redirects to a bare `/`.
The home handler in public/index.php reads the key and unsets it on
render, so a refresh cannot re-trigger the chip and a hand-crafted URL
can't trigger it at all.

TwigViewTest pins both directions: chip renders when the flash is set
(and the sprint name is HTML-escaped per the auto-escape contract),
chip stays absent when the flash is empty.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa před 2 dny
rodič
revize
1f111176cb

+ 13 - 1
public/index.php

@@ -177,6 +177,18 @@ $router->get('/', function (Request $req) use ($view, $pdo, $users, $sprints, $a
 
     $sprintRows = $currentUser === null ? [] : $sprints->allWithCounts();
 
+    // R01-N26: one-shot session flash for the post-delete chip. Set by
+    // `SprintController::delete`; consumed (and cleared) here so a manual
+    // refresh of `/` cannot re-show the green banner. `currentUser` already
+    // forced `SessionGuard::start()`, so $_SESSION is live by this point.
+    $deletedSprintName = '';
+    if (isset($_SESSION['flash_deleted_sprint_name'])
+        && is_string($_SESSION['flash_deleted_sprint_name'])
+    ) {
+        $deletedSprintName = $_SESSION['flash_deleted_sprint_name'];
+        unset($_SESSION['flash_deleted_sprint_name']);
+    }
+
     return Response::html($view->render('home', [
         'title'             => 'Sprint Planner',
         'currentUser'       => $currentUser,
@@ -186,7 +198,7 @@ $router->get('/', function (Request $req) use ($view, $pdo, $users, $sprints, $a
         'oidcConfigured'    => OidcClient::isConfigured(),
         'localAdminEnabled' => LocalAdmin::isEnabled(),
         'authError'         => isset($req->query['auth_error']),
-        'deletedSprintName' => $req->queryString('deleted'),
+        'deletedSprintName' => $deletedSprintName,
         'csrfToken'         => SessionGuard::csrfToken(),
         'sprintRows'        => $sprintRows,
     ]));

+ 9 - 1
src/Controllers/SprintController.php

@@ -1130,7 +1130,15 @@ final class SprintController
             return Response::redirect('/sprints/' . $sprintId . '/settings?error=db_error');
         }
 
-        return Response::redirect('/?deleted=' . rawurlencode($sprint->name));
+        // R01-N26: stash the deleted sprint's name in a one-shot session
+        // flash instead of leaking it via `?deleted=<name>`. The query-string
+        // form let anyone craft `/?deleted=foo` and see a green "Sprint foo
+        // was deleted" chip without an actual delete having happened.
+        // The home route reads this key and unsets it on render; expiry on
+        // first read makes it impossible to re-trigger with a refresh.
+        SessionGuard::start();
+        $_SESSION['flash_deleted_sprint_name'] = $sprint->name;
+        return Response::redirect('/');
     }
 
     // ------------------------------------------------------------------

+ 48 - 0
tests/Http/TwigViewTest.php

@@ -59,6 +59,54 @@ final class TwigViewTest extends TestCase
         self::assertStringContainsString('/assets/js/vendor/htmx.min.js', $html);
     }
 
+    public function testHomeRendersDeletedSprintFlashWhenSet(): void
+    {
+        // R01-N26: the green "Sprint X was deleted" chip is now driven by
+        // the `deletedSprintName` view variable (sourced from a one-shot
+        // session flash, not from `?deleted=`). Pin that the chip appears
+        // for any non-empty string and that the name is HTML-escaped.
+        $html = $this->view->render('home', [
+            'title'             => 'Sprint Planner',
+            'csrfToken'         => 'tok',
+            'currentUser'       => $this->makeUser(true),
+            'schemaVersion'     => 3,
+            'dbPath'            => '/tmp/x',
+            'appEnv'            => 'production',
+            'oidcConfigured'    => false,
+            'localAdminEnabled' => true,
+            'authError'         => false,
+            'deletedSprintName' => 'Sprint <Alpha>',
+            'sprintRows'        => [],
+        ]);
+
+        self::assertStringContainsString('was deleted',         $html);
+        self::assertStringContainsString('Sprint &lt;Alpha&gt;', $html);
+        self::assertStringNotContainsString(
+            '<b>Sprint <Alpha></b>',
+            $html,
+            'sprint name must be Twig-autoescaped — never rendered raw',
+        );
+    }
+
+    public function testHomeOmitsDeletedSprintFlashWhenEmpty(): void
+    {
+        $html = $this->view->render('home', [
+            'title'             => 'Sprint Planner',
+            'csrfToken'         => 'tok',
+            'currentUser'       => $this->makeUser(true),
+            'schemaVersion'     => 3,
+            'dbPath'            => '/tmp/x',
+            'appEnv'            => 'production',
+            'oidcConfigured'    => false,
+            'localAdminEnabled' => true,
+            'authError'         => false,
+            'deletedSprintName' => '',
+            'sprintRows'        => [],
+        ]);
+
+        self::assertStringNotContainsString('was deleted', $html);
+    }
+
     public function testHomeForAnonymousUserHidesRuntimePanel(): void
     {
         $html = $this->view->render('home', [