Five low-priority follow-up items surfaced during adversarial review of #1336 (the implementation PR for #1335). All were explicitly P3 — out-of-scope for the headline fix but worth tracking so they don't get lost.
P3.1 — recovery.php absolute Location header
web/install/recovery.php's direct-hit redirect (added by #1335 m1) emits Location: /install/, an absolute server-root path. On panels deployed under a sub-path (e.g. reverse-proxied at /sbpp/ instead of /), this would 302 to the wrong URL. The fix shape is to either:
- Compute the redirect target from
$_SERVER['SCRIPT_NAME'] / dirname($_SERVER['REQUEST_URI']) so the path is relative to where recovery.php was actually reached, OR
- Document that sub-path hosting requires a manual rewrite rule (and surface a banner in the recovery page when SCRIPT_NAME doesn't end in
/install/recovery.php).
The existing web/init.php's header('Location: install/') already uses the relative form (which is fine because PHP resolves relative to the request URI), so the fix could be as small as dropping the leading / in recovery.php.
P3.2 — testLocalhostHostHeaderDoesNotBypassInstallGuard leaks $_SERVER state
web/tests/integration/InstallGuardTest.php::testLocalhostHostHeaderDoesNotBypassInstallGuard sets \$_SERVER['HTTP_HOST'] = 'localhost' but never restores the prior value in tearDown / a finally block. The test passes because the post-#1335 guard ignores the Host header entirely (so the leaked value never breaks anything), but a future test that DID depend on HTTP_HOST would silently pick up the leaked value.
Fix shape: capture the prior value at the top of the test, restore in the finally block alongside the rmTempRoot() call.
P3.3 — config.php missing + install/ also missing edge case
web/init.php step order is:
- If
config.php missing → header('Location: install/'); exit;
- If
install/ present (and not exempted) → render the install-blocked page.
If both config.php AND install/ are missing (deleted by an over-zealous cleanup), step 1 sends the user to /install/, which 404s. The user lands on the webserver's bare 404 with no panel context.
Fix shape: introduce a third recovery scenario in web/init-recovery.php ("not-installed-and-no-installer") that surfaces the same chrome as the others, explaining the operator needs to re-upload from a release zip. The detection is !file_exists(ROOT.'/config.php') && !file_exists(ROOT.'/install').
P3.4 — docker/php/dev-prepend.php directory naming
The constant SBPP_DEV_KEEP_INSTALL lives in a file under docker/php/, but its purpose is panel-runtime — it's an opt-in for the panel's install-guard. The path is correct (the file is wired via PHP's auto_prepend_file ini at the docker layer), but the directory naming makes the contract a little surprising on first read.
Fix shape (cosmetic): rename docker/php/dev-prepend.php to something like docker/php/sbpp-dev-prepend.php so a grep for SBPP_DEV_KEEP_INSTALL lands on a filename that namespaces it. No functional change. Update the ini reference + comments in docker/Dockerfile accordingly.
P3.5 — Dev-stack image rebuild concern
docker/php/dev-prepend.php is COPYed into the image at Dockerfile build time. A developer who edits it after ./sbpp.sh up won't see the change until they rebuild (./sbpp.sh rebuild). There's no automated guard that catches a drift between the worktree's dev-prepend.php and the image's copy.
Fix shape: either:
- Bind-mount
dev-prepend.php into the running container (so edits take effect on the next request — same model as the bind-mount on web/), OR
- Document the rebuild requirement next to the file (a one-liner comment at the top would suffice).
The bind-mount path is cleaner; the comment-only path is zero-risk.
References: PR #1336 (the implementation PR for #1335), specifically the post-merge review comments. Original audit issue: #1335.
Five low-priority follow-up items surfaced during adversarial review of #1336 (the implementation PR for #1335). All were explicitly P3 — out-of-scope for the headline fix but worth tracking so they don't get lost.
P3.1 —
recovery.phpabsolute Location headerweb/install/recovery.php's direct-hit redirect (added by #1335 m1) emitsLocation: /install/, an absolute server-root path. On panels deployed under a sub-path (e.g. reverse-proxied at/sbpp/instead of/), this would 302 to the wrong URL. The fix shape is to either:$_SERVER['SCRIPT_NAME']/dirname($_SERVER['REQUEST_URI'])so the path is relative to whererecovery.phpwas actually reached, OR/install/recovery.php).The existing
web/init.php'sheader('Location: install/')already uses the relative form (which is fine because PHP resolves relative to the request URI), so the fix could be as small as dropping the leading/inrecovery.php.P3.2 —
testLocalhostHostHeaderDoesNotBypassInstallGuardleaks$_SERVERstateweb/tests/integration/InstallGuardTest.php::testLocalhostHostHeaderDoesNotBypassInstallGuardsets\$_SERVER['HTTP_HOST'] = 'localhost'but never restores the prior value intearDown/ afinallyblock. The test passes because the post-#1335 guard ignores the Host header entirely (so the leaked value never breaks anything), but a future test that DID depend onHTTP_HOSTwould silently pick up the leaked value.Fix shape: capture the prior value at the top of the test, restore in the
finallyblock alongside thermTempRoot()call.P3.3 —
config.phpmissing +install/also missing edge caseweb/init.phpstep order is:config.phpmissing →header('Location: install/'); exit;install/present (and not exempted) → render the install-blocked page.If both
config.phpANDinstall/are missing (deleted by an over-zealous cleanup), step 1 sends the user to/install/, which 404s. The user lands on the webserver's bare 404 with no panel context.Fix shape: introduce a third recovery scenario in
web/init-recovery.php("not-installed-and-no-installer") that surfaces the same chrome as the others, explaining the operator needs to re-upload from a release zip. The detection is!file_exists(ROOT.'/config.php') && !file_exists(ROOT.'/install').P3.4 —
docker/php/dev-prepend.phpdirectory namingThe constant
SBPP_DEV_KEEP_INSTALLlives in a file underdocker/php/, but its purpose is panel-runtime — it's an opt-in for the panel's install-guard. The path is correct (the file is wired via PHP'sauto_prepend_fileini at the docker layer), but the directory naming makes the contract a little surprising on first read.Fix shape (cosmetic): rename
docker/php/dev-prepend.phpto something likedocker/php/sbpp-dev-prepend.phpso a grep forSBPP_DEV_KEEP_INSTALLlands on a filename that namespaces it. No functional change. Update the ini reference + comments indocker/Dockerfileaccordingly.P3.5 — Dev-stack image rebuild concern
docker/php/dev-prepend.phpis COPYed into the image atDockerfilebuild time. A developer who edits it after./sbpp.sh upwon't see the change until they rebuild (./sbpp.sh rebuild). There's no automated guard that catches a drift between the worktree'sdev-prepend.phpand the image's copy.Fix shape: either:
dev-prepend.phpinto the running container (so edits take effect on the next request — same model as the bind-mount onweb/), ORThe bind-mount path is cleaner; the comment-only path is zero-risk.
References: PR #1336 (the implementation PR for #1335), specifically the post-merge review comments. Original audit issue: #1335.