Skip to content

fix(admin-edit-comms): run not-found guard before permission check (#1410) #1415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rumblefrog merged 1 commit into from
May 18, 2026
Merged

fix(admin-edit-comms): run not-found guard before permission check (#1410) #1415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 34 additions & 34 deletions web/pages/admin.edit.comms.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,40 @@
$GLOBALS['PDO']->bind(':bid', $_GET['id']);
$res = $GLOBALS['PDO']->single();

isset($_GET["page"]) ? $pagelink = "&page=" . urlencode($_GET["page"]) : $pagelink = "";

// Not-found guard MUST come BEFORE the permission check (#1410). On a
// request for a non-existent cid `$res` is false; the perm check below
// then reads `$res['aid']` / `$res['gid']` which PHP 8.x evaluates to
// `null` while emitting an `E_WARNING: Trying to access array offset on
// value of type bool`. The `null != $userbank->GetAid()` comparison
// then trips the deny-access branch, so the user sees the misleading
// "You don't have access to this!" toast for what's actually a stale
// link / typo'd cid (and incidentally a side-channel for "does this
// cid exist" enumeration). Running the existence check first surfaces
// the correct "block has been deleted" toast AND clears the warning.
//
// PageDie after the toast emit (parity with every OTHER Toast::emit
// branch in this file — L22 / L31 / the perm check below). Without
// terminating here the perm check would still dereference `$res`
// (false) and emit the warning; and the Renderer::render + the inline
// `<script>` further down dereference `$res` too, where the warning
// lands literally inside the script's string literals (the
// `selectLengthTypeReason` short-echo block that builds JS string
// args from the row fields) — every warning is plain text appearing
// INSIDE quoted JS strings, which is a JavaScript "Invalid or
// unexpected token" parse error. PageDie cuts the render before any
// of that runs.
if (!$res) {
\Sbpp\View\Toast::emit(
'error',
'Error',
'There was an error getting details. Maybe the block has been deleted?',
'index.php?p=commslist' . $pagelink,
);
PageDie();
}

if (!$userbank->HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::EditAllBans)) && (!$userbank->HasAccess(WebPermission::EditOwnBans) && $res['aid'] != $userbank->GetAid()) && (!$userbank->HasAccess(WebPermission::EditGroupBans) && $res['gid'] != $userbank->GetProperty('gid'))) {
\Sbpp\View\Toast::emit(
'error',
Expand All @@ -49,8 +83,6 @@
PageDie();
}

isset($_GET["page"]) ? $pagelink = "&page=" . urlencode($_GET["page"]) : $pagelink = "";

// #1402: per-field inline-error setters now emit vanilla DOM calls
// instead of the MooTools `$('id').setStyle('display', 'block')` shape
// that died with sourcebans.js at #1123 D1. Each error tuple becomes
Expand Down Expand Up @@ -171,38 +203,6 @@
}
}

if (!$res) {
\Sbpp\View\Toast::emit(
'error',
'Error',
'There was an error getting details. Maybe the block has been deleted?',
'index.php?p=commslist' . $pagelink,
);
// PageDie after the toast emit (parity with every OTHER
// Toast::emit branch in this file — L22 / L31 / L49 all
// PageDie after their respective emits). Pre-#1403 this
// branch didn't terminate because the legacy `ShowBox` call
// didn't either: ShowBox itself navigated the browser, so
// letting the rest of the page render was harmless even
// though the SELECT had returned no row (the navigation
// beat the render to the user's eyeballs). Post-lift the
// toast carries the redirect explicitly via the helper's
// 4th arg + theme.js's 1500ms post-paint settle, so the
// user sees the toast, the chrome footer, and then bounces
// to commslist — but they MUST NOT see the borked form
// skeleton in between. Dereferencing $res (false at this
// point because the SELECT returned no row) in
// `Renderer::render` and the inline `<script>` further down
// emits PHP warnings under `display_errors=On`, and those
// warnings land literally inside the script's string
// literals (the selectLengthTypeReason short-echo block
// that builds JS string args from the row fields) — every
// warning is plain text appearing INSIDE quoted JS strings,
// which is a JavaScript `Invalid or unexpected token` parse
// error. PageDie cuts the render before any of that runs.
PageDie();
}

