From 4b24e0c6473b7cdb41d5f9a842c29b681546d91d Mon Sep 17 00:00:00 2001 From: Camiel van Schoonhoven Date: Thu, 22 Oct 2026 14:19:17 -0700 Subject: [PATCH] feat(secrets): harden the secret form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes to the add and replace form, carried over from the superseded workspace-secrets draft (#2886). They apply to Settings → Secrets today and to the team secrets screens that follow. - The value field sets `autocomplete="new-password"`, so browsers stop offering a saved login password for it. - A name containing `/` is refused. The name is a path segment in the replace and delete requests, so such a secret could be created but never changed or removed. - Deleting asks first. A deleted secret makes every run that uses it fail at launch, and once secrets are shared with a team that is everyone's runs. - Description and expiry fields, using the expiry picker from #2886. The backend only writes either when a value is sent, so neither can be cleared once set: replacing keeps the current description and expiry unless a new one is given, and the form says so. Expiry is not enforced by the backend, so the form calls it a reminder. The Settings list shows a secret's description under its name. The E2E `removeSecret` helper now confirms the delete dialog. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../components/AddSecretButton.tsx | 2 +- .../components/AddSecretForm.test.tsx | 210 ++++++++++++++++++ .../components/AddSecretForm.tsx | 106 ++++++++- .../components/ExpiryDatePicker.tsx | 84 +++++++ .../components/RemoveSecretButton.test.tsx | 73 ++++++ .../components/RemoveSecretButton.tsx | 30 ++- .../components/SecretsList.tsx | 18 +- .../components/UpdateSecretButton.tsx | 2 +- .../shared/SecretsManagement/types.ts | 6 +- tests/e2e/helpers.ts | 4 + 10 files changed, 507 insertions(+), 28 deletions(-) create mode 100644 src/components/shared/SecretsManagement/components/AddSecretForm.test.tsx create mode 100644 src/components/shared/SecretsManagement/components/ExpiryDatePicker.tsx create mode 100644 src/components/shared/SecretsManagement/components/RemoveSecretButton.test.tsx diff --git a/src/components/shared/SecretsManagement/components/AddSecretButton.tsx b/src/components/shared/SecretsManagement/components/AddSecretButton.tsx index 0a74a15130..da7597f29b 100644 --- a/src/components/shared/SecretsManagement/components/AddSecretButton.tsx +++ b/src/components/shared/SecretsManagement/components/AddSecretButton.tsx @@ -9,7 +9,7 @@ import type { Secret } from "../types"; import { SecretsQueryKeys } from "../types"; interface AddSecretButtonProps { - secret: Pick; + secret: Pick; disabled?: boolean; onSuccess: () => void; } diff --git a/src/components/shared/SecretsManagement/components/AddSecretForm.test.tsx b/src/components/shared/SecretsManagement/components/AddSecretForm.test.tsx new file mode 100644 index 0000000000..a106b79a53 --- /dev/null +++ b/src/components/shared/SecretsManagement/components/AddSecretForm.test.tsx @@ -0,0 +1,210 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import type { ReactNode } from "react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { toEndOfDayUTC } from "@/utils/date"; + +import { addSecret, updateSecret } from "../secretsStorage"; +import type { Secret } from "../types"; +import { AddSecretForm } from "./AddSecretForm"; + +vi.mock("../secretsStorage", () => ({ + addSecret: vi.fn(), + updateSecret: vi.fn(), +})); + +vi.mock("@/hooks/useToastNotification", () => ({ default: () => vi.fn() })); + +class ResizeObserverMock { + observe = vi.fn(); + unobserve = vi.fn(); + disconnect = vi.fn(); +} +vi.stubGlobal("ResizeObserver", ResizeObserverMock); + +const existing: Secret = { + id: "API_KEY", + name: "API_KEY", + description: "Model access", + createdAt: new Date("2026-10-01T09:00:00Z"), + updatedAt: new Date("2026-10-02T09:00:00Z"), +}; + +function renderForm(existingSecret?: Secret) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + const onSuccess = vi.fn(); + render( + , + { + wrapper: ({ children }: { children: ReactNode }) => ( + + {children} + + ), + }, + ); + return { user: userEvent.setup(), onSuccess }; +} + +const field = (label: string) => screen.getByLabelText(label); + +describe("AddSecretForm", () => { + beforeEach(() => { + vi.mocked(addSecret).mockResolvedValue(true); + vi.mocked(updateSecret).mockResolvedValue(true); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe("adding", () => { + it("adds a secret with a trimmed name and its description", async () => { + const { user, onSuccess } = renderForm(); + + await user.type(field("Secret Name"), " API_KEY "); + await user.type(field("Secret Value"), "s3cret"); + await user.type(field("Description (optional)"), " Model access "); + await user.click(screen.getByRole("button", { name: "Add Secret" })); + + await waitFor(() => expect(onSuccess).toHaveBeenCalled()); + expect(addSecret).toHaveBeenCalledWith({ + name: "API_KEY", + value: "s3cret", + description: "Model access", + expiresAt: undefined, + }); + }); + + it("leaves out a description that was never written", async () => { + const { user } = renderForm(); + + await user.type(field("Secret Name"), "API_KEY"); + await user.type(field("Secret Value"), "s3cret"); + await user.click(screen.getByRole("button", { name: "Add Secret" })); + + await waitFor(() => + expect(addSecret).toHaveBeenCalledWith( + expect.objectContaining({ description: undefined }), + ), + ); + }); + + it("refuses a name with a slash in it", async () => { + const { user } = renderForm(); + + await user.type(field("Secret Name"), "team/API_KEY"); + await user.type(field("Secret Value"), "s3cret"); + + expect(screen.getByText("Secret name cannot contain /")).toBeVisible(); + expect(field("Secret Name")).toHaveAttribute("aria-invalid", "true"); + expect(screen.getByRole("button", { name: "Add Secret" })).toBeDisabled(); + }); + + it("keeps the browser from filling the value with a saved password", () => { + renderForm(); + + expect(field("Secret Value")).toHaveAttribute("type", "password"); + expect(field("Secret Value")).toHaveAttribute( + "autocomplete", + "new-password", + ); + }); + + it("sends the expiry as the end of the chosen day in UTC", async () => { + const now = new Date(); + const chosen = new Date(now.getFullYear(), now.getMonth() + 1, 15); + const { user } = renderForm(); + + await user.type(field("Secret Name"), "API_KEY"); + await user.type(field("Secret Value"), "s3cret"); + await user.click(field("Expires (optional)")); + await user.click(screen.getByRole("button", { name: /next month/i })); + await user.click(screen.getByRole("button", { name: /15th/ })); + await user.click(screen.getByRole("button", { name: "Add Secret" })); + + await waitFor(() => + expect(addSecret).toHaveBeenCalledWith( + expect.objectContaining({ + expiresAt: new Date(toEndOfDayUTC(chosen)), + }), + ), + ); + }); + + it("says the expiry is only a reminder", () => { + renderForm(); + + expect(field("Expires (optional)")).toHaveAccessibleDescription( + "A reminder only. The secret keeps working after this date.", + ); + }); + }); + + describe("replacing", () => { + it("starts from the secret's description", () => { + renderForm(existing); + + expect(field("Secret Name")).toBeDisabled(); + expect(field("Description (optional)")).toHaveValue("Model access"); + expect(field("Description (optional)")).toHaveAccessibleDescription( + "It can be changed but not removed.", + ); + }); + + it("sends only the new value when nothing else changed", async () => { + const { user, onSuccess } = renderForm(existing); + + await user.type(field("New Secret Value"), "n3w"); + await user.click(screen.getByRole("button", { name: "Update Secret" })); + + await waitFor(() => expect(onSuccess).toHaveBeenCalled()); + expect(updateSecret).toHaveBeenCalledWith("API_KEY", { + id: "API_KEY", + value: "n3w", + description: undefined, + expiresAt: undefined, + }); + }); + + it("sends a description that was changed", async () => { + const { user } = renderForm(existing); + + await user.type(field("New Secret Value"), "n3w"); + await user.clear(field("Description (optional)")); + await user.type(field("Description (optional)"), "Agent model access"); + await user.click(screen.getByRole("button", { name: "Update Secret" })); + + await waitFor(() => + expect(updateSecret).toHaveBeenCalledWith( + "API_KEY", + expect.objectContaining({ description: "Agent model access" }), + ), + ); + }); + + it("names the current expiry and keeps it when left empty", () => { + renderForm({ ...existing, expiresAt: new Date("2027-03-03T23:59:59Z") }); + + expect(field("Expires (optional)")).toHaveAccessibleDescription( + "Currently Mar 3, 2027. Leave empty to keep that date.", + ); + }); + + it("still needs a new value", () => { + renderForm(existing); + + expect( + screen.getByRole("button", { name: "Update Secret" }), + ).toBeDisabled(); + }); + }); +}); diff --git a/src/components/shared/SecretsManagement/components/AddSecretForm.tsx b/src/components/shared/SecretsManagement/components/AddSecretForm.tsx index d68b3dcc4c..48a89fac01 100644 --- a/src/components/shared/SecretsManagement/components/AddSecretForm.tsx +++ b/src/components/shared/SecretsManagement/components/AddSecretForm.tsx @@ -4,16 +4,21 @@ import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { Label } from "@/components/ui/label"; import { BlockStack, InlineStack } from "@/components/ui/layout"; +import { Textarea } from "@/components/ui/textarea"; import { Text } from "@/components/ui/typography"; import { cn } from "@/lib/utils"; +import { formatDate, parseUTCAsLocalDate, toEndOfDayUTC } from "@/utils/date"; -import { isValidSecretName, isValidSecretValue, type Secret } from "../types"; +import { isValidSecretValue, type Secret, secretNameProblem } from "../types"; import { AddSecretButton } from "./AddSecretButton"; +import { EXPIRY_FORMAT, ExpiryDatePicker } from "./ExpiryDatePicker"; import { UpdateSecretButton } from "./UpdateSecretButton"; interface AddSecretFormState { name: string; value: string; + description: string; + expiresOn: Date | undefined; nameError: string | null; valueError: string | null; } @@ -21,6 +26,8 @@ interface AddSecretFormState { type AddSecretFormAction = | { type: "SET_NAME"; payload: string } | { type: "SET_VALUE"; payload: string } + | { type: "SET_DESCRIPTION"; payload: string } + | { type: "SET_EXPIRES_ON"; payload: Date | undefined } | { type: "RESET" }; function formReducer( @@ -30,10 +37,7 @@ function formReducer( switch (action.type) { case "SET_NAME": { const name = action.payload; - const nameError = - name.length > 0 && !isValidSecretName(name) - ? "Secret name cannot be empty" - : null; + const nameError = name.length > 0 ? secretNameProblem(name) : null; return { ...state, name, nameError }; } case "SET_VALUE": { @@ -44,6 +48,10 @@ function formReducer( : null; return { ...state, value, valueError }; } + case "SET_DESCRIPTION": + return { ...state, description: action.payload }; + case "SET_EXPIRES_ON": + return { ...state, expiresOn: action.payload }; case "RESET": return initialFormState; default: @@ -54,10 +62,23 @@ function formReducer( const initialFormState: AddSecretFormState = { name: "", value: "", + description: "", + expiresOn: undefined, nameError: null, valueError: null, }; +function expiryHint(existingSecret?: Secret): string { + if (!existingSecret?.expiresAt) { + return "A reminder only. The secret keeps working after this date."; + } + const currentExpiry = formatDate( + parseUTCAsLocalDate(existingSecret.expiresAt.toISOString()), + EXPIRY_FORMAT, + ); + return `Currently ${currentExpiry}. Leave empty to keep that date.`; +} + interface AddSecretFormProps { existingSecret?: Secret; onSuccess: () => void; @@ -69,10 +90,11 @@ export function AddSecretForm({ onSuccess, onCancel, }: AddSecretFormProps) { + const originalDescription = existingSecret?.description ?? ""; const [state, dispatch] = useReducer(formReducer, { ...initialFormState, name: existingSecret?.name ?? "", - value: "", + description: originalDescription, }); const handleNameChange = (e: ChangeEvent) => { @@ -83,13 +105,29 @@ export function AddSecretForm({ dispatch({ type: "SET_VALUE", payload: e.target.value }); }; + const handleDescriptionChange = (e: ChangeEvent) => { + dispatch({ type: "SET_DESCRIPTION", payload: e.target.value }); + }; + + const handleExpiresOnChange = (date: Date | undefined) => { + dispatch({ type: "SET_EXPIRES_ON", payload: date }); + }; + + const isReplace = !!existingSecret; + const isFormValid = - isValidSecretName(state.name) && + (isReplace || secretNameProblem(state.name) === null) && isValidSecretValue(state.value) && - !state.nameError && !state.valueError; - const isReplace = !!existingSecret; + const trimmedDescription = state.description.trim(); + const description = + trimmedDescription !== "" && trimmedDescription !== originalDescription + ? trimmedDescription + : undefined; + const expiresAt = state.expiresOn + ? new Date(toEndOfDayUTC(state.expiresOn)) + : undefined; return ( @@ -101,6 +139,8 @@ export function AddSecretForm({ value={state.name} onChange={handleNameChange} disabled={isReplace} + autoComplete="off" + spellCheck={false} aria-invalid={!!state.nameError} className={cn(state.nameError && "border-destructive")} data-testid="secret-name-input" @@ -122,6 +162,8 @@ export function AddSecretForm({ placeholder="Enter secret value..." value={state.value} onChange={handleValueChange} + autoComplete="new-password" + spellCheck={false} aria-invalid={!!state.valueError} className={cn(state.valueError && "border-destructive")} data-testid="secret-value-input" @@ -136,6 +178,38 @@ export function AddSecretForm({ + + +