fix(send): stop init() destroying the default folder; harden container create & flaky E2E (#930) (#937)

* fix init failure and track errors

* share link guard tests

* update test utils

Author: aaspinwall

packages/send/backend/src/models/containers.ts

@@ -1,5 +1,6 @@
 import { ContainerType, PrismaClient } from '@prisma/client';
 import {
+  BaseError,
   CONTAINER_NOT_CREATED,
   CONTAINER_NOT_FOUND,
   CONTAINER_NOT_UPDATED,
@@ -23,45 +24,47 @@ export async function createContainer(
   parentId: string | null,
   shareOnly: boolean
 ) {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  let query: Record<string, any> = {
-    data: {},
-  };
-  const group = await fromPrismaV2(
-    prisma.group.create,
-    query,
-    GROUP_NOT_CREATED
-  );
-
-  query = {
-    data: {
-      groupId: group.id,
-      userId: ownerId,
-      permission: PermissionType.ADMIN, // Owner has full permissions
-    },
-  };
-  await fromPrismaV2(prisma.membership.create, query, MEMBERSHIP_NOT_CREATED);
-
-  query = {
-    data: {
-      name,
-      ownerId,
-      groupId: group.id,
-      type,
-      shareOnly,
-      createdAt: new Date(),
-      updatedAt: new Date(),
-    },
-  };
-  if (parentId) {
-    query.data['parentId'] = parentId;
-  }
-
-  return await fromPrismaV2(
-    prisma.container.create,
-    query,
-    CONTAINER_NOT_CREATED
-  );
+  // Create the group, the owner's membership, and the container atomically.
+  // Doing these as separate calls left a window where a partially-created
+  // container (e.g. group without membership) could be observed, surfacing as
+  // a spurious 403 / "No Group found". See issue #930.
+  return prisma.$transaction(async (tx) => {
+    const group = await tx.group.create({ data: {} }).catch((err) => {
+      console.error(err);
+      throw new BaseError(GROUP_NOT_CREATED);
+    });
+
+    await tx.membership
+      .create({
+        data: {
+          groupId: group.id,
+          userId: ownerId,
+          permission: PermissionType.ADMIN, // Owner has full permissions
+        },
+      })
+      .catch((err) => {
+        console.error(err);
+        throw new BaseError(MEMBERSHIP_NOT_CREATED);
+      });
+
+    return tx.container
+      .create({
+        data: {
+          name,
+          ownerId,
+          groupId: group.id,
+          type,
+          shareOnly,
+          createdAt: new Date(),
+          updatedAt: new Date(),
+          ...(parentId ? { parentId } : {}),
+        },
+      })
+      .catch((err) => {
+        console.error(err);
+        throw new BaseError(CONTAINER_NOT_CREATED);
+      });
+  });
 }
 
 export async function getItemsInContainer(id: string) {

packages/send/e2e/pages/dev/myFiles.ts

@@ -7,10 +7,11 @@ import {
   downloadFirstFile,
   dragAndDropFile,
   playwrightConfig,
+  requireShareLink,
   saveClipboardItem,
 } from "../../utils/dev/testUtils";
 
-const { password, timeout, shareLinks, fileLinks } = playwrightConfig;
+const { password, timeout, fileLinks } = playwrightConfig;
 
 export async function upload_workflow({ page }: PlaywrightProps) {
   const { folderRowSelector, folderRowTestID, fileCountID, uploadButton, dropZone, tableCellID, passwordInput } =
@@ -150,13 +151,13 @@ export async function download_workflow({ page, context }: PlaywrightProps) {
 
   // Regular window downloads
   let otherPage = await context.newPage();
-  await otherPage.goto(shareLinks["folder-no-password"]!);
+  await otherPage.goto(requireShareLink("folder-no-password"));
   await otherPage.waitForLoadState("networkidle");
   await downloadFirstFile(otherPage);
   await otherPage.close();
 
   otherPage = await context.newPage();
-  await otherPage.goto(shareLinks["folder-with-password"]!);
+  await otherPage.goto(requireShareLink("folder-with-password"));
   await otherPage.waitForLoadState("networkidle");
   await otherPage.getByTestId(passwordInputID).fill(password);
   await otherPage.getByTestId(submitButtonID).click();
@@ -176,14 +177,14 @@ export async function download_workflow({ page, context }: PlaywrightProps) {
 
     // Download share link (folder) without password
     otherPage = await incognitoContext.newPage();
-    await otherPage.goto(shareLinks["folder-no-password"]!);
+    await otherPage.goto(requireShareLink("folder-no-password"));
     await otherPage.waitForLoadState("networkidle");
     await downloadFirstFile(otherPage);
     await otherPage.close();
 
     // Download share link (folder) with password
     otherPage = await incognitoContext.newPage();
-    await otherPage.goto(shareLinks["folder-with-password"]!);
+    await otherPage.goto(requireShareLink("folder-with-password"));
     await otherPage.waitForLoadState("networkidle");
     await otherPage.getByTestId(passwordInputID).fill(password);
     await otherPage.getByTestId(submitButtonID).click();
@@ -193,14 +194,14 @@ export async function download_workflow({ page, context }: PlaywrightProps) {
 
     // Download individual file without password
     otherPage = await incognitoContext.newPage();
-    await otherPage.goto(shareLinks["file-no-password"]!);
+    await otherPage.goto(requireShareLink("file-no-password"));
     await otherPage.waitForLoadState("networkidle");
     await downloadFirstFile(otherPage);
     await otherPage.close();
 
     // Download individual file with password
     otherPage = await incognitoContext.newPage();
-    await otherPage.goto(shareLinks["file-with-password"]!);
+    await otherPage.goto(requireShareLink("file-with-password"));
     await otherPage.waitForLoadState("networkidle");
     await otherPage.getByTestId(passwordInputID).fill(password);
     await otherPage.getByTestId(submitButtonID).click();
@@ -239,12 +240,12 @@ export async function delete_file({ page }: PlaywrightProps) {
 
   // Check that the share links are no longer accessible
   // Folder no password link
-  await page.goto(shareLinks["folder-no-password"]!);
+  await page.goto(requireShareLink("folder-no-password"));
   await page.waitForLoadState("networkidle");
   expect(await page.getByTestId("not_found").textContent()).toContain("This link is no longer active");
 
   // Folder with password link
-  await page.goto(shareLinks["folder-with-password"]!);
+  await page.goto(requireShareLink("folder-with-password"));
   await page.waitForLoadState("networkidle");
   await page.getByTestId(passwordInputID).fill(password);
   await page.getByTestId(submitButtonID).click();

packages/send/e2e/tests/desktop/dev/send.spec.ts

@@ -25,7 +25,15 @@ import {
 } from "../../../pages/dev/myFiles"
 
 import { oidc_login } from "../../../pages/dev/oidc"
-import { setup_browser } from "../../../utils/dev/testUtils"
+import {
+  emptyState,
+  emptystatePath,
+  storageStatePath,
+} from "../../../utils/dev/paths"
+import { resetShareLinks, setup_browser } from "../../../utils/dev/testUtils"
+
+// Re-export for backwards compatibility with anything importing these from the spec.
+export { emptystatePath, storageStatePath }
 
 config.config({ path: path.resolve(__dirname, "./.env") });
 
@@ -39,20 +47,6 @@ export const credentials = {
   TBPRO_PASSWORD: process.env.TBPRO_PASSWORD,
 };
 
-export const storageStatePath = path.resolve(
-  __dirname,
-  "../../../data/lockboxstate.json"
-);
-
-export const emptystatePath = path.resolve(
-  __dirname,
-    "../../../data/emptystate.json"
-);
-const emptyState = {
-  cookies: [],
-  origins: [],
-};
-
 // Cleanup storage state after all tests
 test.afterAll(async () => {
   fs.writeFileSync(storageStatePath, JSON.stringify(emptyState));
@@ -107,6 +101,12 @@ test.describe("File workflows", {
   let page: Page;
   let context: BrowserContext;
 
+  // Start from a clean share-link map so a prior (possibly failed) run can't
+  // leak stale/null links into the download + delete steps below (#930).
+  test.beforeAll(() => {
+    resetShareLinks();
+  });
+
   test.beforeEach(async () => {
     ({ page, context } = await setup_browser());
     await page.goto("/send");

packages/send/e2e/tests/desktop/dev/sharelinks-guard.spec.ts

@@ -0,0 +1,46 @@
+// Unit-level regression tests for the share-link state guards added for #930.
+// These exercise pure helpers (no browser/stack required) so they run fast in CI
+// alongside the dev-desktop suite and document the intended guard behaviour.
+
+import { expect, test } from "@playwright/test";
+
+import { PLAYWRIGHT_TAG_DEV_DESKTOP } from "../../../const/const";
+import {
+  playwrightConfig,
+  requireShareLink,
+  resetShareLinks,
+} from "../../../utils/dev/testUtils";
+
+test.describe("share-link guards (#930)", { tag: [PLAYWRIGHT_TAG_DEV_DESKTOP] }, () => {
+  test("resetShareLinks clears every entry back to null", () => {
+    // Simulate a prior run that populated some links.
+    playwrightConfig.shareLinks["file-no-password"] = "https://example/share/abc";
+    playwrightConfig.shareLinks["folder-with-password"] = "https://example/share/def";
+
+    resetShareLinks();
+
+    for (const value of Object.values(playwrightConfig.shareLinks)) {
+      expect(value).toBeNull();
+    }
+  });
+
+  test("requireShareLink throws an actionable error when a link is missing", () => {
+    resetShareLinks();
+
+    expect(() => requireShareLink("folder-no-password")).toThrow(
+      /Missing share link for "folder-no-password"/
+    );
+    // Must NOT let a null reach page.goto (the cryptic "expected string, got object").
+    expect(() => requireShareLink("folder-no-password")).not.toThrow(
+      /expected string/
+    );
+  });
+
+  test("requireShareLink returns the populated link", () => {
+    resetShareLinks();
+    const link = "https://example/share/xyz";
+    playwrightConfig.shareLinks["file-with-password"] = link;
+
+    expect(requireShareLink("file-with-password")).toBe(link);
+  });
+});

packages/send/e2e/utils/dev/paths.ts

@@ -0,0 +1,19 @@
+// Storage-state file paths shared between the dev spec and test utils.
+// Extracted here to break a circular import (send.spec <-> testUtils) that made
+// the helpers unloadable outside the spec's own load order. See issue #930.
+import path from "path";
+
+export const storageStatePath = path.resolve(
+  __dirname,
+  "../../data/lockboxstate.json"
+);
+
+export const emptystatePath = path.resolve(
+  __dirname,
+  "../../data/emptystate.json"
+);
+
+export const emptyState = {
+  cookies: [],
+  origins: [],
+};

packages/send/e2e/utils/dev/testUtils.ts

@@ -9,7 +9,7 @@ import {
 import { readFileSync } from "fs";
 
 import { fileLocators } from "../../pages/dev/locators";
-import { emptystatePath, storageStatePath } from "../../tests/desktop/dev/send.spec";
+import { emptystatePath, storageStatePath } from "./paths";
 import path from "path";
 
 const sharelinks = {
@@ -29,6 +29,33 @@ export const playwrightConfig = {
   fileLinks: [] as string[],
 };
 
+/**
+ * Reset the module-level `shareLinks` singleton back to its empty baseline.
+ * Call this at suite start so a failed/aborted run can't leak stale (or null)
+ * links into a later run and produce cryptic, misattributed failures (#930).
+ */
+export function resetShareLinks() {
+  for (const key of Object.keys(playwrightConfig.shareLinks)) {
+    playwrightConfig.shareLinks[key] = null;
+  }
+}
+
+/**
+ * Read a share link that an earlier workflow step was supposed to populate.
+ * Throws an actionable error instead of letting a `null` reach `page.goto()`,
+ * where it surfaces as the opaque `goto: url: expected string, got object`.
+ */
+export function requireShareLink(key: string): string {
+  const link = playwrightConfig.shareLinks[key];
+  if (!link) {
+    throw new Error(
+      `Missing share link for "${key}" — an upstream test step did not populate it. ` +
+        `This usually means an earlier workflow (share/upload) failed; check that failure first.`
+    );
+  }
+  return link;
+}
+
 export async function downloadFirstFile(page: Page) {
   const { downloadButton, confirmDownload } = fileLocators(page);
   await downloadButton.click();

packages/send/frontend/src/lib/init.ts

@@ -1,7 +1,11 @@
 import { INIT_ERRORS } from '@send-frontend/apps/send/const';
 import { FolderStore } from '@send-frontend/apps/send/stores/folder-store.types';
-import { Keychain } from '@send-frontend/lib/keychain';
+import {
+  Keychain,
+  restoreKeysUsingLocalStorage,
+} from '@send-frontend/lib/keychain';
 import { useFolderStore } from '@send-frontend/stores';
+import useApiStore from '@send-frontend/stores/api-store';
 import { UserStoreType as UserStore } from '@send-frontend/stores/user-store';
 
 /**
@@ -11,7 +15,7 @@ import { UserStoreType as UserStore } from '@send-frontend/stores/user-store';
  * @param {FolderStore} folderStore - Pinia store for managing folders.
  * @return {Promise<INIT_ERRORS>} - Returns Promise of 0 (success) or an error code typed by INIT_ERRORS.
  */
-async function init(
+async function _init(
   userStore: UserStore,
   keychain: Keychain,
   folderStore: FolderStore | ReturnType<typeof useFolderStore>
@@ -27,6 +31,18 @@ async function init(
     return INIT_ERRORS.NO_KEYCHAIN;
   }
 
+  // Ensure container keys are restored from the server backup before deciding a
+  // default folder is "orphaned". keychain.load() above only reads local storage;
+  // a valid folder's key may live only in the server backup and would otherwise
+  // appear missing here, causing us to destroy a perfectly good container.
+  // This is idempotent and no-ops when no passphrase is available.
+  try {
+    const { api } = useApiStore();
+    await restoreKeysUsingLocalStorage(keychain, api);
+  } catch (error) {
+    console.warn('init(): could not restore keys before folder check', error);
+  }
+
   await folderStore.sync();
   const defaultFolder = folderStore?.defaultFolder;
   const defaultFolderKeyIsMissing =
@@ -49,4 +65,23 @@ async function init(
   return INIT_ERRORS.NONE;
 }
 
+// Single-flight guard: concurrent callers (e.g. the login flow and dbUserSetup
+// firing during the same rapid navigation) share one run instead of each
+// independently deciding to delete + recreate the default folder. See issue #930.
+let inFlight: Promise<INIT_ERRORS> | null = null;
+
+function init(
+  userStore: UserStore,
+  keychain: Keychain,
+  folderStore: FolderStore | ReturnType<typeof useFolderStore>
+): Promise<INIT_ERRORS> {
+  if (inFlight) {
+    return inFlight;
+  }
+  inFlight = _init(userStore, keychain, folderStore).finally(() => {
+    inFlight = null;
+  });
+  return inFlight;
+}
+
 export default init;