Director::forceSSL does not force SSL when using

[wilr]

Might just be something to note in the .env documentation but this is something I caught in production today:

Defined a base URL in .env

SS_BASE_URL="https://www.site.co.nz/"

In app/_config.php: Director::forceSSL();.

Result:

Accessing http://www.site.co.nz incorrectly works, no redirection takes place as the check in Director::is_https() fails with the provided SS_BASE_URL.

Fix is to use the non-ssl version in the .env file but I think that's a simple issue for users to trip up on and could result in users being able to access pages over HTTP.

PR:

• API / BUG - Introduce new request resolver middleware and fix broken forceWWW / forceSSL #7520
• BUG Safely check for is_site_url before parsing a shortcode silverstripe-cms#2007

[dhensby]

My view is we stop recommending using forceSSL - instead use a middleware to force SSL or serverside config.

However, the problem is Director::is_https() check needs changing/ignoring in the forceSSL method as it's looking up whether the base URL is https rather than the current request.

[dhensby]

OK - so I've looked into this - calling forceSSL() from the _config.php is too early in the bootstrap process because the current HTTPRequest object hasn't been built yet (or isn't available for some reason).

This means that you can't make changes based on request state at this point in SS4.

So the solution is either create an SSL middleware or add this to a controller init method.

See #7215 (comment) for some more info

[dhensby]

both forceSSL and forceWWW should be moved to middlewares or just fully deprecated in favour of server side configuration IMO

[dhensby]

I'm going to close this as "not a bug" if we have docs that recommend putting the forceWWW or forceSSL in the _config.php files then we need to amend those.

[wilr]

Well my 2c should be we either remove the function because it doesn't do what it says on the tin, or b we keep the function, detect that HTTPRequest hasn't been built and throw an exception as currently it's a change from 3.6. Forum is littered with use of force_ssl. I'm happy with either approach but we may not be able to remove for 4.0

[dhensby]

@silverstripe/core-team thoughts please?

[tractorcow]

I am in favour of throwing exception to prevent silent failures.

[sminnee]

Not keen to break APIs just now.

Would recommend that Director::forceSSL() is refactored to set the config value of a relevant middleware. Either it can auto-add the middleware upon calling as well (if missing), or we can include the relevant middleware by default. It can throw a deprecation warning if we like.

Then the middleware can do the hard work, as Dan suggests.

I'd probably design the middleware to be a little more generic than just forceSSL: cover force ssl, add www, canonicalise domain name.

[tractorcow]

Maybe not setting a config value (setting config outside of YML isn't recommended anymore) but yes some flag that triggers the later middleware would be great :)

That way forceSSL() in _config.php would work normally.

[sminnee]

Setting config isn't recommended, but neither is Director::forceSSL(), so I think that's okay ;-)

[tractorcow]

Yes we can deprecate it with a new middleware at the same time. :)

[dhensby]

OK - are we happy to add new middleware now we are in RC or will this change have to wait?

[tractorcow]

I think 4.1 is fine.

[wilr]

IMHO it should be 4.0 or at least marked up in bold in the changelog as something to replace as if Director::forceSSL no longer works I can name at least a dozen govt agencies where we're used that rather than in .htaccess since they use different domains.

[sminnee]

Marked as a high bug because SSL matters. Note that fixing this bug will probably involve creating a new configuration point / middleware and deprecating this one, but that doesn't stop is being a bug.

[sminnee]

@wilr

I can name at least a dozen govt agencies where we're used that rather than in .htaccess since they use different domains.

Do you mean that they have multiple certificates / domain names that their site operates under, or is there a single canonical HTTPS domain that everyone should be redirected to? Just wondering to help flesh out how we structure the undeprecated API.

[tractorcow]

Ok, let's fix in rc2?

[stojg]

From an operational and deployment perspective I would like to mention that any redirection logic should not be triggered in CLI environments. This has in the past been surfaced bugs where for example it's been triggered on dev/builds or other tasks with the result of a sudden exit without any output.

[tractorcow]

I believe in CLI the expected behaviour is that it would immediately process the redirected URL. It's not consistently implemented though. We may want to adjust our expectations here. :)

[stojg]

Yeah, absolutely, just want to make sure we all realise there are 2 supported environments for SilverStripe :)

[stojg]

SSL redirection is a specialised case of redirection, other cases are redirecting to a specific domain or subdomain (domain.com -> www.domain.com, ie domain canonicalization).

If you are running several websites on a server you would then need to set up multiple virtual hosts with a list of listening domains.

If then SilverStripe does the redirection, you would need to synchronise the domains and redirections between the SilverStripe application and the web server / forward proxies.

The above might be a bit more complicated case, but I think it highlights that it's easy to cause unexpected bugs in production mode that cannot easily be reproduced in dev / UAT environments.

For simple setups, where the web server just points to a document root for all incoming connections, it's easy enough for SilverStripe to take care of all redirection/domain checking.

I don't have a good answer on where the responsibilities of a web server end and SilverStripes begin.

[tractorcow]

I would propose expected behaviour for forceSSL() in CLI is that the request was processed where is_https() would return true. It could be as simple as modifying the protocol of the current request object, or otherwise it could re-start a request with this flag set.

This would be necessary for code paths which generate links based on the "current url", which should then have the necessary https:// prefix.

[sminnee]

Doing #7509 in 4.1 would make CLI behaviour of forceSSL less important :-)

[tractorcow]

Fix at #7520

For #7509 we can just never run that middleware (it'd be disabled).

[sminnee]

Added to #7509 "Note that the implication of this is that no HTTPMiddlewares would ever get run in CLI mode, which simplifies things such as #7492."

[jmfederico]

I just noticed that when running "sake dev/build" from the command line, the base_path will always be http, even though I have https in SS_BASE_URL.

This is a problem when generating static files (like error-404.html).

I read this issue and #7509 but I am not sure if this has been solved, or if there is a solution I can apply to my project (apart from running dev/build from a browser instead.)

I am guessing this is the same issue, but I am not sure.

Is this something you knew already?

[robbieaverill]

@jmfederico we fixed this recently with #8028, it'll be released soon

[jmfederico]

@robbieaverill thanks, missed that PR.