apps/backend/src/app/api/latest/integrations/idp.ts (1)

179-187: Avoid double-invocation of getOldStackServerSecret() and switch to explicit null check.

getOldStackServerSecret() is called twice — once for the guard and once inside the template literal. Each call re-reads process.env and re-runs jose.base64url.decode, and the two reads are not atomic (an unlikely but real footgun if env changes mid-request, e.g. via a test harness). Cache once.

This also pairs with the return-type fix suggested in jwt.tsx — once that returns null for "unset", the guard here should be an explicit != null check per coding guidelines.

♻️ Proposed fix

     cookies: {
       // oidc-provider passes these to Koa keygrip: index 0 signs new cookies, any entry verifies.
       // During a STACK_SERVER_SECRET rotation, the old-secret-derived key is appended so cookies
       // issued before the rotation remain readable until they expire naturally.
-      keys: [
-        toHexString(await sha512(`oidc-idp-cookie-encryption-key:${getEnvVariable("STACK_SERVER_SECRET")}`)),
-        ...(getOldStackServerSecret()
-          ? [toHexString(await sha512(`oidc-idp-cookie-encryption-key:${getOldStackServerSecret()}`))]
-          : []),
-      ],
+      keys: await (async () => {
+        const oldSecret = getOldStackServerSecret();
+        return [
+          toHexString(await sha512(`oidc-idp-cookie-encryption-key:${getEnvVariable("STACK_SERVER_SECRET")}`)),
+          ...(oldSecret != null
+            ? [toHexString(await sha512(`oidc-idp-cookie-encryption-key:${oldSecret}`))]
+            : []),
+        ];
+      })(),
     },

Or, more cleanly, compute keys into a local above the new Provider(...) call.

As per coding guidelines: "Unless very clearly equivalent from types, prefer explicit null/undefinedness checks over boolean checks."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/backend/src/app/api/latest/integrations/idp.ts` around lines 179 - 187,
Cache the result of getOldStackServerSecret() into a local variable (e.g., const
oldSecret = getOldStackServerSecret()) and compute the keys array in a local
variable before calling new Provider(...); call sha512/toHexString once for each
needed secret using getEnvVariable("STACK_SERVER_SECRET") and oldSecret, and use
an explicit null check (oldSecret != null) instead of a truthy guard so you only
invoke getOldStackServerSecret() one time and avoid double-invocation in the
keys array.

packages/stack-shared/src/utils/jwt.tsx (1)

120-152: Verify downstream JWKS consumer assumptions before merging.

The refactor is sound: signing uses privateJwks[0] (legacy derivation of current primary), and old-secret keys are appended when configured. The JWKS endpoint is correctly implemented with 1-hour caching.

However, the key count range deserves validation: up to 6 keys published normally (2 per audience × 3 audiences), scaling to 12 during dual-secret overlap. While not large, confirm that no downstream consumer (SDKs, clients with limited key caches, or rate-limited JWKS fetch patterns) assumes bounded key counts. Test #10 covers the dual-secret rotation invariant for a single audience; consider adding coverage for the full 3-audience overlap scenario if not already present.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-shared/src/utils/jwt.tsx` around lines 120 - 152, The change
can publish up to 6 keys (or 12 during dual-secret overlap) from
getPrivateJwks/derivePairForSecret/getPrivateJwkFromDerivedSecret when three
audiences are used; before merging, validate downstream JWKS consumers (SDKs,
clients, caches) do not assume a smaller bounded key set and won't break or
rate-limit on larger JWKS responses, and add a unit/integration test that
simulates all three audiences plus dual-secret overlap to assert the JWKS
endpoint returns the expected combined key count and that signing still uses
privateJwks[0]; if any consumer cannot handle the larger set, either document
the new key-count behavior clearly or limit published keys (e.g., prefer current
primary only) and add a note in the codepaths around
getPrivateJwks/getStackServerSecret/getOldStackServerSecret.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/stack-shared/src/utils/jwt.tsx (1)

30-37: ⚠️ Potential issue | 🔴 Critical

Return null when STACK_SERVER_SECRET_OLD is unset.

The helper still requires and returns a string, so the “new secret only” phase can fail instead of omitting secondary keys. Normalize the optional env var to null and use an explicit nullish check at the call site.

Proposed fix

