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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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