fix(credentials): close secret-leak gap in temp-credential cleanup + regression test (#7160)

Co-authored-by: AronPerez <aperez0295@gmail.com>

skyvern-frontend/src/routes/credentials/CredentialsModal.test.tsx

@@ -12,18 +12,24 @@ import { AxiosError, AxiosHeaders } from "axios";
 import { MemoryRouter } from "react-router-dom";
 import { afterEach, describe, expect, it, vi } from "vitest";
 
+import type { CredentialApiResponse } from "@/api/types";
+
 import { getAuthenticatorKeyError } from "./credentialTotpValidation";
 import { CredentialsModal } from "./CredentialsModal";
 import { CredentialModalTypes } from "./useCredentialModalState";
 
 const postMock = vi.hoisted(() => vi.fn());
+const patchMock = vi.hoisted(() => vi.fn());
+const deleteMock = vi.hoisted(() => vi.fn());
+const getMock = vi.hoisted(() => vi.fn());
 const toastMock = vi.hoisted(() => vi.fn());
 
 vi.mock("@/api/AxiosClient", () => ({
   getClient: vi.fn(async () => ({
     post: postMock,
-    patch: vi.fn(),
-    delete: vi.fn(),
+    patch: patchMock,
+    delete: deleteMock,
+    get: getMock,
   })),
 }));
 
@@ -75,6 +81,46 @@ function renderPasswordCredentialsModal() {
   );
 }
 
+const editingPasswordCredential: CredentialApiResponse = {
+  credential_id: "real-cred-id",
+  credential_type: "password",
+  name: "Acme Login",
+  credential: {
+    username: "user@example.com",
+    totp_type: "none",
+    totp_identifier: null,
+  },
+  browser_profile_id: "existing-profile-id",
+  tested_url: "https://example.com/login",
+  user_context: null,
+  save_browser_session_intent: true,
+  folder_id: null,
+  proxy_location: null,
+  proxy_session_id: null,
+};
+
+function renderEditPasswordCredentialsModal() {
+  const queryClient = new QueryClient({
+    defaultOptions: {
+      queries: { retry: false },
+      mutations: { retry: false },
+    },
+  });
+  return render(
+    <QueryClientProvider client={queryClient}>
+      <MemoryRouter>
+        <CredentialsModal
+          isOpen
+          onOpenChange={vi.fn()}
+          overrideType={CredentialModalTypes.PASSWORD}
+          editingCredential={editingPasswordCredential}
+          onStartBackgroundTest={vi.fn()}
+        />
+      </MemoryRouter>
+    </QueryClientProvider>,
+  );
+}
+
 afterEach(() => {
   cleanup();
   vi.clearAllMocks();
@@ -210,3 +256,77 @@ describe("CredentialsModal authenticator save errors", () => {
     expect(toastMock).not.toHaveBeenCalled();
   }, 10_000);
 });
+
+describe("CredentialsModal edit-mode inline test", () => {
+  it("updates the real credential and deletes the temp one instead of renaming the temp credential in place, even if 'save browser session' gets unchecked before saving", async () => {
+    // 1st POST: startTest's /credentials/test-login. 2nd POST: the real
+    // credential's /credentials/{id}/update once Save is clicked.
+    postMock
+      .mockResolvedValueOnce({
+        data: { credential_id: "temp-cred-id", workflow_run_id: "wr-1" },
+      })
+      .mockResolvedValueOnce({
+        data: { credential_id: "real-cred-id", name: "Acme Login" },
+      });
+    getMock.mockResolvedValueOnce({
+      data: {
+        status: "completed",
+        browser_profile_id: "new-profile-id",
+        tested_url: "https://example.com/login",
+      },
+    });
+    patchMock.mockResolvedValue({ data: {} });
+    deleteMock.mockResolvedValue({ data: {} });
+
+    renderEditPasswordCredentialsModal();
+
+    fireEvent.click(screen.getAllByLabelText("Edit credential values")[0]!);
+    const passwordInput = document.querySelector('input[type="password"]');
+    expect(passwordInput).toBeTruthy();
+    fireEvent.change(passwordInput as HTMLInputElement, {
+      target: { value: "rotated-password" },
+    });
+
+    fireEvent.click(screen.getByRole("button", { name: "Test" }));
+
+    // Real 3s poll delay inside the component — wait for the button label
+    // to flip once testStatus reaches "completed".
+    await waitFor(
+      () => {
+        expect(screen.getByRole("button", { name: "Retest" })).toBeTruthy();
+      },
+      { timeout: 8000 },
+    );
+
+    // Uncheck "Save browser session" after the test completed — the checkbox
+    // has no side effect on testStatus/testCredentialId, so this must not be
+    // able to skip cleanup of the now-orphaned temp credential.
+    fireEvent.click(
+      screen.getByLabelText("Save browser session for future logins"),
+    );
+
+    fireEvent.click(screen.getByRole("button", { name: "Update" }));
+
+    await waitFor(() => {
+      expect(deleteMock).toHaveBeenCalledWith("/credentials/temp-cred-id");
+    });
+    await waitFor(() => {
+      expect(postMock).toHaveBeenCalledWith(
+        "/credentials/real-cred-id/update",
+        expect.objectContaining({
+          credential: expect.objectContaining({
+            password: "rotated-password",
+          }),
+        }),
+      );
+    });
+
+    // The bug this guards against: the old code renamed the throwaway temp
+    // credential in place of updating the real one. renameCredentialMutation
+    // is the only path that PATCHes a credential by the *temp* id — assert
+    // it never fired.
+    for (const call of patchMock.mock.calls) {
+      expect(call[0]).not.toBe("/credentials/temp-cred-id");
+    }
+  }, 15_000);
+});

skyvern-frontend/src/routes/credentials/CredentialsModal.tsx

@@ -1019,12 +1019,14 @@ function CredentialsModal({
         return;
       }
 
-      // Edit mode: the inline test's profile is tied to the throwaway temp
-      // credential, so we must overwrite the real credential's secret (below) and
-      // re-run the test against it to attach a fresh profile. Delete the orphaned
-      // temp credential — onSuccess resets the refs before the dialog close
-      // handler would otherwise clean it up.
-      if (isEditMode && hasCompletedInlineTest && testCredentialId) {
+      // Any remaining temp credential is an orphan we're not reusing — either
+      // we're in edit mode (the real credential gets updated below instead),
+      // or "Save browser session" got unchecked after a completed test. Key
+      // this on testCredentialId alone, not hasCompletedInlineTest: the
+      // checkbox is togglable post-test with no side effect on testStatus, so
+      // gating on testAndSave here silently leaked the temp credential (and
+      // the secret it holds) whenever the box was unchecked before saving.
+      if (testCredentialId) {
         const tempCredentialId = testCredentialId;
         getClient(credentialGetter)
           .then((client) => client.delete(`/credentials/${tempCredentialId}`))