ENH Prefer X-Forwarded-Proto header

[emteknetnz]

Issue #11973

Will change the outgoing header to the standard Vary: X-Forwarded-Proto from the non-standard Vary: X-Forwarded-Protocol. We could change this behavior to simply not output either though since we could make the assumption that all sites use HTTPS and the outgoing header is not required.

For in-coming headers, will respect both Vary: X-Forwarded-Protocol and Vary: X-Forwarded-Proto to determine if HTTPS was being used though was terminated.

I've currently got this targeting CMS 5.4, though we may decide that we don't want to change this in a patch release and it may break existing setups that rely on the non-standard X-Forwarded-Protocol header, and instead directly people to a config based solution instead for existing versions, and target this PR to 6 instead.

[blueo]

I'd be pretty tempted to lean towards the simpler case of not including the X-Forwarded-Proto header in the Vary header by default. I think serving different content on the protocol is likely to be very rare nowdays and we should aim for the more common case. The Vary header is treated differently by different vendors (eg cloudflare suggesting Accept where Imperva wants Accept-Encoding only... so maybe making people set it when they know what they're targeting would be better?

[GuySartorelli]

We could change this behavior to simply not output either though since we could make the assumption that all sites use HTTPS and the outgoing header is not required.

That's not an assumption we make anywhere else in the codebase - we treat it as the default but there's several areas of code that change behaviour based on whether http or https is used (notably attributes for the session cookie does this). So we shouldn't make the assumption here either.

[GuySartorelli]

I'd be pretty tempted to lean towards the simpler case of not including the X-Forwarded-Proto header in the Vary header by default. I think serving different content on the protocol is likely to be very rare nowdays and we should aim for the more common case.

I agree with that in concept but we should probably only make that change in a minor release where we can warn people about the change in a changelog, in case they're relying on the header being in vary for.... some reason.

The rest is probably safe/appropriate for a patch - though there is some risk that someone's checking for or trying to remove the header in their own code e.g. in a middleware, and therefore making this change in a patch might be a little risky. I'm on the fence about that but probably would err towards targetting 6.

[GuySartorelli]

Why keep both here instead of just the modern version? I assume this is what you mean by "incoming headers" in the PR description but I didn't see the reason for keeping it in the description.

[emteknetnz]

Yeah we're not really going to get around the behavioural change / backward-compatibility issue, even if it's probably a bit of an edge case in practice.

Feels like solution for now is just update docs - the docs PR did highlight that the existing docs are wrong https://github.com/silverstripe/developer-docs/pull/889/files#r2875260552

We should do this work though target this at CMS 7 (which doesn't even have branches for it yet) - so maybe just put this card in backlog and put a CMS 7 milestone on it?

[GuySartorelli]

Yeah at the very least the docs should probably be updated.

I dunno that it's necessarily major release levels of bad, I'd be fine with it in a minor, but if you wanna hold off until we have 7 branches I'm fine with that too.

[emteknetnz]

I kind of don't see a strong need to do this right now since there's (soon to be) a documented way to sort out what headers are sent

@blueo do you have an opinion on this?

[blueo]

I kind of don't see a strong need to do this right now since there's (soon to be) a documented way to sort out what headers are sent

@blueo do you have an opinion on this?

yeah docs are good (and I've updated the cloud docs too) - I think there's a concern that when deploying on silverstripe cloud the defaults mean that imperva caching is ineffective (unsure about other cache providers). I get the backwards compatibility argument though as it would change behaviour. I'm leaning towards not breaking things and perhaps alerting on it in cloud instead

[emteknetnz]

OK will close this and go docs only

[blueo]

@emteknetnz would we still want the change in v7?

[emteknetnz]

Yeah but that's a long way off, we don't have CMS 7 branches yet so nothing to target. I've backlogged a card with the CMS 7.0 milestone on it - #11977

Note that I've created a new docs PR and closed the first one - the first PR has incorrect information for setting config so double check the cloud docs - silverstripe/developer-docs#892 is the new PR