Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/ad-script/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,10 @@ export {

export {
inferFillText,
isSensitiveFillText,
readRecordedInputVariableName,
recordedInputPlaceholder,
sensitiveFillPlaceholder,
validateRecordedInputVariableName,
} from './internal/recorded-input.ts';

Expand Down
21 changes: 21 additions & 0 deletions packages/ad-script/src/internal/recorded-input.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { CommandFlags } from '@agent-device/contracts/command';
import type { SessionAction } from '@agent-device/contracts/session';
import { AppError } from '@agent-device/kernel/errors';
import { REPLAY_VAR_KEY_RE } from './script.ts';
Expand All @@ -20,6 +21,26 @@ export function validateRecordedInputVariableName(raw: string): string {
return raw;
}

/**
* A fill's text is sensitive when the caller marked it: `--record-as` names it for the script, or
* `--text-stdin` kept it out of argv. Sensitive text never appears as a literal on any surface:
* response data, result, recorded action or diagnostics.
*/
export function isSensitiveFillText<TFlags extends Pick<CommandFlags, 'recordAs' | 'textStdin'>>(
flags: TFlags | undefined,
): flags is TFlags {
return typeof flags?.recordAs === 'string' || flags?.textStdin === true;
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
}

/** What sensitive fill text reads as on any response: its `--record-as` placeholder, else `[REDACTED]`. */
export function sensitiveFillPlaceholder(
flags: Pick<CommandFlags, 'recordAs'> | undefined,
): string {
return typeof flags?.recordAs === 'string'
? recordedInputPlaceholder(flags.recordAs)
: '[REDACTED]';
}

export function recordedInputPlaceholder(variableName: string): string {
return `\${${variableName}}`;
}
Expand Down
18 changes: 18 additions & 0 deletions packages/command-registry/src/batch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ export function validateAndNormalizeBatchSteps(
if (step.flags !== undefined && !isRecord(step.flags)) {
throw new AppError('INVALID_ARGS', `Batch step ${index + 1} flags must be an object.`);
}
assertNoTextStdinStep(step.flags as Record<string, unknown> | undefined, index + 1);
if (step.input !== undefined && !isRecord(step.input)) {
throw new AppError('INVALID_ARGS', `Batch step ${index + 1} input must be an object.`);
}
Expand All @@ -236,6 +237,23 @@ export function validateAndNormalizeBatchSteps(
return normalized;
}

/**
* A batch step has no stdin of its own, and a failed step reports its positionals. The CLI refuses
* `textStdin: true` on a step before reading stdin; this refuses it for every other daemon client.
* `false` is the ordinary argv path, and a value of another type is refused per step by the router.
*/
function assertNoTextStdinStep(
flags: Record<string, unknown> | undefined,
stepNumber: number,
): void {
if (flags?.textStdin !== true) return;
throw new AppError(
'INVALID_ARGS',
`Batch step ${stepNumber}: fill textStdin is not supported because batch steps have no stdin of their own.`,
{ reason: 'fill_text_stdin_in_batch' },
);
}

function buildBatchStepFlags(
parentFlags: BatchFlags | Record<string, unknown> | undefined,
stepFlags: DaemonBatchStep['flags'] | Record<string, unknown> | undefined,
Expand Down
10 changes: 10 additions & 0 deletions packages/command-registry/src/flag-definitions-action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,16 @@ export const ACTION_FLAG_DEFINITIONS: readonly FlagDefinition[] = [
projectConfig: true,
recorded: false,
},
{
key: 'textStdin',
names: ['--text-stdin'],
type: 'boolean',
usageLabel: '--text-stdin',
usageDescription:
'Fill: read the text from stdin (max 64 KiB, one trailing newline removed) instead of argv',
projectConfig: false,
recorded: false,
},
{
key: 'durationMs',
names: ['--duration-ms'],
Expand Down
2 changes: 2 additions & 0 deletions packages/contracts/src/cli-flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ export type CliFlags = CloudProviderProfileFields &
delayMs?: number;
/** Fill: publish the live text as a late-bound ${VAR} in a recorded .ad script. */
recordAs?: string;
/** Fill: the text came from the CLI's stdin rather than argv; keep it out of logs and scripts. */
textStdin?: boolean;
durationMs?: number;
keyframes?: string;
holdMs?: number;
Expand Down
2 changes: 2 additions & 0 deletions packages/contracts/src/client-gesture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ export type FillOptions = DeviceCommandBaseOptions &
delayMs?: number;
/** Publish this fill value as `${VAR}` when script recording is armed. */
recordAs?: string;
/** The text came from the CLI's stdin; keep it out of diagnostics and recorded actions. */
textStdin?: boolean;
verify?: boolean;
};

Expand Down
51 changes: 51 additions & 0 deletions packages/selectors/src/parameterized-recorded-fill.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { test } from 'vitest';
import assert from 'node:assert/strict';
import { parameterizeSensitiveString } from './parameterized-recorded-fill.ts';

test('a value that contains the placeholder collapses the whole string', () => {
assert.equal(
parameterizeSensitiveString(
'could not type "abc[REDACTED]xyz"',
'abc[REDACTED]xyz',
'[REDACTED]',
),
'[REDACTED]',
);
assert.equal(
parameterizeSensitiveString(
'could not type "abc${PASSWORD}xyz"',
'abc${PASSWORD}xyz',
'${PASSWORD}',
),
'${PASSWORD}',
);
});

test('a value that crosses a placeholder already in the string collapses the whole string', () => {
// `D]tail` starts inside the earlier `[REDACTED]` and ends after it, so no segment holds it.
assert.equal(
parameterizeSensitiveString('typed [REDACTED]tail', 'D]tail', '[REDACTED]'),
'[REDACTED]',
);
});

test('a value found only inside placeholders leaves the string as it is', () => {
assert.equal(
parameterizeSensitiveString('value ${DOLLAR} kept', '$', '${DOLLAR}'),
'value ${DOLLAR} kept',
);
assert.equal(
parameterizeSensitiveString('typed ${PASSWORD}', 'PASSWORD', '${PASSWORD}'),
'typed ${PASSWORD}',
);
});

test('an ordinary value is replaced in place and a second pass changes nothing', () => {
const once = parameterizeSensitiveString(
'could not type "s3cret" twice: s3cret',
's3cret',
'[REDACTED]',
);
assert.equal(once, 'could not type "[REDACTED]" twice: [REDACTED]');
assert.equal(parameterizeSensitiveString(once, 's3cret', '[REDACTED]'), once);
});
53 changes: 48 additions & 5 deletions packages/selectors/src/parameterized-recorded-fill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,22 +362,65 @@ function filterSensitiveSelectorCandidates(
return value.filter((candidate) => !carries(candidate));
}

function parameterizeSensitiveString(value: string, literal: string, placeholder: string): string {
export function parameterizeSensitiveString(
value: string,
literal: string,
placeholder: string,
): string {
// The empty literal (`fill <target> "" --record-as VAR`, #2063) matches inside every string:
// it reveals nothing if echoed, so it redacts nothing. Only the fill's own semantic `text`
// field is force-parameterized, by `parameterizeRecordedFillPayload`, not here.
if (!literal) return value;

const unparameterizedSegments = placeholder ? value.split(placeholder) : [value];
if (unparameterizedSegments.every((segment) => !segment.includes(literal))) return value;
if (unparameterizedSegments.every((segment) => !segment.includes(literal))) {
return collapseIfLiteralRemains(value, literal, placeholder);
}

// Replacing whitespace inline would make every ordinary separator look like
// authored secret data. Collapse the whole untrusted string/key instead.
if (!literal.trim()) return placeholder;

return unparameterizedSegments
.map((segment) => segment.replaceAll(literal, placeholder))
.join(placeholder);
return collapseIfLiteralRemains(
unparameterizedSegments
.map((segment) => segment.replaceAll(literal, placeholder))
.join(placeholder),
literal,
placeholder,
);
}

/**
* The output never carries the literal outside a placeholder. A literal that contains the
* placeholder (`abc[REDACTED]xyz`, or `${VAR}` inside a `--record-as VAR` value) or crosses one
* spans the segments split on it, so the segment-wise replace above leaves it whole; the whole
* string collapses to the placeholder instead. A literal found only inside placeholders (`$` in
* `${DOLLAR}`) reveals nothing and stays.
*/
function collapseIfLiteralRemains(value: string, literal: string, placeholder: string): string {
const placeholderStarts: number[] = [];
if (placeholder) {
for (
let at = value.indexOf(placeholder);
at !== -1;
at = value.indexOf(placeholder, at + placeholder.length)
) {
placeholderStarts.push(at);
}
}
// Placeholders do not overlap, so the first one ending at or after an occurrence of the
// literal is the only one that can cover it, and that index only moves forward.
let next = 0;
for (let at = value.indexOf(literal); at !== -1; at = value.indexOf(literal, at + 1)) {
while (
next < placeholderStarts.length &&
placeholderStarts[next]! + placeholder.length < at + literal.length
) {
next += 1;
}
if (next === placeholderStarts.length || placeholderStarts[next]! > at) return placeholder;
}
return value;
}

/**
Expand Down
1 change: 1 addition & 0 deletions scripts/integration-progress-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ function summarizeProviderScenarioFlagExclusions() {
'shardSplit',
'searchPath',
'stepsFile',
'textStdin',
'proxyHost',
'proxyPort',
'stale',
Expand Down
34 changes: 34 additions & 0 deletions src/cli/resolve-cli-options.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { test } from 'vitest';
import assert from 'node:assert/strict';
import fs from 'node:fs';
import { mkdtempForTestSync } from '../__tests__/test-utils/tmp-dir.ts';
import { resolveCliOptions } from './resolve-cli-options.ts';

Expand Down Expand Up @@ -75,3 +76,36 @@ test('the same env default keeps filling doctor, which reads a configured app wi

assert.equal(parsed.flags.targetApp, 'com.example.configured');
});

// #3260: --text-stdin makes fill block on stdin, so only a typed flag may turn it on.
test('fill --text-stdin has no environment default', () => {
const parsed = resolveCliOptions(['fill', '@e3', 'typed text'], {
cwd: process.cwd(),
env: isolatedEnv({ AGENT_DEVICE_TEXT_STDIN: '1' }),
});

assert.equal(parsed.flags.textStdin, undefined);
});

test('a config file cannot set fill --text-stdin', () => {
const configPath = `${mkdtempForTestSync('agent-device-cli-config-')}/config.json`;
fs.writeFileSync(configPath, JSON.stringify({ textStdin: true }));

assert.throws(
() =>
resolveCliOptions(['fill', '@e3', 'typed text', '--config', configPath], {
cwd: process.cwd(),
env: isolatedEnv({}),
}),
/Config key "textStdin" is not allowed/,
);
});

test('a typed --text-stdin reaches fill', () => {
const parsed = resolveCliOptions(['fill', '@e3', '--text-stdin'], {
cwd: process.cwd(),
env: isolatedEnv({}),
});

assert.equal(parsed.flags.textStdin, true);
});
24 changes: 24 additions & 0 deletions src/commands/batch/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -436,3 +436,27 @@ test('batch human output renders per-step results', async () => {
assert.match(result.stdout, /1\. OK Opened: Settings \(7ms\)/);
assert.match(result.stdout, /2\. OK Typed 5 chars \(8ms\)/);
});

test('batch rejects a fill step that asks for stdin text before daemon dispatch', async () => {
const result = await runCliCapture([
'batch',
'--steps',
'[{"command":"fill","input":{"target":{"kind":"ref","ref":"@e3"},"text":"x","textStdin":true}}]',
]);

assert.equal(result.code, 1);
assert.equal(result.calls.length, 0);
assert.match(result.stderr, /Batch step 1: fill textStdin is not supported/);
});

test('batch passes an explicit fill textStdin false through to dispatch', async () => {
const result = await runCliCapture([
'batch',
'--steps',
'[{"command":"fill","input":{"target":{"kind":"ref","ref":"@e3"},"text":"x","textStdin":false}}]',
]);

assert.equal(result.calls.length, 1);
assert.equal((result.calls[0]?.flags?.batchSteps ?? [])[0]?.flags?.textStdin, false);
assert.doesNotMatch(result.stderr, /textStdin is not supported/);
});
14 changes: 14 additions & 0 deletions src/commands/batch/projection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ async function readBatchDaemonStep(
const record = readBatchStepRecord(step, stepNumber);
const command = readBatchStepCommand(record, stepNumber);
const input = readBatchStepInputObject(record, stepNumber) as CommandInput;
assertNoStdinFillStep(command, input, stepNumber);
const runtime = parseBatchStepRuntime(record.runtime, stepNumber);
const prepared = await prepareDaemonCommandRequest(command, input, stepNumber);
return {
Expand All @@ -76,6 +77,19 @@ async function readBatchDaemonStep(
};
}

function assertNoStdinFillStep(
command: BatchCommandName,
input: CommandInput,
stepNumber: number,
): void {
if (command !== PUBLIC_COMMANDS.fill || input.textStdin !== true) return;
throw new AppError(
'INVALID_ARGS',
`Batch step ${stepNumber}: fill textStdin is not supported because batch steps have no stdin of their own. Run that fill on its own with --text-stdin.`,
{ reason: 'fill_text_stdin_in_batch' },
);
}

function readBatchStepCommand(
record: Record<string, unknown>,
stepNumber: number,
Expand Down
Loading
Loading