alee/relicario
1
0
Fork 0
Code Issues Pull Requests 6 Actions Packages Projects Releases Wiki Activity

fix(ext/sw): org_add_item mints a real item id (C1 data-loss) + signer hardening

Browse Source 
C1 (CRITICAL, caught by the PM's independent reviewer; masked by a test that
hard-coded the id): the popup sends id:'' for NEW items — the personal add_item
handler mints one via new_item_id(), but handleOrgAddItem used item.id verbatim.
Every new org item therefore wrote items/<collection>/.enc, so the 2nd add to a
collection OVERWROTE the first (blob data loss) + accumulated duplicate empty-id
manifest entries. Fix: mint the id with w.new_item_id() and use it for the blob
path, the manifest entry, and the encrypted item.

Also:
- m4: sanitize the device name in buildSigner (strip CR/LF/<>) + derive a safe
  email local-part, so a crafted device name can't malform the signed commit's
  author line.
- m1: add update/delete ungranted-collection refusal tests (parity with add).
- Regression test: org_add_item with id:'' now asserts a real 16-hex id lands in
  the path + manifest entry (the test that masked C1 is fixed to mint the id).
- m3 is eliminated by the C1 fix; m2 (TOCTOU dual-read) + m5 (zero-grant +new /
  push-rejection copy) tracked as non-blocking follow-ups.

Full extension suite 584/584; build:all clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LKBbrAjmpVXMEK15pNi3Ha
This commit is contained in:
adlee-was-taken
2026-06-27 18:23:39 -04:00
parent 1ce37f714e
commit df2954dd26
2 changed files with 109 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

90
extension/src/service-worker/__tests__/org-write.test.ts
View File

@@ -72,6 +72,11 @@ const FILTERED_MANIFEST: OrgManifest = {
// Fake encrypted manifest bytes returned by host.readFile('manifest.enc').
const FAKE_ENC_MANIFEST = new Uint8Array([0xaa, 0xbb, 0xcc]);
// The id new_item_id() mints for a new org item. C1 fix: handleOrgAddItem ignores
// the popup's item.id (which is '' for NEW items) and mints a fresh id, so the blob
// path + manifest entry never collide on the empty id.
const MINTED_ID = 'abcd1234abcd1234';
const NEW_ITEM: Item = {
id: 'item-new-001',
title: 'New Login',
@@ -119,6 +124,8 @@ function makeW() {
),
org_manifest_encrypt: vi.fn().mockReturnValue(new Uint8Array([1, 2, 3])),
item_encrypt: vi.fn().mockReturnValue(new Uint8Array([4, 5, 6])),
// new_item_id mints a fresh 16-hex id for new items (C1 fix).
new_item_id: vi.fn().mockReturnValue(MINTED_ID),
// org_manifest_decrypt_filtered is called after every write to re-sync state.manifest.
// Default returns FILTERED_MANIFEST; override per-test to assert specific post-write views.
org_manifest_decrypt_filtered: vi.fn().mockReturnValue(FILTERED_MANIFEST),
@@ -190,7 +197,8 @@ describe('org write handlers', () => {
{ signingPubOpenssh: string },
];
const paths = files.map(f => f.path).sort();
expect(paths).toEqual([`items/prod-infra/${NEW_ITEM.id}.enc`, 'manifest.enc'].sort());
// Path uses the MINTED id (C1 fix), not the popup-supplied item.id.
expect(paths).toEqual([`items/prod-infra/${MINTED_ID}.enc`, 'manifest.enc'].sort());
expect(signer.signingPubOpenssh).toBe('ssh-ed25519 AAAA test');
});
@@ -215,7 +223,7 @@ describe('org write handlers', () => {
expect(collections.has('secret-ops')).toBe(true);
// 2 original entries + 1 new one
expect(written.entries).toHaveLength(3);
expect(written.entries.find(e => e.id === NEW_ITEM.id)?.collection).toBe('prod-infra');
expect(written.entries.find(e => e.id === MINTED_ID)?.collection).toBe('prod-infra');
});
// Test 4: update mutates the matching entry and dual-writes
@@ -372,4 +380,82 @@ describe('org write handlers', () => {
expect(grants).toEqual(['prod-infra']);
expect(fakeState.manifest).toBe(FILTERED_AFTER_DELETE);
});
// Test 10 (C1 regression): the popup sends id:'' for NEW items. The handler MUST
// mint a real id — never write items/<collection>/.enc (which would overwrite the
// previously-added item in the same collection) or push a duplicate empty-id entry.
it('C1: org_add_item with empty item.id mints a real id for BOTH the blob path and the manifest entry', async () => {
const w = makeW();
const fakeState = makeFakeState();
await switchToOrg(fakeState, w);
const newWithEmptyId: Item = { ...NEW_ITEM, id: '' };
const result = await handleOrgAddItem({ collection: 'prod-infra', item: newWithEmptyId }, w);
expect(result).toEqual({ ok: true });
expect(w.new_item_id).toHaveBeenCalled();
// Blob path must use the minted id — NOT items/prod-infra/.enc.
const commitSigned = fakeState.host.commitSigned as ReturnType<typeof vi.fn>;
const [files] = commitSigned.mock.calls[0] as [Array<{ path: string }>];
const itemPath = files.map(f => f.path).find(p => p.startsWith('items/'));
expect(itemPath).toBe(`items/prod-infra/${MINTED_ID}.enc`);
expect(itemPath).not.toBe('items/prod-infra/.enc');
// Manifest entry must carry the minted id; no empty-id entry may exist.
const manifestEncrypt = w.org_manifest_encrypt as ReturnType<typeof vi.fn>;
const [, jsonArg] = manifestEncrypt.mock.calls[0] as [unknown, string];
const written = JSON.parse(jsonArg) as OrgManifest;
expect(written.entries.some(e => e.id === MINTED_ID && e.collection === 'prod-infra')).toBe(true);
expect(written.entries.some(e => e.id === '')).toBe(false);
// The ENCRYPTED item must also carry the minted id (item_encrypt gets withId).
const itemEncrypt = w.item_encrypt as ReturnType<typeof vi.fn>;
const [, itemJson] = itemEncrypt.mock.calls[0] as [unknown, string];
expect((JSON.parse(itemJson) as Item).id).toBe(MINTED_ID);
});
// Test 11 (m1): update parity with add — an entry in an ungranted collection is refused.
it('org_update_item: entry in an ungranted collection → collection_not_granted; commitSigned not called', async () => {
const w = makeW();
const fakeState = makeFakeState(); // granted prod-infra only
await switchToOrg(fakeState, w);
// item-s1 lives in secret-ops (ungranted).
const result = await handleOrgUpdateItem({ id: 'item-s1', item: { ...UPDATED_ITEM, id: 'item-s1' } }, w);
expect(result).toEqual({ ok: false, error: 'collection_not_granted' });
expect(fakeState.host.commitSigned as ReturnType<typeof vi.fn>).not.toHaveBeenCalled();
});
// Test 12 (m1): delete parity with add — an entry in an ungranted collection is refused.
it('org_delete_item: entry in an ungranted collection → collection_not_granted; commitSigned not called', async () => {
const w = makeW();
const fakeState = makeFakeState();
await switchToOrg(fakeState, w);
const result = await handleOrgDeleteItem({ id: 'item-s1' }, w);
expect(result).toEqual({ ok: false, error: 'collection_not_granted' });
expect(fakeState.host.commitSigned as ReturnType<typeof vi.fn>).not.toHaveBeenCalled();
});
// Test 13 (m4): a device name containing CR/LF/<> must be sanitized so the signed
// commit object's `author NAME <EMAIL> …` line cannot be malformed/injected.
it('m4: buildSigner sanitizes a device name with newline / < / >', async () => {
const w = makeW();
w.get_device_info.mockReturnValue({
name: 'evil\nname <injected>',
signing_public_key: 'ssh-ed25519 AAAA test',
deploy_public_key: 'ssh-ed25519 BBBB test',
});
const fakeState = makeFakeState();
await switchToOrg(fakeState, w);
await handleOrgAddItem({ collection: 'prod-infra', item: NEW_ITEM }, w);
const commitSigned = fakeState.host.commitSigned as ReturnType<typeof vi.fn>;
const [, , signer] = commitSigned.mock.calls[0] as [unknown, string, { author: { name: string; email: string } }];
expect(signer.author.name).not.toMatch(/[\r\n<>]/);
expect(signer.author.email).toMatch(/^[A-Za-z0-9._-]+@relicario\.device$/);
});
});

