fix: address P0 and P1 code review findings#5
Open
FredHuFuture wants to merge 16 commits intomkurman:mainfrom
Open
fix: address P0 and P1 code review findings#5FredHuFuture wants to merge 16 commits intomkurman:mainfrom
FredHuFuture wants to merge 16 commits intomkurman:mainfrom
Conversation
The named pipe \.\pipe\cmux was created without any ACL, allowing any process on the machine to connect. Add PipeSecurity that grants FullControl only to the current Windows user via NamedPipeServerStreamAcl.Create.
_readStream.Read() was called outside the lock while Dispose could simultaneously null out _readStream. Introduce a CancellationTokenSource to coordinate shutdown: the ReadLoop now checks the token and acquires _readStream under lock. Dispose signals the CTS, disposes streams under lock, then joins the read thread with a timeout.
The Dispose method killed the process but never joined _waitThread, allowing it to access already-freed handles. Replace WaitForSingleObject with WaitForMultipleObjects that monitors both the process handle and a cancel event. Dispose now signals the cancel event and joins the thread before closing handles.
_oscString had no upper bound, allowing a malicious byte stream to cause OOM. Cap OSC strings at 64 KB and abort the sequence if exceeded. Limit CSI parameters to 256 entries and cap the raw param accumulation buffer proportionally.
The CLI had zero test coverage. Add tests for: - CLI ParseArgs: long/short options, boolean flags, positional args, mixed arguments, empty input - CLI Main: help, version, unknown commands, missing subcommands - NamedPipeServer ParseArgs: key=value, JSON, quoted values, positional - VtParser limits: OSC exceeding 64KB, CSI exceeding 256 params Make CLI ParseArgs internal with InternalsVisibleTo for testability.
…rminalBuffer ScrollbackBuffer: wrap all public methods (Add, AddRange, Clear, indexer, ToList, Count) in lock. TerminalSelection: synchronize Start/Extend/Clear/SelectWord/ SelectLine/SelectAll and property accessors with lock. TerminalBuffer: expose SyncRoot and add lock to GetScrollbackLine, ExportPlainText, CreateSnapshot, and RestoreSnapshot which can be called from the UI thread while the parser runs on the read thread.
MainWindow: unsubscribe SettingsService.SettingsChanged in OnClosing (was subscribed in constructor but never unsubscribed). WorkspaceViewModel: unsubscribe surface WorkingDirectoryChanged when closing a surface and in Dispose to prevent leaked references.
DPAPI encryption previously used null optionalEntropy, and stored ciphertext had no integrity check. Now: - Use application-specific entropy bytes for DPAPI Protect/Unprotect - Prepend HMAC-SHA256 of the ciphertext to each stored value - Verify HMAC with constant-time comparison on read - Fall back to legacy format (no HMAC, no entropy) for existing secrets
The volatile check-then-act on _pendingResponse was not atomic: the listen loop could read it while SendRequestAsync was swapping it. Introduce a dedicated _pendingLock to synchronize all reads and writes of _pendingResponse across the listen loop and request path.
Add _processCreated flag to guard all property accessors (ProcessId, ProcessHandle, HasExited) and Kill against using uninitialized _processInfo values. Clean up attribute list before throwing when CreateProcess fails.
…get task The discarded Task.Run result (_ = Task.Run(...)) could silently swallow exceptions thrown outside the inner try-catch (e.g., during lambda capture or task scheduling). Add ContinueWith with OnlyOnFaulted to observe and log any unhandled task exceptions.
FredHuFuture
force-pushed
the
fix/code-review-p0-p1
branch
from
39f302d to
957b27f
Compare
![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: 39f302dfa7
ℹ️ 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".
blob.Length > 32 also matches legacy DPAPI ciphertext, so existing secrets were misclassified as new-format and returned null on HMAC mismatch. Catch CryptographicException from new-format Unprotect and fall through to legacy Unprotect(..., null, ...) path. Addresses codex review feedback on SecretStoreService.
Previously the parser jumped to Ground on oversized OSC, causing remaining OSC bytes to be interpreted as normal terminal input. Now the parser stays in OscString and silently discards excess bytes until BEL/ST terminates the sequence. Addresses codex review feedback on VtParser.
Add UnicodeWidth utility for detecting wide (2-column) characters such as CJK ideographs, Hangul syllables, and fullwidth forms. TerminalBuffer.WriteChar now sets Width=2 for wide characters, places a padding cell (Width=0) in the next column, and advances the cursor by 2. Early line-wrap is triggered when a wide char won't fit. TerminalControl rendering skips padding cells, draws double-width backgrounds for wide cells, and tracks column span instead of character count for underline/strikethrough decoration width.
Log PseudoConsole creation, process PID, and ReadLoop lifecycle events to cmux-diag.log for easier debugging of terminal startup failures.
Owner
|
@copilot code review |
There was a problem hiding this comment.
Pull request overview
This PR addresses previously identified P0/P1 review findings across IPC security, terminal stability/thread-safety, secret storage integrity, VT parsing hardening, CJK rendering, and adds/expands test coverage for CLI and parser limits.
Changes:
- Hardened IPC + process/session lifecycle: restricted named pipe ACLs; reduced shutdown races by adding cancellation/join semantics in terminal threads.
- Defensive parsing + rendering: added VT parser limits and Unicode wide-character handling across buffer + renderer.
- Reliability/security improvements + tests: added secret store verification logic and expanded CLI/parser unit tests.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Cmux.Tests/CoreTests.cs | Adds CLI, named-pipe arg parsing, and VT parser limit tests. |
| tests/Cmux.Tests/Cmux.Tests.csproj | References Cmux.Cli so CLI internals can be tested. |
| src/Cmux/Views/MainWindow.xaml.cs | Unsubscribes SettingsChanged on window close to prevent leaks. |
| src/Cmux/ViewModels/WorkspaceViewModel.cs | Unsubscribes WorkingDirectoryChanged and disposes surfaces more safely. |
| src/Cmux/Services/AgentRuntimeService.cs | Observes fire-and-forget task failures via faulted continuation. |
| src/Cmux/Controls/TerminalControl.cs | Updates rendering to support wide characters and skip padding cells. |
| src/Cmux.Core/Terminal/VtParser.cs | Adds OSC/CSI size limits to reduce memory risk from malicious streams. |
| src/Cmux.Core/Terminal/UnicodeWidth.cs | Introduces a wcwidth-like helper for wide/fullwidth character detection. |
| src/Cmux.Core/Terminal/TerminalSession.cs | Adds cancellation/join in dispose path; adds diagnostic logging. |
| src/Cmux.Core/Terminal/TerminalSelection.cs | Adds locking around selection state for thread-safety. |
| src/Cmux.Core/Terminal/TerminalProcess.cs | Joins wait thread on dispose; adds cancel event + multi-wait. |
| src/Cmux.Core/Terminal/TerminalBuffer.cs | Adds synchronization hooks and wide-character cell handling. |
| src/Cmux.Core/Terminal/ScrollbackBuffer.cs | Adds locking around buffer operations for thread-safety. |
| src/Cmux.Core/Terminal/ConPtyInterop.cs | Adds P/Invoke declarations for WaitForMultipleObjects + events. |
| src/Cmux.Core/Services/SecretStoreService.cs | Adds HMAC + DPAPI entropy and legacy fallback handling. |
| src/Cmux.Core/IPC/NamedPipeServer.cs | Restricts named pipe access to current user via PipeSecurity. |
| src/Cmux.Core/IPC/DaemonClient.cs | Replaces volatile check-then-act on pending response with a lock. |
| src/Cmux.Core/Cmux.Core.csproj | Adds System.IO.Pipes.AccessControl package dependency. |
| src/Cmux.Cli/Program.cs | Exposes ParseArgs internally for unit testing. |
| src/Cmux.Cli/Cmux.Cli.csproj | Adds InternalsVisibleTo for Cmux.Tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…afety - VtParser: add _oscOverflow flag to suppress dispatch for overflowed OSC sequences - TerminalProcess: validate CreateEventW return, handle WAIT_FAILED in WaitForMultipleObjects - SecretStoreService: remove trivially-forgeable HMAC (public key), rely on DPAPI integrity with entropy; keep backward compat for legacy + HMAC formats - TerminalSession: gate diagnostic logging behind CMUX_DIAG env var, add 2MB log rotation - TerminalControl: lock buffer.SyncRoot during Render to prevent torn reads - CoreTests: strengthen OSC overflow test to assert no dispatch, add positive case
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses all P0 (critical) and P1 (high priority) findings from code review, one commit per fix:
P0 — Immediate fixes
\.\pipe\cmuxto current user viaPipeSecurity/NamedPipeServerStreamAcl.CreateCancellationTokenSourceand acquire_readStreamunder lock; join read thread on DisposeWaitForMultipleObjectswith a cancel event; signal + join on DisposeP1 — High priority fixes
locksynchronization toScrollbackBuffer,TerminalSelection, andTerminalBufferSettingsChangedin MainWindow; unsubscribeWorkingDirectoryChangedin WorkspaceViewModel_pendingResponsewith a dedicated lock_processCreatedguard on all property accessors; clean up on failureContinueWith(OnlyOnFaulted)to prevent silent failuresAdditional fixes (from Codex review & testing)
blob.Length > 32misclassifying legacy DPAPI secrets as new-format; now catches decryption failure and falls back to legacyUnprotect(..., null, ...)pathGroundstate when OSC exceeds max length; now remains inOscStringstate to consume remaining bytes until BEL/ST terminator arrivesUnicodeWidthutility for detecting wide characters (CJK, fullwidth forms, etc.);TerminalBuffer.WriteCharnow advances cursor by 2 columns and places a padding cell for wide chars;TerminalControlrendering skips padding cells and draws wide characters at double width — fixes Chinese/Japanese/Korean text display where cursor position was offsetTest plan
dotnet test)