fix(core): redact typed fill text from stored Observe timeline events - #730
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e74d9de42b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| function redactFillResultText(tool: string, payload: unknown): unknown { | ||
| if (tool !== 'device_fill') return payload; |
There was a problem hiding this comment.
When cdp_interact successfully runs typeText or setFieldValue, the injected helper echoes the entered data under data.text or data.value; mapObservation then stores that object as the event payload. Because this function only redacts results for device_fill, the arguments are hidden but the same typed content remains verbatim in the Observe timeline for every successful CDP interaction. Apply the corresponding action-aware result redaction to cdp_interact as well.
Useful? React with 👍 / 👎.
1 participant
Intent
Fix the shipped rn-dev-agent Observe timeline confidentiality defect identified during independent PR 680 Android verification: device_fill must never leave args.text verbatim in stored Observe timeline events, and a distinctive secret canary must be provably absent. Follow the existing redaction approach rather than introducing a parallel or generalized framework. Redact rather than delete so event ordering, target identity, text length and status metadata, and every non-secret diagnostic field remain available. Do not change exact-fill behavior or unrelated event types. Add causal regression coverage that fails on the pre-fix implementation and passes after the fix, keeping the complete confidentiality argument small and reviewable. Do not claim or modify the exclusively owned NUC Android lane; rely on unit or integration evidence (or a local Mac simulator only if truly needed), and stop rather than contend for shared resources. Do not post GitHub issues, comments, or preserved feedback drafts. Ship the correction through a green PR for captain review, but do not merge it.
What Changed
mapObservationinpackages/rn-dev-agent-core/src/observability/events.tsnow runs fill text through a redaction step before events are stored:device_fill.text,device_batchsteps withaction: 'fill', andcdp_interacttypeText/setFieldValuevalues are replaced with[REDACTED:<type>]plus a sibling<key>Lengthfield, so ordering, target identity, text length, and status metadata stay visible while the plaintext does not.device_fill's result payload, closing the iOS fill timeout-recovery path that echoed the typed text back in its success payload where args-only redaction would not have reached it.test/unit/observe-fill-redaction.test.tswith causal coverage (fails on the pre-fix build, passes after — verified in the Test phase by swapping in the base-commit build), plus a changeset and updated README /observe.mdxsecurity-posture wording.Exact-fill behavior and unrelated event types are untouched; the Review phase left one open info note that the changeset sentence still names only
device_filleven thoughdevice_batchandcdp_interactare now covered.Risk Assessment
✅ Low: The round-1 sibling-leak gap is closed at the same single choke point with an explicit, enumerable per-tool policy, no mutation of caller state, and canary coverage that also pins visible labels as still readable; only the changeset wording lags the shipped scope.
Testing
I proved causality by rebuilding against the pre-fix
events.ts, where the newobserve-fill-redactiontest fails with the canary stored verbatim in args, in the device_fill result payload, and in device_batch/cdp_interact args; on the fixed tree all three cases pass. Beyond unit tests I seeded a live Recorder + Observe server and captured screenshots of the actual Observe web UI, which now renders"text": "[REDACTED:string]"withtextLength: 26in both the ARGS and PAYLOAD panes of device_fill, inside the device_batch fill step, and for cdp_interact typeText, while testID, exact, typed/recovered/verification, error text, event ordering and durations remain fully readable and plain byText selectors stay unredacted. Neighbouring observability and fill suites (88 tests) pass, confirming exact-fill behavior and other event types are untouched; the worktree was left clean and the temporary server/browser shut down./var/folders/wy/khrzvmhd0ss969ydn32ghccm0000gn/T/no-mistakes-evidence/01KZM8JA26AB2H1JY1XV2Z0FNT/observe-ui-device_fill-redacted.png)/var/folders/wy/khrzvmhd0ss969ydn32ghccm0000gn/T/no-mistakes-evidence/01KZM8JA26AB2H1JY1XV2Z0FNT/observe-ui-device_batch-redacted.png)/var/folders/wy/khrzvmhd0ss969ydn32ghccm0000gn/T/no-mistakes-evidence/01KZM8JA26AB2H1JY1XV2Z0FNT/observe-ui-cdp_interact-redacted.png)Evidence: Stored Observe timeline BEFORE fix (canary present)
CANARY_PRESENT_IN_STORED_TIMELINE=trueEvidence: Stored Observe timeline AFTER fix (canary absent)
CANARY_PRESENT_IN_STORED_TIMELINE=falseEvidence: New regression test failing on the pre-fix implementation
Evidence: Same test passing after the fix
Evidence: Reusable Observe seed/serve script used for the manual UI capture
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
packages/rn-dev-agent-core/src/observability/events.ts:179- The redaction is gated ono.tool === 'device_fill', but two sibling tools put the same user-typed secret into the same stored Observe timeline. Concrete path:device_batch({steps:[{action:'fill', testID:'x', text: SECRET}]})→addToolObserver(index.ts:386) →recorder.record→mapObservation— tool name isdevice_batch, soredactDeviceFillTextis skipped, andredact()(src/util/redact.ts) has no rule for atextkey, soargs.steps[0].textis stored and served verbatim. Same forcdp_interactwithaction:'typeText'(top-leveltext, index.ts:2069) andsetFieldValue(value). device_batch's own schema says fill "calls the same exact-fill coordinator as device_fill" (index.ts:3077), so this is the same secret through the same coordinator, not an unrelated event type. Earliest supported shared boundary: extend the existingredactDeviceFillTextguard inmapObservationto cover fill-bearing steps/actions, rather than adding another per-tool patch later. Flagged as ask-user because the intent says not to change unrelated event types.packages/rn-dev-agent-core/src/observability/events.ts:168-summarize()usesargs.testID ?? args.ref ?? args.text ?? ...for its label, so a device_fill observation lacking both testID and ref now summarizes asdevice_fill [REDACTED:string]. This is the correct confidentiality outcome (pre-fix the summary echoed the secret) and device_fill requires a ref in practice, so no action is needed — noting it only so the flatter timeline label is not mistaken for a regression.🔧 Fix: redact fill text from batch and interact events
1 info still open:
.changeset/quiet-observe-fill-text.md:6- The changeset still says "Redactdevice_filltext", but HEAD also redactsdevice_batchfill steps andcdp_interacttypeText/setFieldValue values (src/observability/events.ts:99-122). This line ships verbatim to the published CHANGELOG, so a consumer auditing which surfaces are now confidential would under-read the fix. Widen it to one sentence covering fill text routed through device_fill, device_batch fill steps, and cdp_interact typeText/setFieldValue (keeping the single-sentence changeset convention).🔧 **Test** - 1 issue found → auto-fixed ✅
packages/rn-dev-agent-core/src/observability/events.ts:204- Residual verbatim leak of the same secret in a stored Observe event: on the iOS fill main-thread-timeout recovery path, device_fill's success payload echoestext: args.text(src/runners/rn-fast-runner-client.ts:1696). mapObservation only redactsargs, so the stored event has args.text = "[REDACTED:string]" while payload.text still contains the canary verbatim. Reproduced directly via mapObservation with that result shape (see ios-timeout-recovery-payload-check.txt). Whether to extend the redaction to this payload field, or to stop echoing the text there, is a scope decision for the author.npx tsc(clean build of rn-dev-agent-core, no dist drift)node --test test/unit/observe-fill-redaction.test.ts— 2/2 pass on the fixed buildCausal check: swapped in base-commitdist/observability/events.js(7419435d) and re-ran the same test — 2/2 fail (exit 1); restored the fixed build — 2/2 pass (exit 0)End-to-end: drove an instrumenteddevice_fillcarrying secrethunter2-CANARY-9f3axthrough the production Recorder, started the realObservabilityServer, and readGET /api/stream— the snapshot the Observe UI renders — before and after the fixRegression set:node --test test/unit/observability-events.test.js test/unit/observability-recorder.test.js test/unit/observability-observer.test.js test/unit/observability-observe-tool.test.js test/unit/audit-h1-redact-before-truncate.test.js test/unit/gh-581-narrow-fill.test.ts test/unit/gh-391-fill-ladder.test.ts test/unit/fill-verify.test.js test/unit/device-fill-jsfirst.test.js test/unit/observability-server.test.js— 84/84 passManual probe of the iOS fill timeout-recovery result shape throughmapObservationto check payload-side exposure🔧 Fix: redact fill text echoed in device_fill result payload
✅ Re-checked - no issues remain.
node --test test/unit/observe-fill-redaction.test.tsagainst the pre-fixevents.ts(base 7419435d, rebuilt) — all 3 cases fail with the canary present in stored events (causal proof)node --test test/unit/observe-fill-redaction.test.tson the fixed tree — 3/3 passnode --test test/unit/observability-events.test.js test/unit/observability-recorder.test.js test/unit/observability-observe-tool.test.js test/unit/observability-server.test.js test/unit/fill-verify.test.js test/unit/device-fill-jsfirst.test.js test/unit/gh-391-fill-ladder.test.ts test/unit/gh-581-narrow-fill.test.ts test/unit/story-04-fill-batch-settle.test.js— 88/88 pass (exact-fill and unrelated event types unchanged)Manual: seeded a realRecorderwith device_press / device_fill (canary in args + echoed in result payload) / device_batch fill step / cdp_interact typeText, dumpedrecorder.snapshot()on both the pre-fix and post-fix builds (CANARY_PRESENT_IN_STORED_TIMELINE=true→false)Manual: startedObservabilityServeron that recorder and drove the real Observe web UI withchrome-devtools-axi, expanding each timeline row and screenshotting the ARGS/PAYLOAD detail panes✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.