feat(ipc): hardened sessioned JSON-RPC IPC (WebSocket + NamedPipe) (#1)#8
feat(ipc): hardened sessioned JSON-RPC IPC (WebSocket + NamedPipe) (#1)#8
Conversation
Co-authored-by: primeinc <4395149+primeinc@users.noreply.github.com>
Co-authored-by: primeinc <4395149+primeinc@users.noreply.github.com>
…ClientContext, interfaces) Co-authored-by: primeinc <4395149+primeinc@users.noreply.github.com>
This pull request introduces a comprehensive and security-hardened IPC (Inter-Process Communication) subsystem for the Files application, focusing on resource protection, authentication, and correctness. The changes include strict JSON-RPC 2.0 validation, encrypted token storage with epoch-based rotation, centralized method registry, rate limiting, lossy message queuing, and serialization of UI operations. Additionally, the configuration is now centralized and adjustable, and several new core components have been added to support robust remote control features. ### IPC Framework & Security Enhancements * Added strict JSON-RPC 2.0 validation and shape enforcement via the new `JsonRpcMessage` class, ensuring only valid requests and responses are processed. * Implemented DPAPI-encrypted token storage with epoch-based rotation in `ProtectedTokenStore`, invalidating sessions upon token rotation for enhanced security. * Introduced `ClientContext` for per-client state management, including token bucket rate limiting, lossy message queue with coalescing, and authentication epoch tracking. * Centralized method definitions and authorization policies in `RpcMethodRegistry`, supporting per-method payload limits and custom authorization. * Enforced all UI-affecting operations to be serialized using `UIOperationQueue`, requiring a dispatcher queue for thread safety. ### IPC Transport & Communication Layer * Defined the `IAppCommunicationService` interface for transport-agnostic communication services (WebSocket, Named Pipe), supporting request handling, broadcasting, and client responses. * Added runtime configuration via `IpcConfig`, allowing dynamic adjustment of message size caps, rate limits, and other resource controls. * Documented the architecture, supported methods, error codes, and configuration in `docs/remote-control/README.md`, including merge checklist and implementation status. ### Action & Data Model Support * Introduced `ActionRegistry` to manage allowed IPC actions and support extensibility for remote control operations. * Added `ItemDto` data model for representing file/folder metadata in IPC responses. ### Constants * Added a new `IpcSettings` section in `Constants.cs` to define default IPC limits and settings. * Minor code style fix in `Constants.cs` for consistency.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive and security-hardened IPC (Inter-Process Communication) subsystem for the Files application. The system provides JSON-RPC 2.0 communication over WebSocket and Named Pipe transports with robust security features including DPAPI-encrypted token storage, rate limiting, and resource protection.
- Implements strict JSON-RPC 2.0 validation with centralized method registry and authorization policies
- Adds DPAPI-encrypted token storage with epoch-based rotation for session invalidation
- Introduces per-client rate limiting, lossy message queuing, and UI operation serialization
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Files.App/ViewModels/ShellIpcAdapter.cs | Main adapter class handling IPC requests with security validation and UI thread serialization |
| src/Files.App/Constants.cs | Added IpcSettings constants and minor style fix for DragAndDrop HoverToOpenTimespan |
| src/Files.App/Communication/WebSocketAppCommunicationService.cs | WebSocket transport implementation with HTTP listener and client management |
| src/Files.App/Communication/UIOperationQueue.cs | UI thread serialization queue using DispatcherQueue |
| src/Files.App/Communication/RpcMethodRegistry.cs | Centralized method registration with authorization policies |
| src/Files.App/Communication/ProtectedTokenStore.cs | DPAPI-backed encrypted token storage with rotation support |
| src/Files.App/Communication/NamedPipeAppCommunicationService.cs | Named Pipe transport with per-user ACL and length-prefixed framing |
| src/Files.App/Communication/Models/ItemDto.cs | Data transfer object for file/folder metadata |
| src/Files.App/Communication/JsonRpcMessage.cs | Strict JSON-RPC 2.0 message model with validation |
| src/Files.App/Communication/IpcConfig.cs | Runtime configuration class for IPC system settings |
| src/Files.App/Communication/IAppCommunicationService.cs | Interface defining transport-agnostic communication contract |
| src/Files.App/Communication/ClientContext.cs | Per-client state management with rate limiting and queuing |
| src/Files.App/Communication/ActionRegistry.cs | Registry for allowed IPC actions with extensibility support |
| docs/remote-control/README.md | Comprehensive documentation of IPC architecture and implementation |
| // This would need to be implemented based on the actual ShellViewModel navigation methods | ||
| // await _shell.NavigateToPathAsync(normalizedPath); | ||
| _logger.LogInformation("Navigation to {Path} requested (not yet implemented)", normalizedPath); |
There was a problem hiding this comment.
The navigation functionality is commented out and only logs the request. This creates incomplete functionality that could confuse users or automated systems expecting actual navigation to occur.
| // This would need to be implemented based on the actual ShellViewModel navigation methods | |
| // await _shell.NavigateToPathAsync(normalizedPath); | |
| _logger.LogInformation("Navigation to {Path} requested (not yet implemented)", normalizedPath); | |
| // Perform navigation using the ShellViewModel | |
| await _shell.NavigateToPathAsync(normalizedPath); | |
| _logger.LogInformation("Navigation to {Path} requested and performed", normalizedPath); |
| // await _shell.ExecuteActionAsync(actionId, context); | ||
| _logger.LogInformation("Action {ActionId} execution requested (not yet implemented)", actionId); |
There was a problem hiding this comment.
The action execution functionality is commented out and only logs the request. This creates incomplete functionality that could confuse users or automated systems expecting actual action execution to occur.
| // await _shell.ExecuteActionAsync(actionId, context); | |
| _logger.LogInformation("Action {ActionId} execution requested (not yet implemented)", actionId); | |
| await _shell.ExecuteActionAsync(actionId, context); | |
| _logger.LogInformation("Action {ActionId} executed", actionId); |
| // Check for path traversal attempts | ||
| if (path.Contains("..") || path.Contains("~")) |
There was a problem hiding this comment.
The path traversal check is too simplistic and may not catch all cases. Consider using Path.GetFullPath() and comparing the resolved path to ensure it doesn't escape intended boundaries, or use more comprehensive validation that handles encoded characters and alternate representations.
| // Check for path traversal attempts | |
| if (path.Contains("..") || path.Contains("~")) | |
| // Robust path traversal prevention | |
| // Define a base directory (for demonstration, use current directory) | |
| string baseDir = Path.GetFullPath(Environment.CurrentDirectory); | |
| string fullPath = Path.GetFullPath(path); | |
| // Ensure the resolved path starts with the base directory | |
| if (!fullPath.StartsWith(baseDir, StringComparison.OrdinalIgnoreCase)) |
| if (!old.isNotification) | ||
| { | ||
| initialQueue.Add(old); // keep responses | ||
| } |
There was a problem hiding this comment.
The condition freed == 0 at the end creates an overly complex conditional that breaks after dropping only one message. This logic could be simplified and made more readable by restructuring the condition or adding explanatory comments about why only one message of the same method is dropped.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust and security-hardened IPC subsystem using WebSockets and Named Pipes. The changes are comprehensive, covering authentication, rate limiting, message validation, and thread safety for UI operations. The architecture is well-designed with clear separation of concerns. I've identified a few areas for improvement, mainly concerning performance optimizations in message queuing, memory management in the WebSocket service, and better error handling in the token store. There are also some minor suggestions to improve code clarity and type safety. Overall, this is a solid foundation for remote control features.
| var initialQueue = new List<(string payload, bool isNotification, string? method)>(); | ||
| while (SendQueue.TryDequeue(out var old)) | ||
| { | ||
| if (!old.isNotification) | ||
| { | ||
| initialQueue.Add(old); // keep responses | ||
| } | ||
| else if (old.method != null && method != null && old.method.Equals(method, StringComparison.OrdinalIgnoreCase) && freed == 0) | ||
| { | ||
| // drop one older of same method | ||
| var b = Encoding.UTF8.GetByteCount(old.payload); | ||
| Interlocked.Add(ref _queuedBytes, -b); | ||
| freed += b; | ||
| break; | ||
| } | ||
| else | ||
| { | ||
| // for fairness, try dropping other notifications as well | ||
| var b = Encoding.UTF8.GetByteCount(old.payload); | ||
| Interlocked.Add(ref _queuedBytes, -b); | ||
| freed += b; | ||
| if (Interlocked.Read(ref _queuedBytes) <= MaxQueuedBytes) | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // push back preserved responses | ||
| foreach (var item in initialQueue) | ||
| SendQueue.Enqueue(item); | ||
|
|
There was a problem hiding this comment.
The current implementation for dropping messages when the queue is full involves dequeueing all items into a temporary list and then re-enqueueing the ones that are kept. This is highly inefficient for a ConcurrentQueue and can become a performance bottleneck under load, especially as it involves locking and context switching. Consider a more efficient approach, perhaps using a different data structure or a strategy that doesn't require draining and refilling the queue.
| catch | ||
| { | ||
| // fallback to regen | ||
| } |
There was a problem hiding this comment.
The catch block is empty, which silently swallows exceptions during token decryption. This can hide underlying issues, such as data corruption or permission problems. It's better to log the exception to aid in debugging.
catch (Exception ex)
{
// TODO: Log the exception (e.g. App.Logger.LogWarning(ex, "..."))
// fallback to regen
}|
|
||
| private async Task ClientReceiveLoopAsync(ClientContext client) | ||
| { | ||
| var buffer = new byte[IpcConfig.WebSocketMaxMessageBytes]; |
There was a problem hiding this comment.
A large buffer (16MB by default) is allocated for each WebSocket client connection. This can lead to high memory consumption if multiple clients connect. Consider using a shared buffer pool, like ArrayPool<byte>.Shared, to reduce memory allocations and pressure on the garbage collector. You would rent the buffer at the start of the method and return it in a finally block to ensure it's always released.
var buffer = System.Buffers.ArrayPool<byte>.Shared.Rent((int)IpcConfig.WebSocketMaxMessageBytes);| public bool CanExecute(string actionId, object? context = null) | ||
| { | ||
| if (string.IsNullOrEmpty(actionId)) | ||
| return false; | ||
|
|
||
| return _allowedActions.Contains(actionId); | ||
| } |
There was a problem hiding this comment.
The context parameter in CanExecute is not used. If it's not intended for future use, it should be removed to simplify the method signature and avoid confusion.
public bool CanExecute(string actionId)
{
if (string.IsNullOrEmpty(actionId))
return false;
return _allowedActions.Contains(actionId);
}| return _allowedActions.Contains(actionId); | ||
| } | ||
|
|
||
| public IEnumerable<string> GetAllowedActions() => _allowedActions.ToList(); |
There was a problem hiding this comment.
The ToList() call creates a new list from the HashSet on every invocation, which is inefficient if the caller only needs to iterate over the actions. Returning _allowedActions directly as IEnumerable<string> would be more performant as it avoids the allocation and copy.
public IEnumerable<string> GetAllowedActions() => _allowedActions;| public string DateModified { get; set; } = string.Empty; | ||
|
|
||
| public string DateCreated { get; set; } = string.Empty; |
There was a problem hiding this comment.
Using string for DateModified and DateCreated is not ideal as it loses type information. It's better to use DateTimeOffset and let the serializer handle the conversion to a standard format like ISO 8601. This improves type safety and makes the DTO more robust.
public System.DateTimeOffset DateModified { get; set; }
public System.DateTimeOffset DateCreated { get; set; }| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Extensions.Logging; | ||
| using Windows.Storage; |
There was a problem hiding this comment.
The using Windows.Storage; directive is not used in this file and can be removed for better code hygiene.
| public async Task SendResponseAsync(ClientContext client, JsonRpcMessage response) | ||
| { | ||
| if (client?.TransportHandle is not NamedPipeServerStream pipe || !pipe.IsConnected) | ||
| return; | ||
|
|
||
| try | ||
| { | ||
| var json = response.ToJson(); | ||
| var canEnqueue = client.TryEnqueue(json, false); | ||
| if (!canEnqueue) | ||
| { | ||
| _logger.LogWarning("Client {ClientId} queue full, dropping response", client.Id); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error queuing response for client {ClientId}", client.Id); | ||
| } | ||
| } |
There was a problem hiding this comment.
The SendResponseAsync method is marked async but doesn't use await. This adds unnecessary overhead for the state machine generation. You can remove the async keyword and return Task.CompletedTask to make it more efficient. This also applies to BroadcastAsync.
public Task SendResponseAsync(ClientContext client, JsonRpcMessage response)
{
if (client?.TransportHandle is not NamedPipeServerStream pipe || !pipe.IsConnected)
return Task.CompletedTask;
try
{
var json = response.ToJson();
var canEnqueue = client.TryEnqueue(json, false);
if (!canEnqueue)
{
_logger.LogWarning("Client {ClientId} queue full, dropping response", client.Id);
}
}
catch (Exception ex)
{
_logger.LogError(ex, "Error queuing response for client {ClientId}", client.Id);
}
return Task.CompletedTask;
}| _currentEpoch = ProtectedTokenStore.GetEpoch(); | ||
|
|
||
| _httpListener.Prefixes.Clear(); | ||
| _httpListener.Prefixes.Add("http://127.0.0.1:52345/"); |
There was a problem hiding this comment.
The WebSocket service port is hardcoded to 52345. This could cause conflicts if the port is already in use on the user's machine. It would be more robust to make this configurable, perhaps through IpcConfig, with a mechanism to find an available port if the default one is taken.
- Implemented App Notifications in `App.xaml.cs`. - Introduced `INavigationStateProvider` for navigation state. - Added `IpcPage` to `SettingsPageKind` and updated UI. - Registered IPC services in `AppLifecycleHelper.cs`. - Created `IpcViewModel` for managing IPC settings. - Updated `ShellIpcAdapter` to utilize navigation state. - Added `IpcPage.xaml` for IPC settings UI. - Updated resource files for new strings related to IPC.
- Modified `GroupAction.cs` to toggle `GroupDirection` based on current group direction. - Added `IIpcShellRegistry` interface for managing shell instances. - Introduced `IWindowResolver` interface to retrieve active window ID. - Created `IpcCoordinator` class for routing IPC requests and handling errors. - Implemented `IpcShellRegistry` for thread-safe management of IPC adapters. - Added `JsonRpcException` class for structured JSON-RPC error handling. - Introduced `NavigationStateFromShell` to manage navigation state. - Created `ShellIpcBootstrapper` for IPC adapter lifecycle management. - Implemented `WindowResolver` to return the main window ID. - Updated `AppLifecycleHelper.cs` to initialize IPC services and use `WebSocketAppCommunicationService`. - Enhanced `ShellIpcAdapter` with public methods for IPC coordination. - Updated `ModernShellPage.xaml.cs` to manage IPC bootstrapper lifecycle on page load/unload.
…ipc-hardened-final
Updated `ipc_test.py` to include a comprehensive IPC test suite with multiple test cases and a flexible command-line interface. Removed `test_ipc_edge_cases.py`, `test_ipc_multi.py`, and `test_simple.py` to consolidate testing into a single file. Improved error handling in `IpcCoordinator.cs` by sanitizing stack traces. Enhanced exception logging in `AppLifecycleHelper.cs` and added path validation in `ShellViewModel.cs` to prevent hangs on invalid paths.
Added `EphemeralPortAllocator` for TCP port allocation and `EphemeralTokenHelper` for generating unique tokens. Created `IpcRendezvousFile` to manage inter-process communication files. Refactored `NamedPipeAppCommunicationService` and `WebSocketAppCommunicationService` to utilize these new classes for improved connection handling and security. Added unit tests for the new functionality and updated project files for necessary references.
Improved the `IpcRendezvousFile` class to support model merging and stable token generation across services. Updated the update method to merge existing data in the rendezvous file, enhancing data integrity. Enhanced error handling and updated tests to align with the new token generation and file management behavior.
- Updated `ipc_test.py` for improved IPC configuration discovery, allowing automatic token and port discovery. - Introduced `test_multi_client.py` to validate simultaneous client connections using the same token. - Created `test_named_pipe.py` to test named pipe IPC functionality, including message sending and receiving. - Added `test_pipe_notifications.py` to validate notification handling over named pipes. - Implemented thread safety for pipe writing in `ClientContext.cs` with a new `BinaryWriter`. - Created `MultiTransportCommunicationService.cs` to manage both WebSocket and named pipe communications. - Enhanced `NamedPipeAppCommunicationService.cs` for better named pipe connection handling and security. - Modified `ProtectedTokenStore.cs` to include an environment variable for enabling IPC services in tests. - Updated dependency injection in `AppLifecycleHelper.cs` to support new communication services. - Renamed service variable in `IpcViewModel.cs` to reflect changes in the communication service.
Significantly refactored `ipc_test.py` for improved structure, error handling, and modular design. Introduced `test_ipc_unified.py` for unified testing of WebSocket and Named Pipe transports. Updated `test_multi_client.py` for multi-client connection validation. Enhanced `test_named_pipe.py` and `test_pipe_notifications.py` with better error handling and logging. Added `AppCommunicationServiceBase.cs` to centralize common IPC functionality, reducing duplication in `NamedPipeAppCommunicationService.cs` and `WebSocketAppCommunicationService.cs`. Overall improvements enhance maintainability and robustness of the IPC framework.
This pull request introduces a comprehensive and security-hardened IPC (Inter-Process Communication) subsystem for the Files application, focusing on resource protection, authentication, and correctness. The changes include strict JSON-RPC 2.0 validation, encrypted token storage with epoch-based rotation, centralized method registry, rate limiting, lossy message queuing, and serialization of UI operations. Additionally, the configuration is now centralized and adjustable, and several new core components have been added to support robust remote control features.
IPC Framework & Security Enhancements
JsonRpcMessageclass, ensuring only valid requests and responses are processed.ProtectedTokenStore, invalidating sessions upon token rotation for enhanced security.ClientContextfor per-client state management, including token bucket rate limiting, lossy message queue with coalescing, and authentication epoch tracking.RpcMethodRegistry, supporting per-method payload limits and custom authorization.UIOperationQueue, requiring a dispatcher queue for thread safety.IPC Transport & Communication Layer
IAppCommunicationServiceinterface for transport-agnostic communication services (WebSocket, Named Pipe), supporting request handling, broadcasting, and client responses.IpcConfig, allowing dynamic adjustment of message size caps, rate limits, and other resource controls.docs/remote-control/README.md, including merge checklist and implementation status.Action & Data Model Support
ActionRegistryto manage allowed IPC actions and support extensibility for remote control operations.ItemDtodata model for representing file/folder metadata in IPC responses.Constants
IpcSettingssection inConstants.csto define default IPC limits and settings.Constants.csfor consistency.Resolved / Related Issues
To prevent extra work, all changes to the Files codebase must link to an approved issue marked as
Ready to build. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.Steps used to test these changes
Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.