Skip to content

Commit 78303a8

Browse files
rekmarksclaude
andauthored
fix(kernel-ui): validate branded KRef values instead of unsafe casts (#921)
A follow-up to #917 addressing various items deferred during the introduction of branded kernel identifier types. ## Summary - Replace `as KRef` casts from unvalidated sources with runtime validation in `kernel-ui` - `db-parser.ts`: remove dead `?? ''` fallbacks on regex match groups, add `insistKRef` validation for slot resolution from JSON-parsed DB values - `SendMessageForm.tsx`: validate user-selected target with `isKRef` before sending, replacing the blind `as KRef` cast - Add `isKRef`, `insistKRef`, `KRefStruct` to `setupOcapKernelMock` test utility - Includes prior commit addressing PR review feedback on branded types in `ocap-kernel` ## Test plan - [x] `yarn workspace @metamask/kernel-ui run build` passes - [x] `yarn workspace @metamask/kernel-ui test:dev:quiet` passes (all 34 test files, 263 tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds runtime `KRef` validation in UI and DB parsing paths, which can now reject/throw on previously accepted malformed values. Risk is moderate because it changes input handling and parsing behavior but is narrowly scoped and covered by updated tests. > > **Overview** > **Hardens branded identifier handling** by validating `KRef` values at runtime instead of relying on unchecked `as KRef` casts. > > In `SendMessageForm`, the selected target is now stored as `KRef | null`, validated via `isKRef` on change, and sending is blocked when the target is invalid; tests are updated accordingly and add coverage for failed `KRef` validation. > > In `kernel-ui`’s `db-parser`, regex match fallbacks are removed and `resolveSlot` now calls `insistKRef` when converting JSON-parsed slot strings, making malformed DB data fail fast. Supporting changes extend `setupOcapKernelMock` with `isKRef`/`insistKRef`/`KRefStruct`, and `ocap-kernel` tests/docs are updated (including new `getKnownRelays` test coverage and tighter ID validation cases). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 55a981a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d979a06 commit 78303a8

8 files changed

Lines changed: 117 additions & 35 deletions

File tree

packages/kernel-ui/src/components/SendMessageForm.test.tsx

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { useRegistry } from '../hooks/useRegistry.ts';
1717
import type { ObjectRegistry } from '../types.ts';
1818
import { SendMessageForm } from './SendMessageForm.tsx';
1919

20-
setupOcapKernelMock();
20+
const { setMockBehavior } = setupOcapKernelMock();
2121

2222
vi.mock('../context/PanelContext.tsx', () => ({
2323
usePanelContext: vi.fn(),
@@ -27,7 +27,8 @@ vi.mock('../hooks/useRegistry.ts', () => ({
2727
useRegistry: vi.fn(),
2828
}));
2929

30-
vi.mock('@metamask/kernel-utils', () => ({
30+
vi.mock('@metamask/kernel-utils', async (importOriginal) => ({
31+
...(await importOriginal()),
3132
stringify: vi.fn(),
3233
}));
3334

@@ -45,14 +46,14 @@ describe('SendMessageForm Component', () => {
4546
overview: { name: 'TestVat1', bundleSpec: '' },
4647
ownedObjects: [
4748
{
48-
kref: 'kref1',
49+
kref: 'ko1',
4950
eref: 'eref1',
5051
refCount: '1',
5152
toVats: [],
5253
revoked: 'false',
5354
},
5455
{
55-
kref: 'kref2',
56+
kref: 'ko2',
5657
eref: 'eref2',
5758
refCount: '1',
5859
toVats: [],
@@ -67,7 +68,7 @@ describe('SendMessageForm Component', () => {
6768
overview: { name: 'TestVat2', bundleSpec: '' },
6869
ownedObjects: [
6970
{
70-
kref: 'kref3',
71+
kref: 'ko3',
7172
eref: 'eref3',
7273
refCount: '1',
7374
toVats: [],
@@ -76,7 +77,7 @@ describe('SendMessageForm Component', () => {
7677
],
7778
importedObjects: [
7879
{
79-
kref: 'kref4',
80+
kref: 'ko4',
8081
eref: 'eref4',
8182
refCount: '1',
8283
fromVat: 'vat1',
@@ -144,19 +145,19 @@ describe('SendMessageForm Component', () => {
144145
expect(options).toHaveLength(5); // 1 placeholder + 4 options from mock registry
145146

146147
// Check dropdown options include objects from the registry
147-
expect(screen.getByText('kref1 (TestVat1)')).toBeInTheDocument();
148-
expect(screen.getByText('kref2 (TestVat1)')).toBeInTheDocument();
149-
expect(screen.getByText('kref3 (TestVat2)')).toBeInTheDocument();
150-
expect(screen.getByText('kref4 (TestVat1)')).toBeInTheDocument();
148+
expect(screen.getByText('ko1 (TestVat1)')).toBeInTheDocument();
149+
expect(screen.getByText('ko2 (TestVat1)')).toBeInTheDocument();
150+
expect(screen.getByText('ko3 (TestVat2)')).toBeInTheDocument();
151+
expect(screen.getByText('ko4 (TestVat1)')).toBeInTheDocument();
151152
});
152153

153154
it('updates form values when inputs change', async () => {
154155
const { getByTestId } = render(<SendMessageForm />);
155156

156157
// Change target
157158
const targetSelect = getByTestId('message-target');
158-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
159-
expect(targetSelect).toHaveValue('kref1');
159+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
160+
expect(targetSelect).toHaveValue('ko1');
160161

161162
// Change method
162163
const methodInput = getByTestId('message-method');
@@ -179,7 +180,7 @@ describe('SendMessageForm Component', () => {
179180

180181
// Select a target
181182
const targetSelect = getByTestId('message-target');
182-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
183+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
183184

184185
// Button should now be enabled
185186
expect(sendButton).not.toBeDisabled();
@@ -192,12 +193,26 @@ describe('SendMessageForm Component', () => {
192193
expect(sendButton).toBeDisabled();
193194
});
194195

196+
it('does not set target when value fails KRef validation', async () => {
197+
setMockBehavior({ isKRef: false });
198+
const { getByTestId } = render(<SendMessageForm />);
199+
200+
const targetSelect = getByTestId('message-target');
201+
fireEvent.change(targetSelect, { target: { value: 'not-a-kref' } });
202+
203+
// Target should remain unset, button stays disabled
204+
expect(targetSelect).toHaveValue('');
205+
expect(getByTestId('message-send-button')).toBeDisabled();
206+
207+
setMockBehavior({ isKRef: true });
208+
});
209+
195210
it('calls callKernelMethod with correct parameters when Send button is clicked', async () => {
196211
const { getByTestId } = render(<SendMessageForm />);
197212

198213
// Set up form values
199214
const targetSelect = getByTestId('message-target');
200-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
215+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
201216

202217
const methodInput = getByTestId('message-method');
203218
fireEvent.change(methodInput, { target: { value: 'testMethod' } });
@@ -215,7 +230,7 @@ describe('SendMessageForm Component', () => {
215230
// Check if callKernelMethod was called with correct parameters
216231
expect(callKernelMethod).toHaveBeenCalledWith({
217232
method: 'queueMessage',
218-
params: ['kref1', 'testMethod', expectedArgs],
233+
params: ['ko1', 'testMethod', expectedArgs],
219234
});
220235

221236
// Check if fetchObjectRegistry was called
@@ -232,7 +247,7 @@ describe('SendMessageForm Component', () => {
232247

233248
// Set up form values and submit
234249
const targetSelect = getByTestId('message-target');
235-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
250+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
236251

237252
const sendButton = getByTestId('message-send-button');
238253
await userEvent.click(sendButton);
@@ -255,7 +270,7 @@ describe('SendMessageForm Component', () => {
255270

256271
// Set up form values and submit
257272
const targetSelect = getByTestId('message-target');
258-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
273+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
259274

260275
const sendButton = getByTestId('message-send-button');
261276
await userEvent.click(sendButton);
@@ -277,7 +292,7 @@ describe('SendMessageForm Component', () => {
277292

278293
// Set up form values with invalid JSON
279294
const targetSelect = getByTestId('message-target');
280-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
295+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
281296

282297
const paramsInput = getByTestId('message-params');
283298
fireEvent.change(paramsInput, { target: { value: 'invalid json' } });
@@ -300,7 +315,7 @@ describe('SendMessageForm Component', () => {
300315

301316
// Set up form values - we need a valid target and valid JSON
302317
const targetSelect = getByTestId('message-target');
303-
fireEvent.change(targetSelect, { target: { value: 'kref1' } });
318+
fireEvent.change(targetSelect, { target: { value: 'ko1' } });
304319

305320
// Ensure we have valid JSON in the params field
306321
const paramsInput = getByTestId('message-params');

packages/kernel-ui/src/components/SendMessageForm.tsx

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
TextColor,
1010
} from '@metamask/design-system-react';
1111
import { stringify } from '@metamask/kernel-utils';
12+
import { isKRef } from '@metamask/ocap-kernel';
1213
import type { KRef } from '@metamask/ocap-kernel';
1314
import type { Json } from '@metamask/utils';
1415
import { useState, useMemo } from 'react';
@@ -27,7 +28,7 @@ const inputStyle =
2728
export const SendMessageForm: React.FC = () => {
2829
const { callKernelMethod, logMessage, objectRegistry } = usePanelContext();
2930
const { fetchObjectRegistry } = useRegistry();
30-
const [target, setTarget] = useState('');
31+
const [target, setTarget] = useState<KRef | null>(null);
3132
const [method, setMethod] = useState('__getMethodNames__');
3233
const [paramsText, setParamsText] = useState('[]');
3334
const [result, setResult] = useState<Json | null>(null);
@@ -64,12 +65,15 @@ export const SendMessageForm: React.FC = () => {
6465
}, [objectRegistry]);
6566

6667
const handleSend = (): void => {
68+
if (!target) {
69+
return;
70+
}
6771
Promise.resolve()
6872
.then(() => JSON.parse(paramsText) as Json[])
6973
.then(async (args) =>
7074
callKernelMethod({
7175
method: 'queueMessage',
72-
params: [target as KRef, method, args],
76+
params: [target, method, args],
7377
}),
7478
)
7579
.then((response) => {
@@ -117,8 +121,11 @@ export const SendMessageForm: React.FC = () => {
117121
</label>
118122
<select
119123
id="message-target"
120-
value={target}
121-
onChange={(event) => setTarget(event.target.value)}
124+
value={target ?? ''}
125+
onChange={(event) => {
126+
const { value } = event.target;
127+
setTarget(isKRef(value) ? value : null);
128+
}}
122129
data-testid="message-target"
123130
className={`${inputStyle} cursor-pointer`}
124131
>
@@ -172,7 +179,7 @@ export const SendMessageForm: React.FC = () => {
172179
variant={ButtonVariant.Primary}
173180
size={ButtonSize.Sm}
174181
onClick={handleSend}
175-
isDisabled={!(target.trim() && method.trim())}
182+
isDisabled={!(target && method.trim())}
176183
className="h-9 rounded-md"
177184
data-testid="message-send-button"
178185
>

packages/kernel-ui/src/services/db-parser.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { insistKRef } from '@metamask/ocap-kernel';
12
import type { KRef } from '@metamask/ocap-kernel';
23

34
import type {
@@ -79,17 +80,19 @@ export function parseObjectRegistry(
7980
if ((matches = key.match(/^(v\d+)\.c\.(ko\d+)$/u))) {
8081
matches[1] &&
8182
objCList.push({
82-
vat: matches[1] ?? '',
83-
kref: (matches[2] ?? '') as KRef,
83+
vat: matches[1],
84+
// Safe cast: regex capture group guarantees KRef format
85+
kref: matches[2] as KRef,
8486
eref: value.replace(/^R\s*/u, ''),
8587
});
8688
continue;
8789
}
8890
if ((matches = key.match(/^(v\d+)\.c\.(kp\d+)$/u))) {
8991
matches[1] &&
9092
prmCList.push({
91-
vat: matches[1] ?? '',
92-
kref: (matches[2] ?? '') as KRef,
93+
vat: matches[1],
94+
// Safe cast: regex capture group guarantees KRef format
95+
kref: matches[2] as KRef,
9396
eref: value.replace(/^R\s*/u, ''),
9497
});
9598
continue;
@@ -113,9 +116,10 @@ export function parseObjectRegistry(
113116

114117
// Helper to resolve slots
115118
const resolveSlot = (kref: string): SlotInfo => {
119+
insistKRef(kref);
116120
const entry = objCList.find((item) => item.kref === kref);
117121
return {
118-
kref: kref as KRef,
122+
kref,
119123
eref: entry?.eref ?? null,
120124
vat: entry?.vat ?? null,
121125
};

packages/ocap-kernel/src/store/README.md

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22

33
The kernel store (`makeKernelStore` in `index.ts`) wraps the raw
44
`KernelDatabase` from `@metamask/kernel-store` and exposes typed methods for
5-
all kernel-state access. Raw `kv` is intentionally not exposed — all reads and
6-
writes go through these methods to preserve branded type safety.
5+
all kernel-state access. The raw `kv` store is not exposed through
6+
`KernelStore` — production code reads and writes go through typed methods to
7+
preserve branded type safety. (Test code may access `database.kernelKVStore`
8+
directly.)
79

810
## Data integrity
911

@@ -35,7 +37,8 @@ Validated writes are the first line of defense:
3537

3638
- All values entering the store pass through typed store methods or validated
3739
constructors (e.g., `makeGCAction()`).
38-
- Migration correctness: schema changes must transform all affected keys.
40+
- Any future schema migrations must transform all affected keys to maintain
41+
typed invariants.
3942

4043
See the trust model documentation at the top of `../types.ts` for how branded
4144
types are validated at other boundaries (external input, translators, internal

packages/ocap-kernel/src/store/index.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -522,4 +522,33 @@ describe('kernel store', () => {
522522
expect(preserved).toBe(original);
523523
});
524524
});
525+
526+
describe('getKnownRelays', () => {
527+
it('returns empty array when no relays are stored', () => {
528+
const ks = makeKernelStore(mockKernelDatabase);
529+
expect(ks.getKnownRelays()).toStrictEqual([]);
530+
});
531+
532+
it('returns stored relays', () => {
533+
const ks = makeKernelStore(mockKernelDatabase);
534+
ks.setKnownRelays(['peer1', 'peer2']);
535+
expect(ks.getKnownRelays()).toStrictEqual(['peer1', 'peer2']);
536+
});
537+
538+
it('throws when stored knownRelays is not a JSON array', () => {
539+
const ks = makeKernelStore(mockKernelDatabase);
540+
mockKernelDatabase.kernelKVStore.set('knownRelays', '"not-an-array"');
541+
expect(() => ks.getKnownRelays()).toThrow(
542+
'knownRelays must be an array of strings',
543+
);
544+
});
545+
546+
it('throws when stored knownRelays contains non-string entries', () => {
547+
const ks = makeKernelStore(mockKernelDatabase);
548+
mockKernelDatabase.kernelKVStore.set('knownRelays', '[1, 2, 3]');
549+
expect(() => ks.getKnownRelays()).toThrow(
550+
'knownRelays must be an array of strings',
551+
);
552+
});
553+
});
525554
});

packages/ocap-kernel/src/types.test.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,8 @@ describe('isVatId', () => {
429429
{ name: 'missing number part', value: 'v' },
430430
{ name: 'non-numeric suffix', value: 'va' },
431431
{ name: 'mixed suffix', value: 'v1a' },
432+
{ name: 'float suffix', value: 'v1.5' },
433+
{ name: 'negative', value: 'v-1' },
432434
{ name: 'number', value: 123 },
433435
{ name: 'null', value: null },
434436
])('returns false for $name', ({ value }) => {
@@ -745,6 +747,7 @@ describe('insistGCAction', () => {
745747
it.each([
746748
{ name: 'invalid format', value: 'invalid' },
747749
{ name: 'invalid vatId', value: 'invalid dropExport ko123' },
750+
{ name: 'invalid action type', value: 'v1 invalidAction ko123' },
748751
{ name: 'number', value: 123 },
749752
])('throws for $name', ({ value }) => {
750753
expect(() => insistGCAction(value)).toThrow('not a valid GCAction');

packages/ocap-kernel/src/vats/VatHandle.test.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@ import type { MockInstance } from 'vitest';
1111
import type { KernelQueue } from '../KernelQueue.ts';
1212
import { makeKernelStore } from '../store/index.ts';
1313
import type { KernelStore } from '../store/index.ts';
14-
import type { VRef, EndpointMessage, VatDeliveryResult } from '../types.ts';
14+
import type {
15+
VRef,
16+
ERef,
17+
EndpointMessage,
18+
VatDeliveryResult,
19+
} from '../types.ts';
1520
import { VatHandle } from './VatHandle.ts';
1621
import { makeMapKernelDatabase } from '../../test/storage.ts';
1722

@@ -147,10 +152,10 @@ describe('VatHandle', () => {
147152
sendVatCommandMock.mockReset();
148153
const mockResult: VatDeliveryResult = [[[], []], null];
149154
sendVatCommandMock.mockResolvedValueOnce(mockResult);
150-
const target = 'kp1' as VRef;
155+
const target = 'o+0' as VRef;
151156
const message: EndpointMessage = {
152157
methargs: { body: '["arg1","arg2"]', slots: [] },
153-
result: 'kp123',
158+
result: 'p+1' as ERef,
154159
};
155160
await vat.deliverMessage(target, message);
156161
expect(sendVatCommandMock).toHaveBeenCalledTimes(1);

0 commit comments

Comments
 (0)