Keep SteamID64 HTML pattern braces out of Smarty

[maxijabase]

Description

A valid 17-digit SteamID64 (for example 76561198179807307) was rejected by native HTML validation on Add a comm block, Add a ban, Submit a ban, and the matching edit forms. The browser popover told the operator to enter a 17-digit SteamID64 even though that is exactly what they typed.

Steam2 (STEAM_0:1:N) and Steam3 ([U:1:N]) still worked.

Root cause is Smarty, not the PHP gate. Template source used:

pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"

Smarty's { / } delimiters treat {17} as a tag. The rendered HTML became \d17 (quantifier dropped). The browser then rejected every 17-digit SteamID64. The Steam2 / Steam3 arms use \d+, so they never hit this.

SteamID::HANDLER_STRICT_REGEX on the PHP side was already correct. Submitting via curl / a third-party theme that skips native validation would have succeeded. The existing E2E happy path used STEAM_0:1:14202020, so CI never caught it.

Fix: write the Steam64 arm as \d{ldelim}17{rdelim} so Smarty emits \d{17}. Do not wrap the attribute in {literal}: an unmatched {literal} in a {* *} comment above it pairs with the closer and SmartyTemplateRule misses every variable in between.

Touched templates (every occurrence of this pattern):

• web/themes/default/page_admin_comms_add.tpl (reported surface)
• web/themes/default/page_admin_bans_add.tpl
• web/themes/default/page_admin_edit_ban.tpl
• web/themes/default/page_admin_edit_comms.tpl
• web/themes/default/page_admin_edit_admins_details.tpl
• web/themes/default/page_submitban.tpl

Not affected: install wizard (Steam2-only pattern, no brace quantifier), PHP regexes, JS already inside {literal} blocks.

Motivation and Context

Operators who paste a Community ID into Add Block / Add Ban / Submit Ban hit a native validation popover and cannot submit, even though the value is valid and the server would accept it.

How Has This Been Tested?

• PHPUnit SteamIDValidationOrderTest, including:
  • source pin that every Steam ID form carries \d{ldelim}17{rdelim} (every pattern="STEAM_…" in those files, not just the first)
  • testRenderedSteamPatternKeepsSeventeenDigitQuantifier: Smarty-compiles each extracted pattern with the bound View's DELIMITERS and asserts the HTML is pattern="STEAM_[01]:[01]:\d+|\[U:1:\d+\]|\d{17}"
  • testTemplatesHaveNoBareDigitBraceQuantifiers: scans web/themes/**/*.tpl, skips Views that override DELIMITERS (-{ }-), fails on leftover {<digits>}
• E2E: comms-add-steamid-validation.spec.ts fills 76561198179807307 on add-block, add-ban, and submit inside the existing describe.serial (so a local multi-worker run cannot race truncateE2eDb())
• AGENTS.md convention + anti-pattern updated so a future \d{17} in a .tpl pattern does not regress

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.

[Rushaway]

Review

