Fixes wrong IP address when using a reverse proxy

[x7airworker]

Changes proposed in this pull request:
Two new optional config values:

• reverse_proxy.enabled will enable ip address finding of a user using a reverse proxy at \Http\Middleware\ProxyAddress
• reverse_proxy.allowed controls the allowed reverse proxy IPs used for checking.

Changed Arr::get($request->getServerParams(), 'REMOTE_ADDR', '127.0.0.1'); to $request->getAttribute('ipAddress'); to use the new attribute in the codebase.

Reviewers should focus on:

• The Middleware is added in \Flarum\Foundation\InstalledApp and im unsure if this is the correct/best position.

Confirmed

• Backend changes: 2 failures (WritablePathsTest which is not related to the changes I made).

Required changes:

• Related documentation PR: TODO (Would be good to document the optional config values.)

[tankerkiller125]

Changes look good to me, now we wait for at least one more core dev to approve before we merge.

[askvortsov1]

Overall looks good, left a few comments. Also, I'm not seeing any failing test cases (re: PR description), so we're good on that front :D

[x7airworker]

Overall looks good, left a few comments. Also, I'm not seeing any failing test cases (re: PR description), so we're good on that front :D

I think the failed test cases had to do with my environment as I've written in the description.

[franzliedke]

I am afraid this is not ready for production yet:

1. The X-Forwarded-For header can contain multiple IP addresses, e.g. in setups with multiple reverse proxies / load balancers.
2. If I am not mistaken, the headers need to be explicitly marked as trusted. Right now, if I read the code correctly, a malicious user could provide an IP address in X-ProxyUser-IP and we would incorrectly use this to overwrite the value from the server in X-Forwarded-For.
3. This needs tests!

Beyond that, some code-specific feedback:

1. I personally, would have been fine with adding this at the highest level of middlewares, i.e. in InstalledApp.
2. If we don't add any special error handling (e.g. a message or a status code), there is no need to add KnownError to the new exception class.

It might be worth using a library for this, if we find a good one for PSR-7. Otherwise, a port of Symfony's "trusted proxies" concept may be an option.

@askvortsov1 @tankerkiller125 Can I ask you to follow up on this, either with a revert, an issue for beta.14 or a PR that fixes these problems? A combination of these may be helpful, too. 😜

[tankerkiller125]

@franzliedke the problem I had with putting the middleware at the highest level is that the extender for middleware does not have the ability to overwrite middleware in IntalledApp I don't think.

[franzliedke]

I don't see a big need for extensibility here, personally. It's security-relevant and the middleware can be disabled by changing the config.

But honestly, that is the least of my concerns, just an implementation detail.

[askvortsov1]

Considering our backlog for beta 14, it might be best to revert it, and encourage @x7airworker to open a new PR that takes into account Franz's comments above. I don't want to make this something that might hold back the release.

[tankerkiller125]

I agree @askvortsov1 if you want to rollback I think that's a good decision at the moment. I would but I'm at work :(