Repository navigation
fix(1419): drop api-contract from prod Apache deny list (login spinner under prod Docker image) - #1445
Merged
Conversation
The production Docker image (`docker/Dockerfile.prod` → `ghcr.io/sbpp/sourcebans-pp:*`) ships an Apache hardening conf (`docker/apache/sbpp-prod.conf`) whose `<FilesMatch>` deny list union carried a stray `api-contract\..*` alternation. Apache's `<FilesMatch>` matches the **basename** regardless of which directory the URL resolves under, so the regex also denied the published `web/scripts/api-contract.js` — the autogenerated chrome JS `core/header.tpl` loads on every page render to define the `Actions.*` / `Perms.*` namespaces every `sb.api.call(Actions.PascalName, …)` site in the panel JS depends on. Symptoms under the production Docker image: - Password login spins forever. The form's submit handler in `page_login.tpl` runs `e.preventDefault()` + `setBusy(submitBtn, true)` BEFORE `sb.api.call(Actions.AuthLogin, …)` fires; with `Actions` undefined the call throws and neither `.then` branch ever releases the spinner. Reporter's exact symptom on #1419 ("login spinner runs but nothing happens"). - Every panel chrome action button that drives a JSON action (Notes pane, ban/comm unblock, admin/mod delete, group-ban dispatcher, server refresh, comment edit, …) is dead-on-arrival. - Steam login is unaffected because the OpenID round-trip is server-side redirects with no JS dependency on `Actions.*` — exactly the asymmetry the bug reporter saw. The bug only surfaces under the production Docker image. The dev compose stack uses a different Apache config without the deny union, and tarball installs don't ship Apache at all. Per git archaeology the regex was byte-identical from rc3 → rc5 (`docker/apache/sbpp-prod.conf` was introduced wholesale in ba3c887 with the rule already present); rc3 reporters who "didn't see the bug" had only exercised Steam login. Fix shape: 1. **`docker/apache/sbpp-prod.conf`** — drop `api-contract\..*` from the `<FilesMatch>` regex union and add a load-bearing comment block above the rule explaining that every basename listed there must be a panel-root-only config file with NO published browser asset sharing the basename. Concrete guidance for future deny-rule additions: prefer `<Files "exact-name">` for unique-basename root configs, `<LocationMatch "^/exact-path$">` when the basename could collide with a published asset. 2. **`web/tests/integration/ProdApacheConfigTest.php`** — new per-PR static gate (5 tests / 8 assertions). Pure file-scanning + PCRE matching, no DB / Smarty / vendor bring-up. Four contracts: - **Structural check**: every `<FilesMatch>` regex extracted from the conf is run against every published browser-asset basename under `web/scripts/` and `web/themes/default/js/`. A match anywhere fails the build with the offending regex + asset basename pair named in the message. Assets are discovered dynamically so a new chrome JS file landing in a future PR is automatically protected against the bug class. - **Sister gate** for the `<Files "exact-name">` shape (the conf carries `<Files "config.php">` today; a future `<Files "api-contract.js">` would bypass the FilesMatch gate but hit this one). - **Forward-looking spot-check** pinning the literal substring `api-contract` doesn't reappear inside any deny directive — reads as a direct #1419 reference at PR-review time. - **Sanity check** that the intended hardening targets stay matched (composer.{json,lock}, phpstan.neon, phpunit.xml.dist, package.json, tsconfig.json) so a future refactor of the regex doesn't silently delete the actual hardening. Bug-bites verification: temporarily reverted the conf fix and reran — both the structural gate and the historical-pattern gate fire with informative messages naming the exact regex and asset. 3. **`docker-compose.yml`** + **`docker/README.md`** — bind-mount `./docker:/var/www/html/docker:ro` so the new test can read the conf from inside the dev container. CI's `actions/checkout@v4` pulls the full repo so it works there without changes; the mount is what keeps the local `./sbpp.sh test` runner symmetric with the CI gate. 4. **`AGENTS.md`** — new entry under "Anti-patterns (do NOT reintroduce)" naming the bug class, the symptom asymmetry (Steam login fine, password login dead), the prod-only surfacing, and the regression guard. Out of scope (filed as follow-up rationale in the PR body): - A pre-publish CI smoke-test in `docker-image.yml` that builds the image single-arch with `--load`, runs it, and curls the published assets before signing/publishing. The Docker-image workflow is release-only by design (#1381); a smoke-test there would close the remaining "regression slips through to a tagged release" window. Worth a separate PR — adds ~2-3 minutes to release build time. - `phpstan-baseline.neon` / `phpstan-bootstrap.php` are sensitive but not covered by the conf's `phpstan\..*` regex (no dot after `phpstan`). Widening the deny list is a separate concern; the new structural test now gates "deny rules don't shadow assets" so any such widening can land safely. Gates: PHPStan ✓ (0 errors) · PHPUnit 886 / 3329 ✓ (incl. new ProdApacheConfigTest 5/5) · ts-check ✓ · api-contract clean ✓. Fixes #1419
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The production Docker image (
docker/Dockerfile.prod→ghcr.io/sbpp/sourcebans-pp:*) ships an Apache hardening conf(
docker/apache/sbpp-prod.conf) whose<FilesMatch>deny list unioncarried a stray
api-contract\..*alternation. Apache's<FilesMatch>matches the basename regardless of which directory the URL resolves
under, so the regex also denied the published
web/scripts/api-contract.js— the autogenerated chrome JS
core/header.tplloads on every pagerender to define the
Actions.*/Perms.*namespaces everysb.api.call(Actions.PascalName, …)site in the panel JS depends on.Symptoms under the production Docker image:
page_login.tplrunse.preventDefault()+setBusy(submitBtn, true)BEFORE
sb.api.call(Actions.AuthLogin, …)fires; withActionsundefined the call throws and neither
.thenbranch ever releasesthe spinner. Reporter's exact symptom on Login won't work anymore #1419 ("login spinner runs
but nothing happens").
pane, ban/comm unblock, admin/mod delete, group-ban dispatcher,
server refresh, comment edit, …) is dead-on-arrival.
server-side redirects with no JS dependency on
Actions.*— exactlythe asymmetry the bug reporter saw.
The bug only surfaces under the production Docker image. The dev
compose stack uses a different Apache config without the deny union,
and tarball installs don't ship Apache at all. Per git archaeology the
regex was byte-identical from rc3 → rc5 (
docker/apache/sbpp-prod.confwas introduced wholesale in
ba3c8872with the rule already present);rc3 reporters who "didn't see the bug" had only exercised Steam login.
Fix
docker/apache/sbpp-prod.conf— dropapi-contract\..*fromthe
<FilesMatch>regex union. The single load-bearing change.Also adds a comment block above the rule explaining that every
basename listed there must be a panel-root-only config file with
NO published browser asset sharing the basename, with concrete
guidance for future deny-rule additions: prefer
<Files "exact-name">for unique-basename root configs,
<LocationMatch "^/exact-path$">when basename collisions are a real concern.
web/tests/integration/ProdApacheConfigTest.php— new per-PRstatic gate (5 tests / 8 assertions). Pure file-scanning + PCRE
matching, no DB / Smarty / vendor bring-up. Four contracts:
<FilesMatch>regex extracted fromthe conf is run against every published browser-asset basename
under
web/scripts/andweb/themes/default/js/. A matchanywhere fails the build with the offending regex + asset
basename pair named in the message. Assets are discovered
dynamically so a new chrome JS file landing in a future PR is
automatically protected against the bug class.
<Files "exact-name">shape (the confcarries
<Files "config.php">today; a future<Files "api-contract.js">would bypass the FilesMatch gate buthit this one).
api-contractdoesn't reappear inside any deny directive —reads as a direct Login won't work anymore #1419 reference at PR-review time.
(composer.{json,lock}, phpstan.neon, phpunit.xml.dist,
package.json, tsconfig.json) so a future refactor of the regex
doesn't silently delete the actual hardening.
docker-compose.yml+docker/README.md— bind-mount./docker:/var/www/html/docker:roso the new test can read theconf from inside the dev container. CI's
actions/checkout@v4pulls the full repo so the test runs there without changes; the
mount is what keeps the local
./sbpp.sh testrunner symmetricwith the CI gate.
AGENTS.md— new entry under "Anti-patterns (do NOTreintroduce)" naming the bug class, the symptom asymmetry, the
prod-only surfacing, and the regression guard.
Verification
./sbpp.sh phpstan→ 0 errors../sbpp.sh test→ 886 tests / 3329 assertions all green(includes the new 5-test
ProdApacheConfigTest)../sbpp.sh ts-check→ green../sbpp.sh composer api-contract→ no diff vs checked-in file.Bug-bites verification: temporarily reverted the conf fix
(
git stashonsbpp-prod.conf) and reran the gate. Both thestructural check and the historical-pattern check fire with
informative messages naming the exact offending regex + asset:
Out of scope (follow-ups)
docker-image.yml: build the imagesingle-arch with
--load, run it, curl/scripts/*.js+/themes/default/js/*.js, assert HTTP 200 beforesigning/publishing. The Docker-image workflow is release-only by
design (Production Docker image + production docker-compose for self-hosters (and one-click-deploy gateway) #1381); a smoke-test there would close the remaining
"regression slips through to a tagged release" window. Worth a
separate PR — adds ~2-3 minutes to release build time.
phpstan-baseline.neon/phpstan-bootstrap.phpnot coveredby the conf's
phpstan\..*regex (no dot afterphpstan).Sensitive too (suppressed-warning map / autoload glue). Widening
the deny list is a separate concern; the new structural test now
gates "deny rules don't shadow assets" so any such widening can
land safely.
Test plan
ProdApacheConfigTestpasses after the fix andfails when the fix is reverted (with named offending regex).
release and confirm
curl https://<host>/scripts/api-contract.jsreturns 200 (was 403 pre-fix on rc3-rc5).
under the production Docker image (per Login won't work anymore #1419 reporter's
reproduction).
Reviewer's reading guide
The single load-bearing change is removing
api-contract\..*fromthe
<FilesMatch>regex onsbpp-prod.conf:75. Everything else(test, AGENTS.md anti-pattern, dev-stack mount, README note) is the
regression-prevention scaffold so the same bug shape can't land
silently again.
Fixes #1419