From 4f3cba665bc96420374b3d04e7ad64a74abfb0a2 Mon Sep 17 00:00:00 2001 From: Maximiliano Jabase Date: Mon, 31 Aug 2026 21:01:31 -0300 Subject: [PATCH 1/3] keep steamid64 html pattern braces out of smarty Smarty treated {17} as a tag, so native HTML validation rejected a valid 17-digit SteamID64. --- AGENTS.md | 35 +++- web/includes/SteamID/SteamID.php | 12 +- .../comms-add-steamid-validation.spec.ts | 43 +++++ .../SteamIDValidationOrderTest.php | 153 ++++++++++++++---- web/themes/default/page_admin_bans_add.tpl | 2 +- web/themes/default/page_admin_comms_add.tpl | 13 +- .../page_admin_edit_admins_details.tpl | 2 +- web/themes/default/page_admin_edit_ban.tpl | 2 +- web/themes/default/page_admin_edit_comms.tpl | 2 +- web/themes/default/page_submitban.tpl | 2 +- 10 files changed, 213 insertions(+), 53 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 965142fad..a13d7396e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -776,7 +776,8 @@ of the diff ship together or not at all. `api_comms_add` / `api_bans_add` / `api_admins_add` for the canonical reference shape. The constant's docblock spells out the contract: byte-for-byte symmetry with the form template's - `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"`, the load-bearing + *rendered* `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"` + (template source wraps `{17}` in `{literal}`), the load-bearing `D` modifier (without it `STEAM_0:0:1\n` slips past the gate and 500s on `toSteam2()`), and the deliberate asymmetry with `ID_PATTERNS` (bracketless Steam3 `U:1:N` is excluded from the @@ -787,10 +788,12 @@ of the diff ship together or not at all. the pre-#1423-follow-up-#4 hand-rolled copies silently missed the `D` modifier, producing the newline-bypass class. The client-side native validation in the corresponding form template uses the - matching `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"` (HTML's - `pattern` attribute is implicitly anchored `^…$`, so the PHP - regex carries explicit `^…$`); the browser-native popover is the - UX-first gate that fires BEFORE the IIFE calls `sb.api.call`; the + matching `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}"` + (HTML's `pattern` attribute is implicitly anchored `^…$`, so the PHP + regex carries explicit `^…$`; `{literal}` keeps the `{17}` + quantifier out of Smarty's delimiter parser so a 17-digit SteamID64 + actually matches). The browser-native popover is the UX-first gate + that fires BEFORE the IIFE calls `sb.api.call`; the server-side `preg_match` is the load-bearing security gate for curl-driven / third-party-theme callers that bypass it. @@ -945,7 +948,7 @@ of the diff ship together or not at all. `web/tests/integration/SteamIDValidationOrderTest.php` static- shape-pins the validate-then-convert order across every page handler. The form templates also carry the same - `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"` + + `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}"` + actionable `title="…"` as the JSON-flow add-form templates so the browser-native popover surfaces the same error message pre-flight. @@ -2011,6 +2014,16 @@ without a paired theme toggle in the wizard chrome. `View::DELIMITERS`. `page_youraccount.tpl` was on this list before #1123 B20 rewrote it in standard `{ }` delimiters; do NOT regress it back to `-{ … }-` without a paired edit here. +- Regex quantifiers that use braces (`\d{17}`, `[0-9]{1,3}`, …) in a + `.tpl` file MUST be wrapped in `{literal}…{/literal}` (or written + as `{ldelim}17{rdelim}`). Smarty treats `{17}` as a tag. The Steam + ID `pattern` attributes are the canonical site: + `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}"` + renders as `\d{17}` in the HTML the browser validates against. + JS inside an existing `{literal}` block is already safe. PHP + regexes are not Smarty and stay `\d{17}`. Regression: + `SteamIDValidationOrderTest::testRenderedSteamPatternKeepsSeventeenDigitQuantifier` + + `testTemplatesHaveNoBareDigitBraceQuantifiers`. ### Install wizard (`web/install/`) @@ -4240,6 +4253,16 @@ the spec, target a 1920px viewport, not 1440px. cannot-throw guarantee in handler code, so wrapping it in `try/catch` is a code smell signalling the upstream gate is missing. +- Bare `\d{17}` (or any `{}` regex quantifier) in a Smarty + `.tpl` `pattern` attribute without `{literal}` / `{ldelim}` → + Smarty treats `{17}` as a tag and the browser receives `\d17` + (or drops the quantifier). A valid 17-digit SteamID64 then fails + native HTML validation while STEAM_0:1:N and `[U:1:N]` still + pass (`\d+` has no braces). Wrap as `{literal}\d{17}{/literal}`. + PHP regexes and JS inside an existing `{literal}` block are + fine. Regression: + `SteamIDValidationOrderTest::testRenderedSteamPatternKeepsSeventeenDigitQuantifier` + + `testTemplatesHaveNoBareDigitBraceQuantifiers`. - Hand-rolling the strict SteamID-shape regex literal at the per-handler `preg_match` call site (the pre-#1423-follow-up-#4 shape: `preg_match('/^(?:STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17})$/', $raw)` diff --git a/web/includes/SteamID/SteamID.php b/web/includes/SteamID/SteamID.php index e88dd07f9..7ae72038d 100644 --- a/web/includes/SteamID/SteamID.php +++ b/web/includes/SteamID/SteamID.php @@ -156,10 +156,14 @@ private static function to($format, $steamid) * * The shape is TIGHTER than `ID_PATTERNS` on one axis: the bracketless * Steam3 form (`U:1:N`) is INTENTIONALLY excluded so the gate matches - * the form template's `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"` - * byte-for-byte. Curl-driven callers get the same shape contract a - * form user sees on the pattern-mismatch popover; bracketless Steam3 - * shape stays a library-side convenience for the conversion path + * the form template's rendered `pattern` (Steam2 / bracketed Steam3 / + * 17-digit Steam64). Template source wraps the `{17}` quantifier in + * `{literal}` so Smarty does not eat the braces; the HTML that + * reaches the browser is byte-for-byte + * `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"`. Curl-driven + * callers get the same shape contract a form user sees on the + * pattern-mismatch popover; bracketless Steam3 shape stays a + * library-side convenience for the conversion path * (`SteamID::toSteam2('U:1:1')` still works) but isn't an accepted * panel-input shape. * diff --git a/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts b/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts index beeef9ec7..75d3deb41 100644 --- a/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts +++ b/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts @@ -369,3 +369,46 @@ test.describe.serial('flow: comms-add SteamID validation feedback (#1420)', () = await expect(inlineErr).toContainText(/valid Steam ID|Community ID/i); }); }); + +const VALID_STEAM64 = '76561198179807307'; +const EXPECTED_STEAM_PATTERN = 'STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|\\d{17}'; + +test.describe('flow: SteamID64 native HTML pattern', () => { + test('17-digit SteamID64 passes native validation on add-block / add-ban / submit', async ({ + page, + }, testInfo) => { + test.skip( + testInfo.project.name !== 'chromium', + 'native Constraint Validation is Chromium-identical for this pattern', + ); + + const surfaces: Array<{ url: string; testId: string }> = [ + { url: '/index.php?p=admin&c=comms', testId: 'addcomm-steam' }, + { url: '/index.php?p=admin&c=bans§ion=add-ban', testId: 'addban-steam' }, + { url: '/index.php?p=submit', testId: 'submitban-steam' }, + ]; + + for (const surface of surfaces) { + await page.goto(surface.url); + const steam = page.getByTestId(surface.testId); + await expect(steam).toBeVisible(); + await steam.fill(VALID_STEAM64); + + const validity = await steam.evaluate((el: HTMLInputElement) => ({ + pattern: el.getAttribute('pattern'), + valid: el.validity.valid, + patternMismatch: el.validity.patternMismatch, + })); + + expect( + validity.pattern, + `${surface.testId} rendered pattern must keep the {17} quantifier`, + ).toBe(EXPECTED_STEAM_PATTERN); + expect( + validity.patternMismatch, + `${surface.testId}: ${VALID_STEAM64} must not patternMismatch`, + ).toBe(false); + expect(validity.valid, `${surface.testId}: ${VALID_STEAM64} must be valid`).toBe(true); + } + }); +}); diff --git a/web/tests/integration/SteamIDValidationOrderTest.php b/web/tests/integration/SteamIDValidationOrderTest.php index 462ae0094..3e2de913f 100644 --- a/web/tests/integration/SteamIDValidationOrderTest.php +++ b/web/tests/integration/SteamIDValidationOrderTest.php @@ -8,6 +8,7 @@ namespace Sbpp\Tests\Integration; use PHPUnit\Framework\TestCase; +use Smarty\Smarty; /** * Issue #1420 follow-up #2: page-handler form-POST surfaces that @@ -88,6 +89,21 @@ private function fileContents(string $relative): string return $contents; } + /** + * @return list + */ + private function steamIdFormTemplates(): array + { + return [ + 'themes/default/page_admin_comms_add.tpl', + 'themes/default/page_admin_bans_add.tpl', + 'themes/default/page_admin_edit_ban.tpl', + 'themes/default/page_admin_edit_comms.tpl', + 'themes/default/page_admin_edit_admins_details.tpl', + 'themes/default/page_submitban.tpl', + ]; + } + /** * The load-bearing assertion for `admin.edit.ban.php`. Pre-fix * this handler called `\SteamID\SteamID::toSteam2(...)` as the @@ -273,40 +289,29 @@ public function testPageSubmitDroppedSteamZeroSentinel(): void } /** - * Pin the strict `pattern="…"` attribute on each of the four - * Steam ID inputs across the page-handler form templates. The - * pattern mirrors the server-side `SteamID::isValidID()` - * allowlist (Steam2 / bracketed Steam3 / 17-digit Steam64) so the - * browser blocks submission pre-flight on a typo — the operator - * doesn't pay the round-trip. + * Pin the strict `pattern="…"` attribute on each Steam ID input + * across the page-handler form templates. The pattern mirrors + * the server-side `SteamID::isValidID()` allowlist (Steam2 / + * bracketed Steam3 / 17-digit Steam64) so the browser blocks + * submission pre-flight on a typo. * - * Anchored against the literal regex string so a future - * loosening that drops the `[01]` strict character class or - * widens the quantifier from `\d+` to `\d*` fails the gate. + * The `{17}` quantifier MUST be wrapped in `{literal}` (or + * `{ldelim}`/`{rdelim}`). Smarty treats `{17}` as a tag and + * would emit `\d17`, so a real SteamID64 fails native validation. */ public function testFormTemplatesCarryStrictSteamPattern(): void { - $expected = 'pattern="STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|\\d{17}"'; - - $templates = [ - 'themes/default/page_admin_edit_ban.tpl', - 'themes/default/page_admin_edit_comms.tpl', - 'themes/default/page_admin_edit_admins_details.tpl', - 'themes/default/page_submitban.tpl', - ]; + $expected = 'pattern="STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|{literal}\\d{17}{/literal}"'; - foreach ($templates as $relative) { + foreach ($this->steamIdFormTemplates() as $relative) { $contents = $this->fileContents($relative); $this->assertStringContainsString( $expected, $contents, - "#1420 follow-up #2: {$relative} must carry the strict Steam ID " - . "pattern: `{$expected}`. The pattern mirrors the server-side " - . "`SteamID::isValidID()` allowlist; loosening it (dropping `[01]` " - . "for `[0-9]`, widening `\\d+` to `\\d*`, removing the anchors) " - . "would reintroduce the substring-bypass class of #1420 on the " - . "client side and shift the burden entirely to the server-side " - . "library.", + "{$relative} must carry the strict Steam ID pattern with " + . "`{literal}\\d{17}{/literal}` so Smarty does not eat the " + . "quantifier braces. Loosening `[01]` or widening `\\d+` to " + . "`\\d*` would reintroduce the substring-bypass class of #1420.", ); } } @@ -323,14 +328,7 @@ public function testFormTemplatesCarrySteamPatternTitle(): void { $expectedTitle = 'title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64."'; - $templates = [ - 'themes/default/page_admin_edit_ban.tpl', - 'themes/default/page_admin_edit_comms.tpl', - 'themes/default/page_admin_edit_admins_details.tpl', - 'themes/default/page_submitban.tpl', - ]; - - foreach ($templates as $relative) { + foreach ($this->steamIdFormTemplates() as $relative) { $contents = $this->fileContents($relative); $this->assertStringContainsString( $expectedTitle, @@ -343,6 +341,95 @@ public function testFormTemplatesCarrySteamPatternTitle(): void } } + /** + * Smarty-compile each form's `pattern="STEAM_…"` attribute and + * assert the HTML that reaches the browser still carries the + * `\d{17}` quantifier. A source-only grep cannot catch Smarty + * eating `{17}`. + */ + public function testRenderedSteamPatternKeepsSeventeenDigitQuantifier(): void + { + $compileDir = sys_get_temp_dir() . '/sbpp-test-smarty-steam-pattern-' . getmypid(); + if (!is_dir($compileDir)) { + mkdir($compileDir, 0o775, true); + } + + $theme = new Smarty(); + $theme->setUseSubDirs(false); + $theme->setCompileId('steam-pattern'); + $theme->setCaching(Smarty::CACHING_OFF); + $theme->setForceCompile(true); + $theme->setCompileDir($compileDir); + $theme->setCacheDir($compileDir); + $theme->setEscapeHtml(true); + + $expectedHtml = 'pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"'; + + foreach ($this->steamIdFormTemplates() as $relative) { + $contents = $this->fileContents($relative); + $matched = preg_match( + '/pattern="STEAM_\[01\]:\[01\]:[^"]+"/', + $contents, + $m, + ); + $this->assertSame( + 1, + $matched, + "{$relative} must contain a Steam ID `pattern=\"STEAM_…\"` attribute.", + ); + + $snippetPath = $compileDir . '/snippet.tpl'; + file_put_contents($snippetPath, $m[0]); + $theme->setTemplateDir($compileDir); + $html = $theme->fetch('snippet.tpl'); + + $this->assertSame( + $expectedHtml, + $html, + "{$relative}: Smarty must emit `\\d{17}` in the pattern attribute. " + . "A bare `{17}` is parsed as a Smarty tag and the browser " + . "rejects valid SteamID64 input.", + ); + } + } + + /** + * Fail closed on any `{}` left in a `.tpl` file outside + * `{literal}` / `{* *}` so a future regex quantifier cannot + * silently ship as Smarty output. + */ + public function testTemplatesHaveNoBareDigitBraceQuantifiers(): void + { + $offenders = []; + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator( + ROOT . 'themes', + \FilesystemIterator::SKIP_DOTS, + ), + ); + + foreach ($iterator as $file) { + if (!$file instanceof \SplFileInfo || $file->getExtension() !== 'tpl') { + continue; + } + $src = (string) file_get_contents($file->getPathname()); + $stripped = preg_replace('/\{literal\}.*?\{\/literal\}/s', '', $src) ?? $src; + $stripped = preg_replace('/\{\*.*?\*\}/s', '', $stripped) ?? $stripped; + if (preg_match('/\{[0-9]+\}/', $stripped, $m) === 1) { + $relative = str_replace('\\', '/', substr($file->getPathname(), strlen(ROOT))); + $offenders[] = $relative . ' → ' . $m[0]; + } + } + + $this->assertSame( + [], + $offenders, + 'Bare `{}` in a Smarty template is parsed as a tag. ' + . 'Wrap regex quantifiers in `{literal}…{/literal}` or ' + . '`{ldelim}`/`{rdelim}`.', + ); + } + /** * Pin that `page_submitban.tpl` does NOT carry `novalidate` on * the form. Pre-#1420 the form had `novalidate` which suppressed diff --git a/web/themes/default/page_admin_bans_add.tpl b/web/themes/default/page_admin_bans_add.tpl index b8a30e564..52cb6b3d8 100644 --- a/web/themes/default/page_admin_bans_add.tpl +++ b/web/themes/default/page_admin_bans_add.tpl @@ -169,7 +169,7 @@ data-testid="addban-steam" value="{if $prefill_type == 0}{$prefill_steam}{/if}" placeholder="STEAM_0:1:23498765" - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64."> diff --git a/web/themes/default/page_admin_comms_add.tpl b/web/themes/default/page_admin_comms_add.tpl index 5abb0bcbe..cdddbf877 100644 --- a/web/themes/default/page_admin_comms_add.tpl +++ b/web/themes/default/page_admin_comms_add.tpl @@ -101,11 +101,14 @@ and admin.bans.php's mirror block. #1395 *} {* #1420: native `pattern` attribute mirrors the client-side regex `page_admin_bans_add.tpl`'s IIFE - carries (`STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}`) - so the browser surfaces a popover for empty / bad- - shape values before our submit handler runs. + carries (Steam2 / bracketed Steam3 / 17-digit + Steam64) so the browser surfaces a popover for + empty / bad-shape values before our submit + handler runs. The Steam64 arm is wrapped in a + Smarty literal block so the brace quantifier + reaches the browser unchanged. `title` is what the browser reads aloud / shows in - the popover when the pattern fails — keep it short + the popover when the pattern fails. Keep it short and actionable. The `aria-describedby` ties the help line below to the input for screen readers. An `?steam=…` smart-default carrying an IPv4 will @@ -123,7 +126,7 @@ data-testid="addcomm-steam" value="{$prefill_steam}" placeholder="STEAM_0:1:23498765" - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64." aria-describedby="addcomm-steam-help" required> diff --git a/web/themes/default/page_admin_edit_admins_details.tpl b/web/themes/default/page_admin_edit_admins_details.tpl index efdc30329..47b255ab6 100644 --- a/web/themes/default/page_admin_edit_admins_details.tpl +++ b/web/themes/default/page_admin_edit_admins_details.tpl @@ -74,7 +74,7 @@
diff --git a/web/themes/default/page_admin_edit_ban.tpl b/web/themes/default/page_admin_edit_ban.tpl index d8e43028c..c12f21aaa 100644 --- a/web/themes/default/page_admin_edit_ban.tpl +++ b/web/themes/default/page_admin_edit_ban.tpl @@ -165,7 +165,7 @@ empty (the IP-target case) the browser's pattern check is satisfied (it only fires on non-empty values). *} - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64.">
diff --git a/web/themes/default/page_submitban.tpl b/web/themes/default/page_submitban.tpl index 864ba3085..c649c322a 100644 --- a/web/themes/default/page_submitban.tpl +++ b/web/themes/default/page_submitban.tpl @@ -118,7 +118,7 @@ maxlength="64" value="{$STEAMID}" placeholder="STEAM_0:1:23498765" - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64." autocomplete="off" aria-describedby="submitban-id-or-ip-help" From 21d636c6f3887f104cedc95ddf72c9c5bc8e09ed Mon Sep 17 00:00:00 2001 From: Maximiliano Jabase Date: Mon, 31 Aug 2026 21:31:08 -0300 Subject: [PATCH 2/3] use ldelim for steamid64 html pattern braces A {literal} wrap in the attribute paired with {literal} in a comment and made PHPStan miss edit-ban View properties. --- AGENTS.md | 34 +++++++++++-------- web/includes/SteamID/SteamID.php | 6 ++-- .../SteamIDValidationOrderTest.php | 13 ++++--- web/themes/default/page_admin_bans_add.tpl | 2 +- web/themes/default/page_admin_comms_add.tpl | 6 ++-- .../page_admin_edit_admins_details.tpl | 2 +- web/themes/default/page_admin_edit_ban.tpl | 2 +- web/themes/default/page_admin_edit_comms.tpl | 2 +- web/themes/default/page_submitban.tpl | 2 +- 9 files changed, 39 insertions(+), 30 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a13d7396e..f622b885f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -777,7 +777,7 @@ of the diff ship together or not at all. canonical reference shape. The constant's docblock spells out the contract: byte-for-byte symmetry with the form template's *rendered* `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"` - (template source wraps `{17}` in `{literal}`), the load-bearing + (template source writes `{17}` as `{ldelim}17{rdelim}`), the load-bearing `D` modifier (without it `STEAM_0:0:1\n` slips past the gate and 500s on `toSteam2()`), and the deliberate asymmetry with `ID_PATTERNS` (bracketless Steam3 `U:1:N` is excluded from the @@ -788,9 +788,9 @@ of the diff ship together or not at all. the pre-#1423-follow-up-#4 hand-rolled copies silently missed the `D` modifier, producing the newline-bypass class. The client-side native validation in the corresponding form template uses the - matching `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}"` + matching `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}"` (HTML's `pattern` attribute is implicitly anchored `^…$`, so the PHP - regex carries explicit `^…$`; `{literal}` keeps the `{17}` + regex carries explicit `^…$`; `{ldelim}`/`{rdelim}` keep the `{17}` quantifier out of Smarty's delimiter parser so a 17-digit SteamID64 actually matches). The browser-native popover is the UX-first gate that fires BEFORE the IIFE calls `sb.api.call`; the @@ -948,7 +948,7 @@ of the diff ship together or not at all. `web/tests/integration/SteamIDValidationOrderTest.php` static- shape-pins the validate-then-convert order across every page handler. The form templates also carry the same - `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}"` + + `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}"` + actionable `title="…"` as the JSON-flow add-form templates so the browser-native popover surfaces the same error message pre-flight. @@ -2015,13 +2015,16 @@ without a paired theme toggle in the wizard chrome. #1123 B20 rewrote it in standard `{ }` delimiters; do NOT regress it back to `-{ … }-` without a paired edit here. - Regex quantifiers that use braces (`\d{17}`, `[0-9]{1,3}`, …) in a - `.tpl` file MUST be wrapped in `{literal}…{/literal}` (or written - as `{ldelim}17{rdelim}`). Smarty treats `{17}` as a tag. The Steam - ID `pattern` attributes are the canonical site: - `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}"` + `.tpl` file MUST use `{ldelim}17{rdelim}` (or sit inside an + existing `{literal}` JS block). Smarty treats `{17}` as a tag. + The Steam ID `pattern` attributes are the canonical site: + `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}"` renders as `\d{17}` in the HTML the browser validates against. - JS inside an existing `{literal}` block is already safe. PHP - regexes are not Smarty and stay `\d{17}`. Regression: + Do not open a new `{literal}` pair in an HTML attribute: a + `{literal}` mention in a `{* *}` comment above it will pair with + the closer and SmartyTemplateRule will miss every variable in + between. JS inside an existing `{literal}` block is already safe. + PHP regexes are not Smarty and stay `\d{17}`. Regression: `SteamIDValidationOrderTest::testRenderedSteamPatternKeepsSeventeenDigitQuantifier` + `testTemplatesHaveNoBareDigitBraceQuantifiers`. @@ -4254,13 +4257,16 @@ the spec, target a 1920px viewport, not 1440px. `try/catch` is a code smell signalling the upstream gate is missing. - Bare `\d{17}` (or any `{}` regex quantifier) in a Smarty - `.tpl` `pattern` attribute without `{literal}` / `{ldelim}` → + `.tpl` `pattern` attribute without `{ldelim}`/`{rdelim}` → Smarty treats `{17}` as a tag and the browser receives `\d17` (or drops the quantifier). A valid 17-digit SteamID64 then fails native HTML validation while STEAM_0:1:N and `[U:1:N]` still - pass (`\d+` has no braces). Wrap as `{literal}\d{17}{/literal}`. - PHP regexes and JS inside an existing `{literal}` block are - fine. Regression: + pass (`\d+` has no braces). Write `\d{ldelim}17{rdelim}`. Do not + wrap the arm in a new `{literal}` pair inside the attribute: an + unmatched `{literal}` in a `{* *}` comment above it pairs with + the closer and SmartyTemplateRule reports every View property + between the two as unused. PHP regexes and JS inside an existing + `{literal}` block are fine. Regression: `SteamIDValidationOrderTest::testRenderedSteamPatternKeepsSeventeenDigitQuantifier` + `testTemplatesHaveNoBareDigitBraceQuantifiers`. - Hand-rolling the strict SteamID-shape regex literal at the diff --git a/web/includes/SteamID/SteamID.php b/web/includes/SteamID/SteamID.php index 7ae72038d..20baf8693 100644 --- a/web/includes/SteamID/SteamID.php +++ b/web/includes/SteamID/SteamID.php @@ -157,9 +157,9 @@ private static function to($format, $steamid) * The shape is TIGHTER than `ID_PATTERNS` on one axis: the bracketless * Steam3 form (`U:1:N`) is INTENTIONALLY excluded so the gate matches * the form template's rendered `pattern` (Steam2 / bracketed Steam3 / - * 17-digit Steam64). Template source wraps the `{17}` quantifier in - * `{literal}` so Smarty does not eat the braces; the HTML that - * reaches the browser is byte-for-byte + * 17-digit Steam64). Template source writes the `{17}` quantifier + * as `{ldelim}17{rdelim}` so Smarty does not eat the braces; the + * HTML that reaches the browser is byte-for-byte * `pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"`. Curl-driven * callers get the same shape contract a form user sees on the * pattern-mismatch popover; bracketless Steam3 shape stays a diff --git a/web/tests/integration/SteamIDValidationOrderTest.php b/web/tests/integration/SteamIDValidationOrderTest.php index 3e2de913f..9fc99fb79 100644 --- a/web/tests/integration/SteamIDValidationOrderTest.php +++ b/web/tests/integration/SteamIDValidationOrderTest.php @@ -295,13 +295,16 @@ public function testPageSubmitDroppedSteamZeroSentinel(): void * bracketed Steam3 / 17-digit Steam64) so the browser blocks * submission pre-flight on a typo. * - * The `{17}` quantifier MUST be wrapped in `{literal}` (or - * `{ldelim}`/`{rdelim}`). Smarty treats `{17}` as a tag and - * would emit `\d17`, so a real SteamID64 fails native validation. + * The `{17}` quantifier MUST be written as `{ldelim}17{rdelim}`. + * Smarty treats `{17}` as a tag and would emit `\d17`, so a real + * SteamID64 fails native validation. A `{literal}` wrap in the + * attribute is also wrong: an unmatched `{literal}` in a `{* *}` + * comment above it pairs with the closer and SmartyTemplateRule + * misses every variable in between. */ public function testFormTemplatesCarryStrictSteamPattern(): void { - $expected = 'pattern="STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|{literal}\\d{17}{/literal}"'; + $expected = 'pattern="STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|\\d{ldelim}17{rdelim}"'; foreach ($this->steamIdFormTemplates() as $relative) { $contents = $this->fileContents($relative); @@ -309,7 +312,7 @@ public function testFormTemplatesCarryStrictSteamPattern(): void $expected, $contents, "{$relative} must carry the strict Steam ID pattern with " - . "`{literal}\\d{17}{/literal}` so Smarty does not eat the " + . "`\\d{ldelim}17{rdelim}` so Smarty does not eat the " . "quantifier braces. Loosening `[01]` or widening `\\d+` to " . "`\\d*` would reintroduce the substring-bypass class of #1420.", ); diff --git a/web/themes/default/page_admin_bans_add.tpl b/web/themes/default/page_admin_bans_add.tpl index 52cb6b3d8..830f738a1 100644 --- a/web/themes/default/page_admin_bans_add.tpl +++ b/web/themes/default/page_admin_bans_add.tpl @@ -169,7 +169,7 @@ data-testid="addban-steam" value="{if $prefill_type == 0}{$prefill_steam}{/if}" placeholder="STEAM_0:1:23498765" - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64.">
diff --git a/web/themes/default/page_admin_comms_add.tpl b/web/themes/default/page_admin_comms_add.tpl index cdddbf877..6e747d417 100644 --- a/web/themes/default/page_admin_comms_add.tpl +++ b/web/themes/default/page_admin_comms_add.tpl @@ -104,8 +104,8 @@ carries (Steam2 / bracketed Steam3 / 17-digit Steam64) so the browser surfaces a popover for empty / bad-shape values before our submit - handler runs. The Steam64 arm is wrapped in a - Smarty literal block so the brace quantifier + handler runs. The Steam64 arm uses + `{ldelim}17{rdelim}` so the brace quantifier reaches the browser unchanged. `title` is what the browser reads aloud / shows in the popover when the pattern fails. Keep it short @@ -126,7 +126,7 @@ data-testid="addcomm-steam" value="{$prefill_steam}" placeholder="STEAM_0:1:23498765" - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64." aria-describedby="addcomm-steam-help" required> diff --git a/web/themes/default/page_admin_edit_admins_details.tpl b/web/themes/default/page_admin_edit_admins_details.tpl index 47b255ab6..a349ff780 100644 --- a/web/themes/default/page_admin_edit_admins_details.tpl +++ b/web/themes/default/page_admin_edit_admins_details.tpl @@ -74,7 +74,7 @@
diff --git a/web/themes/default/page_admin_edit_ban.tpl b/web/themes/default/page_admin_edit_ban.tpl index c12f21aaa..f5e24b9dd 100644 --- a/web/themes/default/page_admin_edit_ban.tpl +++ b/web/themes/default/page_admin_edit_ban.tpl @@ -165,7 +165,7 @@ empty (the IP-target case) the browser's pattern check is satisfied (it only fires on non-empty values). *} - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64.">
diff --git a/web/themes/default/page_submitban.tpl b/web/themes/default/page_submitban.tpl index c649c322a..3a525a8d9 100644 --- a/web/themes/default/page_submitban.tpl +++ b/web/themes/default/page_submitban.tpl @@ -118,7 +118,7 @@ maxlength="64" value="{$STEAMID}" placeholder="STEAM_0:1:23498765" - pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|{literal}\d{17}{/literal}" + pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{ldelim}17{rdelim}" title="Enter a Steam ID (STEAM_0:1:23498765), Steam3 ID ([U:1:23498765]), or 17-digit SteamID64." autocomplete="off" aria-describedby="submitban-id-or-ip-help" From 5652bacc9b1b31776a01ee3925080d2eace8f1a4 Mon Sep 17 00:00:00 2001 From: Maximiliano Jabase Date: Sun, 6 Sep 2026 03:13:32 -0300 Subject: [PATCH 3/3] Tighten SteamID64 pattern tests after review. Move the E2E case into the existing serial describe, compile snippets with the bound View delimiters, and skip -{ }- templates in the brace scanner. --- .../comms-add-steamid-validation.spec.ts | 18 +- .../SteamIDValidationOrderTest.php | 155 +++++++++++++++--- 2 files changed, 137 insertions(+), 36 deletions(-) diff --git a/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts b/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts index 75d3deb41..a66090247 100644 --- a/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts +++ b/web/tests/e2e/specs/flows/comms-add-steamid-validation.spec.ts @@ -81,8 +81,10 @@ import { test, expect } from '../../fixtures/auth.ts'; import { truncateE2eDb } from '../../fixtures/db.ts'; const VALID_STEAM = 'STEAM_0:1:14202020'; +const VALID_STEAM64 = '76561198179807307'; const INVALID_STEAM = 'asdf'; const TARGET_NICK = 'e2e-1420-validation'; +const EXPECTED_STEAM_PATTERN = 'STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|\\d{17}'; // `.serial` because every test in this describe runs `truncateE2eDb()` // in `beforeEach`, and a sibling test's API call landing during another @@ -368,20 +370,14 @@ test.describe.serial('flow: comms-add SteamID validation feedback (#1420)', () = await expect(inlineErr).toBeVisible(); await expect(inlineErr).toContainText(/valid Steam ID|Community ID/i); }); -}); - -const VALID_STEAM64 = '76561198179807307'; -const EXPECTED_STEAM_PATTERN = 'STEAM_[01]:[01]:\\d+|\\[U:1:\\d+\\]|\\d{17}'; -test.describe('flow: SteamID64 native HTML pattern', () => { + // Lives in this `.serial` group (not a sibling `describe`) so a + // local `workers: ` run cannot `page.goto` the form while + // another test in this file is inside `truncateE2eDb()`. CI pins + // `workers: 1` and would not see that flake. test('17-digit SteamID64 passes native validation on add-block / add-ban / submit', async ({ page, - }, testInfo) => { - test.skip( - testInfo.project.name !== 'chromium', - 'native Constraint Validation is Chromium-identical for this pattern', - ); - + }) => { const surfaces: Array<{ url: string; testId: string }> = [ { url: '/index.php?p=admin&c=comms', testId: 'addcomm-steam' }, { url: '/index.php?p=admin&c=bans§ion=add-ban', testId: 'addban-steam' }, diff --git a/web/tests/integration/SteamIDValidationOrderTest.php b/web/tests/integration/SteamIDValidationOrderTest.php index 9fc99fb79..8266e2259 100644 --- a/web/tests/integration/SteamIDValidationOrderTest.php +++ b/web/tests/integration/SteamIDValidationOrderTest.php @@ -8,6 +8,17 @@ namespace Sbpp\Tests\Integration; use PHPUnit\Framework\TestCase; +use Sbpp\View\AdminBansAddView; +use Sbpp\View\AdminBansEditView; +use Sbpp\View\AdminCommsAddView; +use Sbpp\View\AdminCommsEditView; +use Sbpp\View\AdminServersRconView; +use Sbpp\View\BlockitView; +use Sbpp\View\EditAdminDetailsView; +use Sbpp\View\KickitView; +use Sbpp\View\LoginView; +use Sbpp\View\SubmitBanView; +use Sbpp\View\View; use Smarty\Smarty; /** @@ -75,6 +86,32 @@ */ final class SteamIDValidationOrderTest extends TestCase { + private static string $steamPatternCompileDir = ''; + + public static function tearDownAfterClass(): void + { + self::removeDir(self::$steamPatternCompileDir); + } + + private static function removeDir(string $dir): void + { + if ($dir === '' || !is_dir($dir)) { + return; + } + $iterator = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS), + \RecursiveIteratorIterator::CHILD_FIRST, + ); + foreach ($iterator as $file) { + if (!$file instanceof \SplFileInfo) { + continue; + } + $path = $file->getPathname(); + $file->isDir() ? @rmdir($path) : @unlink($path); + } + @rmdir($dir); + } + /** * @param string $relative Path relative to `web/`. Resolves against the * test bootstrap's `ROOT` constant. @@ -104,6 +141,50 @@ private function steamIdFormTemplates(): array ]; } + /** + * Bound View for a Steam-ID form template. The render test reads + * `DELIMITERS` from here so it compiles with the same pair the + * panel uses, not a hardcoded `{ }`. + * + * @return class-string + */ + private function viewClassForSteamFormTemplate(string $relative): string + { + return match (basename($relative)) { + 'page_admin_comms_add.tpl' => AdminCommsAddView::class, + 'page_admin_bans_add.tpl' => AdminBansAddView::class, + 'page_admin_edit_ban.tpl' => AdminBansEditView::class, + 'page_admin_edit_comms.tpl' => AdminCommsEditView::class, + 'page_admin_edit_admins_details.tpl' => EditAdminDetailsView::class, + 'page_submitban.tpl' => SubmitBanView::class, + default => throw new \LogicException("No View mapping for {$relative}"), + }; + } + + /** + * Templates whose View overrides `View::DELIMITERS`. `{17}` is + * inert text there (live tags are `-{` / `}-`), so the scanner + * must not tell the next author to wrap them in `{ldelim}`. + * + * @return list + */ + private function nonDefaultDelimiterTemplateBasenames(): array + { + $basenames = []; + foreach ([ + LoginView::class, + BlockitView::class, + KickitView::class, + AdminServersRconView::class, + ] as $class) { + if ($class::DELIMITERS !== View::DELIMITERS) { + $basenames[] = basename($class::TEMPLATE); + } + } + + return $basenames; + } + /** * The load-bearing assertion for `admin.edit.ban.php`. Pre-fix * this handler called `\SteamID\SteamID::toSteam2(...)` as the @@ -316,6 +397,16 @@ public function testFormTemplatesCarryStrictSteamPattern(): void . "quantifier braces. Loosening `[01]` or widening `\\d+` to " . "`\\d*` would reintroduce the substring-bypass class of #1420.", ); + + $count = preg_match_all('/pattern="STEAM_[^"]+"/', $contents, $matches); + $this->assertGreaterThan(0, $count, "{$relative} must contain a Steam ID pattern attribute."); + foreach ($matches[0] as $attr) { + $this->assertStringContainsString( + '{ldelim}17{rdelim}', + $attr, + "{$relative}: every `pattern=\"STEAM_…\"` must keep `{ldelim}17{rdelim}`.", + ); + } } } @@ -352,9 +443,9 @@ public function testFormTemplatesCarrySteamPatternTitle(): void */ public function testRenderedSteamPatternKeepsSeventeenDigitQuantifier(): void { - $compileDir = sys_get_temp_dir() . '/sbpp-test-smarty-steam-pattern-' . getmypid(); - if (!is_dir($compileDir)) { - mkdir($compileDir, 0o775, true); + self::$steamPatternCompileDir = sys_get_temp_dir() . '/sbpp-test-smarty-steam-pattern-' . getmypid(); + if (!is_dir(self::$steamPatternCompileDir)) { + mkdir(self::$steamPatternCompileDir, 0o775, true); } $theme = new Smarty(); @@ -362,37 +453,44 @@ public function testRenderedSteamPatternKeepsSeventeenDigitQuantifier(): void $theme->setCompileId('steam-pattern'); $theme->setCaching(Smarty::CACHING_OFF); $theme->setForceCompile(true); - $theme->setCompileDir($compileDir); - $theme->setCacheDir($compileDir); + $theme->setCompileDir(self::$steamPatternCompileDir); + $theme->setCacheDir(self::$steamPatternCompileDir); $theme->setEscapeHtml(true); + $theme->setTemplateDir(self::$steamPatternCompileDir); $expectedHtml = 'pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"'; foreach ($this->steamIdFormTemplates() as $relative) { + $viewClass = $this->viewClassForSteamFormTemplate($relative); + [$left, $right] = $viewClass::DELIMITERS; + $theme->setLeftDelimiter($left); + $theme->setRightDelimiter($right); + $contents = $this->fileContents($relative); - $matched = preg_match( + $count = preg_match_all( '/pattern="STEAM_\[01\]:\[01\]:[^"]+"/', $contents, - $m, + $matches, ); - $this->assertSame( - 1, - $matched, + $this->assertGreaterThan( + 0, + $count, "{$relative} must contain a Steam ID `pattern=\"STEAM_…\"` attribute.", ); - $snippetPath = $compileDir . '/snippet.tpl'; - file_put_contents($snippetPath, $m[0]); - $theme->setTemplateDir($compileDir); - $html = $theme->fetch('snippet.tpl'); - - $this->assertSame( - $expectedHtml, - $html, - "{$relative}: Smarty must emit `\\d{17}` in the pattern attribute. " - . "A bare `{17}` is parsed as a Smarty tag and the browser " - . "rejects valid SteamID64 input.", - ); + foreach ($matches[0] as $i => $snippet) { + $snippetName = basename($relative, '.tpl') . "-{$i}.tpl"; + file_put_contents(self::$steamPatternCompileDir . '/' . $snippetName, $snippet); + $html = $theme->fetch($snippetName); + + $this->assertSame( + $expectedHtml, + $html, + "{$relative} pattern #{$i}: Smarty must emit `\\d{17}` in the " + . "pattern attribute. A bare `{17}` is parsed as a Smarty tag " + . "and the browser rejects valid SteamID64 input.", + ); + } } } @@ -411,10 +509,15 @@ public function testTemplatesHaveNoBareDigitBraceQuantifiers(): void ), ); + $skipBasenames = $this->nonDefaultDelimiterTemplateBasenames(); + foreach ($iterator as $file) { if (!$file instanceof \SplFileInfo || $file->getExtension() !== 'tpl') { continue; } + if (in_array($file->getFilename(), $skipBasenames, true)) { + continue; + } $src = (string) file_get_contents($file->getPathname()); $stripped = preg_replace('/\{literal\}.*?\{\/literal\}/s', '', $src) ?? $src; $stripped = preg_replace('/\{\*.*?\*\}/s', '', $stripped) ?? $stripped; @@ -427,9 +530,11 @@ public function testTemplatesHaveNoBareDigitBraceQuantifiers(): void $this->assertSame( [], $offenders, - 'Bare `{}` in a Smarty template is parsed as a tag. ' - . 'Wrap regex quantifiers in `{literal}…{/literal}` or ' - . '`{ldelim}`/`{rdelim}`.', + 'Bare `{}` in a default-delimiter Smarty template is parsed ' + . 'as a tag. Wrap regex quantifiers in `{ldelim}`/`{rdelim}` ' + . '(or sit inside `{literal}…{/literal}`). Templates whose View ' + . 'overrides `View::DELIMITERS` (currently `-{ }-`) are skipped: ' + . '`{17}` is inert text there and `{ldelim}` would ship verbatim.', ); }