Repository: remix-run/remix
Pull request #3091 (2022-05-04 17:35:09)

chore(remix-server-runtime): warn against using `expires` in `createCookieSessionStorage` and `createSessionStorage`
specifying an 'expires' will cause all uses of `commitSession` to use that date.

~~this only warns for `createCookieSessionStorageFactory` and `createSessionStorageFactory` (which is used by `createCookieSessionStorage` and `createSessionStorage`) as our generic `createCookie` util doesn't warn about signing cookies, so i left it out here, but easy enough to add it for that too~~

Signed-off-by: Logan McAnsh <logan@mcan.sh>

Closes: #2848

- [ ] Docs
- [x] Tests

Testing Strategy:

This test covers this code: https://github.com/remix-run/remix/blob/29cf8ff5288d97405e09812dfc8f30efa10bb1d2/packages/remix-server-runtime/__tests__/sessions-test.ts#L142

mcansh: adding a test for the warning for "missing secrets" while im in here, and looks like the sessionStorages call out to createCookie so we end up with 2 warnings, moving the linear issue back to `in development` and reassigning it while i look into that 😬

Patch:
diff --git a/packages/remix-server-runtime/cookies.ts b/packages/remix-server-runtime/cookies.ts
index d9853196b..8cacd3808 100644
--- a/packages/remix-server-runtime/cookies.ts
+++ b/packages/remix-server-runtime/cookies.ts
@@ -2,6 +2,7 @@ import type { CookieParseOptions, CookieSerializeOptions } from "cookie";
 import { parse, serialize } from "cookie";
 
 import type { SignFunction, UnsignFunction } from "./crypto";
+import { warnOnce } from "./warnings";
 
 export type { CookieParseOptions, CookieSerializeOptions };
 
@@ -91,6 +92,8 @@ export const createCookieFactory =
       ...cookieOptions,
     };
 
