镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Potential solution for Cookie options(secure) in Local HTTP Environments - #982

Open
darcy521 wants to merge 1 commit into
expressjs:masterfrom
darcy521:master
Open

darcy521 wants to merge 1 commit into
expressjs:masterfrom
darcy521:master

Conversation

@darcy521

@darcy521 darcy521 commented May 4, 2024

Copy link
Copy Markdown

From my understanding, if we are running our application locally without using HTTPS and Nginx or something similar, secure will not work under the logic of express-session.

In the latest version of Chrome and Firefox HTTPS requirements are ignored when the Secure attribute is set by localhost, please see the MDN documents below:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
image
Chrome permits setting the secure attribute for cookies under HTTP on localhost. However, express-session does not create cookies under HTTP, which is why cookies might not appear even if the secure and sameSite are correctly set.

I wonder if we can refine logic to align with Chrome rules, so I tried to do some 'naive' optimization by checking if application is running locally.
image
image

I am aware that there may be some security concerns related to my changes (Like if there are any other Reverse Proxys running locally under production mode, cookie will still be set even though they haven't set 'x-forwarded-proto' headers). If possible, could you happen to identify these issues so I can learn from them? Thank you! :)

@yxshee yxshee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I ran this against current master (it merges cleanly): 12 tests fail, all with ReferenceError: os is not defined. isRunningLocally() uses os, but index.js never requires it, so any request with cookie.secure: true over plain HTTP throws. npm run lint also fails with a parsing error on the arrow function (index.js:717 after merging master), and Object.values and .flat() are not available on the Node 0.8 to 10 versions that CI still tests.

With var os = require('os') added, 6 existing tests still fail, for example "proxy option when enabled should work when no header: should not have Set-Cookie header". The reason is that os.networkInterfaces() returns the server's own interfaces, and loopback is always one of them. On macOS the check returns [ '127.0.0.1', '::1' ], and in a node:22-alpine container the addresses are 127.0.0.1 ::1 172.17.0.3. So isRunningLocally() returns true on effectively every host, and Secure cookies would be sent over plain HTTP in production too. That is the case the existing check is there to prevent.

What browsers allow for Secure cookies is decided by the address the client used to reach the server, not the server's interfaces. The closest per-request signal would be req.socket.localAddress or the Host header. That would still change a security default, though, so a maintainer may want to weigh in on whether this belongs in session at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants