Skip to content

Commit cbdf705

Browse files
committed
fix(version): reject stale tier-1 version.json below v2.0 (#1305)
`Sbpp\Version::resolve()` previously trusted any `configs/version.json` verbatim. The v1.x repo carried that file as a checked-in, hand-edited literal (`{"version": "1.8.1", "git": "1434"}`) until #1070 deleted it and made the release pipeline its sole writer. An operator who upgraded v1.8 → v2.0 with a "skip if exists" overlay tool (FTP, `rsync` without `--delete`, manual directory copy that treats `configs/` as user data) keeps the stale file on disk; the resolver returned its contents verbatim and the chrome footer read `SourceBans++ 1.8.1 | Git: 1434` on a v2.0 install (with `data-version="1.8.1"` poisoning telemetry, bug reports, and E2E specs that key off the attribute). Add a `Version::MIN_TIER1_MAJOR = 2` floor and gate the tier-1 input on it via a new `isAcceptableTier1Version()` helper. A version whose major component is below the floor falls through to tier-2 (`git describe`) or tier-3 (the `'dev'` sentinel) — the operator sees a self-describing fallback instead of phantom v1.x copy. Major-only comparison (vs. a strict `version_compare(..., '2.0.0', '>=')`) is deliberate so pre-release tarballs like `2.0.0-rc.1` — which sort BELOW `2.0.0` under semver's "pre-release < release" rule — stay accepted. The constant carries a docblock that calls out the "bump on every MAJOR release" maintenance burden, paired with a `testFloorConstantIsTwo()` regression guard so the bump becomes a deliberate test edit. Pin every reject branch (stale v1.x with git available + with git absent) and every accept branch (boundary `2.0.0`, pre-release `2.0.0-rc.1`, future `3.0.0`, plus a malformed-version data provider with six edge cases) in `web/tests/unit/VersionTest.php`. Document the v2.0.0 file-ownership contract (and the recovery procedure for already-upgraded installs) in `UPGRADING.md`; cross-link the floor from `AGENTS.md` "Where to find what" and `ARCHITECTURE.md`'s init.php lifecycle step 5. Fixes #1305.
1 parent a9bd72f commit cbdf705

5 files changed

Lines changed: 319 additions & 7 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1275,7 +1275,7 @@ audit (#1207) locked in. New CTAs:
12751275
| Add a shared "1 of these required" badge for an either/or input pair | `web/themes/default/page_submitban.tpl` (`data-required-group="…"` + the inline guard script — vanilla JS `// @ts-check`, blocks submit when both are empty) |
12761276
| Bootstrap (paths, autoload, theme) | `web/init.php` |
12771277
| Routing (`?p=…&c=…&o=…`) | `web/includes/page-builder.php` — unrecognised admin `c=…` returns the 404 page slot via `web/pages/page.404.php` + `Sbpp\View\NotFoundView` (#1207 ADM-1) |
1278-
| Resolve the panel version (`SB_VERSION`, `data-version="…"` footer hook) | `web/includes/Version.php` (`Sbpp\Version::resolve()`) — three-tier fallback: `configs/version.json` → `git describe` → the `'dev'` sentinel (#1207 CC-5) |
1278+
| Resolve the panel version (`SB_VERSION`, `data-version="…"` footer hook) | `web/includes/Version.php` (`Sbpp\Version::resolve()`) — three-tier fallback: `configs/version.json` → `git describe` → the `'dev'` sentinel (#1207 CC-5). Tier-1 input is gated by `Version::MIN_TIER1_MAJOR` (currently `2`) so a stale v1.x `version.json` preserved through a botched v1→v2 upgrade overlay falls through to tier-2/tier-3 instead of poisoning the footer with phantom v1.x copy (#1305). Bump `MIN_TIER1_MAJOR` on every MAJOR release (3.0, 4.0, …) — `web/tests/unit/VersionTest.php::testFloorConstantIsTwo` is the paired regression guard so the bump is a deliberate test edit. |
12791279
| Auth / JWT cookie | `web/includes/Auth/` (`Sbpp\Auth\*` — `Auth.php`, `JWT.php`, `UserManager.php`, `Host.php`, `Handler/{Normal,Steam}AuthHandler.php`; `openid.php` is third-party LightOpenID and stays in the global namespace) |
12801280
| CSRF | `web/includes/Security/CSRF.php` (`Sbpp\Security\CSRF`) |
12811281
| Schema | `web/install/includes/sql/struc.sql` |

‎ARCHITECTURE.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,15 @@ Both scripts include `init.php` first, which performs identical bootstrap.
165165
parsing the user-visible string (#1207 CC-5). Dev-checkout panels
166166
are identified by `SB_VERSION === Version::DEV_SENTINEL`; the
167167
footer's "| Git: <sha>" suffix gates on `SB_GITREV` directly so a
168-
separate boolean isn't needed (#1214).
168+
separate boolean isn't needed (#1214). Tier-1 input is gated by
169+
`Version::MIN_TIER1_MAJOR` (currently `2`) so a stale v1.x
170+
`version.json` preserved through a botched v1→v2 upgrade overlay
171+
falls through to tier-2/tier-3 instead of poisoning the footer
172+
with phantom v1.x copy (#1305 — the v1.x repo carried the file as
173+
a checked-in, hand-edited literal until #1070 deleted it; an
174+
upgrade overlay tool that preserves existing files keeps the v1.x
175+
contents on disk, and pre-#1305 the resolver returned them
176+
verbatim because tier-1 was unconditional).
169177
6. Reads `configs/permissions/web.json` + `sourcemod.json` and `define()`s
170178
each flag as a global PHP constant (`ADMIN_OWNER`, `ADMIN_ADD_BAN`, …).
171179
7. Constructs the global `$theme` (Smarty) with the configured theme dir,

‎UPGRADING.md‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,42 @@ land on the [`CHANGELOG.md`](CHANGELOG.md) instead.
88
Future entries will land here as the project ships major upgrades; this
99
section currently covers the v2.0.0 upgrade path.
1010

11+
## Replace `web/configs/version.json` (#1305, v2.0.0)
12+
13+
**Overlay your v2.0.0 tarball over an existing v1.x install with a
14+
tool that overwrites every file.** v1.x repos shipped a checked-in,
15+
hand-edited `web/configs/version.json` (`{"version": "1.8.1", "git":
16+
"1434"}`); v2.0.0 ships a fresh `version.json` written by the release
17+
pipeline at build time. The two files live at the same path, so an
18+
overlay tool that PRESERVES existing files (FTP "skip if exists",
19+
`rsync` without `--delete`, a manual directory-by-directory copy that
20+
treats `configs/` as user data alongside `configs/permissions/*.json`)
21+
keeps the stale v1.x file on disk.
22+
23+
If that happens on v2.0.0 the panel footer reads
24+
`SourceBans++ 1.8.1 | Git: 1434` instead of the actual installed
25+
version. v2.0.0 ships a structural guard in `Sbpp\Version::resolve()`
26+
(#1305) that REJECTS a tier-1 `version.json` whose major component is
27+
below v2.0.0, so the failure mode is now "footer reads `dev` or the
28+
live `git describe` string" instead of "footer reads phantom v1.x
29+
copy". You still want the file to be the v2.0.0 release pipeline's
30+
fresh write so the footer reads `2.0.0` exactly.
31+
32+
**The fix on an already-upgraded install**: re-extract the v2.0.0
33+
tarball over your install with a tool that overwrites every file
34+
(`unzip -o` instead of `unzip`, `rsync -av --delete` instead of plain
35+
`rsync -av`, a manual `cp` that overwrites). `web/configs/version.json`
36+
is the file at issue; `web/configs/permissions/web.json` and
37+
`web/configs/permissions/sourcemod.json` are the only files in
38+
`web/configs/` that an operator might have hand-edited and want to
39+
preserve, so back those up first if you've customised them.
40+
41+
This file is owned by the release pipeline, not by the operator;
42+
v2.0.0+ tarballs always carry it and v2.0.0+ dev clones gitignore it
43+
(the runtime falls through to `git describe`). There is no in-panel
44+
toggle for the resolved version — it's read once per request from
45+
`web/configs/version.json` (and the two fallback tiers).
46+
1147
## Telemetry (#1126, v2.0.0)
1248

1349
**SourceBans++ 2.0.0 ships with default-on anonymous telemetry.**

‎web/includes/Version.php‎

Lines changed: 85 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
*
1111
* 1. `configs/version.json` — emitted into release tarballs by the
1212
* release pipeline. Self-hosters who installed by unzipping a tarball
13-
* always hit this branch.
13+
* always hit this branch. The file's major version is gated against
14+
* `MIN_TIER1_MAJOR` so a stale v1.x file preserved through a
15+
* botched v1→v2 overlay can never be trusted (#1305).
1416
* 2. `git describe --tags --always` (with `git rev-parse --short HEAD`
1517
* as a sibling) — covers operators running directly off a git
1618
* checkout outside Docker. Returns a tag (e.g. `2.1.0`) when
@@ -38,6 +40,44 @@ final class Version
3840
{
3941
public const DEV_SENTINEL = 'dev';
4042

43+
/**
44+
* Floor for the tier-1 (`configs/version.json`) major component.
45+
*
46+
* The release pipeline (`.github/workflows/release.yml`) writes the
47+
* git tag verbatim into the tarball's `configs/version.json` at build
48+
* time, so a healthy tarball ALWAYS carries `version` aligned with
49+
* the codebase's current major. A file whose major is below this
50+
* floor is structurally stale — the canonical case (#1305) is the
51+
* v1.x repo's checked-in `{"version": "1.8.1", "git": "1434"}` file
52+
* surviving a v1.8 → v2.0 upgrade because the operator's overlay
53+
* tool ("FTP skip if exists", `rsync` without `--delete`, manual
54+
* directory copy that treats `configs/` as user data) preserved it.
55+
* The resolver rejects such files and falls through to tier-2 /
56+
* tier-3 so the footer reads a self-describing fallback (`dev` or
57+
* the live git describe string) instead of phantom v1.x copy.
58+
*
59+
* Bump this on every MAJOR release (3.0, 4.0, …) — patch and minor
60+
* releases stay below the same floor. Forgetting to bump it on a
61+
* future v3.0 silently re-opens the same bug pattern for v2.x
62+
* files in v3.x installs (the maintainer would see the symptom in
63+
* the bug report and bump it then), but the floor check is the
64+
* structural guard that means a stale tier-1 input can never win
65+
* silently regardless of what the upgrade overlay tool did.
66+
*
67+
* Major-only (vs. a full `version_compare` against `MIN_VERSION =
68+
* '2.0.0'`) is deliberate: pre-release tarballs like `2.0.0-rc.1`
69+
* sort BELOW `2.0.0` under PHP's `version_compare` (correctly per
70+
* semver's "pre-release < release" rule), so a strict
71+
* `version_compare(..., '2.0.0', '>=')` would silently reject every
72+
* v2.0.0-rc.* tarball install. Comparing the major component alone
73+
* accepts pre-release tags within the current major while still
74+
* rejecting any v1.x file. The release.yml regex enforces full
75+
* semver on the tag (`^[0-9]+\.[0-9]+\.[0-9]+...`), so a tier-1
76+
* file that doesn't even match `^v?\d+` is doubly suspect (manual
77+
* edit, empty / corrupt JSON) and falls through to tier-2.
78+
*/
79+
public const MIN_TIER1_MAJOR = 2;
80+
4181
/**
4282
* Resolve the version pair `[version, git]` exactly the way
4383
* `init.php` consumes it for the `SB_VERSION` / `SB_GITREV`
@@ -54,6 +94,11 @@ final class Version
5494
* it (the gated branch had already gone obsolete because it
5595
* compared a numeric git rev that no longer exists).
5696
*
97+
* Tier-1 input is gated by `isAcceptableTier1Version()` so a
98+
* stale v1.x file (or a malformed entry) can't poison the chrome
99+
* footer and downstream consumers like the telemetry payload.
100+
* See `MIN_TIER1_MAJOR` for the rationale.
101+
*
57102
* @param callable|null $jsonReader fn(string $path): ?array — defaults to
58103
* file_get_contents + json_decode.
59104
* @param callable|null $gitDescribe fn(): string — defaults to shell_exec.
@@ -72,10 +117,15 @@ public static function resolve(
72117

73118
$tarball = $jsonReader($versionJsonPath);
74119
if (is_array($tarball) && isset($tarball['version'])) {
75-
return [
76-
'version' => (string) $tarball['version'],
77-
'git' => $tarball['git'] ?? 0,
78-
];
120+
$jsonVersion = (string) $tarball['version'];
121+
if (self::isAcceptableTier1Version($jsonVersion)) {
122+
return [
123+
'version' => $jsonVersion,
124+
'git' => $tarball['git'] ?? 0,
125+
];
126+
}
127+
// Stale / malformed tier-1 input: deliberately fall through
128+
// to tier-2 (git describe) and tier-3 (dev sentinel) below.
79129
}
80130

81131
$tag = trim($gitDescribe());
@@ -93,6 +143,36 @@ public static function resolve(
93143
];
94144
}
95145

146+
/**
147+
* True when a tier-1 `configs/version.json` `version` field is
148+
* trustworthy enough to surface as `SB_VERSION`. Keys off
149+
* `MIN_TIER1_MAJOR` — see that constant's docblock for the full
150+
* rationale (#1305).
151+
*
152+
* Reject conditions (all collapse to "fall through to tier-2"):
153+
* - `'1.8.1'` / `'1.x.y'` for any x, y — the canonical v1.x
154+
* baked-in stale file from before #1070's deletion.
155+
* - `'dev'` / `'unknown'` / `''` / any string that doesn't
156+
* start with an optional `v` and a major digit — defensive
157+
* against operator hand-edits and truncated / corrupt JSON.
158+
* - A semver tag whose major is below the floor.
159+
*
160+
* Accept conditions:
161+
* - Any version where the major component (after an optional
162+
* `v` prefix) is `>= MIN_TIER1_MAJOR`. This lets pre-release
163+
* tags within the current major (`2.0.0-rc.1`, `2.1.0-beta.3`)
164+
* through, plus any future major (`3.0.0`, `4.5.2`) without a
165+
* code change.
166+
*/
167+
private static function isAcceptableTier1Version(string $version): bool
168+
{
169+
if (!preg_match('/^v?(\d+)/', $version, $m)) {
170+
return false;
171+
}
172+
173+
return (int) $m[1] >= self::MIN_TIER1_MAJOR;
174+
}
175+
96176
/**
97177
* @return callable(string): ?array<string, mixed>
98178
*/

‎web/tests/unit/VersionTest.php‎

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
namespace Sbpp\Tests\Unit;
66

7+
use PHPUnit\Framework\Attributes\DataProvider;
78
use PHPUnit\Framework\TestCase;
89
use Sbpp\Version;
910

@@ -151,4 +152,191 @@ public function testMissingVersionJsonFallsThrough(): void
151152

152153
$this->assertSame(Version::DEV_SENTINEL, $resolved['version']);
153154
}
155+
156+
/**
157+
* Issue #1305 — the canonical "v1.x file preserved through a
158+
* v1→v2 upgrade overlay" case. The v1.x repo carried
159+
* `web/configs/version.json` as a checked-in, hand-edited file
160+
* with `{"version": "1.8.1", "git": "1434"}`; that file was
161+
* deleted from `main` in #1070 (commit `9d1caefd`, May 2 2026)
162+
* with the release workflow taking over file ownership at build
163+
* time. An operator who upgraded a v1.8 install to v2.0 with a
164+
* "skip if exists" overlay tool (FTP, `rsync` without `--delete`,
165+
* a manual directory-by-directory copy that treats `configs/` as
166+
* user data) keeps the stale file on disk; pre-#1305 the
167+
* resolver returned that file's contents verbatim and the chrome
168+
* footer read `SourceBans++ 1.8.1 | Git: 1434` on a v2.0 install.
169+
*
170+
* The fix: tier-1 input is gated by the major-component floor
171+
* (`Version::MIN_TIER1_MAJOR`). Anything below the floor falls
172+
* through to tier-2 (`git describe`) so the operator sees the
173+
* actual codebase version (or the `'dev'` sentinel — see the
174+
* sibling test below — instead of phantom v1.x copy.
175+
*/
176+
public function testStaleV1JsonFallsThroughToGitDescribe(): void
177+
{
178+
$resolved = Version::resolve(
179+
versionJsonPath: '/whatever',
180+
jsonReader: static fn (): array => [
181+
'version' => '1.8.1',
182+
'git' => '1434',
183+
],
184+
gitDescribe: static fn (): string => "v2.0.0\n",
185+
gitShortRev: static fn (): string => "abc1234\n",
186+
);
187+
188+
$this->assertSame('v2.0.0', $resolved['version']);
189+
$this->assertSame('abc1234', $resolved['git']);
190+
}
191+
192+
/**
193+
* Companion to the test above — same stale-tier-1 case but on a
194+
* production install where git isn't available (no `.git` dir,
195+
* no `git` binary in the image). The fall-through cascade lands
196+
* at tier-3, the `'dev'` sentinel. That's not a perfect outcome
197+
* (the operator sees `dev` instead of the actual `2.0.0` they
198+
* deployed), but it's a self-describing signal that something
199+
* is wrong with the install metadata — and crucially, it's NOT
200+
* the phantom v1.x string that telemetry / bug reports / E2E
201+
* specs would key off.
202+
*/
203+
public function testStaleV1JsonFallsThroughToDevSentinel(): void
204+
{
205+
$resolved = Version::resolve(
206+
versionJsonPath: '/whatever',
207+
jsonReader: static fn (): array => [
208+
'version' => '1.8.1',
209+
'git' => '1434',
210+
],
211+
gitDescribe: static fn (): string => '',
212+
gitShortRev: static fn (): string => '',
213+
);
214+
215+
$this->assertSame(Version::DEV_SENTINEL, $resolved['version']);
216+
$this->assertSame(0, $resolved['git']);
217+
}
218+
219+
/**
220+
* Boundary — a tier-1 file exactly at the floor (`2.0.0`) is
221+
* accepted. The release workflow writes the git tag verbatim, so
222+
* the v2.0.0 tarball's `version.json` reads `"version": "2.0.0"`
223+
* exactly — that's the case this guards.
224+
*/
225+
public function testTarballJsonAtFloorMajorIsAccepted(): void
226+
{
227+
$resolved = Version::resolve(
228+
versionJsonPath: '/whatever',
229+
jsonReader: static fn (): array => [
230+
'version' => '2.0.0',
231+
'git' => 'abc1234',
232+
],
233+
gitDescribe: static fn (): string => self::fail('git describe must not run when JSON tier-1 is acceptable'),
234+
gitShortRev: static fn (): string => self::fail('git rev-parse must not run when JSON tier-1 is acceptable'),
235+
);
236+
237+
$this->assertSame('2.0.0', $resolved['version']);
238+
$this->assertSame('abc1234', $resolved['git']);
239+
}
240+
241+
/**
242+
* Pre-release tags within the current major (`2.0.0-rc.1`,
243+
* `2.1.0-beta.3`) are accepted. The release.yml regex allows
244+
* `^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?...` so a v2.0.0-rc.1
245+
* tarball is a real shape. Compared to a strict
246+
* `version_compare(jsonVersion, '2.0.0', '>=')` floor — which
247+
* would reject `2.0.0-rc.1` because PHP / semver sort
248+
* pre-release identifiers BEFORE the release — major-only
249+
* comparison correctly admits them.
250+
*/
251+
public function testTarballJsonPreReleaseWithinCurrentMajorIsAccepted(): void
252+
{
253+
$resolved = Version::resolve(
254+
versionJsonPath: '/whatever',
255+
jsonReader: static fn (): array => [
256+
'version' => '2.0.0-rc.1',
257+
'git' => 'abc1234',
258+
],
259+
gitDescribe: static fn (): string => self::fail('git describe must not run when pre-release JSON is acceptable'),
260+
gitShortRev: static fn (): string => self::fail('git rev-parse must not run when pre-release JSON is acceptable'),
261+
);
262+
263+
$this->assertSame('2.0.0-rc.1', $resolved['version']);
264+
$this->assertSame('abc1234', $resolved['git']);
265+
}
266+
267+
/**
268+
* Forward-compat — a future major (3.x.y) is accepted without a
269+
* code change here. The floor is a *minimum*, not a *match*;
270+
* future v3 tarballs are perfectly trustworthy on the current v2
271+
* codebase as long as the deployment is consistent (which is the
272+
* operator's contract — the floor is here to catch the *backward*
273+
* mismatch that #1305 surfaces, not to enforce same-major).
274+
*/
275+
public function testTarballJsonFutureMajorIsAccepted(): void
276+
{
277+
$resolved = Version::resolve(
278+
versionJsonPath: '/whatever',
279+
jsonReader: static fn (): array => [
280+
'version' => '3.0.0',
281+
'git' => 'def5678',
282+
],
283+
gitDescribe: static fn (): string => self::fail('git describe must not run when forward-compat JSON is acceptable'),
284+
gitShortRev: static fn (): string => self::fail('git rev-parse must not run when forward-compat JSON is acceptable'),
285+
);
286+
287+
$this->assertSame('3.0.0', $resolved['version']);
288+
$this->assertSame('def5678', $resolved['git']);
289+
}
290+
291+
/**
292+
* Defensive — a malformed `version` field (empty string, free
293+
* text, the `'dev'` sentinel itself somehow slipping into the
294+
* JSON, a hand-edited "unknown" placeholder) doesn't match the
295+
* `^v?\d+` shape and falls through. The release.yml regex
296+
* enforces full semver on the build tag so a real tarball can
297+
* never produce these, but operator hand-edits and corrupted
298+
* JSON are real-world failure modes worth pinning.
299+
*/
300+
#[DataProvider('provideMalformedTier1Versions')]
301+
public function testTarballJsonMalformedFallsThrough(string $malformedVersion): void
302+
{
303+
$resolved = Version::resolve(
304+
versionJsonPath: '/whatever',
305+
jsonReader: static fn () => [
306+
'version' => $malformedVersion,
307+
'git' => 'abc1234',
308+
],
309+
gitDescribe: static fn (): string => 'v2.0.0',
310+
gitShortRev: static fn (): string => 'def5678',
311+
);
312+
313+
$this->assertSame('v2.0.0', $resolved['version'], "malformed tier-1 version '$malformedVersion' should fall through to tier-2");
314+
$this->assertSame('def5678', $resolved['git']);
315+
}
316+
317+
/**
318+
* @return iterable<string, array{0: string}>
319+
*/
320+
public static function provideMalformedTier1Versions(): iterable
321+
{
322+
yield 'empty string' => [''];
323+
yield 'unknown placeholder' => ['unknown'];
324+
yield 'dev sentinel leaked into JSON' => ['dev'];
325+
yield 'free-text edit' => ['SourceBans++'];
326+
yield 'leading dot' => ['.0.0'];
327+
yield 'wrong prefix' => ['ver1.8.1'];
328+
}
329+
330+
/**
331+
* Pin the floor constant explicitly. A future bump of the floor
332+
* (say, when v3.0 ships and the maintainer wants to start
333+
* rejecting v2.x stale files in v3.x installs the same way #1305
334+
* rejects v1.x in v2.x) shows up here as a deliberate test edit
335+
* rather than a silent constant change. Keeps the rationale
336+
* paired with the bump.
337+
*/
338+
public function testFloorConstantIsTwo(): void
339+
{
340+
$this->assertSame(2, Version::MIN_TIER1_MAJOR);
341+
}
154342
}

0 commit comments

Comments
 (0)