Skip to content

fix: Force HTTPS actually stops the plaintext response - #876

Open
blaipr wants to merge 2 commits into
mainfrom
fix/force-https-actually-stops-the-plaintext-response
Open

fix: Force HTTPS actually stops the plaintext response#876
blaipr wants to merge 2 commits into
mainfrom
fix/force-https-actually-stops-the-plaintext-response

Conversation

@blaipr

@blaipr blaipr commented Aug 24, 2026

Copy link
Copy Markdown
Member

Http::checkHttps() sent a bare header('Location: …') — no status code, no exit, no
check that headers had already gone — and then returned. Both entry points called it and
carried straight on: Web\Init through the install and database checks to controller
dispatch, Api\Init likewise. The response was built and sent over the plaintext
connection the setting exists to refuse, and since a Location on a 200 is not a
redirect, no browser acted on the header either. Turning "Force HTTPS" on changed nothing
except adding an inert header.

The sibling refusals in the same method have always done it properly — not installed,
database unreachable, maintenance mode each redirect through the router and then throw.
That throw is the part that was missing, and it is why this now lives beside them:
Http::httpsUrlFor() answers with the address, and
HttpModuleBase::redirectToHttpsIfRequired() sends it and stops the request, in the base
both entry points already extend.

Also: the host was rewritten with str_replace('http', 'https', …), which replaces every
occurrence — an installation at http://httpbin.example was redirected to
https://httpsbin.example, a host that need not exist and need not be theirs. Only the
scheme is rewritten now.

The three existing tests asserted which mock methods had been called and nothing else,
which is exactly why this survived: a method that calls isHttpsEnabled(), isHttps(),
getServerPort() and getHttpHost() satisfies all three whether or not it does anything
useful with them. They assert the address now, and three more assert what the base does
with it — that a plaintext request is redirected and stopped, and that an HTTPS request
and an installation with the setting off are both left alone, so a base that refused
everything would not pass.

Writing that test found a fatal in the first version of this change: logger() is a
global function in namespace SP, and HttpModuleBase is in SP\Infrastructure, so the
bare call resolved to nothing and every redirect would have died on it. It is imported.

blaipr added 2 commits August 24, 2026 08:56
`Http::checkHttps()` sent a bare `header('Location: …')` — no status code, no exit, no
check that headers had already gone — and then returned. Both entry points called it and
carried straight on: `Web\Init` through the install and database checks to controller
dispatch, `Api\Init` likewise. The response was built and sent over the plaintext
connection the setting exists to refuse, and since a `Location` on a 200 is not a
redirect, no browser acted on the header either. Turning "Force HTTPS" on changed nothing
except adding an inert header.

The sibling refusals in the same method have always done it properly — not installed,
database unreachable, maintenance mode each redirect through the router and *then throw*.
That throw is the part that was missing, and it is why this now lives beside them:
`Http::httpsUrlFor()` answers with the address, and
`HttpModuleBase::redirectToHttpsIfRequired()` sends it and stops the request, in the base
both entry points already extend.

Also: the host was rewritten with `str_replace('http', 'https', …)`, which replaces every
occurrence — an installation at http://httpbin.example was redirected to
https://httpsbin.example, a host that need not exist and need not be theirs. Only the
scheme is rewritten now.

The three existing tests asserted which mock methods had been called and nothing else,
which is exactly why this survived: a method that calls isHttpsEnabled(), isHttps(),
getServerPort() and getHttpHost() satisfies all three whether or not it does anything
useful with them. They assert the address now, and three more assert what the base does
with it — that a plaintext request is redirected *and* stopped, and that an HTTPS request
and an installation with the setting off are both left alone, so a base that refused
everything would not pass.

Writing that test found a fatal in the first version of this change: `logger()` is a
global function in namespace `SP`, and `HttpModuleBase` is in `SP\Infrastructure`, so the
bare call resolved to nothing and every redirect would have died on it. It is imported.
…t in its tests too

The integration suite caught the consequence of making the redirect actually stop the
request: ConfigSecurityTest starts from an installation that already has "Force HTTPS" on
and then dispatches over plain HTTP, which is exactly the case Init now refuses — so the
controller never ran and nothing was saved.

The test's premise is what changed, not its assertion. An installation requiring HTTPS is
reached over HTTPS, so the request says so now.

`buildRequest()` grows an optional server array for it, merged last so a test can state
something the defaults do not. Only this one test needed it; nothing else in the suite
turns the setting on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant