PR is ready.

(rebased)

Thank you @nicolas-grekas.

Sorry for arriving this late to this pull request, but is this somehow related to the security issue in 3.3? I'm talking about https://symfony.com/blog/fixing-the-trusted-proxies-configuration-for-symfony-3-3 where the same parameter was removed from the configurations and now is being added back?

This PR builds on the knowledge gathered in the blog post and the related issues you linked. It doesn't reintroduce the original issue.

1. Trusted headers are configurable now, and have a safe default value (not trusting x-forwarded-host)
2. No proxies are trusted by default,
3. Runtime configuration happens very early when handling the request, earlier than HttpCache when it's enabled.
   Did you spot an issue?

• from the DX point of view this change is much better, it removes a lot of "mysterious" code from the index.php file
• I was expecting some reference to the initial security issue since the feature was already there, and was confused in seeing it back
• from the implementation point of view it introduces even more coupling with the request object (in this case calling methods on it father than just blindly forwarding it to the handler).
  a better way could be having an event in the preBoot (but i guess the event dispatcher is not available yet), or a dedicated interface that can be used to hook into the boot process, and having a dedicated class providing the feature of "configuring some global state". in this way other people get a way to hook by them self into the process..
  but I guess that might be out of scope for this change

You're correct. Providing extensibility here is a separate task, that'd need a use case and someone to implement it. OSS dynamics: future will tell if/why and how we should do it.

I'm perfectly aware of how OSS works, it is just that symfony was often looking a bit more in the future and having static method calls here and there rarely has been a good idea.

Edit: I'm aware of how OSS works, do not need reminders, and in my last comment I did not ask you to change anything in the implementation.

May I ask why the default is (and previously was, elsewhere) not to trust X-Forwarded-Host?

I understand that trusting this header has security implications, but if I am not misreading the change made here the prerequisite for evaluating the header is to configure a trusted proxy in the first place? And once you do that (because you have a proxy in place), wouldn't you usually want to also trust X-Forwarded-Host?

Also, on https://symfony.com/doc/master/deployment/proxies.html, the example configuration shown configures the proxy's IP address and uses Request::HEADER_X_FORWARDED_ALL. It does not even mention not trusting X-Forwarded-Host.

@mpdude sure, check #20178

Thank you Nicolas!

Documentation has been re-arranged a bit since #20178, so I hope I did not miss anything that was more visible back then; also I followed the related doc PRs and hope I understood it all correctly.

We all 💯 that we must not trust any proxy or header by default.

David's intention back in #20178 was to make trusting proxies more safe by default by not having any default headers that would be trusted once you configure proxy IPs. He was referring to AWS ELBs, that (to my knowledge, up to the present day) do not set X-Forwarded-Host. Not having any defaults should force people to think through their setup and choose the right set of headers.

Fabien suggested not to have defaults optimized for the AWS case.

I think the current documentation is mostly in line with this: It describes to either provide the IP and use Request::HEADER_X_FORWARDED_ALL; or, for the AWS ELB case, to configure network security appropriately and then use Request::HEADER_X_FORWARDED_AWS_ELB in combination with trusting all IPs.

The default configuration provided in the FrameworkBundle (with this PR) or Flex/recipes in 5.1 however is a bit odd, as it leans towards the AWS use case (Request::HEADER_X_FORWARDED_ALL ^ Request::HEADER_X_FORWARDED_HOST == Request::HEADER_X_FORWARDED_AWS_ELB).

So, for setups with an Apache-based Reverse Proxy (my current use case) you need to explicitly configure the IP + explicitly configure x-forwarded-all/Request::HEADER_X_FORWARDED_ALL. Probably the same for Varnish.

For AWS ELB, you need network-level security config outside the scope of Symfony + trust all IPs.

Bottom line:

It seems to me the defaults

• are not right for the Apache/Varnish case
• the AWS ELB case requires careful extra steps anyway
• are not "more safe" than any other values (what about X-Forwarded-For, for example?)
• do not follow David's suggestion (to my understanding).

Maybe BC prevents us from changing this anyway, but what about going without trusted headers or using x-forwarded-all?

Side note: we often get security reports from ppl that missed that trusted_hosts exists.
But ppl are lazy (I put myself in the bag). Not trusting any headers by default won't mean they'll think about all this. Instead, they'll copy/paste some blogpost/article. We all do this.
This means the solution is more in the documentation than in the code.
For the code part, this PR doesn't aim to change the preexisting default, precisely because it was not the topic.
If you want to lead an improvement on this, please open a separate issue (or better: PR).