Skip to content

Default TrustedHeaderNames are not standard... are they? #17641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
xDaizu opened this issue Feb 1, 2016 · 13 comments
Closed

Default TrustedHeaderNames are not standard... are they? #17641

xDaizu opened this issue Feb 1, 2016 · 13 comments
Labels
Enhancement HttpFoundation

Comments

@xDaizu
Copy link

xDaizu commented Feb 1, 2016

This directly references SymfonyDocs #6197, where I originally posted it by mistake.

In this section, the documentation states:

By default, the following proxy headers are trusted:

X-Forwarded-For Used in getClientIp();
X-Forwarded-Host Used in getHost();
X-Forwarded-Port Used in getPort();
X-Forwarded-Proto Used in getScheme() and isSecure();

I want to make clear that I'm not a proxy expert, but as I understand it, those headers are not standard. They're common (pseudo-standard) but not standard since the RFC 7239 was released in 2014

My points are:

  1. Does it support the standard Forwarded header by default and all its attributes? This is an actual question, as I can't try it right now. If not, it should. Because standards. Standards are cool.
  2. In the same line, I put up to debate if it should support the pseudo-standard headers by default. Is it useful? Yes, it is; but it can be argued that it fights against the extension of the standard, which I think is a burden mid-long term, because people (especially devs, especially me) are lazy and standards are cool and dandy but the path of least resistance is even cooler; and when the standard is not the path of least resistence for the majority, the very concept of standard becomes useless... and that makes me sad 😢

TL;DR; You're not gonna read it? Then you're either a _a)_lazy or a _b)_busy person. Case a)C'mon, I made it fun and easy to read. Case b) I will probably waste your time. This is not the urgent issue you're looking for
@xabbuh xabbuh mentioned this issue Feb 1, 2016
Closed
@stof
Copy link
Member

stof commented Feb 1, 2016

Forwarded is part of the default supported headers:

symfony/src/Symfony/Component/HttpFoundation/Request.php

Line 72 in 83b53f4

protected static $trustedHeaders = array(

Regarding the very common headers from the pre-standard time, they are still used by many systems out there AFAIK. So the goal is to have Symfony working by default in such environments.

@stof
Copy link
Member

stof commented Feb 1, 2016

The doc should be updated to mention the standard header though.

@stof
Copy link
Member

stof commented Feb 1, 2016

hmm, Forwarded seems supported only for the client IPs currently AFAICT. It is not supported for the proto and host. So this qualifies as partial support only

@arjenm
Copy link
Contributor

arjenm commented Apr 4, 2016

I just ran into this myself. It does read the ip from the Forwarded-header, but uses X-Forwarded-Proto to check whether its a secure-request or not in the Request::isSecure() method.

I.e. only the 'for'-section of the Forwarded-header seems to be supported, not the 'proto', 'host' or 'by'.

That partial support was added in #11073

@mattattui
Copy link
Contributor

I just ran headfirst into the partial support for the Forwarded header and spent the last hour or so trying to figure out why my proxy was trusted but generated URLs were ignoring the host.

So… a big 👍 for documentation. And a small plea to add proper support for RFC7239.

@arjenm
Copy link
Contributor

arjenm commented Jun 30, 2016 •
edited
Loading

And apparantly, mixing Forwarded with X-Forwarded-For is now considered an error if they're not exactly the same, but there is still no support to be allowed to only use the Forwarded-header :(

That does add to the confusion, here's that other change #18688

Ideally you'd only provide the standard version: Forwarded: for=U.X.Y.Z, proto=https
But that won't work, you'd have to turn it into: Forwarded: for=U.X.Y.Z, proto=https (or you can remove the https) and a X-Forwarded-Proto: https

An alternative is to fall back to the old non-standard stuff; X-Forwarded-For: U.X.Y.Z

@xDaizu
Copy link
Author

xDaizu commented Jul 22, 2016

@arjenm

Ideally you'd only provide the standard version: Forwarded: for=U.X.Y.Z, proto=https
But that won't work, you'd have to turn it into: Forwarded: for=U.X.Y.Z, proto=https

I'm confused, I can't see the difference between those two...

@arjenm
Copy link
Contributor

arjenm commented Jul 22, 2016

The 'and' is not optional in that second sentence

@nicolas-grekas
Copy link
Member

Closing as we deal with "Forwarded" and with "X-Forwarded-*".

@txwizard
Copy link

In my copy of Symfony 3.3, I see the following comment, "@deprecated since version 3.3, to be removed in 4.0," which begs the question. If the $trustedHeaders array is going away, what mechanism replaces it?

@xabbuh
Copy link
Member

xabbuh commented Jan 30, 2018

@txwizard you may want to read https://symfony.com/blog/fixing-the-trusted-proxies-configuration-for-symfony-3-3

@nicolas-grekas
Copy link
Member

You now need to provide the trusted headers as a bitfield, see eg
https://github.com/symfony/recipes/blob/039d2d6dd0790749f2748d952584d7dbe39c2d4d/symfony/framework-bundle/3.3/public/index.php#L28

@txwizard
Copy link

txwizard commented Feb 1, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@javiereguiluz @mattattui @nicolas-grekas @stof @xabbuh @arjenm @xDaizu @txwizard