From 71cb7f3eae5cfc305cf406f2e82805b13a4c4758 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 14:05:13 -0400 Subject: [PATCH] feat(web): state the file store as a sentence, and copy its path on click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The footer's file states now read "Secrets: Plaintext file. Owner-only permissions." and "Secrets: Encrypted file. Owner-only permissions.", and the band itself became the copy affordance for the path. Three things worth reviewing: - The path is no longer printed on the band. It goes to the clipboard instead, which means the only place it stays *readable* is the button's accessible name — so that carries it in full ("Copy secrets file path: /home/node/.mcp-inspector/secrets.json") rather than a generic "Copy". A clipboard is no use to a screen-reader user who cannot inspect it. - "Owner-only permissions." is a claim of fact, so it is made only when the mode was verified as 0600. `looseMode` is set exactly when it is something else and could not be tightened, and the message says "Mode 0644 — not owner-only." there. A footer that asserted the opposite of the truth about a secrets file is the one failure this band exists to prevent, so it has its own test. - The footer phrases its own copy rather than reusing `secretStorageLabel` for the file case: that label is parenthetical ("File (unencrypted)") because the startup banner appends " at " to it. The tone and the underlying facts still come from the shared helpers, so the two surfaces cannot disagree about anything that matters. Memory's line is still derived from the shared caveat. The click target is an `UnstyledButton` filling the band edge to edge — a real control, reachable by keyboard and announced as a button, rather than a click handler on a div. Padding moved from the band to the inner row so the button has no dead margins. Signed-off-by: cliffhall --- .../SecretStorageFooter.stories.tsx | 14 +- .../SecretStorageFooter.test.tsx | 71 +++++-- .../SecretStorageFooter.tsx | 194 +++++++++++++----- .../ClientSettingsModal.test.tsx | 4 +- .../core/auth/secret-storage-info.test.ts | 4 +- 5 files changed, 216 insertions(+), 71 deletions(-) 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", () => {