Add secure cookies when remote is localhost #858
Conversation
|
Still needs to update tests, marked as draft. |
index.js
Outdated
| if (req.connection.remoteAddress === '127.0.0.1' || | ||
| req.connection.remoteAddress === '::ffff:127.0.0.1' | ||
| ) { |
There was a problem hiding this comment.
Won't this condition be true for most reverse proxy setups? Or does Express mutate req.connection.remoteAddress if trust proxy is enabled? (The documentation is not clear.)
There was a problem hiding this comment.
You are correct. I don't think this will work correctly as any reverse proxy on localhost (think typicall ssl ofloading or even just kubernetes side cars) would end up as thinking this is a localhost connection. Express does not alter any values that are provided from Node.js HTTP core.
There was a problem hiding this comment.
Yeah I see, I think the reasonable way to do this is to check if we are behind a proxy
index.js
Outdated
|
|
||
| // socket is localhost | ||
| if (req.connection.remoteAddress === '127.0.0.1' || | ||
| req.connection.remoteAddress === '::ffff:127.0.0.1' |
There was a problem hiding this comment.
What about IPv6 localhost?
| req.connection.remoteAddress === '::ffff:127.0.0.1' | |
| req.connection.remoteAddress === '::ffff:127.0.0.1' || | |
| req.connection.remoteAddress === '::1' |
There was a problem hiding this comment.
Oh, I forgot about that you are right!
README.md
Outdated
| have your node.js behind a proxy and are using `secure: true`, you need to set | ||
| "trust proxy" in express: | ||
| an https-enabled website (or connection via localhost), i.e., HTTPS is necessary | ||
| for secure cookies. If `secure` is set, and you access your site over HTTP, the |
There was a problem hiding this comment.
HTTPS is necessary
for secure cookies
Probably this line would need reworded with a change like this, as I believe that is the whole point of this change, that HTTP would work for localhost.
There was a problem hiding this comment.
Take a look at 8314c37, I think it is clear that way
| } | ||
|
|
||
| // proxy not explicitly trusted; no proxy means connection is secure | ||
| if (req.headers['x-forwarded-proto'] !== undefined) { |
There was a problem hiding this comment.
Unfortunately this is also not a reliable test, because if the user does not trust the connection as a proxy, this header may not be set if not a proxy or may be forgable by the client.
There was a problem hiding this comment.
Sorry I am not sure what you mean, maybe I am missing some possible network configuration?
Since we detect a localhost connection we could trust the remote (reverse proxy or client) right?
Please correct me if I am wrong
There was a problem hiding this comment.
Not unless the user of this module specifies they trust it. This fixed a reported security vulnerability.
There was a problem hiding this comment.
Oh I see what you mean, so we can conclude that the only thing we can do is return true if the proxy is trusted and the connection is localhost, right?
|
|
||
| - https-enabled website | ||
| - localhost connection | ||
| - node.js behind a proxy and "trust proxy" in express |
There was a problem hiding this comment.
It wouldn't just be behind trust proxy, as you can still have a http connection though a proxy.
There was a problem hiding this comment.
Yeah my bad, does node.js behind an HTTPS proxy and "trust proxy" in express sound good?
I also need to update the localhost one, specifying that "trust proxy" is also needed
|
This has been stale for a while...
So, are you ok with this one? Otherwise I would have no other idea and we can close this.. |
dougwilson
force-pushed
the
master
branch
2 times, most recently
from
9d2e29b to
408229e
Compare
See #857
This PR enables secure cookies to be transmitted when the remoteAddress is localhost (both IPv4 & IPv6)
I also updated the README to match the new behavior, but I need help writing the tests: the one marked 'TODO' currently fails, we would need to create a couple of tests for both localhost connection and non-localhost one.