Rebase against expressjs/session

[hexaltation]

Context

While upgrading dependencies in gristlabs/grist-core#1368
yarn audit signals a vulnerability in @gristlabs/session.

The purpose of current PR is to fix that vulnerability by Upgrading this fork
which is the origin of https://www.npmjs.com/package/@gristlabs/express-session

[fflorent]

This is straightforward to review, LGTM! 🎉

Here is how I reviewed the diff:

$ git clone https://github.com/gristlabs/session
$ cd session/
$ git remote add upstream https://github.com/expressjs/session
$ git fetch upstream
$ gh repo set-default
$ 
This command sets the default remote repository to use when querying the
GitHub API for the locally cloned repository.

gh uses the default repository for things like:

 - viewing and creating pull requests
 - viewing and creating issues
 - viewing and creating releases
 - working with GitHub Actions
 - adding repository and environment secrets

? Which repository should be the default? gristlabs/session
✓ Set gristlabs/session as the default repository for the current directory

$ gh pr checkout 1 # This is the number of the current PR.
$ git diff v1.18.1 # This is the tag of the release on which @hexaltation is based on

You get this diff:

diff --git a/index.js b/index.js
index d41b237..e981623 100644
--- a/index.js
+++ b/index.js
@@ -99,6 +99,9 @@ function session(options) {
   // get the session store
   var store = opts.store || new MemoryStore()
 
+  // get the request-aware domain
+  var requestDomain = opts.requestDomain
+
   // get the trust proxy setting
   var trustProxy = opts.proxy
 
@@ -160,6 +163,10 @@ function session(options) {
     req.session = new Session(req);
     req.session.cookie = new Cookie(cookieOptions);
 
+    if (requestDomain) {
+      req.session.cookie.domain = requestDomain(req);
+    }
+
     if (cookieOptions.secure === 'auto') {
       req.session.cookie.secure = issecure(req, trustProxy);
     }
diff --git a/package.json b/package.json
index e332243..cff0783 100644
--- a/package.json
+++ b/package.json
@@ -1,5 +1,5 @@
 {
-  "name": "express-session",
+  "name": "@gristlabs/express-session",
   "version": "1.18.1",
   "description": "Simple session middleware for Express",
   "author": "TJ Holowaychuk <[email protected]> (http://tjholowaychuk.com)",
@@ -7,7 +7,7 @@
     "Douglas Christopher Wilson <[email protected]>",
     "Joe Wagner <[email protected]>"
   ],
-  "repository": "expressjs/session",
+  "repository": "gristlabs/session",
   "license": "MIT",
   "dependencies": {
     "cookie": "0.7.2",

The above diff corresponds exactly to the 2 commits made by @paulfitz on this branch :
https://github.com/gristlabs/session/commits/multi_domain

[paulfitz]

Thanks for this @hexaltation, and thanks for looking at it @fflorent. Do either of you know why an unresolved conflict is showing on package.json?

[hexaltation]

hello @paulfitz ,
The only conflict I have to solve during the rebase was in package.json
Package name and package version were changed both side.

It seems it still persisting here while the package.json has modified in my commit is the on we want.

P.S. the way I resolved the conflict is dirty as I kept gristlabs/session package name and take expressjs/session package version

[paulfitz]

@hexaltation are you willing to resolve the conflict listed at https://github.com/gristlabs/session/pull/1/conflicts to make your work mergeable? Or do you have a different process in mind for landing it, such as simply replacing the content of the target branch with yours?

[hexaltation]

@paulfitz It's done :)

[paulfitz]

Thanks @hexaltation ! Have you tried running this against Grist to confirm that Grist tests are still passing with it? I imagine the upstream code is pretty stable so I'd expect so.

[hexaltation]

@paulfitz
No, I haven't added it to my other PR (grist-core dependencies update) yet.
I was waiting for a npm release of this package.
But with your question I realize that I can substitute this one in node module to test, at least if it build without any problem and run the tests.

[fflorent]

BTW, would it make sense to add a ~grist1 to the npm version?

In case in the future we need to fix something in this fork without having to update the upstream version.

[hexaltation]

BTW, would it make sense to add a ~grist1 to the npm version?

In case in the future we need to fix something in this fork without having to update the upstream version.

It seems a pretty good idea.
Added to 06171ad

[hexaltation]

BTW, would it make sense to add a ~grist1 to the npm version?

In case in the future we need to fix something in this fork without having to update the upstream version.

In fact ~grist1 isn't a correct suffix.
npm wants -grist1 as dash seems the only authorized separator for suffixes in package version.

Corrected in c515e37

[hexaltation]

Hello @paulfitz,

I've tested it locally in grist-core.
It builds and runs.
The tests have some errors which do not seems to be related to this package (detail bellow).

If you agree that the errors aren't related so please merge and release a npm package.
After It I'll be more than happy to add It to gristlabs/grist-core#1368

Steps to reproduce

build package

$ npm pack --pack-destination ~

Change target in package.json

diff --git a/package.json b/package.json
index 7c41ec7e..8c23710f 100644
--- a/package.json
+++ b/package.json
@@ -114,7 +114,7 @@
     "@googleapis/drive": "8.14.0",
     "@googleapis/oauth2": "1.0.7",
     "@gristlabs/connect-sqlite3": "0.9.11-grist.5",
-    "@gristlabs/express-session": "1.17.0",
+    "@gristlabs/express-session": "file:/home/hexa/gristlabs-express-session-1.18.1-grist1.tgz",
     "@gristlabs/grist-widget": "^0.0.5",
     "@gristlabs/moment-guess": "1.2.4-grist.1",
     "@gristlabs/pidusage": "2.0.17",

Install and build

$ yarn install
$ yarn start

run the tests

yarn test:server
yarns test:gen-server

and I got the following errors that seems not related to this package:

yarn test:server

***
(1) CoreAuditLogger "before each" hook for "streams installation events to a single destination"
TypeError: (0 , AuditLoggerUtils_1.ignoreConfigEvents) is not a function
    at Context.<anonymous> (test/server/lib/CoreAuditLogger.ts:49:23)
    at processImmediate (node:internal/timers:491:21)

***
(2) GristAuditLogger logEventAsync logs audit events
TypeError: auditLogger.logEventAsync is not a function
    at Context.<anonymous> (test/server/lib/GristAuditLogger.ts:67:21)
    at processImmediate (node:internal/timers:491:21)

***
(3) GristAuditLogger logEventAsync throws on failure to log
TypeError: auditLogger.logEventAsync is not a function
    at Context.<anonymous> (test/server/lib/GristAuditLogger.ts:83:21)
    at processImmediate (node:internal/timers:491:21)

***
(4) GristAuditLogger logEventAsync throws if max pending requests exceeded
TypeError: auditLogger.logEventAsync is not a function
    at Context.<anonymous> (test/server/lib/GristAuditLogger.ts:109:21)
    at processImmediate (node:internal/timers:491:21)

yarn test-gen-server

***
(1) HealthCheck home "before all" hook for "has a working simple /status endpoint"
Error: TEST_REDIS_URL is expected
    at Context.<anonymous> (test/gen-server/lib/HealthCheck.ts:22:17)
    at processImmediate (node:internal/timers:491:21)

***
(2) HealthCheck home "after all" hook for "reports error when redis is unhealthy"
TypeError: Cannot read properties of undefined (reading 'stop')
    at Context.<anonymous> (test/gen-server/lib/HealthCheck.ts:36:22)
    at processImmediate (node:internal/timers:491:21)

***
(3) HealthCheck docs "before all" hook for "has a working simple /status endpoint"
Error: TEST_REDIS_URL is expected
    at Context.<anonymous> (test/gen-server/lib/HealthCheck.ts:22:17)
    at processImmediate (node:internal/timers:491:21)

***
(4) HealthCheck docs "after all" hook for "reports error when redis is unhealthy"
TypeError: Cannot read properties of undefined (reading 'stop')
    at Context.<anonymous> (test/gen-server/lib/HealthCheck.ts:36:22)
    at processImmediate (node:internal/timers:491:21)

[paulfitz]

Thanks @hexaltation !

[paulfitz]

@hexaltation @gristlabs/[email protected] has been published.