Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/app.test.ts (1)

158-177: Follow the Map-over-Record guideline.

Coding guidelines ask us to use ES6 Map instead of Record in TS files. Please convert the attribute accumulator to a Map and update the callers to read via .get().

Apply this diff:

-  const parseCookieAttributes = (name: string) => {
+  const parseCookieAttributes = (name: string): Map<string, string> | null => {
     const raw = [...cookieWrites].reverse().find((entry) => entry.trim().toLowerCase().startsWith(`${name.toLowerCase()}=`));
     if (!raw) {
       return null;
     }
     const [, ...attributeParts] = raw.split(";").map((part) => part.trim()).filter(Boolean);
-    const attrs: Record<string, string> = {};
+    const attrs = new Map<string, string>();
     for (const attribute of attributeParts) {
       const [attrName, ...attrValueParts] = attribute.split("=");
-      attrs[attrName.toLowerCase()] = attrValueParts.join("=") || "";
+      attrs.set(attrName.toLowerCase(), attrValueParts.join("=") || "");
     }
     return attrs;
   };

   const refreshAttrs = parseCookieAttributes(`stack-refresh-${clientApp.projectId}`);
-  expect(refreshAttrs?.domain).toBe("example.com");
+  expect(refreshAttrs?.get("domain")).toBe("example.com");

   const accessAttrs = parseCookieAttributes("stack-access");
-  expect(accessAttrs?.domain).toBe("example.com");
+  expect(accessAttrs?.get("domain")).toBe("example.com");

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/app.test.ts (1)

147-166: Restore stubbed globals with try/finally to keep tests isolated.

Globals are stubbed but never restored; wrap body and call vi.unstubAllGlobals() in finally.

-  vi.stubGlobal("window", fakeWindow);
-  vi.stubGlobal("document", fakeDocument);
-  vi.stubGlobal("sessionStorage", fakeSessionStorage);
-
-  const { clientApp } = await createApp(
+  vi.stubGlobal("window", fakeWindow);
+  vi.stubGlobal("document", fakeDocument);
+  vi.stubGlobal("sessionStorage", fakeSessionStorage);
+  try {
+    const { clientApp } = await createApp(
       {
         config: {
           domains: [
             { domain: "https://example.com", handlerPath: "/handler" },
             { domain: "https://*.example.com", handlerPath: "/handler" },
           ],
         }
       },
       {
         client: {
           tokenStore: "cookie",
           noAutomaticPrefetch: true,
         },
       }
-  );
+    );
   ...
-  expect(cookieWrites.some((entry) => entry.toLowerCase().startsWith("stack-refresh=") && entry.toLowerCase().includes("expires="))).toBe(true);
-});
+  expect(cookieWrites.some((entry) => entry.toLowerCase().startsWith("stack-refresh=") && entry.toLowerCase().includes("expires="))).toBe(true);
+  } finally {
+    vi.unstubAllGlobals();
+  }
+});

Also applies to: 168-242

packages/template/src/lib/cookie.ts (1)

204-210: Avoid duplicate cookie deletes; only delete with domain OR without.

Still calls Cookies.remove(name) unconditionally after domain-specific remove. Do one or the other to prevent redundant ops and accidental host-only deletion.

 export function deleteCookieClient(name: string, options: DeleteCookieOptions = {}) {
   ensureClient();
-  if (options.domain !== undefined) {
-    Cookies.remove(name, { domain: options.domain });
-  }
-  Cookies.remove(name);
+  if (options.domain !== undefined) {
+    Cookies.remove(name, { domain: options.domain });
+  } else {
+    Cookies.remove(name);
+  }
 }

packages/template/src/lib/cookie.ts (2)

217-224: Set path=/ explicitly to satisfy __Host- requirements and be explicit.*

