diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx index 38d7a245..2962d601 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx @@ -45,9 +45,15 @@ export const EncryptedFile: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); const band = await canvas.findByTestId("secret-storage-footer"); - await expect(band).toHaveTextContent("Secrets: File (encrypted)"); + await expect(band).toHaveTextContent( + "Secrets: Encrypted file. Owner-only permissions.", + ); + // The path is no longer printed on the band — it goes to the clipboard, + // and stays readable in the button's accessible name. await expect( - canvas.getByText("/home/node/.mcp-inspector/secrets.json"), + canvas.getByRole("button", { + name: "Copy secrets file path: /home/node/.mcp-inspector/secrets.json", + }), ).toBeInTheDocument(); }, }; @@ -68,7 +74,9 @@ export const UnencryptedFile: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); const band = await canvas.findByTestId("secret-storage-footer"); - await expect(band).toHaveTextContent("Secrets: File (unencrypted)"); + await expect(band).toHaveTextContent( + "Secrets: Plaintext file. Owner-only permissions.", + ); await expect(band).toHaveAttribute("data-tone", "warn"); }, }; diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx index a18a7d0b..d8b836da 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx @@ -1,4 +1,5 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { SecretStorageFooter } from "./SecretStorageFooter"; @@ -34,31 +35,69 @@ describe("SecretStorageFooter", () => { expect(band).toHaveAttribute("data-tone", "neutral"); }); - it("shows the path for an encrypted file, quietly", () => { - // Nothing is wrong here, so the band must not shout — the path is the - // useful part and the tone stays neutral. + it("states an encrypted file quietly, and offers its path for copying", () => { + // Nothing is wrong here, so the band must not shout. The path is no longer + // printed on the band — it goes to the clipboard — so the accessible name + // is where it stays readable. renderWithMantine(); const band = screen.getByTestId("secret-storage-footer"); - expect(band).toHaveTextContent("Secrets: File (encrypted)"); - expect( - screen.getByText("/home/node/.mcp-inspector/secrets.json"), - ).toBeInTheDocument(); + expect(band).toHaveTextContent( + "Secrets: Encrypted file. Owner-only permissions.", + ); expect(band).toHaveAttribute("data-tone", "neutral"); + expect( + screen.getByRole("button", { + name: "Copy secrets file path: /home/node/.mcp-inspector/secrets.json", + }), + ).toBeInTheDocument(); }); - it("renders the plaintext caveat inline, not only on hover", () => { - // This is the "loud banner" decision made concrete: a warning you have - // to hover to discover is not a warning. The caveat takes the visible - // slot, displacing the path (which stays in the tooltip). + it("says a plaintext file is plaintext, in the warning tone", () => { + // The "loud warning" decision made concrete: the state is on the band + // itself, not hidden behind a hover. renderWithMantine(); const band = screen.getByTestId("secret-storage-footer"); - expect(band).toHaveTextContent("Secrets: File (unencrypted)"); - expect( - screen.getByText(/Set MCP_INSPECTOR_SECRET_KEY to encrypt them/), - ).toBeInTheDocument(); + expect(band).toHaveTextContent( + "Secrets: Plaintext file. Owner-only permissions.", + ); expect(band).toHaveAttribute("data-tone", "warn"); }); + it("copies the path on click", async () => { + const user = userEvent.setup(); + const writeText = vi.fn().mockResolvedValue(undefined); + // happy-dom has no clipboard; define one so the copy path is exercised + // rather than merely not throwing. + Object.defineProperty(navigator, "clipboard", { + value: { writeText }, + configurable: true, + }); + renderWithMantine(); + await user.click(screen.getByRole("button", { name: /Copy secrets file/ })); + expect(writeText).toHaveBeenCalledWith( + "/home/node/.mcp-inspector/secrets.json", + ); + }); + + it("is inert for a store with no file to point at", () => { + // A keychain or in-memory store has no path, so there is nothing to copy + // and the band must not pretend to be a control. + renderWithMantine(); + expect(screen.queryByRole("button")).toBeNull(); + }); + + it("declines to claim owner-only permissions when the mode says otherwise", () => { + // The one assertion the band must never get wrong: `looseMode` is set + // exactly when 0600 could not be reached, so "Owner-only permissions." + // there would state the opposite of the truth. + renderWithMantine( + , + ); + const band = screen.getByTestId("secret-storage-footer"); + expect(band).toHaveTextContent("Mode 0644 — not owner-only."); + expect(band).not.toHaveTextContent("Owner-only permissions."); + }); + it("warns that an in-memory store loses secrets on exit", () => { renderWithMantine(); const band = screen.getByTestId("secret-storage-footer"); diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 74e81039..29645107 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -1,4 +1,11 @@ -import { Group, Text, Tooltip } from "@mantine/core"; +import type { ReactNode } from "react"; +import { + CopyButton, + Group, + Text, + Tooltip, + UnstyledButton, +} from "@mantine/core"; import { TbAlertTriangle, TbDeviceDesktop, TbLock } from "react-icons/tb"; import { secretStorageCaveat, @@ -34,17 +41,50 @@ export interface SecretStorageFooterProps { const FooterBand = Group.withProps({ variant: "stickyModalFooter", h: SECRET_STORAGE_FOOTER_HEIGHT, - px: "md", - gap: 6, + gap: 0, align: "center", wrap: "nowrap", w: "100%", }); -const FooterLabel = Text.withProps({ +// The icon + message row. Carries the horizontal padding (rather than the band) +// so that when the row is a copy button it covers the band edge to edge — a +// click target with dead margins is a click target people miss. +const FooterRow = Group.withProps({ + h: "100%", + w: "100%", + px: "md", + gap: 6, + align: "center", + wrap: "nowrap", +}); + +// The whole strip becomes the copy affordance for a file-backed store. An +// `UnstyledButton` rather than a click handler on the Group: this is a real +// action, so it needs to be reachable by keyboard and announced as a button. +const CopyTarget = UnstyledButton.withProps({ + h: "100%", + w: "100%", +}); + +// The whole statement, prefix included, as one clamped line. +// +// `lineClamp` rather than `truncate`, and the difference is load-bearing. +// `Modal.Content` sizes its scroll container to its children's **max-content** +// width, and `truncate` sets `white-space: nowrap`, whose max-content is the +// entire sentence — so the band grew the container to 770px inside a 620px +// modal and the *form above it* got clipped at the right edge (measured). +// Clamped text still wraps internally, so its max-content is merely its +// longest word, and the container stays the modal's width; the clamp then +// keeps it to the one line this 32px band has room for. `miw: 0` + `flex: 1` +// let it shrink to the leftover space rather than to its content. +const FooterMessage = Text.withProps({ size: "xs", fw: 500, - variant: "nowrap", + lineClamp: 1, + miw: 0, + flex: 1, + ta: "left", }); // "Secrets:" carries the heavier weight so the band reads as a labelled fact @@ -63,25 +103,6 @@ const FooterLabelPrefix = Text.withProps({ fw: 700, }); -// The detail — a path, or the caveat sentence — is the part allowed to lose -// characters, since the label before it carries the answer. -// -// `lineClamp` rather than `truncate`, and the difference is load-bearing. -// `Modal.Content` sizes its scroll container to its children's **max-content** -// width, and `truncate` sets `white-space: nowrap`, whose max-content is the -// entire sentence — so the band grew the container to 770px inside a 620px -// modal and the *form above it* got clipped at the right edge (measured). -// Clamped text still wraps internally, so its max-content is merely its -// longest word, and the container stays the modal's width; the clamp then -// keeps it to the one line this 32px band has room for. `miw: 0` + `flex: 1` -// let it shrink to the leftover space rather than to its content. -const FooterDetail = Text.withProps({ - size: "xs", - lineClamp: 1, - miw: 0, - flex: 1, -}); - // `multiline` + a fixed width so the sentence wraps instead of forming a // tooltip wider than the modal it explains. Only the `label` is dynamic. const CaveatTooltip = Tooltip.withProps({ @@ -93,6 +114,70 @@ const CaveatTooltip = Tooltip.withProps({ const ICON_SIZE = 14; +/** Octal mode, as `stat` would print it (`0644`). */ +const formatMode = (mode: number): string => mode.toString(8).padStart(4, "0"); + +/** + * The visible statement, per store. + * + * Deliberately not built from `secretStorageLabel` for the file case: that + * label is parenthetical ("File (unencrypted)") because the startup banner + * appends " at " to it, and the footer wants a sentence instead. The + * two surfaces have different shapes, so each phrases its own; the *tone* + * and the underlying facts still come from the shared helpers, which is + * what keeps them from disagreeing about anything that matters. + */ +function footerMessage(info: SecretStorageInfo): string { + if (info.kind === "keyring") return secretStorageLabel(info); + if (info.kind === "memory") { + // Derived rather than restated — memory's consequence is the caveat, and + // the banner prints the same sentence. + const caveat = secretStorageCaveat(info); + return caveat + ? `${secretStorageLabel(info)}: ${caveat}` + : secretStorageLabel(info); + } + const encryption = info.plaintext ? "Plaintext" : "Encrypted"; + // "Owner-only permissions." is a claim of fact, so it is only made when the + // mode was actually verified as 0600. `looseMode` is set precisely when it + // is something else and could not be tightened, and saying "owner-only" + // there would be the footer asserting the opposite of what is true — the + // one failure this whole band exists to prevent. + const permissions = + info.looseMode === undefined + ? "Owner-only permissions." + : `Mode ${formatMode(info.looseMode)} — not owner-only.`; + return `${encryption} file. ${permissions}`; +} + +/** + * The hover text, per store. Undefined leaves the band without a tooltip. + * + * For a file-backed store it always ends with the copy hint, because the + * path is no longer shown on the band — it is in the clipboard instead, and + * an affordance nobody knows about is not one. + */ +function footerTooltip(info: SecretStorageInfo): string | undefined { + if (info.kind !== "file" || !info.path) return undefined; + const parts: string[] = []; + if (info.plaintext) { + // Advice that can actually clear the condition: someone who has already + // set the passphrase is waiting on the next write, not on themselves. + parts.push( + info.pendingEncryption + ? "Re-encrypted the next time a secret is saved." + : "Set MCP_INSPECTOR_SECRET_KEY to encrypt.", + ); + } + if (info.looseMode !== undefined) { + parts.push( + `Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — anyone who can read the file can read the secrets in it.`, + ); + } + parts.push("Click to copy file path"); + return parts.join(" "); +} + /** * The permanent "here is where your secrets go" strip at the bottom of the * Client Settings and Server Settings modals (#1950). @@ -121,46 +206,59 @@ const ICON_SIZE = 14; export function SecretStorageFooter({ info }: SecretStorageFooterProps) { if (!info) return null; - const caveat = secretStorageCaveat(info); const warn = secretStorageTone(info) === "warn"; const color = warn ? "var(--inspector-warning-text)" : "var(--inspector-text-secondary)"; - // The caveat wins the visible slot when there is one; otherwise the path, - // which only the (quiet) encrypted-file case has. - const detail = caveat ?? info.path; const Icon = warn ? TbAlertTriangle : info.kind === "keyring" ? TbDeviceDesktop : TbLock; - const band = ( - + const row = ( + {/* A react-icons glyph, not a Mantine factory component — outside the `.withProps()` rule, and it takes `color` as a prop rather than a style. */} - {/* The colon lives here rather than in `secretStorageLabel`, because the - label is also used on its own — by `secretStorageSummary` for the - startup banner, where a trailing colon before " at " would be - wrong. It appears only when something follows it. */} - - Secrets:{" "} - {secretStorageLabel(info)} - {detail ? ":" : ""} - - {detail && {detail}} + + Secrets: {footerMessage(info)} + + + ); + + const tip = footerTooltip(info); + const band = (content: ReactNode) => ( + + {content} ); - // Tooltip content: the caveat plus the path when both exist (the unencrypted - // file case), since the visible slot can only show one of them. - const tip = [caveat, info.path].filter(Boolean).join(" "); - if (!tip) return band; + // A keychain or in-memory store has no path, so there is nothing to copy and + // the band stays inert — no button, no tooltip beyond what it already shows. + if (!info.path) return band(row); - return {band}; + return ( + + {({ copied, copy }) => + band( + + {/* The accessible name carries the path itself: a screen-reader + user gets no benefit from a clipboard they cannot inspect, so + the one place the location is still *readable* is here. */} + + {row} + + , + ) + } + + ); } diff --git a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx index 097165c5..927b5c2e 100644 --- a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx @@ -495,7 +495,9 @@ describe("ClientSettingsModal", () => { // Full-text assertion: the "Secrets:" prefix is its own ``, which // `getByText`'s direct-text-node matching would not see as one string. const band = screen.getByTestId("secret-storage-footer"); - expect(band).toHaveTextContent("Secrets: File (unencrypted)"); + expect(band).toHaveTextContent( + "Secrets: Plaintext file. Owner-only permissions.", + ); expect(band).toHaveAttribute("data-tone", "warn"); }); diff --git a/clients/web/src/test/core/auth/secret-storage-info.test.ts b/clients/web/src/test/core/auth/secret-storage-info.test.ts index 5bfcd48b..12e9c397 100644 --- a/clients/web/src/test/core/auth/secret-storage-info.test.ts +++ b/clients/web/src/test/core/auth/secret-storage-info.test.ts @@ -76,9 +76,7 @@ describe("secretStorageCaveat", () => { }); it("names the loss for memory", () => { - expect(secretStorageCaveat(memory)).toContain( - "lost on exit", - ); + expect(secretStorageCaveat(memory)).toContain("lost on exit"); }); it("names the fix for an unencrypted file, not just the problem", () => {