Skip to content

Follow-ups from #1335: install-wizard P3 items #1338

Description

@rumblefrog

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:

  1. If config.php missing → header('Location: install/'); exit;
  2. 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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions