Skip to content

Commit 2230e33

Browse files
committed
fix(fill): redact only a sensitive fill's own text from its error
- The error exit replaced every value registered for the request across the whole error. Maestro inputText and --record-as literals are registered too, so a short value such as "1" rewrote logPath, diagnosticId and the error of a non-fill replay. Only a fill marked sensitive is redacted now, only its own text, and only in message, hint, cause and details, with the same placeholder as the success path (${VAR} with --record-as, [REDACTED] otherwise). - Refuse textStdin on a daemon batch step: a failed step returns its positionals, and the CLI already refuses it there. - Remove redactRegisteredSensitiveValues from host-kit.
1 parent 0115290 commit 2230e33

10 files changed

Lines changed: 236 additions & 35 deletions

File tree

‎packages/ad-script/src/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ export {
6969
isSensitiveFillText,
7070
readRecordedInputVariableName,
7171
recordedInputPlaceholder,
72+
sensitiveFillPlaceholder,
7273
validateRecordedInputVariableName,
7374
} from './internal/recorded-input.ts';
7475

‎packages/ad-script/src/internal/recorded-input.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ export function isSensitiveFillText<TFlags extends Pick<CommandFlags, 'recordAs'
3232
return typeof flags?.recordAs === 'string' || flags?.textStdin === true;
3333
}
3434

35+
/** What sensitive fill text reads as on any response: its `--record-as` placeholder, else `[REDACTED]`. */
36+
export function sensitiveFillPlaceholder(
37+
flags: Pick<CommandFlags, 'recordAs'> | undefined,
38+
): string {
39+
return typeof flags?.recordAs === 'string'
40+
? recordedInputPlaceholder(flags.recordAs)
41+
: '[REDACTED]';
42+
}
43+
3544
export function recordedInputPlaceholder(variableName: string): string {
3645
return `\${${variableName}}`;
3746
}

