- 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
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.
The phpunit.xml split into unit + integration testsuites requires the
integration directory to exist on disk, even when empty. The .gitkeep
keeps the directory present in fresh checkouts and on CI; the first real
integration test in a later task will replace the placeholder.
Final-review-driven changes (Tasks 6/M1, I1 from the ledger):
- Drop the redundant abilities-list check in CombatEngine::useAbility.
By construction (Scenario::__construct enforces the allowlist), any
ability the unit 'knows' is also archetype-allowed; the second check
alone is sufficient and removes a dead code path.
- Update testUseAbilityRejectsAnAbilityTheArchetypeForbids to assert the
'Unit archetype forbids that ability.' message it actually exercises.
- Compute $template once in Scenario::__construct's per-unit loop and
drop the redundant catalog lookup on the next line.
Keith Solomon
Claude
Keith SolomonandKeith Solomon