diff --git a/CLAUDE.md b/CLAUDE.md index a1d5477..b80c945 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -525,7 +525,7 @@ a second copy. `rp-login/step-up.ts` re-exports every name, and `tests/rp-login.step-up.mjs` passes unchanged, which is what says the move was non-breaking. -**Reveal lives in `FactValue`'s own state**, per value, and nowhere else. Lifted +**Reveal lives in `AttributeValue`'s own state**, per value, and nowhere else. Lifted to the pane and keyed by fact id it would be a store of "things unhidden" that outlives the card the person was looking at and is one refactor from a *Show all*. Component state cannot become that: it dies with the element, so leaving @@ -538,7 +538,7 @@ again, or catching the refusal and rethrowing it; matching the step-up code in `unverifiedApproveRequest` instead of the verified `context`; dropping the `previewId` cross-check; moving the verify/sign half back up beside the RP flow; a surface that formats a value itself instead of rendering -`FactValue` (the second surface is always the one added later, and a value +`AttributeValue` (the second surface is always the one added later, and a value masked on the card and printed in the strip is masked nowhere); greying a mask with `c.faint`, which is this pane's word for "the agent sent no value" and so makes a fact the holder has look like one they do not; reintroducing a compiled @@ -546,6 +546,31 @@ table as a fallback for a registry that has not loaded; or letting a UI string imply the console does not hold what it hides. +**A listing is read to the end, and that is the client's job.** Every +`persona/…/list` task is cursor-paginated, and the specification is explicit: a +producer MUST NOT infer exhaustion from a short page — only an absent +`nextCursor` means the end. `personaAttributeList`, `personaProfileList` and +`listBindings` returned the first page and dropped the cursor, which nothing +downstream could detect, because a short array is indistinguishable from a +complete one: past the agent's page size (100 by default) the identity map drew +a face pointing at attributes that were not in its own list, under counts that +agreed with each other because they counted the same truncated array. All three +now follow the cursor through `collectPages` (`core/src/util/pages.ts`), and +`listBindings` returns a document with no `nextCursor` because there is nothing +left to fetch. `limit` on those calls is the **page size to ask for**, never a +cap on the result. + +**The bound throws rather than truncating.** `MAX_PAGES` guards against a far +side that will not end — a cursor that repeats, or a pool beyond anything a +picture can draw — and returning what had been collected would reintroduce the +defect one layer down and with a longer array. It is set high enough (50 pages, +25,000 records at the maximum page size) that reaching it means a fault rather +than a large pool. + +**What breaks it:** reading `.attributes` / `.profiles` / `.personas` off a +single response again; treating `limit` as a cap; or catching the bound's error +and returning a partial list. + **The console's components are rendered in tests, and this is how.** `tests/harness/` holds module hooks and a DOM so `node --test` can mount a pane. Two things it does that are not obvious: it resolves a `./thing.js` diff --git a/packages/core/src/admin/persona.ts b/packages/core/src/admin/persona.ts index 990d176..0681625 100644 --- a/packages/core/src/admin/persona.ts +++ b/packages/core/src/admin/persona.ts @@ -35,6 +35,7 @@ // context. A context never pulls, and there is no task in this file that would // let it. +import { collectPages } from "../util/pages.js"; import type { TaskParty, TrustTaskSender } from "../vta/channel.js"; import { buildTrustTask } from "../vta/trust-task.js"; @@ -202,11 +203,27 @@ export interface AttributeListParams extends PersonaHolderParams { * needs to see that something went stale rather than have it quietly omitted. */ includeStale?: boolean; + /** + * The page size to ask for, **not** a cap on what comes back: this call + * follows `nextCursor` to the end. Left unset the agent picks (100 today). + */ limit?: PersonaAttributeListPayload["limit"]; + /** Where to start. Everything from there is returned, not one page of it. */ cursor?: PersonaAttributeListPayload["cursor"]; } -/** Enumerate the pool. Metadata only unless `includeValues` is set. */ +/** + * Enumerate the pool. Metadata only unless `includeValues` is set. + * + * **Reads to the end**, following `nextCursor`. It used to return the first page + * and drop the cursor, which the specification names directly as the mistake — + * "a producer MUST NOT infer exhaustion from a short page — only an absent + * `nextCursor` means the end" — and which is invisible from the outside: a + * holder past the agent's page size got a silently short pool, and the console's + * identity map drew a face pointing at attributes that were not in it. + * + * See `collectPages` for what happens when the far side will not end. + */ export async function personaAttributeList( sender: TrustTaskSender, params: AttributeListParams, @@ -219,15 +236,17 @@ export async function personaAttributeList( ...(params.limit !== undefined ? { limit: params.limit } : {}), ...(params.cursor !== undefined ? { cursor: params.cursor } : {}), }; - const res = await holderCall( - sender, - params, - ATTRIBUTE_LIST, - ATTRIBUTE_LIST_RESPONSE, - "persona/attribute/list/1.0", - payload, - ); - return res.attributes ?? []; + return collectPages("persona/attribute/list", async (cursor) => { + const res = await holderCall( + sender, + params, + ATTRIBUTE_LIST, + ATTRIBUTE_LIST_RESPONSE, + "persona/attribute/list/1.0", + cursor === undefined ? payload : { ...payload, cursor }, + ); + return { items: res.attributes ?? [], nextCursor: res.nextCursor }; + }); } export interface AttributePutParams extends PersonaHolderParams { @@ -369,8 +388,10 @@ export async function personaAttributeDelete( // ── Profiles ──────────────────────────────────────────────────────────────── -/** Every profile the holder has. Names and entries; never resolved values — - * see {@link personaProfileGet} for why there is no `resolve` here. */ +/** Every profile the holder has — **to the end of the listing**, like + * {@link personaAttributeList}. Names and entries; never resolved values, see + * {@link personaProfileGet} for why there is no `resolve` here. `limit` is the + * page size to ask for, not a cap on the result. */ export async function personaProfileList( sender: TrustTaskSender, params: PersonaHolderParams & { limit?: PersonaProfileListPayload["limit"]; cursor?: string }, @@ -379,15 +400,17 @@ export async function personaProfileList( ...(params.limit !== undefined ? { limit: params.limit } : {}), ...(params.cursor !== undefined ? { cursor: params.cursor } : {}), }; - const res = await holderCall( - sender, - params, - PROFILE_LIST, - PROFILE_LIST_RESPONSE, - "persona/profile/list/1.0", - payload, - ); - return res.profiles ?? []; + return collectPages("persona/profile/list", async (cursor) => { + const res = await holderCall( + sender, + params, + PROFILE_LIST, + PROFILE_LIST_RESPONSE, + "persona/profile/list/1.0", + cursor === undefined ? payload : { ...payload, cursor }, + ); + return { items: res.profiles ?? [], nextCursor: res.nextCursor }; + }); } export interface ProfileGetParams extends PersonaHolderParams { diff --git a/packages/core/src/persona/bindings.ts b/packages/core/src/persona/bindings.ts index d7c88bd..068aac2 100644 --- a/packages/core/src/persona/bindings.ts +++ b/packages/core/src/persona/bindings.ts @@ -28,6 +28,8 @@ import { type PersonaBindingListResponsePayload, } from "@openvtc/trust-tasks/persona/binding/list/1.0/payload"; +import { collectPages } from "../util/pages.js"; + import { call, type PersonaCallerParams } from "./call.js"; export type PersonaBinding = PersonaBindingGetResponsePayload; @@ -67,7 +69,22 @@ export interface ListBindingsParams extends PersonaCallerParams { cursor?: PersonaBindingListPayload["cursor"]; } -/** Every persona bound in this context. */ +/** + * Every persona present in this context — **to the end of the listing**. + * + * Not "every persona *bound*": the response carries `bound` per entry precisely + * because an unbound persona is still present, and a caller deciding what a + * context knows of the holder needs both kinds. + * + * The returned document carries no `nextCursor`, because there is nothing left + * to fetch. That absence is the honest report of what this now does, and a + * caller that used to ignore the member is correct by construction rather than + * by luck — which is what it was before: two console surfaces read `.personas` + * off the first page and drew the result as the whole truth. + * + * `limit` is the page size to ask for; `cursor` is where to start. Neither caps + * the result. + */ export async function listBindings( sender: TrustTaskSender, params: ListBindingsParams, @@ -77,12 +94,16 @@ export async function listBindings( ...(params.limit !== undefined ? { limit: params.limit } : {}), ...(params.cursor !== undefined ? { cursor: params.cursor } : {}), }; - return call( - sender, - params, - BINDING_LIST, - BINDING_LIST_RESPONSE, - "persona/binding/list", - payload, - ); + const personas = await collectPages("persona/binding/list", async (cursor) => { + const res = await call( + sender, + params, + BINDING_LIST, + BINDING_LIST_RESPONSE, + "persona/binding/list", + cursor === undefined ? payload : { ...payload, cursor }, + ); + return { items: res.personas ?? [], nextCursor: res.nextCursor }; + }); + return { personas }; } diff --git a/packages/core/src/util/index.ts b/packages/core/src/util/index.ts index 2db2aab..e092b85 100644 --- a/packages/core/src/util/index.ts +++ b/packages/core/src/util/index.ts @@ -2,4 +2,5 @@ // `util` is the bottom of the layering, and the boundary test enforces it. export * from "./base64url.js"; +export * from "./pages.js"; export * from "./timing.js"; diff --git a/packages/core/src/util/pages.ts b/packages/core/src/util/pages.ts new file mode 100644 index 0000000..91c173d --- /dev/null +++ b/packages/core/src/util/pages.ts @@ -0,0 +1,74 @@ +// Reading a cursor-paginated listing to the end. +// +// ## Why a helper rather than a loop at each call site +// +// Every `persona/…/list` task in the persona family answers with a page and a +// `nextCursor`, and the specification is explicit about how that must be read: +// "a producer MUST NOT infer exhaustion from a short page — only an absent +// `nextCursor` means the end." The clients here were named `...List`, documented +// as *enumerate the pool* and *every persona bound in this context*, and +// returned the first page while dropping the cursor on the floor. +// +// That failure is invisible in exactly the way that matters. Nothing errors, +// nothing looks partial, and the console's identity map — whose whole premise is +// one picture of everything — draws a face pointing at attributes that are not +// in the list, under counts that agree with each other because they are all +// counting the same truncated array. A picture that reads as complete while +// being partial is the one wrong answer that pane must never give. +// +// ## The bound is not a page limit, it is a loop guard +// +// `maxPages` exists because a cursor comes from the far side: an agent that +// returns the same cursor forever, or one whose pool genuinely exceeds what any +// caller could draw, must not spin this in a service worker. It is set high +// enough that reaching it means something is wrong rather than something is +// large. +// +// **Reaching it throws.** Returning what was collected would reintroduce the +// defect this exists to fix, one layer down and with a longer array — and a +// caller cannot tell a short answer from a complete one, which is the whole +// problem. An error names the situation and the surface can say so. + +/** One page, in the shape every `…/list` response shares. */ +export interface Page { + items: T[]; + nextCursor?: string | undefined; +} + +/** How many pages `collectPages` will read before deciding the far side is + * misbehaving. At the specification's maximum page size of 500 this is 25,000 + * records — beyond any pool a person curates by hand, which is the point. */ +export const MAX_PAGES = 50; + +/** + * Follow `nextCursor` until it is absent, and return everything. + * + * `fetchPage` is called with `undefined` first and with each cursor after it. + * `what` names the listing in the error, because "too many pages" without a + * subject is a message nobody can act on. + */ +export async function collectPages( + what: string, + fetchPage: (cursor?: string | undefined) => Promise>, + maxPages: number = MAX_PAGES, +): Promise { + const all: T[] = []; + let cursor: string | undefined; + for (let page = 0; page < maxPages; page += 1) { + const next: Page = await fetchPage(cursor); + all.push(...next.items); + // Absence is the only exhaustion signal — a short page is not one, and an + // empty page carrying a cursor is a legal answer this must keep following. + if (next.nextCursor === undefined) return all; + // A cursor that does not move is the far side looping. Caught here rather + // than by the page bound alone so the message says which fault it was. + if (next.nextCursor === cursor) { + throw new Error(`${what}: your agent returned the same page cursor twice, so the listing cannot be finished`); + } + cursor = next.nextCursor; + } + throw new Error( + `${what}: still more after ${maxPages} pages — refusing to keep asking, because a listing this long is ` + + `an agent misbehaving rather than a pool this size`, + ); +} diff --git a/packages/core/tests/admin.persona.mjs b/packages/core/tests/admin.persona.mjs index 82c4afe..57cb32f 100644 --- a/packages/core/tests/admin.persona.mjs +++ b/packages/core/tests/admin.persona.mjs @@ -26,6 +26,7 @@ import { personasBlockingDelete, PROFILE_DELETE_BOUND, } from "../dist/admin/index.js"; +import { listBindings } from "../dist/persona/index.js"; const HOLDER = { did: "did:key:zHolder" }; const SERVICE = { did: "did:webvh:QmAgent:agent.example" }; @@ -411,3 +412,82 @@ test("a values listing can ask for the sensitive ones, and does not by default", await personaAttributeList(plain, { ...PARTIES, includeValues: true }); assert.ok(!("includeSensitive" in plain.sent[0].envelope.payload)); }); + +// ── A listing is read to the end, not to the first page ──────────────────── +// +// `*List` clients returned the first page and dropped `nextCursor`, which the +// specification names as the mistake — "a producer MUST NOT infer exhaustion +// from a short page" — and which nothing downstream could detect: a short array +// is indistinguishable from a complete one. The console's identity map drew the +// result as the whole truth. + +/** Answers with each reply in turn, recording what it was asked. */ +function pages(...replies) { + const sent = []; + return { + sent, + send(envelope) { + sent.push({ envelope }); + return Promise.resolve(replies[sent.length - 1] ?? replies[replies.length - 1]); + }, + }; +} + +const poolAttribute = (id) => ({ + attributeId: id, + type: "name.legal", + valueType: "string", + provenance: { kind: "selfAsserted" }, + version: 1, + updatedAt: "x", +}); + +test("the pool is read to the end, and the cursor goes back with the next request", async () => { + const r = pages( + { attributes: [poolAttribute("a1")], nextCursor: "c1" }, + { attributes: [poolAttribute("a2")] }, + ); + const all = await personaAttributeList(r, { ...PARTIES }); + assert.deepEqual(all.map((a) => a.attributeId), ["a1", "a2"]); + assert.equal(r.sent.length, 2); + assert.equal(r.sent[0].envelope.payload.cursor, undefined, "the first request invents no cursor"); + assert.equal(r.sent[1].envelope.payload.cursor, "c1"); +}); + +test("paging preserves the rest of the request, so a narrowed listing stays narrowed", async () => { + // The second page of a `typePrefix` query that forgot the prefix would return + // the whole pool — and `reveal-value.ts` matches by id, so it would quietly + // read every value the holder has to answer a question about one. + const r = pages( + { attributes: [poolAttribute("a1")], nextCursor: "c1" }, + { attributes: [poolAttribute("a2")] }, + ); + await personaAttributeList(r, { ...PARTIES, typePrefix: "phone", includeValues: true, includeSensitive: true }); + const second = r.sent[1].envelope.payload; + assert.equal(second.typePrefix, "phone"); + assert.equal(second.includeValues, true); + assert.equal(second.includeSensitive, true); +}); + +test("faces are read to the end too", async () => { + const r = pages( + { profiles: [{ profileId: "p1", name: "One", entries: [], version: 1, updatedAt: "x" }], nextCursor: "c1" }, + { profiles: [{ profileId: "p2", name: "Two", entries: [], version: 1, updatedAt: "x" }] }, + ); + const all = await personaProfileList(r, { ...PARTIES }); + assert.deepEqual(all.map((p) => p.profileId), ["p1", "p2"]); +}); + +test("every persona in a context is read to the end, and no cursor comes back", async () => { + // The returned document carries no `nextCursor` because there is nothing left + // to fetch — the two console surfaces that ignored the member are correct by + // construction now rather than by luck. + const r = pages( + { personas: [{ personaDid: "did:key:zA", bound: true }], nextCursor: "c1" }, + { personas: [{ personaDid: "did:key:zB", bound: false }] }, + ); + const res = await listBindings(r, { ...PARTIES, contextId: "openvtc" }); + assert.deepEqual(res.personas.map((p) => p.personaDid), ["did:key:zA", "did:key:zB"]); + assert.equal(res.nextCursor, undefined); + assert.equal(r.sent[1].envelope.payload.contextId, "openvtc", "the context survives the second request"); +}); diff --git a/packages/core/tests/util.pages.mjs b/packages/core/tests/util.pages.mjs new file mode 100644 index 0000000..bf09980 --- /dev/null +++ b/packages/core/tests/util.pages.mjs @@ -0,0 +1,81 @@ +// Reading a cursor-paginated listing to the end. +// +// The defect this closes is invisible from the outside — a short array that +// looks exactly like a complete one — so the assertions here are mostly about +// *how many times the far side was asked*, which is the only observable +// difference between reading a listing and reading the first page of one. + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { collectPages, MAX_PAGES } from "../dist/util/index.js"; + +/** Serves `pages` in order, recording the cursor it was asked with each time. */ +function pager(pages) { + const asked = []; + return { + asked, + fetch: async (cursor) => { + asked.push(cursor); + return pages[asked.length - 1]; + }, + }; +} + +test("a single page with no cursor is the whole answer", async () => { + const p = pager([{ items: [1, 2, 3] }]); + assert.deepEqual(await collectPages("x", p.fetch), [1, 2, 3]); + assert.deepEqual(p.asked, [undefined], "no cursor was invented for a listing that ended"); +}); + +test("every page is followed, and the pages are concatenated in order", async () => { + const p = pager([ + { items: [1, 2], nextCursor: "c1" }, + { items: [3, 4], nextCursor: "c2" }, + { items: [5] }, + ]); + assert.deepEqual(await collectPages("x", p.fetch), [1, 2, 3, 4, 5]); + assert.deepEqual(p.asked, [undefined, "c1", "c2"]); +}); + +test("a short page is not the end — only an absent cursor is", async () => { + // The specification says this outright, and it is the exact inference the + // old clients made: they took the first page and stopped. + const p = pager([ + { items: [1], nextCursor: "c1" }, + { items: [2, 3, 4] }, + ]); + assert.deepEqual(await collectPages("x", p.fetch), [1, 2, 3, 4]); +}); + +test("an empty page carrying a cursor is followed, not treated as the end", async () => { + // A legal answer: the agent filtered a page down to nothing and has more. + const p = pager([ + { items: [], nextCursor: "c1" }, + { items: [7] }, + ]); + assert.deepEqual(await collectPages("x", p.fetch), [7]); +}); + +test("a cursor that does not move is reported as the agent looping", async () => { + const p = pager([ + { items: [1], nextCursor: "same" }, + { items: [2], nextCursor: "same" }, + ]); + await assert.rejects(() => collectPages("persona/attribute/list", p.fetch), /same page cursor twice/); +}); + +test("a listing that will not end throws rather than returning a short answer", async () => { + // Returning what was collected would reintroduce the very defect this exists + // to fix — a caller cannot tell a truncated array from a complete one. + let n = 0; + const endless = async () => ({ items: [n], nextCursor: `c${++n}` }); + await assert.rejects(() => collectPages("persona/profile/list", endless, 3), /still more after 3 pages/); + await assert.rejects(() => collectPages("persona/profile/list", endless, 3), /persona\/profile\/list/); +}); + +test("the default bound is high enough that reaching it means a fault", async () => { + // 50 pages at the specification's maximum page size is 25,000 records. The + // number matters: a bound low enough to be reached by a real pool would be + // the truncation bug with an error message. + assert.ok(MAX_PAGES >= 50); +}); diff --git a/packages/extension/src/manager/claim-sensitivity.ts b/packages/extension/src/manager/claim-sensitivity.ts index 70b0ba5..4e52560 100644 --- a/packages/extension/src/manager/claim-sensitivity.ts +++ b/packages/extension/src/manager/claim-sensitivity.ts @@ -140,7 +140,7 @@ function emailLocal(text: string): string { * `treatmentFor`, which is where that decision is applied and where the reason * an unregistered token's mask follows it is written down. */ -export function maskedFact( +export function maskedValue( registry: ClaimTypeRegistry | null, type: string, text: string, diff --git a/packages/extension/src/manager/panes/persona-editors.tsx b/packages/extension/src/manager/panes/persona-editors.tsx index 4925b46..f3ec514 100644 --- a/packages/extension/src/manager/panes/persona-editors.tsx +++ b/packages/extension/src/manager/panes/persona-editors.tsx @@ -43,7 +43,7 @@ import { contextHeading, formatInstant } from "../format.js"; import { type Authority, type Parties } from "../use-vta.js"; import { holderGate } from "../holder-gate.js"; import type { ClaimTypeRegistry } from "@openvtc/pnm-core/persona"; -import { maskedFact, treatmentFor, type Sensitivity } from "../claim-sensitivity.js"; +import { maskedValue, treatmentFor, type Sensitivity } from "../claim-sensitivity.js"; import { revealAttributeValue } from "../reveal-value.js"; import { composeEntries, lockedRefs, preservedEntries, tickedFrom } from "../profile-entries.js"; import { personaCandidates } from "../persona-candidates.js"; @@ -75,7 +75,7 @@ export function Label({ children }: { children: React.ReactNode }) { * with no value. * * **Not exported, and that is the enforcement.** Every value this pane draws - * goes through `FactValue` below, which is where a sensitive one is hidden. A + * goes through `AttributeValue` below, which is where a sensitive one is hidden. A * surface that could reach the raw rendering would be one mask away from * printing a passport number in full, and it would look like ordinary code. */ @@ -116,7 +116,7 @@ function formatValue(value: unknown): { text: string; withheld: boolean } { * the console or navigating anywhere re-hides everything, and revealing the * same attribute in two places is two deliberate acts rather than one. */ -export function FactValue({ +export function AttributeValue({ type, value, sensitivity, @@ -159,7 +159,7 @@ export function FactValue({ const [refused, setRefused] = useState(null); const { text, withheld } = formatValue(revealed ? revealed.value : value); - const { text: hidden, masked } = maskedFact(registry, type, text, sensitivity); + const { text: hidden, masked } = maskedValue(registry, type, text, sensitivity); // **A withheld value is never masked.** The mask is a statement that a value // is here and is being kept off the screen; drawing it over "not on this @@ -1199,7 +1199,7 @@ export function ResolvedProfile({ style={{ display: "flex", gap: 10, flexWrap: "wrap", alignItems: "baseline" }} > {claim.type} - {f.label} · )} -
{attribute.type} -
{name ? ( - + ) : ( — )} @@ -119,7 +119,7 @@ function StrangerCard({ {rest.map((f) => ( {f.label ?? f.type} - + ))}
@@ -204,7 +204,7 @@ export function GuidedSetup({ {attributes.map((a) => (
{a.type} - +
))}
diff --git a/packages/extension/tests/manager-claim-sensitivity.test.mts b/packages/extension/tests/manager-claim-sensitivity.test.mts index e9289a5..eab9d67 100644 --- a/packages/extension/tests/manager-claim-sensitivity.test.mts +++ b/packages/extension/tests/manager-claim-sensitivity.test.mts @@ -14,7 +14,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { maskText, maskedFact, treatmentFor, isSensitiveFor } from "../src/manager/claim-sensitivity.ts"; +import { maskText, maskedValue, treatmentFor, isSensitiveFor } from "../src/manager/claim-sensitivity.ts"; /** * The table as `persona/claim-types/list` serves it — the agent's own, not a @@ -75,7 +75,7 @@ const UNREGISTERED = { sensitivity: "high", mask: "full" }; const treatmentOf = (type: string) => treatmentFor(REGISTRY, type).treatment; const isSensitive = (type: string) => isSensitiveFor(REGISTRY, type); const drawn = (type: string, text: string, override?: "normal" | "high") => - maskedFact(REGISTRY, type, text, override); + maskedValue(REGISTRY, type, text, override); // ── The registry's own answers ────────────────────────────────────────────── @@ -314,7 +314,7 @@ test("a family member invented under a gated family keeps the family's mask", () test("an email is masked, which is what the registry asked for all along", () => { // `email.*` is `normal`/`emailLocal`: worth hiding from the person behind - // you, not worth withholding from every listing. `maskedFact` used to gate on + // you, not worth withholding from every listing. `maskedValue` used to gate on // `high` and drew it in full, while `isSensitive` called it hidden — so the // strip promised a Show button that was never rendered. const { text, masked } = drawn("email.personal", "glenn.gore@example.com");