Repository navigation
Mask a secret typed in a hosted terminal - #139
Conversation
In a session with a key reader, as hosted WebSocket and Telnet terminals have, AskSecretAsync read the secret through the line editor, which echoed every typed character in clear text and ignored the mask, with or without a timeout. Such a session now reads it key by key under the same rule as the console: only the mask is echoed, or nothing without one, Backspace erases a mask, Esc cancels, and Enter ends the line with the carriage return a remote terminal needs. That rule lives in one place, shared by the console and the key reader. A terminal disconnecting at the prompt now fails it, rather than handing the command an empty answer, and a prompt with a timeout treats only the timer's own cancellation as a timeout, so Esc cancels it too. Fixes #138
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 8d3e47ec9f
ℹ️ 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".
ReadSecretWithCountdownAsync caught OperationCanceledException whenever the caller's token was live, so Esc in a local console secret prompt with a timeout came back as no answer: an empty secret or a re-prompt. The timer never throws on this path (the read loop returns null on timeout), so the catch only ever swallowed Esc. It is removed, as in ReadLineWithCountdownAsync. The countdown loop now reads keys through an internal LocalKeySource so the behaviour is tested with a scripted key source and a fake clock.
|
@codex review |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Reviewed exact head 260e93e against 8bf5d2f. Hosted secret masking is fixed in the exercised default/custom/invisible-mask, editing, cancellation, disconnect and concurrent-session cases. The shared local key rule and timed local Esc fix also passed focused controls.
One P2 remains in the PR's explicitly changed timeout semantics: Esc accepted before the deadline can still be swallowed if the timer fires while its echo is being erased. This race existed in base too; it is an incomplete fix of the new 'only its timer counts as timeout; Esc cancels' guarantee, not a newly introduced plaintext leak. See the inline deterministic reproduction and controls.
Completed validation: Release solution build with -warnaserror, zero warnings/errors; full local suite 2,374 passed, zero failed, 3 skipped. Independent hosted and local/PTY reviews, including head/base controls; I personally reran the controlled Esc/deadline and ordinary timeout cases. CI checks are green (release skipped); checkout clean. Live network transports and Windows PTY were not exercised locally. No merge performed.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ompt's deadline A timed prompt in a hosted terminal decided it had timed out from the timer's state when it caught the exception. Esc accepted before the deadline was read as a timeout if the timer fired while its erase was echoed, or while VtKeyReader waited to tell a lone Esc from a sequence; an answer submitted with Enter was lost the same way, and so was a terminal stream that ended. A secret prompt with AllowEmpty then returned "", and a text prompt its default. Esc and an ended stream now throw an internal OperationCanceledException subtype, which the timeout filter excludes; a key reader that stops without its token being cancelled is classified the same way. Once Enter or Esc settles the prompt, a cancelled flush of its echo no longer undoes it. A hosted secret prompt also kept honouring its deadline after the first key, so a deadline mid-entry left the rest of the secret for the next reader, which echoes in clear. As at the local console, the deadline now stops once a typed key arrives; a terminal report such as a resize does not count as typing.
|
@codex review |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Revalidated exact head 8a24d40 against base 8bf5d2f and previous reviewed head 260e93e.
The original Esc/flush race now passes, and hosted secret masking, deadline-after-first-key handling, subsequent clear-text input, VT disambiguation and ordinary editor compatibility passed bounded head/previous/base controls. Three independent reviews completed; one residual P2 remains in the explicitly promised transport-cancellation/timeout separation.
Independent output cancellation during WriteAsync can still be swallowed as a timeout after the deadline fires, losing an accepted Enter/Esc outcome; a third-party reader's independent disconnect before typing has the same late-state ambiguity. Detailed deterministic reproductions and positive controls are in the existing finding thread, consolidated rather than duplicated: #139 (comment)
Parent reran the original six race/timeout controls, the eight-case production WebSocketTextWriter/control-socket matrix, and custom-reader controls. Full Release solution build with -warnaserror: zero warnings/errors; 2,383 tests passed, zero failed, 3 skipped. CI green; checkout clean. These remaining scenarios also existed on the previous head: incomplete in-scope fixes, not newly introduced regressions. No end-to-end network transport or Windows PTY exercised locally. No merge performed.
The timeout filter still decided from the timer's state when it caught the exception, so a transport that cancelled its own write while a settled key was echoed, or a third-party key reader reporting a disconnect with CancellationToken.None, became a timeout once the deadline had fired. A cancellation is now a timeout only when it carries the deadline's token. A key reader that observes the deadline through a token linked to its own lifetime still times out: ReadRemoteKeyAsync attributes a cancellation of a token linked to the one it supplied to that token, and IReplKeyReader.ReadKeyAsync now documents which token its exception carries. The comments and docs no longer claim a disconnect cancels every hosted prompt: that holds for secret prompts read through a key reader.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Revalidated exact head 66ffec4 against previous 8a24d40 and base 8bf5d2f.
The previous independent WebSocket writer cancellation and tokenless reader disconnect findings are fixed; parent reran both and their genuine-timeout/Esc/Enter controls. Secret masking and subsequent-input regression probes remained clean.
One introduced P2 blocks approval: genuine timed-text deadline cancellation through a linked FlushAsync token now propagates TaskCanceledException rather than selecting the configured default. Identical head/previous/base executable controls confirm introduction by the new exception-token equality requirement. Only the deadline fires; caller and writer-lifetime cancellation are both false. Detailed reproduction and controls in the existing thread: #139 (comment)
Completed full local validation: Release solution build with -warnaserror, 0 warnings/errors; 2,389 succeeded, 0 failed, 3 skipped. All applicable CI checks are green. Two independent reviews completed; the cancellation review found this regression and the security/editor regression scope was clean. Worktree clean; no merge performed.
…n a timeout 66ffec4 made a timed prompt time out only on an exception carrying the deadline's own token. A writer whose FlushAsync links that token with its own lifetime reports a genuine deadline with the linked token, so a hosted timed text prompt threw TaskCanceledException instead of returning its default. Only an operation handed the prompt's token can be cancelled by its deadline, so the cancellation is now attributed there: a key read and an echo flush report a cancellation of another token, while the prompt's token is cancelled, as the prompt token's own (PromptCancellation). The echo write is never handed the deadline, so a writer cancelling its own write still fails the prompt. Linkage cannot be observed: a flush whose own lifetime ends at the very moment the deadline fires is taken as a timeout. The IReplKeyReader doc now limits "a prompt fails when the input ended" to secret prompts.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Revalidated exact head 416840a5f1c6be39420a64f11ef2876e80212b2b against previous 66ffec446b91847c2f4a39847d5072bc4e6f1147 and base 8bf5d2f415df9029b05e664b3d6e037d2d473a5a.
The final linked-flush timeout finding is fixed: the original deterministic counterexample now returns DEFAULT. Parent rebuilt and reran the 24-case cancellation harness, 38-case earlier race harness, and 8-case production WebSocket writer harness. Esc/Enter, tokenless disconnection, independent writer cancellation, and genuine direct/linked deadlines retain their expected outcomes. Caller cancellation still propagates on the exercised flush paths (normalization can change TaskCanceledException to its OperationCanceledException base type). The documented indistinguishable simultaneous-token limitation is not treated as an additional finding.
Release solution build with -warnaserror: 0 warnings/errors. Full suite: 2,391 passed, 0 failed, 3 expected skips. Independent exact-head audit passed 20 assertions covering cancellation boundaries, default/custom/invisible masks, backspace/navigation, typing beyond the initial secret deadline and a subsequent clear-text editor read, with no introduced P0–P2 found. Worktree clean.
No remaining actionable code finding identified in this revalidation. Approval is nevertheless deferred because the MCP HTTP (Inspector) CI job failed, rather than passed: https://github.com/yllibed/repl/actions/runs/39711387293/job/119604669499 . Inspector exceeded the three-minute deadline with no stdout/stderr. The same opt-in test locally fails earlier with a missing @modelcontextprotocol/core/index.js package imported by Inspector. These are different observed failures, not proof of one shared cause or flakiness. The test/workflow files are unchanged by this PR. A targeted CI rerun was attempted but gh reports that this job cannot be rerun.
Please resolve or obtain a passing run of that integration check before final approval. This COMMENT documents that the previously reported code blocker is corrected; it is not a new request-changes finding against unrelated MCP code. No merge performed.
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved exact head 416840a5f1c6be39420a64f11ef2876e80212b2b, superseding my previous request-changes verdicts.
The final linked-flush deadline regression is fixed, and earlier Esc/Enter, tokenless-disconnect and independent WebSocket writer cancellation findings remain corrected. Parent rebuilt and reran the external 24-case cancellation, 38-case race and 8-case production-writer probes. An independent exact-head audit passed 20 assertions covering cancellation boundaries, secret masking (default/custom/invisible), editing, typing beyond the initial deadline, and a subsequent clear-text read without secret-tail spill. No remaining actionable introduced P0–P2 identified in the reviewed scope; the documented simultaneous-token attribution limitation is acknowledged.
Full local Release build with -warnaserror: 0 warnings/errors. Full suite: 2,391 passed, 0 failed, 3 expected skips. The opt-in MCP HTTP test also passed locally using a separately prepared Inspector installation, without product-code changes.
Immediately before this approval, live head/base were rechecked and all applicable CI checks were passing, including MCP HTTP (Inspector), Windows/macOS/Linux builds, packaging, real-shell smoke and process-signal stress. Release publication is skipped as expected. The previous CI-only approval hold is therefore cleared.
No merge performed.
… of answering it A text, confirmation or choice prompt whose hosted terminal disconnected settled as unanswered, through PromptFallback: it answered its default, so a dropped connection could confirm a default-yes action for a user who was no longer there, or failed with InvalidOperationException under PromptFallback.Fail. It now cancels the prompt with an OperationCanceledException, as a secret prompt already does since #139. Redirected input that ends still leaves the prompt to its fallback, as scripted input relies on. The prompts document the cancellation, PromptFallback says it does not apply to a disconnect, and the abandonment message is shared.
Fixes #138.
What changes
AskSecretAsyncnow reads the secret key by key: only the mask is echoed, or nothing whenMaskisnull. Backspace erases a mask, Esc cancels, and Enter ends the line with\r\n, as the line editor writes it for remote terminals. This applies with or without a timeout.ApplySecretKey), shared by the local console's reader and the key reader. The two paths drifting apart is how AskSecretAsync echoes the secret in clear text in hosted terminal sessions #138 happened. The local console's behaviour is unchanged, key for key.OperationCanceledException, rather than handing the command an empty answer or re-prompting over a dead transport.Notes
AskText, choices, confirmations) in a hosted session still read through the line editor. Without a timeout, they still turn a disconnect into their default answer; aligning them is left for a follow-up.AskSecretOptions.Timeoutno longer promises an inline countdown everywhere: only a local console shows it.AskSecretAsyncnow documents theOperationCanceledException.Verification
Given_RemoteSecretPrompt: the echo with a default mask, with a custom mask and a timeout, and with no mask; Esc, with and without a timeout; a disconnect, with and without a timeout. Every test fails againstmain(the output there wasPin: qzx 7), with a successful build.