Relying on library defaults is brittle; __Host-* cookies must have Path=/ and no Domain. Add path: "/" on all writes.

 export function setCookieClient(name: string, value: string, options: SetCookieOptions = {}) {
   ensureClient();
   Cookies.set(name, value, {
     expires: options.maxAge === undefined ? undefined : new Date(Date.now() + (options.maxAge) * 1000),
     domain: options.domain,
     secure: options.secure,
+    path: "/",
   });
 }

---

173-188: isSecure(): LGTM with a small nit.

Looks good. Optionally check x-forwarded-proto before reading cookies to skip an extra lookup.

apps/e2e/tests/js/app.test.ts (2)

217-229: Prefer Map over Record for parsed cookie attributes.

Aligns with repo guideline to use ES6 Maps in TS. Also avoids accidental proto collisions.

-  const parseCookieAttributes = (name: string) => {
+  const parseCookieAttributes = (name: string): Map<string, string> | null => {
     const raw = [...cookieWrites].reverse().find((entry) => entry.trim().toLowerCase().startsWith(`${name.toLowerCase()}=`));
     if (!raw) {
       return null;
     }
     const [, ...attributeParts] = raw.split(";").map((part) => part.trim()).filter(Boolean);
-    const attrs: Record<string, string> = {};
-    for (const attribute of attributeParts) {
-      const [attrName, ...attrValueParts] = attribute.split("=");
-      attrs[attrName.toLowerCase()] = attrValueParts.join("=") || "";
-    }
-    return attrs;
+    const attrs = new Map<string, string>();
+    for (const attribute of attributeParts) {
+      const [attrName, ...attrValueParts] = attribute.split("=");
+      attrs.set(attrName.toLowerCase(), attrValueParts.join("=") || "");
+    }
+    return attrs;
   };

Update callsites accordingly, e.g.:

-const defaultAttrs = parseCookieAttributes(defaultCookieName);
-expect(defaultAttrs?.domain).toBeUndefined();
-expect(defaultAttrs).not.toBeNull();
-expect(Object.prototype.hasOwnProperty.call(defaultAttrs!, "secure")).toBe(true);
+const defaultAttrs = parseCookieAttributes(defaultCookieName);
+expect(defaultAttrs).not.toBeNull();
+expect(defaultAttrs!.has("domain")).toBe(false);
+expect(defaultAttrs!.has("secure")).toBe(true);

-const customAttrs = parseCookieAttributes(customCookieName);
-expect(customAttrs?.domain).toBe("example.com");
+const customAttrs = parseCookieAttributes(customCookieName);
+expect(customAttrs!.get("domain")).toBe("example.com");

As per coding guidelines.

---

231-238: Strengthen assertions: enforce Path=/ and Secure on both cookies.

Assert __Host-* invariants and that the domain cookie is also Secure.

-  expect(defaultAttrs?.domain).toBeUndefined();
-  expect(defaultAttrs).not.toBeNull();
-  expect(Object.prototype.hasOwnProperty.call(defaultAttrs!, "secure")).toBe(true);
+  expect(defaultAttrs).not.toBeNull();
+  expect(defaultAttrs!.has("domain")).toBe(false);
+  expect(defaultAttrs!.get("path")).toBe("/");
+  expect(defaultAttrs!.has("secure")).toBe(true);

-  const customAttrs = parseCookieAttributes(customCookieName);
-  expect(customAttrs?.domain).toBe("example.com");
+  const customAttrs = parseCookieAttributes(customCookieName);
+  expect(customAttrs!.get("domain")).toBe("example.com");
+  expect(customAttrs!.has("secure")).toBe(true);
+  expect(customAttrs!.get("path")).toBe("/");

packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (2)

619-647: Choose the trusted parent domain that matches the current base domain.

_current implementation may pick the wrong candidate when multiple parent domains exist (e.g., example.com vs example.co.uk). Prefer the one matching the current base domain; fall back to the first.

-  private async _fetchTrustedParentDomain(): Promise<string | null> {
+  private async _fetchTrustedParentDomain(): Promise<string | null> {
     const project = Result.orThrow(await this._interface.getClientProject());
     const domains = project.config.domains;
@@
-    const selected = candidates[0] ?? null;
-    return selected;
+    // Prefer the candidate that matches the current base domain (browser or server)
+    let currentBase: string | undefined;
+    if (isBrowserLike()) {
+      currentBase = this._getBrowserBaseDomain();
+    } else {
+      currentBase = await this._getServerBaseDomain();
+    }
+    const selected =
+      (currentBase && candidates.find((c) => c === currentBase)) ??
+      candidates[0] ??
+      null;
+    return selected;

---

539-550: Minor: remove unreachable return.

After the try/catch both branches return; the final return is unreachable in Next builds. Safe to drop.

   private async _getServerBaseDomain(): Promise<string | undefined> {
     // IF_PLATFORM next
     try {
       const resolvedHeaders = typeof nextHeaders === "function" ? await nextHeaders() : nextHeaders;
       const hostHeader = resolvedHeaders?.get("x-forwarded-host") ?? resolvedHeaders?.get("host");
       return hostHeader ? extractBaseDomainFromHost(hostHeader) : undefined;
     } catch {
       return undefined;
     }
     // END_PLATFORM
-    return undefined;
   }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

apps/e2e/tests/js/cookies.test.ts (10)

1-2: Avoid dist import; drop TextEncoder import

• Importing from a package’s dist path is brittle. Prefer the public entrypoint.
• TextEncoder is global in Node ≥18; the explicit import is unnecessary.

Apply one of these (verify export availability):

-import { encodeBase32 } from "@stackframe/stack-shared/dist/utils/bytes";
-import { TextEncoder } from "util";
+import { encodeBase32 } from "@stackframe/stack-shared";
// If not re‑exported:
// import { encodeBase32 } from "@stackframe/stack-shared/utils/bytes";

If you keep encodeBase32 usage, rely on the global TextEncoder (no import needed).

---

56-74: Make cookie deletion semantics closer to browsers

Right now, cookies are deleted only when the value is empty. Real deletions also happen via Max-Age=0 or a past Expires. Aligning the shim avoids stale entries lingering in cookieStore.

Apply:

   set: (value: string) => {
     cookieWrites.push(value);
     const [pair] = value.split(";").map((part) => part.trim()).filter(Boolean);
     if (!pair) {
       return;
     }
     const [rawName, ...rawValueParts] = pair.split("=");
     const name = rawName.trim();
     const storedValue = rawValueParts.join("=");
-    if (storedValue === "") {
-      cookieStore.delete(name);
-    } else {
-      cookieStore.set(name, storedValue);
-    }
+    // Parse attributes for deletion semantics
+    const attrs = new Map<string, string>();
+    for (const part of value.split(";").slice(1)) {
+      const [k, ...v] = part.trim().split("=");
+      attrs.set(k.toLowerCase(), v.join("=") || "");
+    }
+    const maxAge = attrs.get("max-age");
+    const expires = attrs.get("expires");
+    const isExpired =
+      maxAge === "0" ||
+      (expires ? !Number.isNaN(Date.parse(expires)) && Date.parse(expires) <= Date.now() : false);
+    if (storedValue === "" || isExpired) cookieStore.delete(name);
+    else cookieStore.set(name, storedValue);
   },

---

76-79: Restore stubbed globals after each test

Globals are stubbed but never un-stubbed, risking cross-test leakage. Add a teardown.

-import { vi } from "vitest";
+import { vi, afterEach } from "vitest";
...
 vi.stubGlobal("window", fakeWindow);
 vi.stubGlobal("document", fakeDocument);
 vi.stubGlobal("sessionStorage", fakeSessionStorage);
 
 return {
   cookieStore,
   cookieWrites,
   location,
 };
}
+
+// Ensure no pollution across tests
+afterEach(() => {
+  // Vitest >=1.2
+  (vi as any).unstubAllGlobals?.();
+});

If your Vitest version lacks unstubAllGlobals, store previous values in setupBrowserCookieEnv and return a cleanup() you call in tests.

---

98-110: Normalize Domain attribute (handle leading dot)

Some setters write Domain=.example.com. Normalizing avoids brittle assertions.

-  for (const attribute of attributeParts) {
+  for (const attribute of attributeParts) {
     const [attrName, ...attrValueParts] = attribute.split("=");
-    attrs.set(attrName.toLowerCase(), attrValueParts.join("=") || "");
+    const key = attrName.toLowerCase();
+    let val = attrValueParts.join("=") || "";
+    if (key === "domain") val = val.replace(/^\./, "");
+    attrs.set(key, val);
   }

---

163-168: Trim long waits to keep e2e fast

10s waits will slow CI. Unless truly necessary, reduce to 2–3s and tighten polling.

-const customReady = await waitUntil(() => cookieStore.has(customCookieName), 10_000);
+const customReady = await waitUntil(() => cookieStore.has(customCookieName), 3_000);
...
-const valuesEqual = await waitUntil(() => cookieStore.get(customCookieName) === cookieStore.get(defaultCookieName), 10_000);
+const valuesEqual = await waitUntil(() => cookieStore.get(customCookieName) === cookieStore.get(defaultCookieName), 3_000);

Optionally make intervalMs smaller for these checks (e.g., 50ms).

Also applies to: 172-174

---

181-185: Assert full __Host- invariants

For __Host- cookies, browsers require Secure, no Domain, and Path=/. You’re asserting the first two; add Path=/ to catch regressions.

 expect(defaultAttrs).not.toBeNull();
 expect(defaultAttrs?.has("secure")).toBe(true);
 expect(defaultAttrs?.get("domain")).toBeUndefined();
+expect(defaultAttrs?.get("path")).toBe("/");

Confirm the writer sets Path=/ for __Host- cookies.

---

186-188: Relax exact match on Domain to tolerate leading dot

Avoid false negatives if the implementation writes .example.com.

- expect(customAttrs?.get("domain")).toBe("example.com");
+ expect(customAttrs?.get("domain")).toBe("example.com"); // after helper normalization
+ // If you don't normalize in the helper, use:
+ // expect(customAttrs?.get("domain")?.replace(/^\./, "")).toBe("example.com");

---

243-296: Consider snapshotting cookie attribute maps

Where attributes are stable (e.g., default insecure cookie), inline snapshots can surface unintended changes (e.g., SameSite flips) per test guidelines.

// Example:
expect(Object.fromEntries(insecureAttrs!)).toMatchInlineSnapshot(`
  {
    "path": "/",
    // "samesite": "lax", // include if applicable/stable
  }
`);

As per coding guidelines

---

313-320: Prefer Map over Record; convert at call site if needed

Guideline favors ES6 Map. You can still feed an object to the existing API via Object.fromEntries.

-const cookieMap: Record<string, string> = {
-  [defaultCookieName]: staleCookieValue,
-  [customCookieName]: freshCookieValue,
-  "stack-access": JSON.stringify(["fresh-token", "fresh-access-token"]),
-};
-
-const tokens = (clientApp as any)._getTokensFromCookies(cookieMap);
+const cookieMap = new Map<string, string>([
+  [defaultCookieName, staleCookieValue],
+  [customCookieName, freshCookieValue],
+  ["stack-access", JSON.stringify(["fresh-token", "fresh-access-token"])],
+]);
+const tokens = (clientApp as any)._getTokensFromCookies(Object.fromEntries(cookieMap));

If _getTokensFromCookies already accepts a Map, pass it directly.

---

298-320: Avoid coupling to private API

_getTokensFromCookies is an internal method; tests may break on refactors. Prefer a public surface or a small exported helper.

Expose a stable helper (e.g., extractTokensFromCookies(cookies: Map<string,string>|Record<string,string>)) and test that instead.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro