From 53a05ac8bdc8f85a6087adfd3859d9fd9ae38d73 Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Mon, 14 Sep 2026 17:33:48 +0200 Subject: [PATCH] fix(manager): a key's new name is a name, and the field is where the key is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renaming a key in the console could not succeed. The field opened pre-filled with the key's current id, which for every DID-bound key is a DID URL — and `keys/rename` will not take one. The agent's gate is `vti_common::identifier::validate_identifier`: `[A-Za-z0-9._-]`, at most 64 bytes, because a key id lands as a store key and a caller who can inject `:` collides with an adjacent keyspace. So the only thing pressing Save could produce was keys/rename/0.1 failed: malformed request: validation error: new_key_id is 86 bytes; maximum is 64 with nothing on screen saying what would have worked. The specification is no help here and cannot be: `keys/rename/0.1` declares `newKeyId` as a non-empty string and leaves the shape to the agent, so the narrower rule is the agent's and this console has to know it. ## The rule, applied before the send `key-name.ts` is the agent's rule, phrased for the person typing — a **pre-flight, not the decision**. The agent still answers and its refusal still renders; nothing here may widen the rule, because a value this accepts and the agent refuses is a message the operator can act on, while the reverse is a name the agent would have taken and the console pretended it would not. A DID-bound key now opens with an **empty** field and a derived name as its *placeholder* — `vdr-host-key-0` from `did:webvh:QmRqx…:webvh.storm.ws:vdr-host#key-0`. A placeholder rather than a value on purpose: a pre-filled suggestion is one press away from a rename nobody chose, and this rename is a one-way door. The id a DID document addresses the key by contains `:` and `#`, so the agent cannot be given it back, and the editor says so rather than leaving it to be discovered. ## And the field is under the key now It rendered inside the actions column, the narrowest in the table, which showed about eight characters of an 86-byte value. `Table` already has `expanded` for exactly this — a detail rendered in the flow beneath the row it belongs to — so the editor goes there, full width, with focus moved into the field and Escape closing it. ## Tests `keys-pane.render.test.mts` is rendered rather than unit, because both defects are ones a person sees and a model does not: the type checker was happy with a field that could only fail, and `key-name.ts`'s own tests would pass with the editor still drawing in a 190px column. `manager-form-state.test.mts`'s prop-seed detector only matched a prop read as the whole `useState` argument, so `useState(nameable ? record.keyId : "")` read as a component with no state at all. Widened to find the prop anywhere in the call — it flags no existing component, which is the point: the narrow match was missing seeds, not tolerating them. `rooms-create.tsx` gains a note on something found while confirming the above and reported upstream: for a templated mint the key record's id and the DID document's verification method disagree by one (`room`/`room-host` number VMs from `#key-1`; the create path saves records from `#key-0`), so the document's `#key-1` names the signing key and the keystore's names the x25519 one. The offer there is by prefix and key type, never by fragment number, which is why it is unaffected. Signed-off-by: Glenn Gore --- packages/extension/src/manager/key-name.ts | 86 ++++++++++++ packages/extension/src/manager/panes/keys.tsx | 128 +++++++++++++++--- .../src/manager/panes/rooms-create.tsx | 12 ++ packages/extension/tests/key-name.test.mts | 78 +++++++++++ .../extension/tests/keys-pane.render.test.mts | 117 ++++++++++++++++ .../tests/manager-form-state.test.mts | 8 +- 6 files changed, 409 insertions(+), 20 deletions(-) create mode 100644 packages/extension/src/manager/key-name.ts create mode 100644 packages/extension/tests/key-name.test.mts create mode 100644 packages/extension/tests/keys-pane.render.test.mts diff --git a/packages/extension/src/manager/key-name.ts b/packages/extension/src/manager/key-name.ts new file mode 100644 index 0000000..9ebdc1c --- /dev/null +++ b/packages/extension/src/manager/key-name.ts @@ -0,0 +1,86 @@ +// What a key may be called, and what the console must say before it renames one. +// +// `keys/rename/0.1` says only that `newKeyId` is a non-empty string — the +// agent's gate is narrower than the specification's, and it is the one that +// answers. `vti_common::identifier::validate_identifier` accepts +// `[A-Za-z0-9._-]` up to 64 bytes and rejects everything else, because a key id +// lands as a store key and a caller who can inject `:` or `/` collides with an +// adjacent keyspace. +// +// So this module exists for the same reason `grant-command.ts` does: the rule +// is the agent's, but a console that does not apply it hands the operator a +// refusal instead of a form. It is a **pre-flight, not the decision** — the +// agent still answers, and its refusal is still rendered. Nothing here may +// widen the rule; a value this module accepts and the agent refuses is a +// message the operator can act on, while the reverse is a name the agent would +// have taken and the console pretended it would not. +// +// The shape that made this necessary: every key this agent mints for a DID is +// named by a DID URL (`did:webvh:…#key-0`), which is 86 bytes of `:` and `#` — +// not a name the rename gate can ever accept. The old editor pre-filled the +// field with exactly that, so the only thing pressing Save could produce was +// `new_key_id is 86 bytes; maximum is 64`. + +/** The agent's cap, in bytes — `validate_identifier`'s `MAX_IDENTIFIER_LEN`. */ +export const MAX_KEY_NAME_BYTES = 64; + +const ALLOWED = /^[A-Za-z0-9._-]+$/; + +/** + * Why the agent would refuse this name, or `null` if it would take it. + * + * Phrased for the operator rather than as the agent's own wording: the agent + * reports bytes and a character class, which is an accurate description of a + * gate and a poor description of what to type instead. + */ +export function keyNameProblem(name: string): string | null { + if (name === "") return "A name cannot be empty."; + // The common mistake is pasting the id that is already on screen. It fails + // the character rule *and* the length one, and neither message says the + // thing worth knowing: a name is not an address. + if (name.startsWith("did:")) { + return "A name is not a DID URL — your agent stores one as a plain label: letters, digits, and . - _ only."; + } + if (!ALLOWED.test(name)) { + return "Your agent accepts letters, digits, and . - _ only."; + } + const bytes = new TextEncoder().encode(name).length; + if (bytes > MAX_KEY_NAME_BYTES) { + return `That is ${bytes} characters; your agent accepts at most ${MAX_KEY_NAME_BYTES}.`; + } + return null; +} + +/** Whether a key's current id is one the agent would accept as a new name — + * false for every DID-bound key, which is most of them. */ +export function isNameable(keyId: string): boolean { + return keyNameProblem(keyId) === null; +} + +/** + * A name to offer for a key that has none — derived from the DID URL it is + * addressed by today. + * + * `did:webvh:Qm…:webvh.storm.ws:vdr-host#key-0` becomes `vdr-host-key-0`: the + * last path segment, which is what a person calls that identity, and the + * fragment, which is what distinguishes its keys from each other. Offered as a + * *placeholder* rather than a value — a pre-filled suggestion is one press away + * from a rename nobody chose, and a rename detaches the key from the document + * that names it. + * + * Returns `""` when nothing usable survives, which the caller must treat as + * "no suggestion" rather than as a name. + */ +export function suggestKeyName(keyId: string): string { + const hash = keyId.indexOf("#"); + const address = hash === -1 ? keyId : keyId.slice(0, hash); + const fragment = hash === -1 ? "" : keyId.slice(hash + 1); + const tail = address.split(":").filter(Boolean).pop() ?? ""; + const joined = [tail, fragment].filter(Boolean).join("-"); + const cleaned = joined + .replace(/[^A-Za-z0-9._-]+/g, "-") + .replace(/-{2,}/g, "-") + .replace(/^[-._]+|[-._]+$/g, ""); + // Cleaned is ASCII, so bytes and characters agree and a slice is safe. + return cleaned.slice(0, MAX_KEY_NAME_BYTES).replace(/[-._]+$/, ""); +} diff --git a/packages/extension/src/manager/panes/keys.tsx b/packages/extension/src/manager/panes/keys.tsx index 57fc04a..ecccf24 100644 --- a/packages/extension/src/manager/panes/keys.tsx +++ b/packages/extension/src/manager/panes/keys.tsx @@ -11,7 +11,7 @@ // key is not an administration task, it is use, and a console that offers a // "sign this" box turns an audit trail of key management into an oracle. -import { useCallback, useState } from "react"; +import { useCallback, useEffect, useId, useRef, useState } from "react"; import { keysCreate, keysList, @@ -29,6 +29,12 @@ import { Loading, LoadError, Table, Truncated, type Column } from "../table.js"; import { useAsync } from "../use-async.js"; import { formatDate } from "../format.js"; import { hasRole, type Authority, type Parties } from "../use-vta.js"; +import { + isNameable, + keyNameProblem, + suggestKeyName, + MAX_KEY_NAME_BYTES, +} from "../key-name.js"; import type { ContextSelection } from "../context-column.js"; const PAGE = 50; @@ -198,37 +204,101 @@ function CreateKey({ ); } -function RenameKey({ +/** + * Rename a key — the editor, which opens beneath the row rather than inside the + * actions column. + * + * Two things this is shaped by, both of which the previous version got wrong. + * + * **A name is not the id.** Every key an agent mints for a DID is addressed by + * a DID URL, and `keys/rename` will not take one back: the agent's gate is + * `[A-Za-z0-9._-]` up to 64 bytes (`key-name.ts`), so the pre-filled current id + * was the one value guaranteed to be refused — an operator pressing Save got + * `new_key_id is 86 bytes; maximum is 64` and no way to read what would work. + * The field now starts empty for such a key, with a suggestion as placeholder, + * and the rule is stated before it is broken. + * + * **And it is a one-way door.** A DID document addresses this key by the id it + * has today; the agent cannot be given that id again, because it contains `:` + * and `#`. So the warning is not decoration — nothing undoes it. + */ +function RenameEditor({ parties, record, + onClose, onDone, }: { parties: Parties; record: KeyRecord; + onClose: () => void; onDone: () => void; }) { - const [open, setOpen] = useState(false); - const [next, setNext] = useState(record.keyId); + const nameable = isNameable(record.keyId); + const [next, setNext] = useState(nameable ? record.keyId : ""); const [busy, setBusy] = useState(false); const [error, setError] = useState(null); const [pending, setPending] = useState(null); + const fieldId = useId(); + const input = useRef(null); - if (!open) { - return ( - - ); - } + // Opened from a button several columns to the right; without this the caret + // is nowhere and the row that just appeared looks like a decoration. + useEffect(() => { + input.current?.focus(); + }, []); + + const typed = next.trim(); + const suggestion = nameable ? "" : suggestKeyName(record.keyId); + // Nothing typed is not a problem to report — it is the state the field opens + // in. Reporting it would put a red line under an empty box. + const problem = typed === "" ? null : keyNameProblem(typed); + const unchanged = typed === record.keyId; return ( -
- setNext(e.target.value)} /> +
+ + setNext(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Escape") onClose(); + }} + /> + + Letters, digits, and . - _ — up to{" "} + {MAX_KEY_NAME_BYTES} characters. Your agent refuses anything else. + {suggestion && ( + <> + {" "} + The field is empty on purpose —{" "} + {suggestion} is a suggestion, not a + value. + + )} + + + {!nameable && ( + + This key is addressed as {record.keyId}, + which is how a DID document names it. A name cannot contain{" "} + : or{" "} + #, so renaming it detaches the key from + that document and your agent will not accept the old id back. + + )} + {problem && {problem}} {error && {error}} {pending && } +
-
@@ -274,6 +344,10 @@ export function KeysPane({ contextHeading?: string | undefined; }) { const [limit, setLimit] = useState(PAGE); + // Which key is being renamed, held here rather than in the row: the editor + // renders as the table's expanded row, which is a sibling of the cells, not + // a child of one. + const [renaming, setRenaming] = useState(null); const list = useAsync( () => @@ -329,7 +403,12 @@ export function KeysPane({ revoked ) : (
- + label="Revoke" disabledReason={revokeDenied} @@ -372,6 +451,19 @@ export function KeysPane({ columns={columns} rows={list.data.keys} rowKey={(k) => k.keyId} + // Beneath the key it renames, full width — the actions column is + // the narrowest in the table and a DID-shaped field squeezed into + // it shows about eight characters of the thing being edited. + expanded={(k) => + renaming === k.keyId ? ( + setRenaming(null)} + onDone={list.reload} + /> + ) : null + } empty={ contextId ? `No keys in ${contextId}. Keys minted into this context appear here.` diff --git a/packages/extension/src/manager/panes/rooms-create.tsx b/packages/extension/src/manager/panes/rooms-create.tsx index 5b7a661..2a37359 100644 --- a/packages/extension/src/manager/panes/rooms-create.tsx +++ b/packages/extension/src/manager/panes/rooms-create.tsx @@ -68,6 +68,18 @@ // records by that prefix — a convention of this agent rather than a contract, // which is why typing one stays possible. // +// **The key record's id and the DID document's verification method are not the +// same string for a templated mint, and here they disagree by one.** The `room` +// and `room-host` built-in templates (`vta-sdk/templates/`) number their +// verification methods `#key-1` (signing) and `#key-2` (key agreement), while +// the create path saves the key records as `#key-0` and `#key-1` — so the +// document's `#key-1` names the signing key and the keystore's names the +// x25519 one. The public keys still line up positionally, which is why a room +// credential (the VTA signs it naming `{room_did}#key-1`, matching the +// template) verifies; what does not survive is *addressing a key by the id the +// document publishes*. Hence the filter below: the offer is by prefix and +// `keyType !== "x25519"`, never by fragment number. Reported upstream. +// // Minting can succeed and registration fail. The DID is real and the key // identifier is the half nobody writes down, so `Minted` keeps both on screen // and the flow switches to the existing-identity path pre-filled: the retry diff --git a/packages/extension/tests/key-name.test.mts b/packages/extension/tests/key-name.test.mts new file mode 100644 index 0000000..2f765d3 --- /dev/null +++ b/packages/extension/tests/key-name.test.mts @@ -0,0 +1,78 @@ +// What the console lets an operator type into the rename field. +// +// The rule is the agent's (`vti_common::identifier`), applied here so a refusal +// arrives as a sentence under the field rather than as +// `new_key_id is 86 bytes; maximum is 64` after the fact. Every assertion is a +// shape the live console produced: a DID-URL key id pre-filled into the field, +// and the suggestion offered in its place. + +import { strict as assert } from "node:assert"; +import { test } from "node:test"; + +import { + isNameable, + keyNameProblem, + suggestKeyName, + MAX_KEY_NAME_BYTES, +} from "../src/manager/key-name.js"; + +const DID_KEY = "did:webvh:QmRqxWCVUE6Y474EPdNZmR9m3giTRyDgmXSdidvEy7iSwq:webvh.storm.ws:vdr-host#key-1"; + +test("an ordinary slug is accepted", () => { + for (const ok of ["app-signing-key-2026", "vdr.host", "_private", "CamelCase", "0"]) { + assert.equal(keyNameProblem(ok), null, ok); + assert.equal(isNameable(ok), true, ok); + } +}); + +test("the id the field used to be pre-filled with is refused, and says why", () => { + // The bug this module ends: the only thing pressing Save could do was fail. + assert.equal(isNameable(DID_KEY), false); + const why = keyNameProblem(DID_KEY); + assert.ok(why?.includes("not a DID URL"), why ?? "no reason given"); +}); + +test("a separator is refused whatever its length", () => { + // These are the agent's own attack shapes — a key id lands as a store key. + for (const bad of ["global:evil", "../../etc", "my/ctx", "with space", "quote\"it"]) { + assert.notEqual(keyNameProblem(bad), null, bad); + } +}); + +test("the length cap is the agent's, and counts bytes", () => { + assert.equal(keyNameProblem("a".repeat(MAX_KEY_NAME_BYTES)), null); + const over = keyNameProblem("a".repeat(MAX_KEY_NAME_BYTES + 1)); + assert.ok(over?.includes(String(MAX_KEY_NAME_BYTES)), over ?? "no reason given"); +}); + +test("empty is a state, not a violation of the character rule", () => { + assert.ok(keyNameProblem("")?.includes("empty")); +}); + +test("the suggestion names the identity and the key, and would be accepted", () => { + const s = suggestKeyName(DID_KEY); + assert.equal(s, "vdr-host-key-1"); + assert.equal(keyNameProblem(s), null); +}); + +test("a suggestion is derived from the last path segment, not the SCID", () => { + const s = suggestKeyName( + "did:webvh:Qma9EKoeEJqtWpvAdGdrsZMcEFL5QpKrVYsJDyVxd87pCd:webvh.storm.ws:rooms:open-demo#key-0", + ); + assert.equal(s, "open-demo-key-0"); +}); + +test("every suggestion is a name the agent would take", () => { + // The placeholder is read as an offer. One the agent refuses is worse than + // none — it teaches the rule wrongly at exactly the moment it is being read. + for (const id of [ + DID_KEY, + "did:key:z6MkExample#z6MkExample", + "did:webvh:Qm:host#key-0", + "did:peer:2.Ez6LS.Vz6Mk#key-1", + ]) { + const s = suggestKeyName(id); + assert.notEqual(s, "", id); + assert.equal(keyNameProblem(s), null, `${id} → ${s}`); + } +}); diff --git a/packages/extension/tests/keys-pane.render.test.mts b/packages/extension/tests/keys-pane.render.test.mts new file mode 100644 index 0000000..ba2e258 --- /dev/null +++ b/packages/extension/tests/keys-pane.render.test.mts @@ -0,0 +1,117 @@ +// The keys pane, rendered — and specifically the rename editor. +// +// Both bugs this pins were on screen in the live console and invisible to the +// type checker and to `key-name.ts`'s own tests. +// +// **The field was pre-filled with the key's id**, which for every DID-bound key +// is a DID URL: `:` and `#` the agent's identifier gate refuses, at 86 bytes +// against a 64-byte cap. The only thing pressing Save could produce was +// `keys/rename/0.1 failed: … new_key_id is 86 bytes; maximum is 64`, and +// nothing on screen said what would have worked. +// +// **And it rendered inside the actions column**, the narrowest in the table, so +// the field showed about eight characters of the value being edited. It belongs +// under the row, where the width is the pane's. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { agent, h, render, PARTIES } from "./harness/dom.mjs"; +import { KeysPane } from "../src/manager/panes/keys.js"; + +const LIST = "keys/list/0.1"; +const RENAME = "keys/rename/0.1"; + +const DID = "did:webvh:QmRqxWCVUE6Y474EPdNZmR9m3giTRyDgmXSdidvEy7iSwq:webvh.storm.ws:vdr-host"; +const DID_KEY = `${DID}#key-0`; + +const key = (keyId: string, keyType = "ed25519") => ({ + keyId, + keyType, + status: "active", + origin: "derived", + createdAt: "2026-09-11T00:00:00Z", + contextId: "vdr", +}); + +const mount = async (keys: unknown[]) => { + const a = agent({ + [LIST]: { keys, total: keys.length, offset: 0, limit: 50 }, + [RENAME]: { keyId: "renamed", updatedAt: "2026-09-14T00:00:00Z" }, + }); + const screen = await render( + h(KeysPane, { parties: PARTIES, authority: null, contextId: "vdr" } as never), + { chrome: { runtime: { sendMessage: a.sendMessage } } }, + ); + return { a, screen }; +}; + +/** The rename field — the only text input inside the table. */ +const field = (screen: { all: (s: string) => Element[] }) => + screen.all("td input[type='text'], td input:not([type])")[0] as + | (Element & { value: string }) + | undefined; + +test("the editor opens beneath the key, not inside the actions column", async () => { + const { screen } = await mount([key(DID_KEY)]); + await screen.click(screen.button("Rename")); + const input = field(screen); + assert.ok(input, "no field appeared"); + // The full-width detail row is a ``; a field in the actions + // column has no such ancestor and is as wide as "Rename" is. + const cell = input.closest("td"); + assert.ok(cell?.hasAttribute("colspan"), "the editor is not in the table's full-width row"); +}); + +test("a DID-bound key opens with an empty field, and offers a name that would work", async () => { + const { screen } = await mount([key(DID_KEY)]); + await screen.click(screen.button("Rename")); + const input = field(screen)!; + assert.equal(input.value, "", "the field is pre-filled with a value the agent always refuses"); + assert.equal(input.getAttribute("placeholder"), "vdr-host-key-0"); + // Nothing typed: there is nothing to save, and the button says so rather + // than sending a refusal the operator has to read as an error. + assert.ok((screen.button("Save") as HTMLButtonElement).disabled); +}); + +test("the rule is on screen before it is broken", async () => { + const { screen } = await mount([key(DID_KEY)]); + await screen.click(screen.button("Rename")); + const text = screen.text(); + assert.match(text, /up to 64 characters/); + // And the one-way door: the agent cannot be given a DID URL back. + assert.match(text, /will not accept the old id back/); +}); + +test("pasting the id back is refused here, with the reason, and nothing is sent", async () => { + const { a, screen } = await mount([key(DID_KEY)]); + await screen.click(screen.button("Rename")); + await screen.type(field(screen)!, DID_KEY); + assert.match(screen.text(), /not a DID URL/); + assert.ok((screen.button("Save") as HTMLButtonElement).disabled); + assert.equal( + a.calls.filter((c: { type: string }) => c.type.endsWith(RENAME)).length, + 0, + "a name the agent would refuse was sent anyway", + ); +}); + +test("a valid name is sent as typed, against the id the key has today", async () => { + const { a, screen } = await mount([key(DID_KEY)]); + await screen.click(screen.button("Rename")); + await screen.type(field(screen)!, "vdr-host-signing"); + await screen.click(screen.button("Save")); + await screen.settle(); + const call = a.calls.find((c: { type: string }) => c.type.endsWith(RENAME)); + assert.ok(call, "nothing was sent"); + assert.deepEqual(call.payload, { keyId: DID_KEY, newKeyId: "vdr-host-signing" }); +}); + +test("a key that already has a name opens on it, and Save waits for a change", async () => { + const { screen } = await mount([key("app-signing-key")]); + await screen.click(screen.button("Rename")); + assert.equal(field(screen)!.value, "app-signing-key"); + assert.ok( + (screen.button("Save") as HTMLButtonElement).disabled, + "renaming a key to the name it has is a write with nothing in it", + ); +}); diff --git a/packages/extension/tests/manager-form-state.test.mts b/packages/extension/tests/manager-form-state.test.mts index 54aabef..c87381a 100644 --- a/packages/extension/tests/manager-form-state.test.mts +++ b/packages/extension/tests/manager-form-state.test.mts @@ -41,7 +41,7 @@ const ROOT = fileURLToPath(new URL("../src/manager", import.meta.url)); * that adding a key is inconvenient. */ const KEYED_ELSEWHERE: Record = { - RenameKey: "rendered per row by `Table`, which keys each on rowKey", + RenameEditor: "rendered as `Table`'s expanded row, inside the fragment keyed on rowKey", ChangeRole: "rendered per row by `Table`, which keys each on rowKey", }; @@ -75,8 +75,12 @@ function seededComponents(): { name: string; where: string }[] { const body = src.slice(m.index + m[0].length); const end = body.search(/\n(?:export )?function /); const scope = end === -1 ? body : body.slice(0, end); + // The prop may be read anywhere inside the `useState(…)` call, not only + // as its whole argument: `useState(nameable ? record.keyId : "")` is + // seeded from `record` exactly as much as `useState(record.keyId)` is, + // and the narrower match read it as a component with no state at all. const seeds = props.some((p) => - new RegExp(`useState\\(\\s*${p}\\.`).test(scope) || new RegExp(`useState\\(\\s*${p}\\?\\.`).test(scope), + new RegExp(`useState\\([^;]*?\\b${p}\\??\\.`).test(scope), ); if (seeds) found.push({ name, where: rel }); }