30
extension/src/service-worker/router/org-handlers.ts
View File

@@ -120,8 +120,14 @@ function buildSigner(w: WasmModule): CommitSigner | null {
deploy_public_key: string;
} | null;
if (!info) return null;
// Sanitize the device name: CR/LF/<> would malform the commit object's
// `author NAME <EMAIL> …` line. The identity is cosmetic to the org hook (it
// matches on the signing-key fingerprint, not the text), so a sanitized
// display name + a safe email local-part is sufficient.
const name = info.name.replace(/[\r\n<>]/g, ' ').trim() || 'relicario-device';
const emailLocal = name.replace(/[^A-Za-z0-9._-]/g, '-') || 'device';
return {
author: { name: info.name, email: `${info.name}@relicario.device` },
author: { name, email: `${emailLocal}@relicario.device` },
signingPubOpenssh: info.signing_public_key,
rawSign: (b: Uint8Array) => base64ToUint8Array((w.sign_for_git(b) as { signature: string }).signature),
};
@@ -148,22 +154,28 @@ export async function handleOrgAddItem(
// LANDMINE: decrypt FULL (unfiltered) manifest — never use state.manifest.
const enc = await state.host.readFile('manifest.enc');
const full = w.org_manifest_decrypt(state.handle, enc) as OrgManifest;
// The popup sends id:'' for NEW items; the personal add_item handler mints the
// id via new_item_id() and the org path must do the same. Using item.id
// verbatim would write every new item to items/<collection>/.enc and OVERWRITE
// the previous one (blob data loss + duplicate empty-id manifest entries).
const id = w.new_item_id() as string;
const withId: Item = { ...item, id };
full.entries.push({
id: item.id,
type: item.type,
title: item.title,
tags: item.tags,
modified: item.modified,
id,
type: withId.type,
title: withId.title,
tags: withId.tags,
modified: withId.modified,
collection,
});
const manifestBytes = w.org_manifest_encrypt(state.handle, JSON.stringify(full)) as Uint8Array;
const itemBytes = w.item_encrypt(state.handle, JSON.stringify(item)) as Uint8Array;
const itemBytes = w.item_encrypt(state.handle, JSON.stringify(withId)) as Uint8Array;
await state.host.commitSigned(
[
{ path: `items/${collection}/${item.id}.enc`, content: itemBytes },
{ path: `items/${collection}/${id}.enc`, content: itemBytes },
{ path: 'manifest.enc', content: manifestBytes },
],
`org add ${item.type} to ${collection}`,
`org add ${withId.type} to ${collection}`,
signer,
);
// Re-sync the cached grant-filtered manifest so org_list_items reflects this write.