-export function getOldStackServerSecret(): string {
-  const STACK_SERVER_SECRET_OLD = getEnvVariable("STACK_SERVER_SECRET_OLD");
+export function getOldStackServerSecret(): string | null {
+  const STACK_SERVER_SECRET_OLD = getEnvVariable("STACK_SERVER_SECRET_OLD", "");
+  if (STACK_SERVER_SECRET_OLD === "") return null;
   try {
     jose.base64url.decode(STACK_SERVER_SECRET_OLD);
   } catch (e) {
     throw new StackAssertionError("STACK_SERVER_SECRET_OLD is set but not a valid base64url string. Remove it, or set it to the previous STACK_SERVER_SECRET value.", { cause: e });
   }
   return STACK_SERVER_SECRET_OLD;
 }

   const primaryPair = await derivePairForSecret(getStackServerSecret());
   const oldSecret = getOldStackServerSecret();
-  const oldPair = oldSecret ? await derivePairForSecret(oldSecret) : [];
+  const oldPair = oldSecret != null ? await derivePairForSecret(oldSecret) : [];

Also applies to: 147-148

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-shared/src/utils/jwt.tsx` around lines 30 - 37, Update
getOldStackServerSecret to treat STACK_SERVER_SECRET_OLD as optional: if
getEnvVariable("STACK_SERVER_SECRET_OLD") returns undefined/null/empty, return
null instead of throwing or returning a string; only call jose.base64url.decode
when the env value is present and, on decode failure, throw the existing
StackAssertionError with the decode error as cause. Change the function
signature/return type to allow null and ensure the callers use an explicit
nullish check before using the returned value.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/stack-shared/src/utils/jwt.test.ts (1)

73-82: ⚠️ Potential issue | 🟠 Major

Re-check Deploy 1 signing against cached JWKS consumers.

These specs still require new tokens to be signed with the new kid immediately once both secrets are configured. If any verifier holds a pre-Deploy-1 JWKS cache, those tokens can fail verification until cache refresh; Phase 1 should publish the new key while continuing to sign with the old key, then flip signing after the cache window if cached JWKS consumers are in scope.

Also applies to: 99-145

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-shared/src/utils/jwt.test.ts` around lines 73 - 82, The test
assumes immediate flip-to-new-kid signing after setRotatingEnv, but Phase 1
requires publishing the new key while continuing to sign with the old key so
cached JWKS consumers don't fail; update the test around setRotatingEnv,
signJWT, getPrivateJwks, verifyJWT and jose.decodeProtectedHeader to assert that
after calling setRotatingEnv the new JWKS entry exists (getPrivateJwks contains
the new kid) while the freshly signed JWT still carries the previous (old) kid
and verifies successfully; then separately simulate the post-cache-refresh path
(or trigger the rotate-to-new-signing step) and assert that subsequent signJWT
tokens use the new kid. Ensure you reference setRotatingEnv, signJWT, verifyJWT,
getPrivateJwks and jose.decodeProtectedHeader when making these assertions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

docker/server/.env.example (1)

9-10: Clarify the comment to specify what to remove and when.

The comment "Remove after the grace window" is ambiguous—it doesn't specify what to remove (just STACK_SERVER_SECRET_OLD, or both lines 9-10?) or the grace window duration. Given the PR description mentions a ~1 day transition period and the reviewer's finding that getOldStackServerSecret returns an empty string (not null) when unset, it's critical to completely remove the line rather than leaving an empty value.

📝 Proposed clarification

-# Remove after the grace window
-STACK_SERVER_SECRET_OLD=23-wuNpik0gIW4mruTz25rbIvhuuvZFrLOLtL7J4tyo
+# TODO next-release: Remove STACK_SERVER_SECRET_OLD after the ~1 day grace period
+STACK_SERVER_SECRET_OLD=23-wuNpik0gIW4mruTz25rbIvhuuvZFrLOLtL7J4tyo

This makes it clear that:

• The entire STACK_SERVER_SECRET_OLD line should be removed (not just emptied)
• The approximate grace window duration is ~1 day
• The removal is tracked as a next-release cleanup item

Based on learnings, the codebase uses // TODO next-release to mark temporary backward-compatibility shims for single-release cleanup.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker/server/.env.example` around lines 9 - 10, Update the ambiguous comment
above STACK_SERVER_SECRET_OLD to explicitly state that the entire
STACK_SERVER_SECRET_OLD line should be removed (not just emptied), specify the
grace window (~1 day), and mark it for follow-up by adding the project’s cleanup
marker (e.g., "// TODO next-release"); ensure this matches the runtime behavior
of getOldStackServerSecret which returns an empty string when unset so the line
must be deleted in the next release rather than left present with no value.