\Sbpp\View\Renderer::render($theme, new \Sbpp\View\AdminCommsEditView(
ban_name: (string) $res['name'],
ban_authid: trim((string) $res['authid']),
Expand Down
271 changes: 271 additions & 0 deletions web/tests/integration/AdminEditCommsCheckOrderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,271 @@
<?php
// SourceBans++ (c) 2014-2026 SourceBans++ Dev Team
// Licensed under Creative Commons Attribution-NonCommercial-ShareAlike 3.0.
// See LICENSE.md for the full license text and THIRD-PARTY-NOTICES.txt for attributions.

declare(strict_types=1);

namespace Sbpp\Tests\Integration;

use PHPUnit\Framework\TestCase;

/**
* Issue #1410: `web/pages/admin.edit.comms.php` ran the permission check
* BEFORE the not-found guard, so a non-Owner admin who hit the page
* with a `?id=` pointing at a non-existent cid:
*
* 1. Saw the wrong toast — the misleading "You don't have access to
* this!" body instead of the correct "There was an error getting
* details. Maybe the block has been deleted?" body. The user was
* then confused about WHAT went wrong (perm vs deleted-block),
* and the perm-flavored body also worked as a side-channel for
* "does this cid exist?" enumeration (the perm check fires for
* both "you don't own a real block" AND "the block doesn't
* exist", so an unauthorised user can't tell the two apart).
* 2. Triggered a PHP 8.x `E_WARNING: Trying to access array offset
* on value of type bool` on every such request. The perm check
* reads `$res['aid']` / `$res['gid']`; PHP 8.x evaluates
* `false['aid']` to `null` but emits the warning. PHP 9 will
* turn the warning into a `TypeError` — the bug shape is on a
* paths-to-fatal trajectory.
*
* Root cause: the original code ran the perm check at the top of the
* file (after the SELECT) but the `if (!$res)` guard was 130 lines
* further down, AFTER the POST-handling block. The fix moves the
* not-found guard to immediately after the SELECT, BEFORE the perm
* check, so `$res` is known-truthy by the time the perm check runs.
*
* # Why static-shape tests (not process-isolated `require`)
*
* The bug condition is "perm check runs before not-found guard on a
* bad-cid request". All three runtime paths that exercise the bug
* (bad cid + admin / bad cid + non-Owner / good cid + non-Owner)
* unconditionally call `PageDie()` (`exit;` after rendering the
* footer). PHPUnit's `RunInSeparateProcess` mode relies on the test
* method returning normally so the subprocess can write its result
* back; `exit` mid-test breaks the serializer and PHPUnit reports
* the test as errored. Sister `ToastEmitRegressionTest.php`
* documents the same constraint (lines 425-432) and reaches for
* static-grep tests for exactly the same reason.
*
* The static-shape pins below directly catch the bug:
*
* - {@see testNotFoundGuardRunsBeforePermissionCheck} — the
* load-bearing assertion. If a future refactor moves the
* `if (!$res)` block back down below the perm check, this test
* fails before the page does in production.
* - {@see testNotFoundBranchEmitsNotFoundToastBody} — pins
* the user-visible toast body the not-found branch surfaces.
* - {@see testPermissionDeniedBranchEmitsNoAccessToastBody} — pins
* the user-visible toast body the perm-denied branch surfaces.
* This is the regression guard for "the fix mustn't accidentally
* also weaken the perm check"; a future cleanup that drops the
* perm check entirely would fail here.
* - {@see testNotFoundGuardCallsPageDieAfterToast} — pins
* that the not-found branch terminates via `PageDie()` after the
* toast emit, so the buggy `$res['aid']` read in the perm check
* (and the inline-script string-builds further down) never run
* on the bad-cid path.
*
* The runtime side is covered by the existing
* `web/tests/e2e/specs/flows/admin-edit-comms-toast.spec.ts` (E2E
* suite, which doesn't have PHPUnit's serializer constraint) — that
* spec drives the page end-to-end and asserts the visible toast
* paints. This static-shape gate is the fast-feedback complement.
*
* The bug surfaced during the #1403 toast-lift adversarial review.
*/
final class AdminEditCommsCheckOrderTest extends TestCase
{
/** Path resolved at runtime — `ROOT` is defined by the test bootstrap. */
private function pageContents(): string
{
$contents = (string) @file_get_contents(ROOT . 'pages/admin.edit.comms.php');
$this->assertNotEmpty(
$contents,
'admin.edit.comms.php must exist and be readable; #1410 regression guard is meaningless otherwise.',
);
return $contents;
}

/**
* The load-bearing assertion. The `if (!$res)` not-found guard
* MUST come before the Owner|EditAllBans permission check in
* `admin.edit.comms.php`. Reversing the order is the bug
* (#1410): `$res['aid']` / `$res['gid']` reads in the perm
* check evaluate to `null` on a falsy `$res`, the user gets
* the misleading "You don't have access" toast, AND PHP 8.x
* emits an `E_WARNING`.
*/
public function testNotFoundGuardRunsBeforePermissionCheck(): void
{
$contents = $this->pageContents();

// Anchor on substrings unique to each guard. The not-found
// anchor is the literal `if (!$res) {` opener — the only
// occurrence in the file. The perm-check anchor is the
// `HasAccess(WebPermission::mask(WebPermission::Owner, …))`
// call, also unique (no other site combines Owner with
// EditAllBans).
$notFoundPos = strpos($contents, 'if (!$res) {');
$permCheckPos = strpos(
$contents,
'HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::EditAllBans))',
);

$this->assertNotFalse(
$notFoundPos,
'Expected `if (!$res) {` not-found guard in admin.edit.comms.php — was it renamed or removed?',
);
$this->assertNotFalse(
$permCheckPos,
'Expected the `HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::EditAllBans))` perm check in admin.edit.comms.php — was it renamed or removed?',
);

$this->assertLessThan(
$permCheckPos,
$notFoundPos,
"#1410: The `if (!\$res)` not-found guard MUST come BEFORE the "
. "Owner|EditAllBans permission check in admin.edit.comms.php. "
. "Reversing the order is the bug: on a request for a non-existent "
. "cid `\$res` is false, the perm check then reads `\$res['aid']` / "
. "`\$res['gid']` which PHP 8.x evaluates to null while emitting "
. "E_WARNING (PHP 9 will fatal), and the user sees the misleading "
. "\"You don't have access to this!\" toast instead of the correct "
. "\"There was an error getting details. Maybe the block has been "
. "deleted?\" toast.",
);
}

/**
* Pin the user-visible toast body the not-found branch surfaces.
* A future refactor that "tidies up" the message (e.g. drops the
* "Maybe the block has been deleted?" hint, or genericises the
* body to "Not found") would regress the UX without firing any
* of the structural gates. The body is what the user reads to
* understand what just happened.
*/
public function testNotFoundBranchEmitsNotFoundToastBody(): void
{
$contents = $this->pageContents();

$this->assertStringContainsString(
'There was an error getting details. Maybe the block has been deleted?',
$contents,
'#1410: The not-found branch must surface a body explaining the most '
. 'likely cause (cid was deleted / stale link). Dropping the second '
. 'sentence loses the actionable hint; renaming the toast loses the '
. 'continuity with the corresponding branch on sister '
. '`admin.edit.ban.php` which uses the same phrasing.',
);
}

/**
* Pin the user-visible toast body the perm-denied branch surfaces.
* This is the regression guard for "the fix mustn't accidentally
* also weaken the perm check" (the task description called this
* out explicitly). A future PR that removes the perm check
* entirely — or genericises the body to share with the not-found
* branch — would silently regress security by surfacing the
* benign "block has been deleted" message even when the user is
* genuinely unauthorized to edit an existing block.
*/
public function testPermissionDeniedBranchEmitsNoAccessToastBody(): void
{
$contents = $this->pageContents();

$this->assertStringContainsString(
"You don't have access to this!",
$contents,
'#1410: The perm-denied branch must still emit the "no access" toast. '
. 'Removing it would silently surface the benign "block has been '
. 'deleted" message to a user who is genuinely unauthorized to edit '
. 'an existing block — security regression.',
);

// Defensive: the perm check itself must still be present.
// String-matching the full ternary is brittle, so we just
// assert the `HasAccess(WebPermission::EditOwnBans)` /
// `HasAccess(WebPermission::EditGroupBans)` arms survive as
// the per-aid / per-gid scope gates. Drop both arms and the
// perm check collapses to "owner-only".
$this->assertStringContainsString(
'HasAccess(WebPermission::EditOwnBans)',
$contents,
'#1410: The perm check\'s EditOwnBans arm must survive — without '
. 'it a non-Owner admin can\'t edit their own blocks even when '
. 'they have the per-row flag.',
);
$this->assertStringContainsString(
'HasAccess(WebPermission::EditGroupBans)',
$contents,
'#1410: The perm check\'s EditGroupBans arm must survive — without '
. 'it a non-Owner admin can\'t edit their group\'s blocks even '
. 'when they have the per-row flag.',
);
}

/**
* Pin that the not-found branch terminates via `PageDie()` after
* the `Toast::emit` call. Without the terminator the perm check
* (which reads `$res['aid']` / `$res['gid']`) AND the Renderer +
* inline `<script>` (which read `$res['name']` / `$res['authid']` /
* `$res['length']` / `$res['type']` / `$res['reason']`) all run
* against a falsy `$res`, emitting one PHP `E_WARNING` per
* dereference — and the warnings land literally inside the
* `<script>` body via the `selectLengthTypeReason('<?=...?>', ...)`
* short-echo block, producing a JavaScript "Invalid or unexpected
* token" parse error that kills the page-tail hydration. The
* `PageDie()` is what stops the cascade.
*/
public function testNotFoundGuardCallsPageDieAfterToast(): void
{
$contents = $this->pageContents();

// Slice the file from the `if (!$res) {` opener to the next
// closing `}` at column 0 (the canonical "end of guard block"
// shape in this file). The slice must contain BOTH the
// Toast::emit call AND a PageDie() call.
$start = strpos($contents, 'if (!$res) {');
$this->assertNotFalse($start, 'Expected `if (!$res) {` block opener.');

// Find the matching `}` — the file uses one-line `}` at
// column 0 to close each guard block; grab the first such
// occurrence after the opener.
$end = strpos($contents, "\n}", $start);
$this->assertNotFalse($end, 'Expected matching `}` for the not-found guard.');

$slice = substr($contents, $start, $end - $start);

$this->assertStringContainsString(
'Toast::emit(',
$slice,
'#1410: The not-found branch must emit a toast (the user feedback).',
);
$this->assertStringContainsString(
'PageDie()',
$slice,
'#1410: The not-found branch MUST terminate via `PageDie()` after '
. 'the `Toast::emit` call. Without termination the perm check and '
. 'the page-tail script run against `$res === false`, each '
. 'dereference emits a PHP warning, and the warnings land literally '
. 'inside the inline `<script>` body producing a JavaScript parse '
. 'error. PageDie cuts the render before any of that runs.',
);

// Ordering: the toast must emit BEFORE the PageDie (otherwise
// PageDie's `exit;` fires before `echo` and the user sees
// nothing).
$toastPos = strpos($slice, 'Toast::emit(');
$diePos = strpos($slice, 'PageDie()');
$this->assertNotFalse($toastPos);
$this->assertNotFalse($diePos);
$this->assertLessThan(
$diePos,
$toastPos,
'#1410: `Toast::emit(...)` must run BEFORE `PageDie()` so the toast '
. 'echoes onto the response before the chrome footer flushes + '
. '`exit;` terminates the request.',
);
}
}
Loading