Root cause and fix are correct, and the coverage is unusually thorough for a one-character-class bug. {17} is a valid Smarty expression tag (auto-literal only spares { followed by whitespace), so it rendered as 17 and the browser received \d17 — which not only rejects every real SteamID64 but would have accepted junk like 917. {ldelim}17{rdelim} is the right escape here, and I agree with the call to avoid opening a new {literal} pair inside an attribute given how SmartyTemplateRule pairs it against a {literal} mention in a preceding {* *} comment.

What I verified locally on fix/steamid64-html-pattern:

• All six pattern="STEAM_…" occurrences under web/themes/ are converted; grep -rn 'pattern="STEAM_' --include=*.tpl shows no stragglers.
• themes/default/install/page_admin.tpl is correctly untouched — pattern="STEAM_[01]:[01]:[0-9]+", no brace quantifier.
• The one remaining \d{17} in a template (page_admin_bans_add.tpl:333, the IIFE's JS regex) sits inside the {literal} block spanning 277–431, so it is genuinely safe and correctly excluded by the new scanner.
• None of the six views override View::DELIMITERS, so {ldelim}/{rdelim} really are live tags on all of these surfaces. (Only LoginView, BlockitView, KickitView, AdminServersRconView use -{ }-.)
• CI is green on all five required checks.

No blocking objection. Notes below, roughly in descending order of value.

1. The PR description no longer matches the code

The body says the fix is {literal}\d{17}{/literal}; 21d636c6 replaced that with {ldelim}17{rdelim}, and the second approach is the one being merged (and the one AGENTS.md now mandates). Since this body is what lands in the squash commit and is what the next person greps for, it's worth rewriting the "Fix:" paragraph — including the sentence about {literal} being the answer, which the new AGENTS.md anti-pattern explicitly warns against.

2. The new E2E test escapes the file's serial guard

test.describe('flow: SteamID64 native HTML pattern') is a plain describe, sitting next to a describe.serial whose own comment spells out why that's unsafe:

.serial because every test in this describe runs truncateE2eDb() in beforeEach, and a sibling test's API call landing during another test's truncate-and-reseed window gets "forbidden" (the admin row was momentarily gone).

With fullyParallel: true and local workers: <cores>, the new test's page.goto('/index.php?p=admin&c=comms') can land inside exactly that window, get bounced to login, and fail on expect(steam).toBeVisible(). CI pins workers: 1 so the gate won't see it, which makes it the worst kind of flake — local only. Either move the test inside the existing describe.serial, or add test.describe.configure({ mode: 'serial' }) at file scope.

3. testRenderedSteamPatternKeepsSeventeenDigitQuantifier hardcodes the default delimiters

new Smarty() compiles every snippet with { }. That is right for all six templates today, but the assertion's premise is "this is how the app renders it", and the app doesn't universally render with { }. If a Steam ID input ever lands in a -{ }- template, this test would happily assert against the wrong grammar (and {ldelim} would ship to the browser verbatim). Deriving the pair from the bound View's DELIMITERS — the same constant SmartyTemplateRule already reads — would make the test say what it means.

4. Smaller things in the same test

• preg_match grabs only the first pattern="STEAM_…" per file. testFormTemplatesCarryStrictSteamPattern is likewise assertStringContainsString, so a template that grows a second Steam input with a bare {17} would be caught only by the directory scanner, not by the two tests that are nominally about these six files. preg_match_all + a loop closes that.
• The six iterations all write to the same $compileDir . '/snippet.tpl' and set templateDir === compileDir. It works, but only because setForceCompile(true) is on and Smarty re-reads the source each pass — that's a lot of weight on an implementation detail for a test whose whole point is not trusting Smarty. $theme->fetch('eval:' . $m[0]) drops the file juggling entirely; failing that, a per-template filename.
• Nothing removes sys_get_temp_dir() . '/sbpp-test-smarty-steam-pattern-<pid>' afterwards. A tearDownAfterClass unlink would keep repeated local runs from accumulating snippets plus their compiled .php.

5. The directory scanner is over-broad on -{ }- templates

testTemplatesHaveNoBareDigitBraceQuantifiers walks all of themes/ and flags any {<digits>} outside {literal} / {* *}. In page_login.tpl, page_blockit.tpl, page_kickit.tpl, page_admin_servers_rcon.tpl a bare {17} is inert text, and those files also wouldn't use {literal} to opt out. No offenders today, so it passes — but the next person who writes a legitimate [0-9]{1,3} in the RCON template gets a failure with a message telling them to apply a fix that would actually break it. Skipping files whose View overrides DELIMITERS (or at least saying so in the failure message) would help.

Separately: this is a general template-hygiene rule living in a class named SteamIDValidationOrderTest. Given web/includes/PHPStan/SmartyTemplateRule.php already exists and already reads DELIMITERS, PHPStan looks like the more natural home — it'd also make the delimiter awareness fall out for free. Non-blocking, and I understand co-locating it with the regression it came from.

6. Follow-up thought

This pattern now exists six times in templates, once more as a JS regex in page_admin_bans_add.tpl, and again as SteamID::HANDLER_STRICT_REGEX — and the PR's response is to pin the duplication byte-for-byte in three tests. That's consistent with the convention AGENTS.md documents, so I'm not asking to change it here. But this bug is a direct consequence of the copies: a single PHP-side constant surfaced to the templates as a variable would have made "Smarty ate the braces" structurally impossible, and would collapse three source-pinning tests into one. Might be worth an issue.

[Rushaway]

Re-review @ 5652bacc

@maxijabase All five points from the previous round are addressed, and the three test changes are real fixes rather than paper-overs. The production change itself was already right in 21d636c6 and is unchanged. CI is green on all five required checks. No blocking objection — everything below is test hardening.

Previous round

#	Point	Status
1	PR body still described the {literal} approach	Fixed — the body now documents \d{ldelim}17{rdelim} and carries the anti-pattern warning, so the squash commit will be accurate
2	E2E case escaped the file's serial guard	Fixed — moved inside test.describe.serial, with a comment saying why
3	Render test hardcoded { }	Fixed — viewClassForSteamFormTemplate() + $viewClass::DELIMITERS
4	First-match-only, shared snippet file, no cleanup	Fixed — preg_match_all in both source tests, per-template basename-{i}.tpl, tearDownAfterClass
5	Scanner over-broad on -{ }- templates	Fixed — skip list plus a failure message that explains the skip

What I checked on the branch: all six pattern="STEAM_…" occurrences carry {ldelim}17{rdelim}; git grep -nE '\{[0-9]+\}' -- '*.tpl' leaves exactly one hit, the IIFE's JS regex at page_admin_bans_add.tpl:333, inside the {literal} block spanning 277–431; the six View→template mappings and the four -{ }- classes all resolve to real TEMPLATE / DELIMITERS constants. I read the tests rather than running them (no vendor tree on this machine) and relied on the green PHPUnit / Playwright / Static-analysis jobs.

Notes below in descending order of value.

1. The scanner strips {literal} before {* *} — the exact hazard this PR documents

SteamIDValidationOrderTest.php:522-523:

$stripped = preg_replace('/\{literal\}.*?\{\/literal\}/s', '', $src) ?? $src;
$stripped = preg_replace('/\{\*.*?\*\}/s', '', $stripped) ?? $stripped;

Literals first, comments second. That is the same ordering that produced the 21d636c6 regression: a {literal} mentioned inside a {* *} comment pairs with the next real {/literal} and the whole span in between disappears. Here the consequence is a false negative — the scanner stops seeing offenders in the blanked region and passes silently.

This is not hypothetical shape-matching. Three templates already write {literal} inside a comment:

• box_admin_bans_search.tpl:36
• box_admin_comms_search.tpl:35
• box_admin_log_search.tpl:32

All three happen to close on the same line, so today the strip is harmless and the test passes. The first comment that mentions {literal} without a same-line closer turns the scanner into a no-op for everything below it.

Swapping the two lines fixes it, and also matches Smarty's own lexer, which consumes {* *} as a unit and never sees a {literal} inside one.

The same one-line ordering issue lives in SmartyTemplateRule::parseTemplate() (web/includes/PHPStan/SmartyTemplateRule.php:249), which strips literals on raw source with no comment pass at all. That is the actual bug behind the AGENTS.md rule you just wrote — "do not open a new {literal} pair in an HTML attribute" is a workaround for our own PHPStan rule, not for Smarty. Fixing line 249 would retire the anti-pattern instead of documenting it forever. Out of scope here; worth an issue.

2. The render test compiles the attribute in isolation, so it cannot see a surrounding {literal}

testRenderedSteamPatternKeepsSeventeenDigitQuantifier extracts pattern="STEAM_…" with a regex and writes just that fragment to its own .tpl. By construction the fragment has no surrounding context, so the one thing that would make {ldelim} ship verbatim to the browser — the input sitting inside a {literal} region — is invisible to it.

Walk that regression through the whole gate:

• testFormTemplatesCarryStrictSteamPattern — source contains {ldelim}17{rdelim} ✅ passes
• testRenderedSteamPatternKeepsSeventeenDigitQuantifier — isolated fragment compiles to \d{17} ✅ passes
• testTemplatesHaveNoBareDigitBraceQuantifiers — strips the literal region first, sees nothing ✅ passes
• E2E — covers add-block, add-ban, submit; the three edit forms are not covered ❌ no signal

The browser then receives pattern="…|\d{ldelim}17{rdelim}". JS treats an invalid quantifier as literal characters, so that arm matches <digit>{ldelim}17{rdelim} — every SteamID64 rejected again, on the three surfaces nothing watches. Combined with note 1 this is the same failure class you fixed, reachable through a different door.

Cheap close: capture offsets (preg_match_all(..., PREG_OFFSET_CAPTURE)), compute the {literal}…{/literal} spans once, and assert each pattern="STEAM_ offset falls outside all of them. Rendering the full template would need every View property bound, so that is not worth chasing.

3. nonDefaultDelimiterTemplateBasenames() hardcodes the four -{ }- views

SteamIDValidationOrderTest.php:171. That list now exists in three places: the DELIMITERS constants themselves, the AGENTS.md prose at "Templates with non-default delimiters (currently …)", and this array. SmartyTemplateRule needs none of them because it reads the constant off the class it is visiting.

The $class::DELIMITERS !== View::DELIMITERS guard covers the direction where a view goes back to { }. The other direction is the one that bites: a new -{ }- view added later is not in the array, so its first legitimate {17} fails this test with a message instructing the author to write {ldelim}17{rdelim} — which would ship verbatim in that template. That is the failure mode the skip list exists to prevent, just deferred.

Deriving it removes the copy:

foreach (glob(ROOT . 'includes/View/*.php') ?: [] as $file) {
    $class = 'Sbpp\\View\\' . basename($file, '.php');
    if (is_subclass_of($class, View::class) && $class::DELIMITERS !== View::DELIMITERS) {
        $basenames[] = basename($class::TEMPLATE);
    }
}

Minor, same method: skipping by basename skips that filename in every theme. Only default exists today, so it is correct now and would over-skip if a second theme ever lands with its own default-delimiter page_login.tpl.

4. The scanner regex does not cover the quantifier forms AGENTS.md names

SteamIDValidationOrderTest.php:524 matches /\{[0-9]+\}/, but the rule you added to AGENTS.md says:

Regex quantifiers that use braces (\d{17}, [0-9]{1,3}, …) in a .tpl file MUST use {ldelim}17{rdelim}

{1,3} and {2,} are invisible to the scanner. Lower severity than the others — those forms are a Smarty parse error rather than a silent rewrite, so they fail loudly at render time instead of shipping a broken pattern — but the documented rule and the test that claims to enforce it should agree. /\{[0-9]+(?:,[0-9]*)?\}/ closes the gap.

5. Smaller things

• preg_match at line 524 reports only the first offender per file. Three bad quantifiers in one template take three CI rounds to clear. preg_match_all and collect them all.
• testFormTemplatesCarryStrictSteamPattern now asserts twice with partial overlap: assertStringContainsString($expected, …) pins the full shape but only for the first occurrence, and the preg_match_all loop pins only {ldelim}17{rdelim} for every occurrence. Neither pins the full shape on every occurrence. assertSame($expected, $attr) inside the loop does both and lets the first assertion go.
• self::$steamPatternCompileDir is assigned inside the test body, so tearDownAfterClass correctly no-ops under --filter. Fine as written, just noting it reads as deliberate.

6. Still open from last round

No issue was opened for the duplication, and the count went up rather than down: six template copies, the JS regex in page_admin_bans_add.tpl, SteamID::HANDLER_STRICT_REGEX, EXPECTED_STEAM_PATTERN in the spec, and two more string literals in the PHPUnit file. Three of the pins are tests whose entire job is to notice when the copies drift. Surfacing the PHP constant to the templates as a View property would have made "Smarty ate the braces" structurally impossible and would collapse those three tests into one. Not for this PR — say the word and I will open the issue.