‎packages/command-registry/src/batch.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,7 @@ export function validateAndNormalizeBatchSteps(
222222
if (step.flags !== undefined && !isRecord(step.flags)) {
223223
throw new AppError('INVALID_ARGS', `Batch step ${index + 1} flags must be an object.`);
224224
}
225+
assertNoTextStdinStep(step.flags as Record<string, unknown> | undefined, index + 1);
225226
if (step.input !== undefined && !isRecord(step.input)) {
226227
throw new AppError('INVALID_ARGS', `Batch step ${index + 1} input must be an object.`);
227228
}
@@ -236,6 +237,22 @@ export function validateAndNormalizeBatchSteps(
236237
return normalized;
237238
}
238239

240+
/**
241+
* A batch step has no stdin of its own, and a failed step reports its positionals. The CLI refuses
242+
* `textStdin` on a step before reading stdin; this refuses it for every other daemon client.
243+
*/
244+
function assertNoTextStdinStep(
245+
flags: Record<string, unknown> | undefined,
246+
stepNumber: number,
247+
): void {
248+
if (flags?.textStdin === undefined) return;
249+
throw new AppError(
250+
'INVALID_ARGS',
251+
`Batch step ${stepNumber}: fill textStdin is not supported because batch steps have no stdin of their own.`,
252+
{ reason: 'fill_text_stdin_in_batch' },
253+
);
254+
}
255+
239256
function buildBatchStepFlags(
240257
parentFlags: BatchFlags | Record<string, unknown> | undefined,
241258
stepFlags: DaemonBatchStep['flags'] | Record<string, unknown> | undefined,

‎packages/host-kit/src/diagnostics.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ export {
55
emitDiagnostic,
66
flushDiagnosticsToSessionFile,
77
getDiagnosticsMeta,
8-
redactRegisteredSensitiveValues,
98
registerDiagnosticSensitiveValue,
109
type ResourceDiagnostic,
1110
updateDiagnosticsScope,

‎packages/host-kit/src/internal/diagnostics.ts‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,16 +107,6 @@ export function registerDiagnosticSensitiveValue(value: string): void {
107107
scope.sortedSensitiveValues = undefined;
108108
}
109109

110-
/**
111-
* Replace this request's registered sensitive values in `input`, as its diagnostics already do. For
112-
* an error leaving the daemon: a backend message can echo a value the caller kept out of argv.
113-
*/
114-
export function redactRegisteredSensitiveValues<T>(input: T): T {
115-
const scope = diagnosticsStorage.getStore();
116-
if (!scope || scope.sensitiveValues.size === 0) return input;
117-
return replaceSensitiveValues(input, sortedScopeSensitiveValues(scope)) as T;
118-
}
119-
120110
/**
121111
* Sum the number of diagnostic events emitted in the current scope whose phase
122112
* is one of `phases`. Backed by the flush-surviving `phaseCounts` tally, so it
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/**
2+
* The router redacts an error only for a fill the caller marked sensitive. Another command can
3+
* register sensitive values too (a Maestro `inputText` does), and its error, including a
4+
* `logPath` that happens to contain such a value, comes back unchanged.
5+
*/
6+
import fs from 'node:fs';
7+
import path from 'node:path';
8+
import { expect, test, vi } from 'vitest';
9+
10+
vi.mock('@agent-device/platform-apple/runner/operations', async (importOriginal) => {
11+
const actual =
12+
await importOriginal<typeof import('@agent-device/platform-apple/runner/operations')>();
13+
return { ...actual, stopIosRunnerSession: vi.fn(async () => {}) };
14+
});
15+
16+
vi.mock('../device/device-ready.ts', () => ({ ensureDeviceReady: vi.fn(async () => {}) }));
17+
18+
import { AppError } from '@agent-device/kernel/errors';
19+
import type { DeviceRuntimeGateway, RuntimeFacts } from '@agent-device/contracts/platform-runtime';
20+
import type { PlatformRuntimeOperations } from '@agent-device/contracts/platform-runtime-operations';
21+
import { createTestDeviceInventoryGateways } from '../../__tests__/test-utils/device-inventory-gateways.ts';
22+
import { makeIosAppSession } from '../../__tests__/test-utils/session-factories.ts';
23+
import { makeSessionStore } from '../../__tests__/test-utils/store-factory.ts';
24+
import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts';
25+
import { LeaseRegistry } from '../lease-registry.ts';
26+
import {
27+
createRequestHandler,
28+
lifecycleDeviceRuntimeGateway,
29+
} from './test-device-runtime-gateway.ts';
30+
31+
const SESSION = 'maestro-input';
32+
33+
const typeText = vi.fn(async (_input: { text: string }) => {
34+
throw new AppError('COMMAND_FAILED', 'keyboard is not ready');
35+
});
36+
37+
function withTypeText(
38+
facts: RuntimeFacts<PlatformRuntimeOperations>,
39+
): RuntimeFacts<PlatformRuntimeOperations> {
40+
return { ...facts, operations: { ...facts.operations, typeText: { available: true } } };
41+
}
42+
43+
const typingDeviceRuntimeGateway: DeviceRuntimeGateway<PlatformRuntimeOperations> = {
44+
inspectFacts: async (device) =>
45+
withTypeText(await lifecycleDeviceRuntimeGateway.inspectFacts(device)),
46+
bind: async (request) => {
47+
const binding = await lifecycleDeviceRuntimeGateway.bind(request);
48+
return {
49+
...binding,
50+
facts: withTypeText(binding.facts),
51+
operations: { ...binding.operations, typeText },
52+
};
53+
},
54+
shutdown: async () => {},
55+
};
56+
57+
test('a Maestro replay error comes back unchanged after its inputText registered a short value', async () => {
58+
const sessionStore = makeSessionStore('agent-device-router-error-redaction-');
59+
sessionStore.publish(SESSION, makeIosAppSession(SESSION));
60+
const root = mkdtempForTestSync('agent-device-router-error-redaction-');
61+
const flowPath = path.join(root, 'flow.yaml');
62+
fs.writeFileSync(flowPath, 'appId: com.example.app\n---\n- inputText: "1"\n');
63+
const handler = createRequestHandler({
64+
logPath: path.join(root, 'daemon.log'),
65+
token: 'test-token',
66+
sessionStore,
67+
leaseRegistry: new LeaseRegistry(),
68+
deviceInventoryGateways: createTestDeviceInventoryGateways(),
69+
trackDownloadableArtifact: () => 'artifact-id',
70+
deviceRuntimeGateway: typingDeviceRuntimeGateway,
71+
});
72+
73+
const result = await handler({
74+
token: 'test-token',
75+
session: SESSION,
76+
command: 'replay',
77+
positionals: [flowPath],
78+
flags: { replayBackend: 'maestro', platform: 'ios' },
79+
meta: { requestId: 'req-maestro-input-1' },
80+
});
81+
82+
expect(typeText).toHaveBeenCalledWith(expect.objectContaining({ text: '1' }));
83+
expect(result.ok).toBe(false);
84+
if (result.ok) return;
85+
expect(JSON.stringify(result.error)).not.toContain('[REDACTED]');
86+
expect(result.error.logPath).toContain('req-maestro-input-1');
87+
expect(fs.existsSync(result.error.logPath!)).toBe(true);
88+
});

‎src/daemon/__tests__/request-router-fill-text-stdin.test.ts‎

Lines changed: 68 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
1-
import { createTestDeviceInventoryGateways } from '../../__tests__/test-utils/device-inventory-gateways.ts';
21
/**
32
* #3260: `fill --text-stdin` keeps the value out of argv, so it must not come back on the
43
* response either. Sent through the real request handler to a web session, the narrowest
54
* runtime that still runs the full fill admission, dispatch and response projection.
65
*/
76
import { expect, test } from 'vitest';
7+
import fs from 'node:fs';
88
import path from 'node:path';
9+
import { createTestDeviceInventoryGateways } from '../../__tests__/test-utils/device-inventory-gateways.ts';
910
import type { WebProvider } from '@agent-device/platform-web';
11+
import type { DaemonRequest } from '../daemon-request.ts';
1012
import type { SessionState } from '../session-state.ts';
1113
import { createRequestHandler } from './test-device-runtime-gateway.ts';
1214
import { LeaseRegistry } from '../lease-registry.ts';
@@ -25,6 +27,19 @@ function fillWithStdinText(
2527
session: SessionState,
2628
flags: Record<string, unknown> = {},
2729
fillFails?: (text: string) => Error,
30+
text = SECRET,
31+
) {
32+
return sendToWebSession(
33+
session,
34+
{ command: 'fill', positionals: ['10', '20', text], flags: { textStdin: true, ...flags } },
35+
fillFails,
36+
);
37+
}
38+
39+
function sendToWebSession(
40+
session: SessionState,
41+
request: Pick<DaemonRequest, 'command' | 'positionals' | 'flags'>,
42+
fillFails?: (text: string) => Error,
2843
) {
2944
const sessionStore = makeSessionStore('agent-device-router-fill-text-stdin-');
3045
sessionStore.publish(session.name, session);
@@ -65,9 +80,7 @@ function fillWithStdinText(
6580
const response = handler({
6681
token: 'test-token',
6782
session: session.name,
68-
command: 'fill',
69-
positionals: ['10', '20', SECRET],
70-
flags: { textStdin: true, ...flags },
83+
...request,
7184
meta: { requestId: 'req-fill-text-stdin' },
7285
});
7386
return { response, typed, sessionStore };
@@ -142,3 +155,54 @@ test('a backend error that echoes the --text-stdin value does not return it', as
142155
expect(JSON.stringify(result)).not.toContain(SECRET);
143156
if (!result.ok) expect(result.error.message).toBe('could not type "[REDACTED]" into the field');
144157
});
158+
159+
test('a backend error that echoes a --record-as stdin value shows its placeholder', async () => {
160+
const { response, typed } = fillWithStdinText(
161+
makeAuthoringSession('web', { device: WEB_DESKTOP_DEVICE }),
162+
{ recordAs: 'PASSWORD' },
163+
(text) => new Error(`could not type "${text}" into the field`),
164+
);
165+
166+
const result = await response;
167+
expect(result.ok).toBe(false);
168+
expect(typed).toEqual([SECRET]);
169+
if (!result.ok) expect(result.error.message).toBe('could not type "${PASSWORD}" into the field');
170+
});
171+
172+
test('a short --text-stdin value is redacted from the error text but not from its log path', async () => {
173+
// `stdin` also appears in this request's id, which names its diagnostics file.
174+
const { response } = fillWithStdinText(
175+
makeSession('web', { device: WEB_DESKTOP_DEVICE }),
176+
{},
177+
(text) => new Error(`could not type "${text}" into the field`),
178+
'stdin',
179+
);
180+
181+
const result = await response;
182+
expect(result.ok).toBe(false);
183+
if (result.ok) return;
184+
expect(result.error.message).toBe('could not type "[REDACTED]" into the field');
185+
expect(result.error.logPath).toContain('req-fill-text-stdin');
186+
expect(fs.existsSync(result.error.logPath!)).toBe(true);
187+
expect(result.error.diagnosticsRecord?.requestId).toBe('req-fill-text-stdin');
188+
});
189+
190+
test('a batch step carrying --text-stdin is refused before typing and returns no part of the value', async () => {
191+
const { response, typed } = sendToWebSession(makeSession('web', { device: WEB_DESKTOP_DEVICE }), {
192+
command: 'batch',
193+
positionals: [],
194+
flags: {
195+
batchSteps: [
196+
{ command: 'fill', positionals: ['10', '20', SECRET], flags: { textStdin: true } },
197+
],
198+
},
199+
});
200+
201+
const result = await response;
202+
expect(result.ok).toBe(false);
203+
if (result.ok) return;
204+
expect(result.error.code).toBe('INVALID_ARGS');
205+
expect(result.error.details?.reason).toBe('fill_text_stdin_in_batch');
206+
expect(typed).toEqual([]);
207+
expect(JSON.stringify(result)).not.toContain(SECRET);
208+
});

‎src/daemon/interaction/internal/interaction-common.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type { MultiTargetAnnotationV1 } from '@agent-device/contracts/replay';
1010
import {
1111
inferFillText,
1212
isSensitiveFillText,
13-
recordedInputPlaceholder,
13+
sensitiveFillPlaceholder,
1414
} from '@agent-device/ad-script';
1515
import { parameterizeRecordedFillPayload } from '@agent-device/selectors/parameterized-recorded-fill';
1616
import type { InteractionFinalizationOperations } from './types.ts';
@@ -94,9 +94,6 @@ function computeMultiTargetEvidence(recordedTargets: {
9494
return source && destination ? { source, destination } : undefined;
9595
}
9696

97-
/** What sensitive fill text reads as when no `--record-as` placeholder names it. */
98-
const REDACTED_FILL_TEXT = '[REDACTED]';
99-
10097
function parameterizeFillPayloads(params: {
10198
command: string;
10299
positionals: string[];
@@ -114,10 +111,7 @@ function parameterizeFillPayloads(params: {
114111
flags: params.flags,
115112
result: params.result,
116113
});
117-
const placeholder =
118-
typeof params.flags?.recordAs === 'string'
119-
? recordedInputPlaceholder(params.flags.recordAs)
120-
: REDACTED_FILL_TEXT;
114+
const placeholder = sensitiveFillPlaceholder(params.flags);
121115
return [
122116
parameterizeRecordedFillPayload(params.result, literal, placeholder),
123117
parameterizeRecordedFillPayload(params.responseData, literal, placeholder),

‎src/daemon/request-router.ts‎

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ import {
3737
emitDiagnostic,
3838
flushDiagnosticsToSessionFile,
3939
getDiagnosticsMeta,
40-
redactRegisteredSensitiveValues,
4140
registerDiagnosticSensitiveValue,
4241
withDiagnosticsScope,
4342
} from '@agent-device/host-kit/diagnostics';
@@ -56,7 +55,11 @@ import {
5655
import { unsupportedSaveScriptFlagResponse } from './request-save-script-policy.ts';
5756
import { canRunReplayScopedAction } from './daemon-command-registry.ts';
5857
import { isWebSession } from './web-session-names.ts';
59-
import { inferFillText, isSensitiveFillText } from '@agent-device/ad-script';
58+
import {
59+
inferFillText,
60+
isSensitiveFillText,
61+
sensitiveFillPlaceholder,
62+
} from '@agent-device/ad-script';
6063
import { createPlatformRequestScope } from './platform-request-scope.ts';
6164
import { createOwnerScopedDeviceClaimReconciler } from './device/device-claim-owner-recovery.ts';
6265
import { isConfinedToAppLease, scopeRequestSession } from './request-admission.ts';
@@ -208,7 +211,10 @@ export function createRequestHandler(deps: RequestRouterDeps): DaemonInvokeFn {
208211
repairExpiredIfTombstoned(req, response.error, sessionStore),
209212
sessionStore,
210213
);
211-
return { ok: false, error: redactRegisteredSensitiveValues(enrichDaemonError(error)) };
214+
return {
215+
ok: false,
216+
error: redactSensitiveFillTextInError(req, enrichDaemonError(error)),
217+
};
212218
}
213219
// Phase 4 (agent-cost) grafts on the success path. Runs inside the
214220
// diagnostics scope so cost can read this request's runner-round-trip tally.
@@ -511,15 +517,48 @@ function customActionFlagsResponse(req: DaemonRequest): DaemonResponse | undefin
511517
return undefined;
512518
}
513519

520+
/** The text of a fill the caller marked sensitive, or `undefined` for any other request. */
521+
function sensitiveFillText(req: DaemonRequest): string | undefined {
522+
if (req.command !== 'fill' || !isSensitiveFillText(req.flags)) return undefined;
523+
return inferFillText({
524+
ts: 0,
525+
command: 'fill',
526+
positionals: req.positionals ?? [],
527+
flags: req.flags,
528+
});
529+
}
530+
514531
function registerParameterizedFillDiagnosticValue(req: DaemonRequest): void {
515-
if (req.command !== 'fill' || !isSensitiveFillText(req.flags)) return;
516-
registerDiagnosticSensitiveValue(
517-
inferFillText({
518-
ts: 0,
519-
command: 'fill',
520-
positionals: req.positionals ?? [],
521-
flags: req.flags,
522-
}),
532+
const text = sensitiveFillText(req);
533+
if (text !== undefined) registerDiagnosticSensitiveValue(text);
534+
}
535+
536+
/** Error fields that carry text a backend may have built from the value it was typing. */
537+
const ERROR_TEXT_FIELDS: ReadonlySet<string> = new Set(['message', 'hint', 'cause', 'details']);
538+
539+
/**
540+
* A backend error can quote the text a sensitive fill failed to type. Only that fill's own text is
541+
* redacted, and only in the error's text fields: `logPath`, `diagnosticId` and the other
542+
* daemon-owned fields come back as they are, and so does every other request's error.
543+
*/
544+
function redactSensitiveFillTextInError(req: DaemonRequest, error: DaemonError): DaemonError {
545+
const text = sensitiveFillText(req);
546+
if (!text) return error;
547+
const replace = (value: string) => value.replaceAll(text, sensitiveFillPlaceholder(req.flags));
548+
return Object.fromEntries(
549+
Object.entries(error).map(([field, value]) => [
550+
field,
551+
ERROR_TEXT_FIELDS.has(field) ? mapErrorStrings(value, replace) : value,
552+
]),
553+
) as DaemonError;
554+
}
555+
556+
function mapErrorStrings(value: unknown, replace: (value: string) => string): unknown {
557+
if (typeof value === 'string') return replace(value);
558+
if (Array.isArray(value)) return value.map((entry) => mapErrorStrings(entry, replace));
559+
if (!value || typeof value !== 'object') return value;
560+
return Object.fromEntries(
561+
Object.entries(value).map(([key, entry]) => [key, mapErrorStrings(entry, replace)]),
523562
);
524563
}
525564

0 commit comments

Comments
 (0)