Improve documentation for (unused) trusted proxy headers

Is there an opportunity to add additional test coverage for instances where these $settings are set to ''?

The documentation changes LGTM, though.

Thoughts on this issue/patch:

a) Make a follow-up issue now to add tests as in #9/12, and add it to the issue summary, so that the question doesn't come up again.

b) Grammar nitpick:

 * A selection of different reverse proxy headers are supported (see below), but

A selection ... is supported

This of course sounds weird, so maybe better as:

Various reverse proxy headers are supported...

c) E.g. nitpick:

+ * corresponding setting to an empty value, e.g. ''.

E.g. means "for example", which I think is not what you mean (maybe it is?). Anyway, best to be specific. Why not just say

corresponding setting to ''.

which is shorter, and gets the point across, without using "e.g.", which is often misunderstood and confused with "i.e.", and is also punctuated incorrectly in your text. We actually have a docs writing style somewhere that suggested never using "e.g." and "i.e." in docs, because of them being so commonly confused with each other.

d) Item (c) applies in a bunch of places.

e) Other than those nitpicks, this looks very clear. Thanks for the issue and patch!

These changes still LGTM.

If you're going to use either "for example" or "e.g.", we should get the punctuation correct. It should be:

To do this, set the corresponding setting to an empty value; for example, ''.

But anyway... is it really checked using the PHP empty() function? Or just evaluated as a Boolean? Or is it checked to see if it is == ''? Or ??? Can you guarantee that -- how is it documented and where? Because if you say "setting to an empty value", that implies to me that it is really being checked with empty(), but I'm not sure it really is.

I really think you should just say to set it to '' and leave it at that, since you know that works, and why, really, would you want to provide alternatives?

As another note...
https://github.com/symfony/http-foundation/blob/3.4/Request.php#L671
shows the @param for the $key and $value are both string variables. So ... yes, the text in the doc block does say "setting to an empty value", but the fact that the @params are typed as string implies they should be strings, hence '' is really the way to say "don't use this".

There's another issue about the wording of the reverse_proxy_headers: https://www.drupal.org/project/drupal/issues/2673572

The docs look good here to me! Mostly... Two questions/thoughts:

a)

/**
  * Set this value if your proxy server sends the client IP in a header
- * other than X-Forwarded-For.
+ * other than X-Forwarded-For. If you are using a proxy server but not using
+ * this header, set this to ''.
  */
 # $settings['reverse_proxy_header'] = 'X_CLUSTER_CLIENT_IP';

Is the default here actually x-forwarded-for? Because in other settings, such as

/**
  * Set this value if your proxy server sends the client protocol in a header
- * other than X-Forwarded-Proto.
+ * other than X-Forwarded-Proto. If you are using a proxy server but not using
+ * this header, set this to ''.
  */
 # $settings['reverse_proxy_proto_header'] = 'X_FORWARDED_PROTO';

it looks to me as though the default setting is put into the commented-out code line. But in the reverse_proxy_header line, if the docs are correct, the default isn't the setting in the code line.

I think we should be consistent, and always put the defaults into the code line. So, in this first doc block, either the docs are wrong or the code line is wrong (I have no idea which it would be -- not my area of expertise).

b) We do have a documentation standard that says all 1st-line docs in /** */ doc blocks should be one sentence of less than 80 characters, followed by a blank line, and optionally some more documentation. None of the items in this patch qualify. I realize the docs in this file didn't follow the standard before the patch either, but it is normal if patching an area of docs to also make it comply with the docs standards.

Don' forget to fix descriptions, they are still copy/past typos in your last patch ("sends the client protocol in a header"), see #2673572 for more detailed descriptions.

This looks very good to me! I think you can see from the patch why our docs standards ask for one < 80 character line to start each documentation header -- it really makes the documentation easier to scan and understand.

That seems to have been a random test failure. Created issue #3031842: Random fail in OffCanvasTest and will submit for retest.

Same random test fail again. :(

It should be add that reverse_proxy_addresses allow subnet notation and IPV6 address.
@see
\Symfony\Component\HttpFoundation\Request::isFromTrustedProxy => \Symfony\Component\HttpFoundation\IpUtils::checkIp/checkIp4/checkIp6 : "IPv4 address or subnet in CIDR notation"

Suggestion:

 /**
+ * Reverse Proxy Addresses.
+ *
  * Specify every reverse proxy IP address in your environment.
+ * String or array of IPV4/6 address or subnet in CIDR notation.
  * This setting is required if $settings['reverse_proxy'] is TRUE.
  */
 # $settings['reverse_proxy_addresses'] = ['a.b.c.d', ...];

Adding this information might be a good idea, but I am not sure... What is CIDR? I am not familiar with that acronym. Is it different from the usual IP address notation?

If CIDR is just the normal IP address notation, then I think this added line is probably not needed. The line above says you need to put in every reverse proxy IP address in the environment, and the example setting line shows that it is an array of strings in 'a.b.c.d' notation... ???

@jhodgdon: The array in the example is confusing, it could induce that only a list of IP is a valid input, i.e. : no subnet notation.
Another source of confusion is that Drupal 7 does not allow such subnet notation.

As for CIDR, it's taken from the symfony comment, plus, at this point of the settings the user should have the needed knowledge.

@mcdruid : Done: #3032746

I'm going to postpone this on #3030501: [Symfony 4] Drupal\Core\StackMiddleware\ReverseProxyMiddleware calls Symfony\Component\HttpFoundation\Request::setTrustedHeaderName() which does not exist as that patch fixes the implementation to accord for Symfony's deprecations and changes this significantly.

Added a closely related issue. Furthermore, I guess the documentation should point out that the default checks for HTTP_X variables. The current doc at least didn't make that clear to me.

I checked the $_SERVER variables which showed me "HTTP_X_FORWARDED_FOR" as key for example.

So I tried to override the defaults by:

$settings['reverse_proxy_header'] = 'HTTP_X_FORWARDED_FOR';
$settings['reverse_proxy_proto_header'] = 'HTTP_X_FORWARDED_PROTO';
$settings['reverse_proxy_host_header'] = 'HTTP_X_FORWARDED_HOST';
$settings['reverse_proxy_port_header'] = 'HTTP_X_FORWARDED_PORT';

which was wrong, instead I should have used the defaults, but the documentation says:

/**
* Set this value if your proxy server sends the client IP in a header
* other than X-Forwarded-For.
*/

So finally I'd suggest to explicitely add the key name to the documentation?:

/**
* Set this value if your proxy server sends the client IP in a header
* other than X-Forwarded-For ("HTTP_X_FORWARDED_FOR").
*/

Looks like the ticket this was postponed on has landed. Setting back to needs review in case things changed since.

It would be good to show the same info for reverse_proxy_addresses as Symfony have in their docs: https://symfony.com/doc/current/deployment/proxies.html#solution-settrus...

Maybe an option would be document it as

 # $settings['reverse_proxy_addresses'] = ['a.b.c.d', 'e.f.g.h/24', ...];

@jcnventura There's a dedicated issue for that at #3032746: Improve documentation for reverse proxy addresses setting - I added your suggestion there as a patch

I don't see anything in this patch that is still relevant now that $settings['reverse_proxy_trusted_headers'] is used, so closing - please reopen if I missed something