Skip to content
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

Add secure cookies when remote is localhost #858

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,16 @@ attribute is not set.
the cookie back to the server in the future if the browser does not have an HTTPS
connection.

Please note that `secure: true` is a **recommended** option. However, it requires
an https-enabled website, i.e., HTTPS is necessary for secure cookies. If `secure`
is set, and you access your site over HTTP, the cookie will not be set. If you
have your node.js behind a proxy and are using `secure: true`, you need to set
"trust proxy" in express:
Please note that `secure: true` is a **recommended** option. However, for the cookie
to be set, it requires one of these conditions:

- https-enabled website
- localhost connection
- node.js behind a proxy and "trust proxy" in express
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't just be behind trust proxy, as you can still have a http connection though a proxy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


If none of these are true, the cookie will not be set.

Here is an example with "trust proxy":

```js
var app = express()
Expand Down
19 changes: 19 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,25 @@ function hash(sess) {
*/

function issecure(req, trustProxy) {

// socket is localhost
if (req.connection.remoteAddress === '127.0.0.1' ||
req.connection.remoteAddress === '::ffff:127.0.0.1' ||
req.connection.remoteAddress === '::1'
) {
// if proxy is trusted; localhost connection is secure for sure
if (trustProxy === true) {
return true;
}

// proxy not explicitly trusted; no proxy means connection is secure
if (req.headers['x-forwarded-proto'] !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not unless the user of this module specifies they trust it. This fixed a reported security vulnerability.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return true;
}

// proxy connected from localhost, we need to do other checks
}

// socket is https server
if (req.connection && req.connection.encrypted) {
return true;
Expand Down
1 change: 1 addition & 0 deletions test/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ describe('session()', function(){
.expect(200, done)
})

// TODO: Fix tests
it('should work when no header', function(done){
request(server)
.get('/')
Expand Down