Use default concrete.security.trusted_proxies.ips configuration key?

[mlocati]

Since version 8.0.0, concrete5 calls Request::setTrustedProxies when it boots, reading the concrete.security.trusted_proxies.ips configuration key.

Since this cloudflare_proxy package requires concrete5 8.2.0+, is there any reason to save the trusted IPs in another configuration key and call Request::setTrustedProxies another time?

[aembler]

No, there is not. I imagine this package can be simplified

…

On Fri, Jun 1, 2018 at 3:31 AM, Michele Locati ***@***.***> wrote: Since version 8.0.0 <https://github.com/concrete5/concrete5/blob/8.0/concrete/src/Foundation/Runtime/Boot/DefaultBooter.php#L358>, concrete5 calls Request::setTrustedProxies when it boots, reading the concrete.security.trusted_proxies.ips configuration key. Since this cloudflare_proxy package requires concrete5 8.2.0+, is there any reason to save the trusted IPs in another configuration key and call Request::setTrustedProxies another time? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgNwWKiWl5mRpHlfiMizMswX6UotG6iks5t4RgRgaJpZM4UWeC7> .

[KorvinSzanto]

The reason we don't overwrite core config is that we need to be able to uninstall this package cleanly. With overwriting core config, we leave the site in a "broken" state on uninstall.

[KorvinSzanto]

Whoops didn't mean to close.

We can either use $config->set() to overwrite config at runtime, or we can use a middleware to manage the symfony trusted proxies directly which is what I would prefer.

[mlocati]

People can always change the trusted IPs, and with concretecms/concretecms#6703 there'll be an UI too

[KorvinSzanto]

People can always change the trusted IPs, and with concretecms/concretecms#6703 there'll be an UI too

Okay, so they can visually diff the IPs if they know which ones are cloudflares and if they understand what IPs are and all that.

I don't see why you'd choose to do that rather than just merging in the configuration at runtime.

[mlocati]

I don't see why you'd choose to do that rather than just merging in the configuration at runtime.

Let's assume that concretecms/concretecms#6703 is merged (or that another 3rd party package implements something like that to visually edit the trusted IPs): in this case, we'd call $config->save(concrete.security.trusted_proxies.ips, $ips).
If we patch this settings within the package on_start, by calling something like

$config->set(
    `concrete.security.trusted_proxies.ips`,
    array_merge(
        $config->get(`concrete.security.trusted_proxies.ips`),
        $cloudflareIPs
   )
);

we'd have a duplicated list of cloudflare IPs.

Furthermore, adding more startup logic just because uninstalling the package would leave the setting untouched seems too overcomplicated to me...