+    warnOnceAboutExpiresCookie(name, options.expires);
+
     return {
       get name() {
         return name;
@@ -245,3 +248,14 @@ function myUnescape(value: string): string {
   }
   return result;
 }
+
+function warnOnceAboutExpiresCookie(name: string, expires?: Date) {
+  warnOnce(
+    !expires,
+    `The "${name}" cookie has an "expires" property set. ` +
+      `This will cause the expires value to not be updated when the session is committed. ` +
+      `Instead, you should set the expires value when serializing the cookie. ` +
+      `You can use \`commitSession(session, { expires })\` if using a session storage object, ` +
+      `or \`cookie.serialize("value", { expires })\` if you're using the cookie directly.`
+  );
+}
diff --git a/scripts/publish-private.js b/scripts/publish-private.js
index 89b8d9783..37fe0bb00 100644
--- a/scripts/publish-private.js
+++ b/scripts/publish-private.js
@@ -10,6 +10,10 @@ function getTaggedVersion() {
   return output.replace(/^v/g, "");
 }
 
+/**
+ * @param {string} dir
+ * @param {string} tag
+ */
 function publish(dir, tag) {
   execSync(`npm publish --tag ${tag} ${dir}`, { stdio: "inherit" });
 }
@@ -23,7 +27,14 @@ async function run() {
   }
 
   let prerelease = semver.prerelease(taggedVersion);
-  let tag = prerelease ? prerelease[0] : "latest";
+  let prereleaseTag = prerelease ? String(prerelease[0]) : undefined;
+  let tag = prereleaseTag
+    ? prereleaseTag.includes("nightly")
+      ? "nightly"
+      : prereleaseTag.includes("experimental")
+      ? "experimental"
+      : prereleaseTag
+    : "latest";
 
   // Publish all @remix-run/* packages
   for (let name of [
diff --git a/scripts/publish.js b/scripts/publish.js
index 32d893f04..12accadb2 100644
--- a/scripts/publish.js
+++ b/scripts/publish.js
@@ -10,11 +10,14 @@ function getTaggedVersion() {
   return output.replace(/^v/g, "");
 }
 
+/**
+ * @param {string} dir
+ * @param {string} tag
+ */
 function publish(dir, tag) {
-  execSync(
-    `npm publish --access public${tag != null ? ` --tag ${tag}` : ""} ${dir}`,
-    { stdio: "inherit" }
-  );
+  execSync(`npm publish --access public --tag ${tag} ${dir}`, {
+    stdio: "inherit",
+  });
 }
 
 async function run() {
@@ -31,7 +34,7 @@ async function run() {
     ? prereleaseTag.includes("nightly")
       ? "nightly"
       : prereleaseTag.includes("experimental")
-      ? null
+      ? "experimental"
       : prereleaseTag
     : "latest";
 


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 28918bbca..55f4862f0 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -10,6 +10,7 @@ on:
       - v*
     paths-ignore:
       - "docs/**"
+      - "scripts/**"
       - "**/README.md"
   pull_request: {}
 
diff --git a/packages/remix-server-runtime/__tests__/cookies-test.ts b/packages/remix-server-runtime/__tests__/cookies-test.ts
index 8fa2ff00b..e04b1723f 100644
--- a/packages/remix-server-runtime/__tests__/cookies-test.ts
+++ b/packages/remix-server-runtime/__tests__/cookies-test.ts
@@ -155,4 +155,35 @@ describe("cookies", () => {
     });
     expect(setCookie2).toContain("Path=/about");
   });
+
+  describe("warnings when providing options you may not want to", () => {
+    let spy = spyConsole();
+
+    it("warns against using `expires` when creating the cookie instance", async () => {
+      createCookie("my-cookie", { expires: new Date(Date.now() + 60_000) });
+      expect(spy.console).toHaveBeenCalledTimes(1);
+      expect(spy.console).toHaveBeenCalledWith(
+        'The "my-cookie" cookie has an "expires" property set. This will cause the expires value to not be updated when the session is committed. Instead, you should set the expires value when serializing the cookie. You can use `commitSession(session, { expires })` if using a session storage object, or `cookie.serialize("value", { expires })` if you\'re using the cookie directly.'
+      );
+    });
+  });
 });
+
+function spyConsole() {
+  // https://github.com/facebook/react/issues/7047
+  let spy: any = {};
+
+  beforeAll(() => {
+    spy.console = jest.spyOn(console, "warn").mockImplementation(() => {});
+  });
+
+  beforeEach(() => {
+    spy.console.mockClear();
+  });
+
+  afterAll(() => {
+    spy.console.mockRestore();
+  });
+
+  return spy;
+}
diff --git a/packages/remix-server-runtime/__tests__/sessions-test.ts b/packages/remix-server-runtime/__tests__/sessions-test.ts
index 74e6bbffa..98f62e7de 100644
--- a/packages/remix-server-runtime/__tests__/sessions-test.ts
+++ b/packages/remix-server-runtime/__tests__/sessions-test.ts
@@ -139,6 +139,33 @@ describe("Cookie session storage", () => {
     await expect(() => commitSession(session)).rejects.toThrow();
   });
 
+  describe("warnings when providing options you may not want to", () => {
+    let spy = spyConsole();
+
+    it("warns against using `expires` when creating the session", async () => {
+      createCookieSessionStorage({
+        cookie: {
+          secrets: ["secret1"],
+          expires: new Date(Date.now() + 60_000),
+        },
+      });
+
+      expect(spy.console).toHaveBeenCalledTimes(1);
+      expect(spy.console).toHaveBeenCalledWith(
+        'The "__session" cookie has an "expires" property set. This will cause the expires value to not be updated when the session is committed. Instead, you should set the expires value when serializing the cookie. You can use `commitSession(session, { expires })` if using a session storage object, or `cookie.serialize("value", { expires })` if you\'re using the cookie directly.'
+      );
+    });
+
+    it("warns when not passing secrets when creating the session", async () => {
+      createCookieSessionStorage({ cookie: {} });
+
+      expect(spy.console).toHaveBeenCalledTimes(1);
+      expect(spy.console).toHaveBeenCalledWith(
+        'The "__session" cookie is not signed, but session cookies should be signed to prevent tampering on the client before they are sent back to the server. See https://remix.run/api/remix#signing-cookies for more information.'
+      );
+    });
+  });
+
   describe("when a new secret shows up in the rotation", () => {
     it("unsigns old session cookies using the old secret and encodes new cookies using the new secret", async () => {
       let { getSession, commitSession } = createCookieSessionStorage({
@@ -168,3 +195,22 @@ describe("Cookie session storage", () => {
     });
   });
 });
+
+function spyConsole() {
+  // https://github.com/facebook/react/issues/7047
+  let spy: any = {};
+
+  beforeAll(() => {
+    spy.console = jest.spyOn(console, "warn").mockImplementation(() => {});
+  });
+
+  beforeEach(() => {
+    spy.console.mockClear();
+  });
+
+  afterAll(() => {
+    spy.console.mockRestore();
+  });
+
+  return spy;
+}