-
-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
RFC standard Forwarded:
header should be supported
#20
Comments
Hi @HartS thanks for opening this issue. You are correct and I wanted to just use that from the beginning, of course the issue being that it seems like all the cloud provides, cdns, etc. only seem to provide as x-forwarded-for and/or their own proprietary header. When these providers don't align on adding to forwarded, it makes it difficult to determine in which order the various headers should be read in, and a security issue when they don't add or strip forwarded header (since then the ultimate client can set it to anything). I haven't looked in a while so I wonder if the state of this has improved at all. |
As for this, I think there is some confusion. The express-session middleware only relies on the value of req.protocol , which the "trust proxy" setting can be used to derrive this from the x-forwarded-proto header (which this module does not read or anything), but users can always define that to use whatever mechanism works for their environment. There is no restriction that requires the use of this module's logic. |
That's more or less what I had assumed, and I haven't looked to deeply into how express and express-sessions handle this, other than seeing an express doc note about using proxy-addr for So perhaps there's an undocumented way to configure how express-session determines when to trust a proxy also, or an alternative way to do this in express already made available via some middleware. I did a cursory search on npm and didn't find an alternative however, so I suspect it would be roll-your-own, which is possibly less secure and more bug-prone than having support in the package express already uses for this, where theoretically it would get more attention from the open-source community :P |
Ah, gotcha. I will see where it is documented. Funny enough you touched on why this module does not currently look at forwarded header (even though I would like it to): it is currently insecure to do so due to how providers are settings up these headers and their configurations. We actually backed out that implementation years ago and a security researcher brought it up and we had to remove it to no longer be flagged as having a vulnerability in the module. |
Hmm.. that makes sense. Perhaps the 'safe' thing to do would be to only look at the |
Hi @chriswilty sorry there is a school opening a bunch of spam prs at the moment so it it temp. If there is a vulnerability you would like to report, you can find the security policy here: https://expressjs.com/en/resources/contributing.html#security-policies-and-procedures (I deleted your comment since you said a few things and seemed to indicate that you think it is a vulnerability so just trying to keep details private while we try to understand by you contacting us first). |
Since 2014, RFC7239 has standardized the
Forwarded:
header which allows chaining of multiple forwards in a more extensible way.An example of this header being set when the origin request passes through only one reverse proxy might look like
As far as I can tell (from
proxy-addr
docs, and also attempting to useexpress-session
with this header andapp.set('trust proxy', 'loopback')
, this is being ignored byproxy-addr
and therefore causing theForwarded:
header to be unsupported by upstream middleware such asexpress-session
which relies on things being set in the request byproxy-addr
.The text was updated successfully, but these errors were encountered: