mcp: mask secrets a page echoes during a password write

[suchintan]

https://github.com/Skyvern-AI/rustwright-cloud/pull/161

[claude]

Summary

This is a large, carefully engineered fix for MCP password-secret leakage (echoes into ARIA labels, status regions, iframe titles, dialogs, etc.), backed by an unusually thorough fixture/test suite. The design is sound overall, but the "dialog masking" guarantee has a real gap in fill_form's success path, and the hard-coded resolve timeout combined with a full-DOM scan is a plausible source of false "partial completion" reports on large pages. No unsafe/unwrap/panic! were introduced, and this PR doesn't touch the PyO3/napi FFI surface.

🔴 Critical Issues (1)

• fill_form's all-fields-succeeded path can leak a secret through a pending dialog. Type's success path always finishes through committed_sensitive_post_action_snapshot, which re-checks has_pending_modal() after resolving taint and redacts the stored dialog message if one is pending (actor.rs ~1595-1625). fill_form, however, calls resolve_sensitive_snapshot_tracking inline per field (actor.rs:2481-2500), and only takes the dialog-aware path when that resolve call itself errors/times out. If a field's oninput opens a dialog asynchronously (e.g. setTimeout(() => alert(this.value), 50)) rather than synchronously blocking the resolve evaluate, resolve returns Ok before the dialog appears, and the loop falls through to the final self.committed_post_action_snapshot(request) at actor.rs:2503 — which passes sensitive_value: None and therefore never calls redact_pending_dialogs. The eventual modal_response() then renders the stored Dialog pending: ... message="<secret>" verbatim. This directly contradicts the new CHANGELOG line "a write ... raises a dialog ... returns the pending-modal notice with the secret masked" for the one write path (fill_form) that doesn't share Type's always-dialog-aware success exit. There's no test exercising a dialog opened during a successful fill_form write (only the single-field Type dialog fixture/tests exist), which is consistent with this being an uncaught gap rather than a deliberately accepted limitation.

🟡 Suggestions (3)

• Hard-coded 1s resolve timeout vs. an O(DOM-size) scan. resolve_sensitive_snapshot_tracking uses a fixed ActionOptions::timeout(1_000.0) (actor.rs:1663), but the page-side resolve walks document.querySelectorAll('*') and calls getComputedStyle/getBoundingClientRect per element (mirroring the same scan done in begin) to detect visibility changes. On a large/complex real-world page (e.g. an SSO login embedded in a heavy SPA), this can plausibly exceed 1s, causing a completely successful password write to time out on resolve and get reported as "may have partially completed" (Err(_) branch) instead of a clean success. Consider deriving this timeout from the request's remaining budget (like sensitive_tracking_expiry_ms does for the begin step) rather than a flat constant, or trimming the visibility scan.
• Extra CDP round-trip on every text write, not just password fields. begin_sensitive_snapshot_tracking is called unconditionally at the top of both type()'s and fill_form's textbox/slider dispatch, before the actual fill/type happens, and only returns false from inside the page after checking input.type === 'password'. Every browser_type/text fill_form field now pays one extra synchronous evaluate round trip on the hot path even when the target is never a password field. Worth confirming this latency addition is acceptable, or gating it on a cheaper signal.
• writeStatus can mislabel a no-op as "partial." In RESOLVE_SENSITIVE_SNAPSHOT_TRACKING_JS, writeStatus compares the live value against the pre-action baseline captured in begin. For clear && slowly writes, if the explicit clear succeeds but the subsequent type_text fails before any character lands, the target's value differs from the original (pre-clear) baseline, so this is classified partial even though zero secret characters were ever written — producing a "field ... was partially written" message that overstates what happened. Not a security issue (masking is still applied conservatively), but the completion message is misleading.

📝 Minor / Style (1)

• FFI boundary (PyO3 Bound<'py>/GIL, napi threadsafe functions) and Python/Node parity are unaffected — this PR is scoped entirely to the internal mcp/ actor and its injected page JS, so those checklist items don't apply here.