- Rename $secret to $csrfSecret in public/index.php to make
intent clear at every call site; the value is the CSRF/turn-token
HMAC key, not just 'the secret'.
- Drop the stale 'Configure the router with the eight routes' comment
in public/index.php; the router now has sixteen routes.
- Drop the duplicate .bf-toast rule in public/assets/styles.css; the
second declaration (the new yellow box) is the active one, the
first (legacy green pill) is dead code from the editor era.
- Add a trailing newline to src/Views/home.php.
- Tighten the matchId regex in two tests from {16,} to {16} since
bin2hex(random_bytes(8)) always produces exactly 16 hex chars; the
spec's {16,} stays in production TurnToken.
No behavior changes; addresses the deferred-minors parked during
per-task review.
When a browser session predates a server restart with a fresh
secret.key, the old __csrf HMAC cookie is no longer verifiable
under the new secret. Every action POST returns 403 until the user
manually clears cookies, because the existing-pair check on the
front controller was 'is the cookie present?' rather than 'does
the cookie verify under the current secret?'.
The new check at the top of the front controller computes the
expected HMAC of the raw-token cookie under the current secret and
constant-time-compares it to the __csrf cookie. If they don't match,
the controller re-mints a fresh pair on the response, replacing the
browser's stale cookies with a working set. The next click of 'Play
bundled' then succeeds without user intervention.
Test coverage in tests/Integration/StaleCookieRotationTest.php:
- testStaleCookiesAreReplacedOnNextRequest
- testValidCookiesAreNotReIssuedOnNextRequest
- testMissingTokenCookieIsFilledInOnNextRequest
public/index.php had a line that overwrote the local $secret with
$_SERVER['__csrf_secret'] ?? '' just before constructing the four
match-action handler closures. The synthetic key is only written
to the request's $server array (line 105), not to the global
$_SERVER superglobal, so the read always returned ''. The handlers
were constructed with an empty secret, TurnToken::verify then
HMAC'd the supplied token against '' which never matched the token
that PostStartMatch had minted with the real (32-byte) secret, and
every action returned 409 {error: turn}.
Fix: delete the line that re-reads $secret; the closures now use the
outer $secret (read from var/secret.key or BATTLEFORGE_SECRET at
script start).
CSS Grid without an explicit grid-template-columns defaults to a
single-column layout, so the 64-144 tiles stacked vertically and
the user saw a long column instead of an 8x8 (or 10x8 or 12x12)
battlefield. Set the column count via a CSS variable on the grid
root, written by match.js after the match snapshot loads (CSS attr()
for non-content properties is not yet shipped in Safari or Firefox).
Plan 3c: JavaScript frontend (storage.js, grid-editor.js, toolchain).
Adds the Node toolchain (ESLint airbnb-base + Prettier) with a CI step,
public/js/storage.js (bfStorage module for localStorage pre-fill and the home
page's recent-scenarios list), public/js/grid-editor.js (battlefield grid
interactions and JSON fetch save), public/assets/archetypes.json (the four
ArchetypeTemplate definitions), four placeholder PNGs at
public/assets/placeholders/, and updated <script> tags in home.php and
team-editor.php and a data-scenario-id attribute + script tag in
battlefield-editor.php. Also adds *.js and *.json to .gitattributes to
enforce LF line endings on those file types.
196/196 tests pass; PHPStan level 6 clean; ESLint 0 errors; Prettier clean.
Two fix commits were made during the per-task review loop: brief's URL
parsing slice index in storage.js was off-by-one (slice(-2,-1) returned
'edit' instead of the id), and brief's buildScenarioJson emitted a nested
objective shape that didn't match the backend's flat objectiveId/X/Y
fields. Both were corrected.
Plan 3b: editor handlers, views, and front controller.
Adds the 6 view templates (layout, home, team-editor, battlefield-editor,
match-stub deleted, upload-result deleted), the 5 remaining HTTP handlers
(GetTeamEditor, PostTeamEditor, GetBattlefieldEditor, PostBattlefieldEditor,
PostImageUpload, PostStartMatch), the real front controller in
public/index.php, and the FullFlowTest integration test. The final
whole-branch review found 3 Critical issues (broken upload-asset URL contract,
upload token source mismatch, and an XSS in the PostTeamEditor success page)
plus 2 Important issues (dead home.php/match-stub.php/upload-result.php);
all were fixed in 4 follow-up commits before this merge.
196/196 tests pass; PHPStan level 6 clean; PHPCS 0 errors on new files.
The web app is fully functional from the browser: php -S 0.0.0.0:8000 -t public
serves the home page, team editor, battlefield editor, image upload endpoint,
and asset-serving endpoint end-to-end.
Neither src/Views/match-stub.php nor src/Views/upload-result.php is
referenced by any handler or route in Plan 3b:
- match-stub is the Plan 4 battle-interface placeholder; Plan 3b does
not register a /match route.
- upload-result is the iframe-style post-upload response; Plan 3b
returns JSON from POST /assets/upload and Plan 3c will wire the
client-side upload form.
Both files were over-eager scaffolding from Task 3 (6a0b147). Drop
them in a follow-up commit rather than amend the shared Task 3 commit.
The handler had a stale inline heredoc that used a '{{ csrf }}' string
substitution to inject the token. The home.php template created in Task 2
is the canonical version (uses render_layout + Escape::attr). Switching
to the template removes the duplicated markup and the ad-hoc str_replace
escape path.
PostTeamEditor interpolated the scenario id into a <script> block as
'localStorage.setItem("scenario:<id>", ...)', so an id containing
'</script><script>...</script>' would break out of the JS-string context
and execute in the browser. Use json_encode with JSON_HEX_TAG / HEX_AMP /
HEX_APOS / HEX_QUOT so the id is always a safe JS string literal.
Adds a PostTeamEditorTest case that submits '</script><script>alert(1)</script>'
and asserts the literal '<script>alert(1)</script>' substring is absent
from the response. Updates the existing happy-path and FullFlowTest
assertions to match the new 'scenario:"demo"' (quoted) form.
- ImageUploadService::store() now returns /assets/uploads/<token>/<file>
so the URL the upload handler emits matches the new GET route.
- public/index.php registers a dedicated GET /assets/uploads/{userToken}/{filename}
route; the legacy /assets/{kind}/{filename} is kept as a fallback.
- GetAssets reads the upload token from $request->server['__uploads_token']
(the front controller threads it that way) instead of the cookies bag.
- Adds GetAssets unit tests for the upload-token happy path and a
token-mismatch 404, plus a FullFlowTest step that uploads and re-fetches
the asset through the front controller.
Add FullFlowTest, a single end-to-end test that exercises the home page, the team editor POST, the battlefield editor POST, and the start-match POST through the front controller (no php -S).
Wiring fixes in public/index.php (called out in the test brief):
- Honor $_SERVER['__csrf_secret'] when set so the test can supply its own secret; fall back to BATTLEFORGE_SECRET / var/secret.key otherwise.
- Read the raw body from $_SERVER['__raw_body'] when set; fall back to php://input otherwise. PHP CLI's php://input is empty, so an explicit hook is needed for JSON bodies.
- Return the response status from the front controller so the test can assert the HTTP status it set via http_response_code().
Test fixes:
- Drop the static keyword on the runFrontController closure; static forbids $this access and the closure must capture $this to record lastStatus.
- Drop the redundant '?? ''' on the ob_get_clean() result; (string) ob_get_clean() is always a string.
- Set $_SERVER['__raw_body'] alongside $this->rawBody for the JSON requests so the front controller can read the body.
Keith Solomon