Skip to content

feat: parameterize sensitive recorded inputs#1369

Draft
thymikee wants to merge 4 commits into
mainfrom
agent/parameterize-recorded-inputs
Draft

feat: parameterize sensitive recorded inputs#1369
thymikee wants to merge 4 commits into
mainfrom
agent/parameterize-recorded-inputs

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

  • add an explicit fill --record-as <VAR> authoring contract across CLI, Node, and MCP surfaces
  • execute the live fill with the real value while retaining only ${VAR} in session state, diagnostics, target evidence, and published .ad scripts
  • preserve ordinary literal fills, reject unsafe flag combinations, and fail missing replay values before dispatch
  • document the security boundary and safe login/bootstrap workflow in ADR 0017 and CLI help

Closes #1348

Validation

  • pnpm check:quick
  • pnpm check:fallow --base origin/main
  • pnpm exec vitest run --project provider-integration --maxWorkers=4 (144 passed)
  • pnpm test:integration:node (15 passed, 6 live-only tests skipped)
  • pnpm test:integration:progress:check
  • bounded coverage run passed 4,576 tests; after the final evidence-sanitization edge case, the focused recorder/provider tests passed and the sole unrelated coverage timeout passed in isolation

SkillGym was not completed locally because its external model runners were unavailable from the sandbox.

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.8 MB 1.8 MB +6.9 kB
JS gzip 578.2 kB 580.5 kB +2.4 kB
npm tarball 692.9 kB 695.3 kB +2.4 kB
npm unpacked 2.4 MB 2.4 MB +7.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 25.3 ms 26.5 ms +1.2 ms
CLI --help 54.7 ms 58.4 ms +3.7 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/snapshot-diagnostics.js +6.3 kB +2.0 kB
dist/src/cli-help.js +697 B +258 B
dist/src/interaction.js +530 B +198 B
dist/src/registry.js +398 B +179 B
dist/src/sdk-android-adb.js +396 B +161 B

@thymikee

Copy link
Copy Markdown
Member Author

Needs changes at d98628b:

  1. P1: replayed parameterized fills add recordAs after ${VAR} expansion, then use the replay-scoped invoker, which bypasses runRequestWithinScope and registerParameterizedFillDiagnosticValue. Backend/dispatch diagnostics can therefore retain the expanded secret. Route replay-scoped actions through the same registration boundary and add a production-route regression test that makes the backend echo the value into diagnostics.

  2. P1: replaceRecordedInputLiteral recursively rewrites every string and drops any selector/selector-chain entry containing the literal. A valid short/common value such as a corrupts unrelated response text and drops stable selectors like id="password", contrary to ADR 0017’s byte-preserved provenance rule. Parameterize only fields that semantically carry the fill value; test a one-character value alongside unrelated strings/selectors containing that character.

  3. P2: inferFillText trims values and the recorder skips falsy literals, so ${PASSWORD} resolved from PASSWORD= or whitespace is re-recorded as a literal empty value rather than preserving the placeholder during replay/repair. Preserve the source placeholder independently of whether the resolved value is empty and cover empty/whitespace replay values.

CI is green, but the current provider scenario is mocked. After fixes, provide live simulator/device evidence that a parameterized fill executes the literal while the response, session state/events, diagnostics, temp/target .ad content, and replayed recording contain only ${VAR}. No separately authorized cross-vendor review was performed.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed all three review findings in ff980e9:

  • replay-scoped invocations now register parameterized fill values with diagnostics redaction, with a production-route regression that has the provider echo the resolved value into request diagnostics
  • recording now parameterizes only semantic fill-value fields and exact value-bearing selector/evidence entries, preserving unrelated strings and stable selector provenance (including a one-character a regression)
  • replay recording derives placeholder provenance from the unresolved source action, including empty and whitespace-only resolved values

Validation:

  • focused recorder/replay tests: 12/12 passed
  • changed provider scenario: 3/3 passed
  • isolated reruns for aggregate load timeouts: 49/49 passed
  • typecheck, build, format, lint, layering, fallow, and MCP metadata checks passed
  • full coverage run reached 4,576/4,581 passing; its five unrelated timeout failures all passed in the isolated rerun above

Live iOS simulator evidence: recorded a Settings Search fill with --record-as LIVE_SECRET, confirmed the app received the literal, replayed the published script with --env, and confirmed the published/re-recorded scripts, session artifacts, and diagnostics retained only ${LIVE_SECRET} / [REDACTED]. All manual sessions were closed.

@thymikee

Copy link
Copy Markdown
Member Author

