does phpClientHttpsFix or the OVERWRITEPROTOCOL env variable actually do anything?

[bellis-ai]

I see a lot of issues trying to fix CSP issues by using the phpClientHttpsFix.enabled = true variable (which sets up the OVERWRITEPROTOCOL env variable on the pod)

However, a quick search of the actual nextcloud repo shows that this environment variable is referenced nowhere. Personal testing with this variable led to nothing happening at all...

On the converse, it appears that it's necessary to set the 'overwriteprotocol' => 'https' config variable if you want the desired behavior.

<?php
$CONFIG = array (
  'overwriteprotocol' => 'https'
);

[n2qz]

The environment variable is used by the Nextcloud docker containers

[jessebot]

It looks like we do add in these env vars according to our _helpers.tpl: https://github.com/nextcloud/helm/blob/a491977458b5111cc0afd06386aa63ef46720f4c/charts/nextcloud/templates/_helpers.tpl#L68-L72

This has however been updated to move to the _helpers.tpl from deployment.yaml in the past 4 months: https://github.com/nextcloud/helm/blame/f2d273101171d98177cdfd3220660365d2f03d7d/charts/nextcloud/templates/_helpers.tpl

@bellis-ai, is this still affecting you with the latest version of the helm chart? As @n2qz pointed out, it does still seem to be in use for the docker container, so a good test to start with would be seeing if the issue is still present when using the ENV var directly when running the docker container. If it's still broken, we should open a ticket in that repo to take a further look there. I can try and test this out on my own infra when I have some time, but I am working on these issues in my off hours, so there may be some delay. Thanks for all your patience through this 🙏

[jessebot]

When we merge #464, it also includes a new config file from upstream that makes setting these env vars and ensuring they get added to a config.php file a little easier: https://github.com/nextcloud/helm/pull/464/files#diff-3fc01dc1e82863fedf0ba6b6d5dd1e21b32b8fb0813d97916de95536bd4a5a73R1-R30

After that's merged, @provokateurin, should we create a section in the values.yaml like 🤔 :

nextcloud:
  reverseProxy:
    overwriteHost: ""
    overwriteProtocol: ""
    overwriteCliUrl: ""
    overwriteWebRoot: ""
    overwriteCondAddr: ""
    trustedProxies: ""

And we'd also include more info from the reverse proxy overwrite parameters docs too?

Just so that it's clear how you update these values? I personally feel like phpClientHttpsFix is confusing 🤷 I know it would be a breaking change, but right now, I feel like it's so unclear and our config table here just says:

Parameter	Description	Default
phpClientHttpsFix.enabled	Sets OVERWRITEPROTOCOL for https ingress redirect	false
phpClientHttpsFix.protocol	Sets OVERWRITEPROTOCOL for https ingress redirect	https

[provokateurin]

Adding such a seciont sounds good to me :+1: