Skip to content

Commit 5a189c4

Browse files
szdziedzicclaude
szdziedzic
and
claude
committed
fix: tighten daemon policy batch preflight and provider inventory
- Batch preflight checks each step's close --shutdown and udid/serial, not only its command name, so the whole batch is refused up front. - An empty udid/serial names no device and is not a policy denial. - A provider answering with only out-of-scope devices falls back to filtered local inventory instead of hiding allowed local devices. - Digest test pins all three rule sections. - ADR 0029: remote clients neither send nor verify the policy env. Co-Authored-By: Claude Opus 5.5 <[email protected]>
1 parent 58d5723 commit 5a189c4

4 files changed

Lines changed: 113 additions & 21 deletions

File tree

‎docs/adr/0029-daemon-policy.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ means a running daemon has exactly one policy, and its digest identifies it.
101101
claims, session teardown) is not request work and is not device-scoped: refusing it would leak
102102
the processes and claims it exists to release. It acts only on records in the daemon's own state
103103
dir. The capability gate still applies, so recovery cannot shut a device down.
104+
- `AGENT_DEVICE_DAEMON_POLICY` configures a daemon started on this host. A client connected to a
105+
remote daemon (`AGENT_DEVICE_DAEMON_BASE_URL`, `connect proxy`) neither sends nor verifies it;
106+
the remote host's operator owns that daemon's policy. Publishing the digest over `/health` so a
107+
remote client can check it is a follow-up that needs a daemon wire change.
104108
- An invalid policy's startup error names the policy path on the daemon's stderr. That output
105109
reaches only the host process that started the daemon, and the operator needs the path to fix
106110
the file; request-time denials never name it.

‎src/__tests__/daemon-policy-file.test.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,16 @@ test('the digest names the rules, not their order or source path', () => {
5252
},
5353
'/elsewhere/policy.json',
5454
);
55-
const different = parseDaemonPolicy({ version: 1, commands: { deny: ['boot'] } }, SOURCE);
55+
const base = { version: 1, devices: { allow: [{ udid: 'a' }] }, commands: { deny: ['boot'] } };
56+
const baseDigest = parseDaemonPolicy(base, SOURCE).digest;
57+
const variants = [
58+
{ ...base, commands: { deny: ['boot', 'shutdown'] } },
59+
{ ...base, devices: { allow: [{ udid: 'b' }] } },
60+
{ ...base, capabilities: { deny: ['device-shutdown'] } },
61+
];
5662

5763
expect(reordered.digest).toBe(first.digest);
58-
expect(different.digest).not.toBe(first.digest);
64+
for (const variant of variants) {
65+
expect(parseDaemonPolicy(variant, SOURCE).digest).not.toBe(baseDigest);
66+
}
5967
});

‎src/daemon/__tests__/daemon-policy-router.test.ts‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { makeIosSession } from '../../__tests__/test-utils/session-factories.ts'
1212
import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts';
1313
import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts';
1414
import { parseDaemonPolicy } from '../../daemon-policy-file.ts';
15+
import { assertDaemonPolicyAdmitsRequest } from '../daemon-policy.ts';
1516
import type { DaemonRequest } from '../daemon-request.ts';
1617
import { LeaseRegistry } from '../lease-registry.ts';
1718
import {
@@ -35,7 +36,7 @@ function policy(raw: Record<string, unknown>) {
3536

3637
function makeHandler(
3738
daemonPolicy: ReturnType<typeof policy>,
38-
options: { inventory?: readonly DeviceInfo[] } = {},
39+
options: { inventory?: readonly DeviceInfo[]; providerInventory?: readonly DeviceInfo[] } = {},
3940
) {
4041
const sessionStore = makeSessionStore('agent-device-daemon-policy-');
4142
sessionStore.set('default', makeIosSession('default', { appBundleId: 'com.example.app' }));
@@ -49,6 +50,14 @@ function makeHandler(
4950
deviceRuntimeGateway: { ...lifecycleDeviceRuntimeGateway, bind, inspectFacts },
5051
deviceInventoryGateways: createTestDeviceInventoryGateways({
5152
local: async () => options.inventory ?? [],
53+
provider: options.providerInventory
54+
? {
55+
discover: async () => ({
56+
kind: 'inventory',
57+
devices: [...(options.providerInventory ?? [])],
58+
}),
59+
}
60+
: undefined,
5261
}),
5362
trackDownloadableArtifact: () => 'artifact-id',
5463
daemonPolicy,
@@ -161,3 +170,56 @@ test('device inventory lists only the devices the policy allows', async () => {
161170
data: { devices: [expect.objectContaining({ id: IOS_SIMULATOR.id })] },
162171
});
163172
});
173+
174+
test('a batch step that shuts down or names a foreign device is refused before any step runs', async () => {
175+
const { handler } = makeHandler(
176+
policy({
177+
devices: { allow: [{ udid: IOS_SIMULATOR.id }] },
178+
capabilities: { deny: ['device-shutdown'] },
179+
}),
180+
);
181+
182+
const shutdownStep = await handler(
183+
request('batch', {
184+
flags: {
185+
batchSteps: [{ command: 'app-switcher' }, { command: 'close', flags: { shutdown: true } }],
186+
},
187+
}),
188+
);
189+
const foreignStep = await handler(
190+
request('batch', {
191+
flags: {
192+
batchSteps: [
193+
{ command: 'app-switcher' },
194+
{ command: 'open', flags: { udid: OTHER_SIMULATOR.id } },
195+
],
196+
},
197+
}),
198+
);
199+
200+
expectPolicyDenied(shutdownStep, 'capability');
201+
expectPolicyDenied(foreignStep, 'device');
202+
expect(systemRuntimeSpies.appSwitcher).not.toHaveBeenCalled();
203+
});
204+
205+
test('an empty device selector names no device and is not a policy denial', () => {
206+
const pinned = policy({ devices: { allow: [{ udid: IOS_SIMULATOR.id }] } });
207+
208+
expect(() =>
209+
assertDaemonPolicyAdmitsRequest(pinned, request('open', { flags: { udid: '', serial: ' ' } })),
210+
).not.toThrow();
211+
});
212+
213+
test('a provider answering with only out-of-scope devices does not hide allowed local ones', async () => {
214+
const { handler } = makeHandler(policy({ devices: { allow: [{ udid: IOS_SIMULATOR.id }] } }), {
215+
inventory: [IOS_SIMULATOR],
216+
providerInventory: [OTHER_SIMULATOR],
217+
});
218+
219+
const response = await handler(request('devices', { session: undefined }));
220+
221+
expect(response).toMatchObject({
222+
ok: true,
223+
data: { devices: [expect.objectContaining({ id: IOS_SIMULATOR.id })] },
224+
});
225+
});

‎src/daemon/daemon-policy.ts‎

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,21 +26,33 @@ const PUBLIC_COMMAND_NAMES: ReadonlySet<string> = new Set(Object.values(PUBLIC_C
2626

2727
/** Refuses a request the policy denies before it resolves a device or takes a lock. */
2828
export function assertDaemonPolicyAdmitsRequest(policy: DaemonPolicy, req: DaemonRequest): void {
29-
assertCommandAdmitted(policy, req.command);
30-
if (req.command === 'batch') {
31-
for (const step of req.flags?.batchSteps ?? []) {
32-
assertCommandAdmitted(policy, normalizeBatchCommandName(step.command));
33-
}
29+
assertInvocationAdmitted(policy, req.command, req.flags ?? {});
30+
if (req.command !== 'batch') return;
31+
// Refuse the whole batch before any step runs; each step is admitted again when it runs.
32+
for (const step of req.flags?.batchSteps ?? []) {
33+
assertInvocationAdmitted(policy, normalizeBatchCommandName(step.command), {
34+
...step.input,
35+
...step.flags,
36+
});
3437
}
35-
if (req.command === 'close' && req.flags?.shutdown === true) {
38+
}
39+
40+
function assertInvocationAdmitted(
41+
policy: DaemonPolicy,
42+
command: string,
43+
fields: Readonly<Record<string, unknown>>,
44+
): void {
45+
assertCommandAdmitted(policy, command);
46+
if (command === 'close' && fields.shutdown === true) {
3647
assertDaemonPolicyAllowsCapability(policy, 'device-shutdown');
3748
}
38-
for (const id of [req.flags?.udid, req.flags?.serial]) {
39-
if (id !== undefined && policy.deviceIds && !policy.deviceIds.has(id.trim())) {
40-
throw policyDenied(policy, 'device', `Device ${id} is outside this daemon's policy.`, {
41-
deviceId: id,
42-
});
43-
}
49+
if (!policy.deviceIds) return;
50+
for (const value of [fields.udid, fields.serial]) {
51+
const id = typeof value === 'string' ? value.trim() : '';
52+
if (!id || policy.deviceIds.has(id)) continue;
53+
throw policyDenied(policy, 'device', `Device ${id} is outside this daemon's policy.`, {
54+
deviceId: id,
55+
});
4456
}
4557
}
4658

@@ -77,13 +89,19 @@ export function restrictDeviceInventoryToDaemonPolicy(
7789
const localOnly: DeviceInventoryGateway = Object.freeze({
7890
discover: async (request, scope) => allowed(await gateways.localOnly.discover(request, scope)),
7991
});
92+
// A provider that answers with only out-of-scope devices must not hide allowed local devices.
93+
const discoverWithSource: ProviderAwareDeviceInventoryGateway['discoverWithSource'] = async (
94+
request,
95+
scope,
96+
) => {
97+
const discovery = await gateways.providerFirst.discoverWithSource(request, scope);
98+
const devices = allowed(discovery.devices);
99+
if (devices.length > 0 || discovery.source === 'local') return { ...discovery, devices };
100+
return { devices: await localOnly.discover(request, scope), source: 'local' };
101+
};
80102
const providerFirst: ProviderAwareDeviceInventoryGateway = Object.freeze({
81-
discover: async (request, scope) =>
82-
allowed(await gateways.providerFirst.discover(request, scope)),
83-
discoverWithSource: async (request, scope) => {
84-
const discovery = await gateways.providerFirst.discoverWithSource(request, scope);
85-
return { ...discovery, devices: allowed(discovery.devices) };
86-
},
103+
discover: async (request, scope) => (await discoverWithSource(request, scope)).devices,
104+
discoverWithSource,
87105
});
88106
return Object.freeze({ ...gateways, localOnly, providerFirst });
89107
}

0 commit comments

Comments
 (0)