Re-review at ff980e9: the prior replay-diagnostics, one-character corruption, and empty/whitespace placeholder findings are fixed with meaningful coverage. One blocker remains: parameterizeRecordedFillPayload now rewrites only top-level text, exact refLabel, and selected selector-chain entries. Backend echoes in fields such as message or nested settle.diff therefore pass unchanged into both the public response and recorded session state, violating #1348’s requirement that the literal never reach either boundary. The delta removes the previous fixtures that exercised message: Filled <secret> and settle.diff: value=<secret> and replaces them with safe strings, so tests no longer prove the security guarantee. Preserve unrelated provenance while dropping/redacting all value-bearing backend and settle output, and restore production-route coverage with nested/backend echoes. All CI is green; reported live iOS verification does not exercise this leak path. No separately authorized cross-vendor review was performed.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the remaining blocker in d8f0071.

  • Parameterized fill results now recursively scrub value-bearing arbitrary backend fields and nested settle output before both the public response and session recording boundaries.
  • Stable root selector/ref provenance and structured settle/evidence metadata remain unchanged; semantic selector-chain candidates carrying the literal are removed.
  • Restored recorder regressions for message: Filled <secret> and nested settle.diff echoes.
  • Added a finalizeTouchInteraction production-boundary regression with arbitrary nested backend echoes and a provider-backed active-session scenario proving a real settle snapshot diff exposes ${SEARCH_TERM} but never the literal.

Validation: pnpm format, pnpm typecheck, pnpm check:fallow --base origin/main, focused response/recorder/replay tests (13/13), provider scenario (3/3), and the full coverage run (541 files, 4,582/4,582 tests) all pass.

@thymikee

Copy link
Copy Markdown
Member Author

P1 — Arbitrary backend output can still retain the literal. At d8f0071, parameterizeSensitiveString replaces identifier-like values only when bounded by non-identifier characters. A secret such as TOKEN123 therefore survives in strings like prefixTOKEN123suffix or TOKEN1234. The recursive walker also sanitizes values but leaves object keys untouched, so { TOKEN123: true } retains the secret. Selector-chain filtering inherits the same delimiter gap. These values reach both the public response and recorded session, violating #1348 and ADR 0017’s guarantee that the real value never crosses either boundary.

The new response/settle regressions are meaningful for delimiter-separated echoes, but they do not cover concatenated strings or keys. Please scrub every literal occurrence in untrusted backend/settle output and keys while preserving only explicitly structural provenance, then add production-route regressions for concatenated, prefix/suffix, and object-key echoes. Live evidence still does not exercise this leak path.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed the new P1 in 8af3931.

  • Untrusted backend and settle strings now replace every non-empty literal occurrence, including prefix/suffix concatenation. Arbitrary object keys are scrubbed recursively as well.
  • Structural preservation is path-specific to the known response schemas (cost, evidence, resolution, and settle), so an unknown object nested under settle cannot claim trusted fields such as id or kind.
  • Value-bearing selector-chain terms with embedded literals are removed, while stable selector/ref provenance remains unchanged. Post-fill refLabel occurrences are parameterized too; the provider replay regression exposed and now covers that production path.
  • Added handler-route coverage for concatenated backend values and keys, boundary coverage for prefix/suffix and unknown nested settle objects, and a provider-backed settle/replay scenario whose accessibility diff contains prefix<secret>suffix. Public response, replay session state, and script retain only prefix${SEARCH_TERM}suffix.

Validation: focused handler/recorder/replay tests 14/14, provider scenario 3/3, format, lint, typecheck, layering, fallow, MCP metadata, and build passed. The bounded full coverage run passed 541 files and 4,583/4,583 tests.

@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 8af39311; two P1 edge cases still violate ADR 0017:

  1. Whitespace-only literals survive concatenated untrusted output. parameterizeSensitiveString only replaces an empty/whitespace literal when the whole string equals it. A supported value such as three spaces therefore remains in prefix suffix, nested settle text, selector terms, and arbitrary keys. The new concatenation tests cover only non-whitespace values. Use a safe drop/redaction policy for these ambiguous literals and add handler/recorder regressions.

  2. The sanitizer is not idempotent across response and recorder boundaries. finalizeTouchInteraction parameterizes the result before SessionStore.recordAction, then recordActionEntry parameterizes it again. If the literal occurs in the placeholder—for example literal PASSWORD with recordAs=PASSWORD, or literal $—the second pass corrupts ${PASSWORD} and nested values/keys/ref labels. Preserve already-inserted placeholders or ensure one sanitization pass, with a handler-to-recorder overlap regression.

The prior non-whitespace concatenation/key leak is otherwise fixed, structural trust is path-specific, and current provider/handler regressions are meaningful for their covered inputs. All exact-head checks are green. No ready label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameterize sensitive inputs before publishing .ad recordings

1 participant