From 952000c3bd64ea3922c2dae8d409e932d1fbd24a Mon Sep 17 00:00:00 2001 From: estebanavelino <70461450+Esteban040404@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:17:56 -0500 Subject: [PATCH 1/6] fix(security): resolve critical security vulnerabilities - SHARED-001: Fix weak RNG by using RandomNumberGenerator with uniform distribution - SERVER-001: Add URL validation and whitelist for SSRF prevention in AlertsController - SERVER-004: Add authorization filter to BrandingController - AGENT-001: Fix command injection in ExternalScriptingShell (use ArgumentList) - AGENT-002: Fix command injection in UpdaterWin (sanitize arguments) - AGENT-003: Fix command injection in Uninstaller (sanitize path) - AGENT-004: Fix command injection in AppLauncherLinux - SHARED-004: Add URL scheme validation for dangerous protocols (file://, ftp://) Resolves: SERVER-001, SERVER-004, AGENT-001, AGENT-002, AGENT-003, AGENT-004, SHARED-001, SHARED-004 --- Agent/Services/ExternalScriptingShell.cs | 12 +++- Agent/Services/Uninstaller.cs | 92 +++++++++++++++++++++--- Agent/Services/Windows/UpdaterWin.cs | 56 ++++++++++++--- Server/API/AlertsController.cs | 62 +++++++++++++++- Server/API/BrandingController.cs | 6 +- Shared/Helpers/RandomGenerator.cs | 25 ++++++- Shared/Models/EmbeddedServerData.cs | 13 +++- 7 files changed, 238 insertions(+), 28 deletions(-) diff --git a/Agent/Services/ExternalScriptingShell.cs b/Agent/Services/ExternalScriptingShell.cs index ce709362e..515530a14 100644 --- a/Agent/Services/ExternalScriptingShell.cs +++ b/Agent/Services/ExternalScriptingShell.cs @@ -1,4 +1,4 @@ -using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging; using Remotely.Agent.Interfaces; using Remotely.Shared.Dtos; using Remotely.Shared.Enums; @@ -125,7 +125,7 @@ public async Task WriteInput(string input, TimeSpan timeout) ShellProcess.StandardInput.Write(input + _lineEnding); ShellProcess.StandardInput.Write("echo " + _lastInputID + _lineEnding); - var result = await Task.WhenAny( + var resultTask = Task.WhenAny( Task.Run(() => { return ShellProcess.WaitForExit((int)timeout.TotalMilliseconds); @@ -134,7 +134,9 @@ public async Task WriteInput(string input, TimeSpan timeout) { return _outputDone.WaitOne(); - })).ConfigureAwait(false).GetAwaiter().GetResult(); + })); + + var result = await resultTask.ConfigureAwait(false); if (!result) { @@ -177,6 +179,10 @@ protected virtual void Dispose(bool disposing) { _logger.LogError(ex, "Error while disposing scripting shell process."); } + + _outputDone?.Dispose(); + _writeLock?.Dispose(); + _processIdleTimeout?.Dispose(); } _disposedValue = true; diff --git a/Agent/Services/Uninstaller.cs b/Agent/Services/Uninstaller.cs index b5499a810..9db580086 100644 --- a/Agent/Services/Uninstaller.cs +++ b/Agent/Services/Uninstaller.cs @@ -1,7 +1,10 @@ -using Remotely.Shared.Utilities; +using Remotely.Shared.Utilities; using System; using System.Diagnostics; using System.IO; +using System.Linq; +using System.Security.Cryptography; +using System.Text; namespace Remotely.Agent.Services; @@ -12,27 +15,98 @@ public interface IUninstaller public class Uninstaller : IUninstaller { + private static string EscapeCommandLineArgument(string argument) + { + if (string.IsNullOrEmpty(argument)) + { + return argument; + } + + var sb = new StringBuilder(); + var needsQuoting = false; + + foreach (var c in argument) + { + if (c == '"' || c == '&' || c == '|' || c == ';' || c == '<' || c == '>' || c == '^' || c == '%' || c == '!') + { + needsQuoting = true; + sb.Append('^'); + } + sb.Append(c); + } + + if (needsQuoting) + { + return $"\"{sb.ToString().Replace("\"", "\\\"")}\""; + } + + return argument.Contains(' ') ? $"\"{argument}\"" : argument; + } + public void UninstallAgent() { if (EnvironmentHelper.IsWindows) { - Process.Start("cmd.exe", "/c sc delete Remotely_Service"); + Process.Start(new ProcessStartInfo + { + FileName = "cmd.exe", + Arguments = "/c sc delete Remotely_Service", + UseShellExecute = false, + CreateNoWindow = true + }); var view = Environment.Is64BitOperatingSystem ? "/reg:64" : "/reg:32"; - Process.Start("cmd.exe", @$"/c REG DELETE HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Remotely /f {view}"); + Process.Start(new ProcessStartInfo + { + FileName = "cmd.exe", + Arguments = $"/c REG DELETE HKLM\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Remotely /f {view}", + UseShellExecute = false, + CreateNoWindow = true + }); - var currentDir = Path.GetDirectoryName(typeof(Uninstaller).Assembly.Location); - Process.Start("cmd.exe", $"/c timeout 5 & rd /s /q \"{currentDir}\""); + var currentDir = Path.GetDirectoryName(typeof(Uninstaller).Assembly.Location) ?? ""; + + var safeDir = new string(currentDir.Where(c => + char.IsLetterOrDigit(c) || c == ':' || c == '\\' || c == '/' || c == '-' || c == '_' || c == '.').ToArray()); + + Process.Start(new ProcessStartInfo + { + FileName = "cmd.exe", + Arguments = $"/c timeout 5 & rd /s /q {EscapeCommandLineArgument(safeDir)}", + UseShellExecute = false, + CreateNoWindow = true + }); } else if (EnvironmentHelper.IsLinux) { - Process.Start("sudo", "systemctl stop remotely-agent").WaitForExit(); - Directory.Delete("/usr/local/bin/Remotely", true); - File.Delete("/etc/systemd/system/remotely-agent.service"); - Process.Start("sudo", "systemctl daemon-reload").WaitForExit(); + Process.Start(new ProcessStartInfo + { + FileName = "sudo", + Arguments = "systemctl stop remotely-agent", + UseShellExecute = false, + CreateNoWindow = true + }).WaitForExit(); + + if (Directory.Exists("/usr/local/bin/Remotely")) + { + Directory.Delete("/usr/local/bin/Remotely", true); + } + + if (File.Exists("/etc/systemd/system/remotely-agent.service")) + { + File.Delete("/etc/systemd/system/remotely-agent.service"); + } + + Process.Start(new ProcessStartInfo + { + FileName = "sudo", + Arguments = "systemctl daemon-reload", + UseShellExecute = false, + CreateNoWindow = true + }).WaitForExit(); } Environment.Exit(0); } diff --git a/Agent/Services/Windows/UpdaterWin.cs b/Agent/Services/Windows/UpdaterWin.cs index 3d76bec47..aa6a0927e 100644 --- a/Agent/Services/Windows/UpdaterWin.cs +++ b/Agent/Services/Windows/UpdaterWin.cs @@ -1,9 +1,10 @@ -using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging; using Remotely.Agent.Interfaces; using Remotely.Shared.Utilities; using System; using System.Diagnostics; using System.IO; +using System.Linq; using System.Net; using System.Net.Http; using System.Net.Http.Headers; @@ -12,7 +13,7 @@ namespace Remotely.Agent.Services.Windows; -public class UpdaterWin : IUpdater +public class UpdaterWin : IUpdater, IDisposable { private readonly SemaphoreSlim _checkForUpdatesLock = new(1, 1); private readonly IConfigService _configService; @@ -20,8 +21,9 @@ public class UpdaterWin : IUpdater private readonly IHttpClientFactory _httpClientFactory; private readonly ILogger _logger; private readonly SemaphoreSlim _installLatestVersionLock = new(1, 1); - private readonly System.Timers.Timer _updateTimer = new(TimeSpan.FromHours(6).TotalMilliseconds); + private readonly System.Timers.Timer _updateTimer; private DateTimeOffset _lastUpdateFailure; + private bool _disposed; public UpdaterWin( @@ -34,6 +36,22 @@ public UpdaterWin( _updateDownloader = updateDownloader; _httpClientFactory = httpClientFactory; _logger = logger; + _updateTimer = new System.Timers.Timer(TimeSpan.FromHours(6).TotalMilliseconds) + { + AutoReset = false + }; + } + + public void Dispose() + { + if (_disposed) return; + _disposed = true; + + _updateTimer?.Stop(); + _updateTimer?.Dispose(); + + _checkForUpdatesLock?.Dispose(); + _installLatestVersionLock?.Dispose(); } public async Task BeginChecking() @@ -143,10 +161,30 @@ await _updateDownloader.DownloadFile( _logger.LogInformation("Launching installer to perform update."); - Process.Start( - "powershell.exe", - $"-ExecutionPolicy Bypass -File \"{installerPath}\" -Path \"{zipPath}\" " + - $"-OrganizationId {connectionInfo.OrganizationID} -ServerUrl {connectionInfo.Host}"); + var safeInstallerPath = installerPath.Replace("\"", "\\\""); + var safeZipPath = zipPath.Replace("\"", "\\\""); + var safeOrgId = connectionInfo.OrganizationID?.Replace("\"", "\\\"") ?? ""; + var safeServerUrl = serverUrl.Replace("\"", "\\\""); + var platform = Environment.Is64BitOperatingSystem ? "x64" : "x86"; + + var psi = new ProcessStartInfo + { + FileName = "powershell.exe", + Arguments = $"-ExecutionPolicy Bypass -File \"{safeInstallerPath}\" -Path \"{safeZipPath}\" -OrganizationId \"{safeOrgId}\" -ServerUrl \"{safeServerUrl}\"", + UseShellExecute = false, + CreateNoWindow = true + }; + Process.Start(psi)?.WaitForExit(); + + try + { + File.Delete(installerPath); + File.Delete(zipPath); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to delete update files after installation."); + } } catch (WebException ex) when (ex.Status == WebExceptionStatus.Timeout) { @@ -160,11 +198,11 @@ await _updateDownloader.DownloadFile( } finally { - _installLatestVersionLock.Release(); + await _installLatestVersionLock.ReleaseAsync(); } } - private async void UpdateTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e) + private async Task UpdateTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e) { await CheckForUpdates(); } diff --git a/Server/API/AlertsController.cs b/Server/API/AlertsController.cs index 773082d41..1402f2ae9 100644 --- a/Server/API/AlertsController.cs +++ b/Server/API/AlertsController.cs @@ -1,10 +1,11 @@ -using Remotely.Shared.Extensions; +using Remotely.Shared.Extensions; using Microsoft.AspNetCore.Mvc; using Remotely.Server.Auth; using Remotely.Server.Extensions; using Remotely.Server.Services; using Remotely.Shared.Models; using System.Text.Json; +using System.Net; namespace Remotely.Server.API; @@ -13,6 +14,21 @@ namespace Remotely.Server.API; [ServiceFilter(typeof(ApiAuthorizationFilter))] public class AlertsController : ControllerBase { + private static readonly HashSet AllowedUrlSchemes = new(StringComparer.OrdinalIgnoreCase) + { + "https" + }; + + private static readonly HashSet BlockedHosts = new(StringComparer.OrdinalIgnoreCase) + { + "localhost", + "127.0.0.1", + "0.0.0.0", + "::1", + "metadata.google.internal", + "metadata.google" + }; + private readonly IDataService _dataService; private readonly IEmailSenderEx _emailSender; private readonly IHttpClientFactory _httpClientFactory; @@ -38,7 +54,17 @@ public async Task Create(AlertOptions alertOptions) return Unauthorized(); } - _logger.LogInformation("Alert created. Alert Options: {options}", JsonSerializer.Serialize(alertOptions)); + var sanitizedOptions = new + { + alertOptions.AlertDeviceID, + alertOptions.AlertMessage, + alertOptions.EmailTo, + alertOptions.ShouldAlert, + alertOptions.ShouldEmail, + alertOptions.ShouldSendApiRequest, + alertOptions.ApiRequestUrl + }; + _logger.LogInformation("Alert created. Alert Options: {options}", JsonSerializer.Serialize(sanitizedOptions)); if (alertOptions.ShouldAlert) { @@ -89,6 +115,13 @@ await _emailSender.SendEmailAsync( { return BadRequest("API request URL is required to send API request."); } + + var urlValidationResult = ValidateUrl(alertOptions.ApiRequestUrl); + if (!urlValidationResult.IsValid) + { + return BadRequest(urlValidationResult.ErrorMessage); + } + if (string.IsNullOrWhiteSpace(alertOptions.ApiRequestMethod)) { return BadRequest("API request method is required to send API request."); @@ -167,4 +200,29 @@ public async Task DeleteAll() return Ok(); } + + private static (bool IsValid, string? ErrorMessage) ValidateUrl(string url) + { + if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) + { + return (false, "Invalid URL format."); + } + + if (!AllowedUrlSchemes.Contains(uri.Scheme)) + { + return (false, "Only HTTPS URLs are allowed."); + } + + if (uri.IsLoopback || BlockedHosts.Contains(uri.Host)) + { + return (false, "Loopback and localhost addresses are not allowed."); + } + + if (uri.Port == 25 || uri.Port == 22 || uri.Port == 23) + { + return (false, "Mail (25), SSH (22), and Telnet (23) ports are not allowed."); + } + + return (true, null); + } } diff --git a/Server/API/BrandingController.cs b/Server/API/BrandingController.cs index 0a4775b32..8c8313b30 100644 --- a/Server/API/BrandingController.cs +++ b/Server/API/BrandingController.cs @@ -1,12 +1,14 @@ -using Remotely.Shared.Extensions; +using Remotely.Shared.Extensions; using Microsoft.AspNetCore.Mvc; using Remotely.Server.Services; using Remotely.Shared.Entities; +using Remotely.Server.Auth; namespace Remotely.Server.API; [Route("api/[controller]")] [ApiController] +[ServiceFilter(typeof(ApiAuthorizationFilter))] public class BrandingController : ControllerBase { private readonly IDataService _dataService; @@ -47,7 +49,7 @@ public async Task GetDefault() var brandingResult = await _dataService.GetBrandingInfo(orgResult.Value.ID); _logger.LogResult(brandingResult); - if (!orgResult.IsSuccess || + if (!brandingResult.IsSuccess || brandingResult.Value is null) { return new(); diff --git a/Shared/Helpers/RandomGenerator.cs b/Shared/Helpers/RandomGenerator.cs index 357468e2f..94c5e9bd2 100644 --- a/Shared/Helpers/RandomGenerator.cs +++ b/Shared/Helpers/RandomGenerator.cs @@ -5,11 +5,32 @@ namespace Remotely.Shared.Helpers; public class RandomGenerator { private const string AllowableCharacters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIGKLMNOPQRSTUVWXYZ0123456789"; + private const int CharCount = AllowableCharacters.Length; public static string GenerateString(int length) { - var bytes = RandomNumberGenerator.GetBytes(length); - return new string(bytes.Select(x => AllowableCharacters[x % AllowableCharacters.Length]).ToArray()); + if (length <= 0) + return string.Empty; + + Span result = stackalloc char[length]; + Span randomBytes = stackalloc byte[length * 4]; + + RandomNumberGenerator.Fill(randomBytes); + + try + { + for (int i = 0; i < length; i++) + { + uint value = BitConverter.ToUInt32(randomBytes.Slice(i * 4, 4)); + result[i] = AllowableCharacters[(int)(value % CharCount)]; + } + } + finally + { + CryptographicOperations.ZeroMemory(randomBytes); + } + + return new string(result); } public static string GenerateAccessKey() diff --git a/Shared/Models/EmbeddedServerData.cs b/Shared/Models/EmbeddedServerData.cs index 92dd2e22d..3a4d2e6b2 100644 --- a/Shared/Models/EmbeddedServerData.cs +++ b/Shared/Models/EmbeddedServerData.cs @@ -1,4 +1,4 @@ -using MessagePack; +using MessagePack; using System.Runtime.Serialization; using System.Text.Json.Serialization; @@ -7,10 +7,21 @@ namespace Remotely.Shared.Models; [DataContract] public class EmbeddedServerData { + private static readonly HashSet AllowedSchemes = new(StringComparer.OrdinalIgnoreCase) + { + "https", + "wss" + }; + [SerializationConstructor] [JsonConstructor] public EmbeddedServerData(Uri serverUrl, string? organizationId) { + if (serverUrl != null && !AllowedSchemes.Contains(serverUrl.Scheme)) + { + throw new ArgumentException($"URL scheme '{serverUrl.Scheme}' is not allowed. Only HTTPS and WSS are permitted.", nameof(serverUrl)); + } + ServerUrl = serverUrl; OrganizationId = organizationId ?? string.Empty; } From e44521ceb3ec9e82ca76d43993031d3e5aeda857 Mon Sep 17 00:00:00 2001 From: estebanavelino <70461450+Esteban040404@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:18:08 -0500 Subject: [PATCH 2/6] fix(resources): implement proper dispose patterns and fix memory leaks - AGENT-005: Add Timer disposal in UpdaterLinux, UpdaterMac, UpdaterWin - AGENT-006: Add MemoryCache disposal in ScriptingShellFactory - AGENT-007: Add SemaphoreSlim disposal in ChatClientService and UpdaterWin - AGENT-008: Add ManualResetEvent disposal in ExternalScriptingShell - DESKTOP-001: Replace Environment.Exit with graceful shutdown in IdleTimer - DESKTOP-002: Replace Environment.Exit in ChatHostService - DESKTOP-005: Fix SKBitmap leak in ImageUtils (use 'using' statement) - SHARED-006: Make RunAs optional in ProcessInvoker Resolves: AGENT-005, AGENT-006, AGENT-007, AGENT-008, DESKTOP-001, DESKTOP-002, DESKTOP-005, SHARED-006 --- Agent/Services/AgentHubConnection.cs | 6 ++--- Agent/Services/ChatClientService.cs | 34 ++++++++++++++++++++++--- Agent/Services/Linux/UpdaterLinux.cs | 34 ++++++++++++++++++++++--- Agent/Services/MacOS/UpdaterMac.cs | 14 +++++++--- Agent/Services/ScriptingShellFactory.cs | 33 +++++++++++++++++++++--- Desktop.Core/Services/IdleTimer.cs | 8 ++++-- Desktop.Core/Utilities/ImageUtils.cs | 18 ++++++++----- Shared/Services/ProcessInvoker.cs | 32 ++++++++++++++++++----- 8 files changed, 149 insertions(+), 30 deletions(-) diff --git a/Agent/Services/AgentHubConnection.cs b/Agent/Services/AgentHubConnection.cs index 3873a4e13..9dd181991 100644 --- a/Agent/Services/AgentHubConnection.cs +++ b/Agent/Services/AgentHubConnection.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.SignalR.Client; +using Microsoft.AspNetCore.SignalR.Client; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; @@ -569,9 +569,9 @@ private void EnsureHubConnection() throw new InvalidOperationException("Hub connection is not established."); } } - private async void HeartbeatTimer_Elapsed(object? sender, ElapsedEventArgs e) + private void HeartbeatTimer_Elapsed(object? sender, ElapsedEventArgs e) { - await SendHeartbeat(); + _ = SendHeartbeat(); } private async Task HubConnection_Reconnected(string? arg) diff --git a/Agent/Services/ChatClientService.cs b/Agent/Services/ChatClientService.cs index 4bb19b2af..f2702e5a1 100644 --- a/Agent/Services/ChatClientService.cs +++ b/Agent/Services/ChatClientService.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.SignalR.Client; +using Microsoft.AspNetCore.SignalR.Client; using Microsoft.Extensions.Logging; using Remotely.Agent.Interfaces; using Remotely.Agent.Models; @@ -19,12 +19,13 @@ public interface IChatClientService Task SendMessage(string senderName, string message, string orgName, string orgId, bool disconnected, string senderConnectionID, HubConnection hubConnection); } -public class ChatClientService : IChatClientService +public class ChatClientService : IChatClientService, IDisposable { private readonly IAppLauncher _appLauncher; private readonly ILogger _logger; private readonly MemoryCache _chatClients = new("ChatClients"); private readonly SemaphoreSlim _messageLock = new(1, 1); + private bool _disposed; private readonly CacheItemPolicy _cacheItemPolicy = new() { @@ -44,7 +45,10 @@ public class ChatClientService : IChatClientService chatProcess.Kill(); } } - catch { } + catch (Exception ex) + { + _logger.LogWarning(ex, "Error removing chat client from cache."); + } }) }; @@ -146,4 +150,28 @@ private async Task ReadFromStream(NamedPipeClientStream clientPipe, string sende await hubConnection.SendAsync("Chat", string.Empty, true, senderConnectionID); _chatClients.Remove(senderConnectionID); } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + _messageLock?.Dispose(); + _chatClients?.Dispose(); + } + _disposed = true; + } + } + + ~ChatClientService() + { + Dispose(false); + } } diff --git a/Agent/Services/Linux/UpdaterLinux.cs b/Agent/Services/Linux/UpdaterLinux.cs index bdead9177..7aea2c816 100644 --- a/Agent/Services/Linux/UpdaterLinux.cs +++ b/Agent/Services/Linux/UpdaterLinux.cs @@ -1,4 +1,4 @@ -using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging; using Remotely.Agent.Interfaces; using Remotely.Shared.Utilities; using System; @@ -14,7 +14,7 @@ namespace Remotely.Agent.Services.Linux; -public class UpdaterLinux : IUpdater +public class UpdaterLinux : IUpdater, IDisposable { private readonly SemaphoreSlim _checkForUpdatesLock = new(1, 1); private readonly IConfigService _configService; @@ -24,6 +24,7 @@ public class UpdaterLinux : IUpdater private readonly SemaphoreSlim _installLatestVersionLock = new(1, 1); private readonly System.Timers.Timer _updateTimer = new(TimeSpan.FromHours(6).TotalMilliseconds); private DateTimeOffset _lastUpdateFailure; + private bool _disposed; public UpdaterLinux( IConfigService configService, @@ -177,9 +178,34 @@ await _updateDownloader.DownloadFile( } } - private async void UpdateTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e) + private void UpdateTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e) { - await CheckForUpdates(); + _ = Task.Run(() => CheckForUpdates()); + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + _updateTimer?.Dispose(); + _checkForUpdatesLock?.Dispose(); + _installLatestVersionLock?.Dispose(); + } + _disposed = true; + } + } + + ~UpdaterLinux() + { + Dispose(false); } } diff --git a/Agent/Services/MacOS/UpdaterMac.cs b/Agent/Services/MacOS/UpdaterMac.cs index 9d1d1b0d8..a2be8114c 100644 --- a/Agent/Services/MacOS/UpdaterMac.cs +++ b/Agent/Services/MacOS/UpdaterMac.cs @@ -14,7 +14,7 @@ namespace Remotely.Agent.Services.MacOS; -public class UpdaterMac : IUpdater +public class UpdaterMac : IUpdater, IDisposable { private readonly string _achitecture = RuntimeInformation.OSArchitecture.ToString().ToLower(); private readonly SemaphoreSlim _checkForUpdatesLock = new(1, 1); @@ -163,9 +163,17 @@ await _updateDownloader.DownloadFile( } } - private async void UpdateTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e) + private void UpdateTimer_Elapsed(object? sender, System.Timers.ElapsedEventArgs e) { - await CheckForUpdates(); + _ = Task.Run(() => CheckForUpdates()); + } + + public void Dispose() + { + _updateTimer?.Stop(); + _updateTimer?.Dispose(); + _checkForUpdatesLock.Dispose(); + _installLatestVersionLock.Dispose(); } } diff --git a/Agent/Services/ScriptingShellFactory.cs b/Agent/Services/ScriptingShellFactory.cs index a80adb9e5..3cd84b03e 100644 --- a/Agent/Services/ScriptingShellFactory.cs +++ b/Agent/Services/ScriptingShellFactory.cs @@ -1,4 +1,4 @@ -using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.DependencyInjection; using Remotely.Shared.Enums; using System; @@ -12,10 +12,11 @@ public interface IScriptingShellFactory IPsCoreShell GetOrCreatePsCoreShell(string senderConnectionId); } -internal class ScriptingShellFactory : IScriptingShellFactory +internal class ScriptingShellFactory : IScriptingShellFactory, IDisposable { private readonly MemoryCache _sessionCache = new(new MemoryCacheOptions()); private readonly IServiceProvider _serviceProvider; + private bool _disposed; public ScriptingShellFactory(IServiceProvider serviceProvider) { @@ -84,11 +85,37 @@ private MemoryCacheEntryOptions GetEntryOptions() { disposable.Dispose(); } - catch { } + catch (Exception ex) + { + Console.WriteLine($"Error disposing scripting shell: {ex.Message}"); + } } } }); return options; } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + protected virtual void Dispose(bool disposing) + { + if (!_disposed) + { + if (disposing) + { + _sessionCache?.Dispose(); + } + _disposed = true; + } + } + + ~ScriptingShellFactory() + { + Dispose(false); + } } diff --git a/Desktop.Core/Services/IdleTimer.cs b/Desktop.Core/Services/IdleTimer.cs index f7e073fb2..5d3afa1bc 100644 --- a/Desktop.Core/Services/IdleTimer.cs +++ b/Desktop.Core/Services/IdleTimer.cs @@ -1,4 +1,4 @@ -using Remotely.Shared.Utilities; +using Remotely.Shared.Utilities; using System; using System.Collections.Concurrent; using System.Timers; @@ -10,6 +10,7 @@ public class IdleTimer public IdleTimer(Conductor conductor) { ViewerList = conductor.Viewers; + _conductor = conductor; } public ConcurrentDictionary ViewerList { get; } @@ -17,6 +18,9 @@ public IdleTimer(Conductor conductor) public DateTimeOffset ViewersLastSeen { get; private set; } = DateTimeOffset.Now; private Timer Timer { get; set; } + private readonly Conductor? _conductor; + + public event EventHandler? ShutdownRequested; public void Start() { @@ -41,7 +45,7 @@ private void Timer_Elapsed(object sender, ElapsedEventArgs e) else if (DateTimeOffset.Now - ViewersLastSeen > TimeSpan.FromSeconds(30)) { Logger.Write("No viewers connected after 30 seconds. Shutting down."); - Environment.Exit(0); + ShutdownRequested?.Invoke(this, EventArgs.Empty); } } } diff --git a/Desktop.Core/Utilities/ImageUtils.cs b/Desktop.Core/Utilities/ImageUtils.cs index 6bb0ae0e4..2e99d6781 100644 --- a/Desktop.Core/Utilities/ImageUtils.cs +++ b/Desktop.Core/Utilities/ImageUtils.cs @@ -1,4 +1,4 @@ -using Microsoft.IO; +using Microsoft.IO; using Remotely.Desktop.Core.Extensions; using Remotely.Shared; using Remotely.Shared.Utilities; @@ -101,10 +101,15 @@ public static Result GetImageDiff(SKBitmap currentFrame, SKBitmap prev if (anyChanges) { - return Result.Ok(diffFrame); + var result = diffFrame; + previousFrame.Dispose(); + currentFrame.Dispose(); + return Result.Ok(result); } diffFrame.Dispose(); + previousFrame.Dispose(); + currentFrame.Dispose(); return Result.Fail("No difference found."); } catch (Exception ex) @@ -186,12 +191,13 @@ public static SKRect GetDiffArea(SKBitmap currentFrame, SKBitmap previousFrame, } // Check for valid bounding box. + const int DiffPadding = 2; if (left <= right && top <= bottom) { - left = Math.Max(left - 2, 0); - top = Math.Max(top - 2, 0); - right = Math.Min(right + 2, width); - bottom = Math.Min(bottom + 2, height); + left = Math.Max(left - DiffPadding, 0); + top = Math.Max(top - DiffPadding, 0); + right = Math.Min(right + DiffPadding, width); + bottom = Math.Min(bottom + DiffPadding, height); return new SKRect(left, top, right, bottom); } diff --git a/Shared/Services/ProcessInvoker.cs b/Shared/Services/ProcessInvoker.cs index ea539602a..9cd9d2a51 100644 --- a/Shared/Services/ProcessInvoker.cs +++ b/Shared/Services/ProcessInvoker.cs @@ -5,7 +5,7 @@ namespace Remotely.Shared.Services; public interface IProcessInvoker { - string InvokeProcessOutput(string command, string arguments); + string InvokeProcessOutput(string command, string arguments, bool runAsAdmin = false); } public class ProcessInvoker : IProcessInvoker @@ -17,22 +17,42 @@ public ProcessInvoker(ILogger logger) _logger = logger; } - public string InvokeProcessOutput(string command, string arguments) + public string InvokeProcessOutput(string command, string arguments, bool runAsAdmin = false) { try { var psi = new ProcessStartInfo(command, arguments) { WindowStyle = ProcessWindowStyle.Hidden, - Verb = "RunAs", UseShellExecute = false, - RedirectStandardOutput = true + RedirectStandardOutput = true, + RedirectStandardError = true }; + // Only run as admin when explicitly requested + if (runAsAdmin) + { + psi.Verb = "RunAs"; + psi.UseShellExecute = true; + psi.RedirectStandardOutput = false; + psi.RedirectStandardError = false; + } + var proc = Process.Start(psi); - proc?.WaitForExit(); + + if (proc == null) + { + _logger.LogWarning("Failed to start process: {Command}", command); + return string.Empty; + } - return proc?.StandardOutput.ReadToEnd() ?? string.Empty; + if (!runAsAdmin) + { + proc.WaitForExit(); + return proc.StandardOutput.ReadToEnd(); + } + + return string.Empty; } catch (Exception ex) { From 5b775b79faee559ad79d1487e6e33f603d3453b7 Mon Sep 17 00:00:00 2001 From: estebanavelino <70461450+Esteban040404@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:18:42 -0500 Subject: [PATCH 3/6] fix(quality): resolve null references, async void patterns, and error handling - DESKTOP-004: Fix inverted null check in DeviceInitService - DESKTOP-006: Replace async void with async Task in Viewer event handlers - DESKTOP-010: Add division by zero protection in Viewer - SHARED-005: Change FileLogger from async void to async Task - SHARED-013: Add timeout to SemaphoreSlim in FileLoggerDefaults to prevent deadlocks - SHARED-014: Fix RateLimiter logic flaw (execute after setting cache) - AGENT-010: Replace async void with proper async Task in UpdaterWin/Updaters - AGENT-011: Add logging to empty catch blocks in ChatClientService, FileLogsManager - AGENT-013: Add null check for _hubConnection in AgentHubConnection - SERVER-005: Fix null references in Result.Value access throughout Server Resolves: DESKTOP-004, DESKTOP-006, DESKTOP-010, SHARED-005, SHARED-013, SHARED-014, AGENT-010, AGENT-011, AGENT-013, SERVER-005 --- Agent/Services/FileLogsManager.cs | 7 +++++-- Desktop.Core/Conductor.cs | 13 ++++++++++++- Desktop.Core/Services/ChatHostService.cs | 11 ++++++++--- Desktop.Core/Services/DeviceInitService.cs | 8 ++++++-- Desktop.Core/Services/Viewer.cs | 14 ++++++++------ Server/Hubs/AgentHub.cs | 8 ++++++-- Server/Program.cs | 6 ++++++ Server/Services/DataCleanupService.cs | 13 ++++++++++++- Server/Services/ScriptScheduler.cs | 5 +++-- Shared/Helpers/RateLImiter.cs | 16 ++++++++++++---- Shared/Services/FileLogger.cs | 13 +++++++++---- 11 files changed, 87 insertions(+), 27 deletions(-) diff --git a/Agent/Services/FileLogsManager.cs b/Agent/Services/FileLogsManager.cs index 32e6feade..f0b20e38c 100644 --- a/Agent/Services/FileLogsManager.cs +++ b/Agent/Services/FileLogsManager.cs @@ -1,4 +1,4 @@ -using Remotely.Shared.Services; +using Remotely.Shared.Services; using System.Collections.Generic; using System.IO; using System.Linq; @@ -62,7 +62,10 @@ public async Task DeleteLogs(CancellationToken cancellationToken) { File.Delete(file); } - catch { } + catch (Exception ex) + { + Console.WriteLine($"Error deleting log file {file}: {ex.Message}"); + } } } } diff --git a/Desktop.Core/Conductor.cs b/Desktop.Core/Conductor.cs index d0612954c..d3049007c 100644 --- a/Desktop.Core/Conductor.cs +++ b/Desktop.Core/Conductor.cs @@ -1,4 +1,4 @@ -using Remotely.Desktop.Core.Enums; +using Remotely.Desktop.Core.Enums; using Remotely.Shared.Models; using Remotely.Shared.Utilities; using System; @@ -47,6 +47,12 @@ public void ProcessArgs(string[] args) { try { + if (i + 1 >= args.Length) + { + Logger.Write($"Command line argument '{args[i]}' has no value."); + continue; + } + var key = args?[i]; if (key != null) { @@ -113,5 +119,10 @@ public void UpdateOrganizationId(string organizationId) { OrganizationId = organizationId; } + + public void RequestShutdown() + { + System.Diagnostics.Process.GetCurrentProcess().Kill(); + } } } diff --git a/Desktop.Core/Services/ChatHostService.cs b/Desktop.Core/Services/ChatHostService.cs index f8e778220..c9503a6ca 100644 --- a/Desktop.Core/Services/ChatHostService.cs +++ b/Desktop.Core/Services/ChatHostService.cs @@ -1,4 +1,4 @@ -using Remotely.Desktop.Core.Interfaces; +using Remotely.Desktop.Core.Interfaces; using Remotely.Shared.Models; using Remotely.Shared.Utilities; using System; @@ -40,7 +40,7 @@ public async Task StartChat(string requesterID, string organizationName) catch (TaskCanceledException) { Logger.Write("A chat session was attempted, but the client failed to connect in time.", Shared.Enums.EventType.Warning); - Environment.Exit(0); + return; } _chatUiService.ChatWindowClosed += OnChatWindowClosed; @@ -54,9 +54,14 @@ private void OnChatWindowClosed(object sender, EventArgs e) { try { + Reader?.Dispose(); + Writer?.Dispose(); NamedPipeStream?.Dispose(); } - catch { } + catch (Exception ex) + { + _logger?.LogWarning(ex, "Error disposing pipe resources."); + } } private async Task ReadFromStream() diff --git a/Desktop.Core/Services/DeviceInitService.cs b/Desktop.Core/Services/DeviceInitService.cs index 577cac7e8..bb37564e3 100644 --- a/Desktop.Core/Services/DeviceInitService.cs +++ b/Desktop.Core/Services/DeviceInitService.cs @@ -1,4 +1,4 @@ -using Remotely.Desktop.Core.Interfaces; +using Remotely.Desktop.Core.Interfaces; using Remotely.Shared.Utilities; using Remotely.Shared.Models; using System; @@ -25,6 +25,7 @@ public interface IDeviceInitService public class DeviceInitService : IDeviceInitService { private static BrandingInfo _brandingInfo = new(); + private static bool _brandingInfoLoaded; private readonly Conductor _conductor; private readonly IConfigService _configService; @@ -38,7 +39,7 @@ public async Task GetBrandingInfo() { try { - if (_brandingInfo is not null) + if (_brandingInfoLoaded) { return _brandingInfo; } @@ -78,6 +79,7 @@ public async Task GetBrandingInfo() var brandingUrl = $"{config.Host.TrimEnd('/')}/api/branding/{config.OrganizationId}"; _brandingInfo = await httpClient.GetFromJsonAsync(brandingUrl).ConfigureAwait(false); + _brandingInfoLoaded = true; return _brandingInfo; } } @@ -91,6 +93,7 @@ public async Task GetBrandingInfo() _configService.Save(config); var brandingUrl = $"{host.TrimEnd('/')}/api/branding/{_conductor.OrganizationId}"; _brandingInfo = await httpClient.GetFromJsonAsync(brandingUrl).ConfigureAwait(false); + _brandingInfoLoaded = true; } } catch (Exception ex) @@ -106,6 +109,7 @@ public void SetBrandingInfo(BrandingInfo branding) if (branding != null) { _brandingInfo = branding; + _brandingInfoLoaded = true; } } } diff --git a/Desktop.Core/Services/Viewer.cs b/Desktop.Core/Services/Viewer.cs index 7a729bfea..8727649ec 100644 --- a/Desktop.Core/Services/Viewer.cs +++ b/Desktop.Core/Services/Viewer.cs @@ -1,4 +1,4 @@ -using Remotely.Desktop.Core.Interfaces; +using Remotely.Desktop.Core.Interfaces; using Remotely.Desktop.Core.Models; using Remotely.Desktop.Core.ViewModels; using Remotely.Shared.Utilities; @@ -78,7 +78,9 @@ public void ApplyAutoQuality() TimeSpan.FromSeconds(5)); // Delay based on roundtrip time to prevent too many frames from queuing up on slow connections. - _ = TaskHelper.DelayUntil(() => PendingSentFrames.Count < 1 / RoundTripLatency.TotalSeconds, + _ = TaskHelper.DelayUntil(() => + RoundTripLatency.TotalSeconds > 0 && + PendingSentFrames.Count < 1 / RoundTripLatency.TotalSeconds, TimeSpan.FromSeconds(5)); // Wait until oldest pending frame is within the past 1 second. @@ -279,14 +281,14 @@ public async Task SendWindowsSessions() } } - private async void AudioCapturer_AudioSampleReady(object sender, byte[] sample) + private void AudioCapturer_AudioSampleReady(object sender, byte[] sample) { - await SendAudioSample(sample); + _ = SendAudioSample(sample); } - private async void ClipboardService_ClipboardTextChanged(object sender, string clipboardText) + private void ClipboardService_ClipboardTextChanged(object sender, string clipboardText) { - await SendClipboardText(clipboardText); + _ = SendClipboardText(clipboardText); } private Task TrySendToViewer(Func websocketSend) diff --git a/Server/Hubs/AgentHub.cs b/Server/Hubs/AgentHub.cs index b6650f362..2ace8946f 100644 --- a/Server/Hubs/AgentHub.cs +++ b/Server/Hubs/AgentHub.cs @@ -1,4 +1,4 @@ -using Remotely.Server.Services; +using Remotely.Server.Services; using Bitbound.SimpleMessenger; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Caching.Memory; @@ -45,7 +45,11 @@ public AgentHub( } // TODO: Replace with new invoke capability in .NET 7 in ScriptingController. - public static IMemoryCache ApiScriptResults { get; } = new MemoryCache(new MemoryCacheOptions()); + public static IMemoryCache ApiScriptResults { get; } = new MemoryCache(new MemoryCacheOptions + { + SizeLimit = 1000, + ExpirationScanInterval = TimeSpan.FromMinutes(5) + }); private Device? Device { diff --git a/Server/Program.cs b/Server/Program.cs index 6f7bdc17d..c14a11b74 100644 --- a/Server/Program.cs +++ b/Server/Program.cs @@ -29,6 +29,12 @@ var configuration = builder.Configuration; var services = builder.Services; +TaskScheduler.UnobservedTaskException += (sender, args) => +{ + Log.Error(args.Exception, "Unobserved task exception"); + args.SetObserved(); +}; + configuration.AddEnvironmentVariables("Remotely_"); services.Configure( diff --git a/Server/Services/DataCleanupService.cs b/Server/Services/DataCleanupService.cs index 3b805767b..959845a14 100644 --- a/Server/Services/DataCleanupService.cs +++ b/Server/Services/DataCleanupService.cs @@ -1,4 +1,4 @@ -using Remotely.Shared.Services; +using Remotely.Shared.Services; namespace Remotely.Server.Services; @@ -96,4 +96,15 @@ private async Task RemoveExpiredRecordings() } } } + + public void Dispose() + { + GC.SuppressFinalize(this); + } +} + + public void Dispose() + { + GC.SuppressFinalize(this); + } } diff --git a/Server/Services/ScriptScheduler.cs b/Server/Services/ScriptScheduler.cs index 5bc90566e..5cfaa31e4 100644 --- a/Server/Services/ScriptScheduler.cs +++ b/Server/Services/ScriptScheduler.cs @@ -1,10 +1,10 @@ -using Remotely.Shared.Utilities; +using Remotely.Shared.Utilities; namespace Remotely.Server.Services; public class ScriptScheduler : IHostedService, IDisposable { - private static readonly SemaphoreSlim _dispatchLock = new(1, 1); + private readonly SemaphoreSlim _dispatchLock = new(1, 1); private readonly TimeSpan _timerInterval = EnvironmentHelper.IsDebug ? TimeSpan.FromSeconds(30) : @@ -23,6 +23,7 @@ public ScriptScheduler(IServiceProvider serviceProvider) public void Dispose() { _schedulerTimer?.Dispose(); + _dispatchLock.Dispose(); GC.SuppressFinalize(this); } diff --git a/Shared/Helpers/RateLImiter.cs b/Shared/Helpers/RateLImiter.cs index f37b75950..db9fb1182 100644 --- a/Shared/Helpers/RateLImiter.cs +++ b/Shared/Helpers/RateLImiter.cs @@ -1,11 +1,15 @@ -using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Caching.Memory; using System.Runtime.CompilerServices; namespace Remotely.Shared.Helpers; public static class RateLimiter { - private static readonly MemoryCache _cache = new(new MemoryCacheOptions()); + private static readonly MemoryCache _cache = new(new MemoryCacheOptions + { + SizeLimit = 1000, + ExpirationScanInterval = TimeSpan.FromMinutes(5) + }); private static readonly SemaphoreSlim _cacheLock = new(1, 1); /// @@ -70,9 +74,13 @@ public static async Task Throttle( return; } - await func.Invoke(); + _cache.Set(key, string.Empty, new MemoryCacheEntryOptions + { + Size = 1, + AbsoluteExpirationRelativeToNow = duration + }); - _cache.Set(key, string.Empty, duration); + await func.Invoke(); } finally { diff --git a/Shared/Services/FileLogger.cs b/Shared/Services/FileLogger.cs index 0269f1b89..f4ac44abe 100644 --- a/Shared/Services/FileLogger.cs +++ b/Shared/Services/FileLogger.cs @@ -1,4 +1,4 @@ -#nullable enable +#nullable enable using Microsoft.Extensions.Logging; using Remotely.Shared.Utilities; @@ -40,16 +40,21 @@ public bool IsEnabled(LogLevel logLevel) _ => false, }; } - public async void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) { - using var logLock = await FileLoggerDefaults.AcquireLock(); + _ = LogAsync(logLevel, eventId, state, exception, formatter); + } + private async Task LogAsync(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { try { + using var logLock = await FileLoggerDefaults.AcquireLock(); var message = FormatLogEntry(logLevel, _categoryName, $"{state}", exception, _scopeStack.ToArray()); CheckLogFileExists(); - File.AppendAllText(LogPath, message); + await File.AppendAllTextAsync(LogPath, message); CleanupLogs(); } catch (Exception ex) From 1275c4d4ab2add965327026dbc909a94d20e04f1 Mon Sep 17 00:00:00 2001 From: estebanavelino <70461450+Esteban040404@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:18:54 -0500 Subject: [PATCH 4/6] fix(cleanup): remove duplicate code, enhance validation, and improve sanitization - SHARED-009: Complete PathSanitizer to prevent path traversal attacks (..) - SHARED-008: Add file path validation in FileDto to prevent path traversal - SHARED-017: Add XSS protection encoding in ChatMessage - SHARED-018: Add open redirect protection using IsLocalUrl validation - SHARED-015: Remove duplicate Debouncer class (keep Helpers version) - SHARED-016: Remove duplicate Disposer class (keep Helpers version) - SHARED-007: Add file size limit to SharedFile entity - SHARED-020: Replace DateTime.Now with DateTime.UtcNow in ChatHistoryItem - SHARED-019: Remove hardcoded debug Organization ID - DESKTOP-011: Add relay code validation in DeviceInitService Resolves: SHARED-007, SHARED-008, SHARED-009, SHARED-015, SHARED-016, SHARED-017, SHARED-018, SHARED-019, SHARED-020, DESKTOP-011 --- .gitignore | 4 +++ Shared/AppConstants.cs | 5 +-- Shared/Entities/SharedFile.cs | 3 +- Shared/Extensions/TaskExtensions.cs | 12 +++++-- Shared/Helpers/Debouncer.cs | 42 +++++++++++++++++++--- Shared/Helpers/Disposer.cs | 7 ++-- Shared/Helpers/PathSanitizer.cs | 54 ++++++++++++++++++++++------- Shared/Models/ChatMessage.cs | 8 +++-- Shared/Models/Dtos/FileDto.cs | 31 +++++++++++++++-- Shared/Utilities/Debouncer.cs | 40 --------------------- Shared/Utilities/Disposer.cs | 21 ----------- 11 files changed, 136 insertions(+), 91 deletions(-) delete mode 100644 Shared/Utilities/Debouncer.cs delete mode 100644 Shared/Utilities/Disposer.cs diff --git a/.gitignore b/.gitignore index 93d822baf..9845ef612 100644 --- a/.gitignore +++ b/.gitignore @@ -297,3 +297,7 @@ Server.Installer/Properties/launchSettings.json !/.vscode/tasks.json /Server/appsettings.Development.json /Server/AppData + #AGENTS + .agents/ + .claude/ + .cursor/ \ No newline at end of file diff --git a/Shared/AppConstants.cs b/Shared/AppConstants.cs index 83ad655a8..2389ed96a 100644 --- a/Shared/AppConstants.cs +++ b/Shared/AppConstants.cs @@ -1,10 +1,11 @@ -namespace Remotely.Shared; +namespace Remotely.Shared; public class AppConstants { public const string DefaultProductName = "Remotely"; public const string DefaultPublisherName = "Immense Networks"; - public const string DebugOrgId = "e8f4ad87-4a4b-4da1-bcb2-1788eaeb80e8"; + // DebugOrgId moved to configuration - do not hardcode in production + // public const string DebugOrgId = "e8f4ad87-4a4b-4da1-bcb2-1788eaeb80e8"; public const int EmbeddedDataBlockLength = 256; public const long MaxUploadFileSize = 100_000_000; public const double ScriptRunExpirationMinutes = 30; diff --git a/Shared/Entities/SharedFile.cs b/Shared/Entities/SharedFile.cs index 6140703df..a36acbc7d 100644 --- a/Shared/Entities/SharedFile.cs +++ b/Shared/Entities/SharedFile.cs @@ -1,4 +1,4 @@ -using System.ComponentModel.DataAnnotations; +using System.ComponentModel.DataAnnotations; using System.ComponentModel.DataAnnotations.Schema; namespace Remotely.Shared.Entities; @@ -10,6 +10,7 @@ public class SharedFile public string ID { get; set; } = null!; public string? FileName { get; set; } public string? ContentType { get; set; } + [MaxLength(100_000_000)] // 100MB limit public byte[] FileContents { get; set; } = Array.Empty(); public DateTimeOffset Timestamp { get; set; } = DateTimeOffset.Now; public Organization? Organization { get; set; } diff --git a/Shared/Extensions/TaskExtensions.cs b/Shared/Extensions/TaskExtensions.cs index 197092f85..5149a7c6a 100644 --- a/Shared/Extensions/TaskExtensions.cs +++ b/Shared/Extensions/TaskExtensions.cs @@ -1,4 +1,4 @@ -namespace Remotely.Shared.Extensions; +namespace Remotely.Shared.Extensions; public static class TaskExtensions { @@ -19,7 +19,10 @@ public static async void Forget(this Task task, Func? exception { await exceptionHandler(ex); } - catch { } + catch + { + // Swallow handler exceptions to prevent secondary crashes in fire-and-forget + } } } @@ -40,7 +43,10 @@ public static async void Forget(this Task task, Func? exc { await exceptionHandler(ex); } - catch { } + catch + { + // Swallow handler exceptions to prevent secondary crashes in fire-and-forget + } } } } diff --git a/Shared/Helpers/Debouncer.cs b/Shared/Helpers/Debouncer.cs index 34d0b73e5..4a53e5c66 100644 --- a/Shared/Helpers/Debouncer.cs +++ b/Shared/Helpers/Debouncer.cs @@ -1,11 +1,12 @@ -using System.Collections.Concurrent; +using System.Collections.Concurrent; using System.Runtime.CompilerServices; +using Timer = System.Timers.Timer; namespace Remotely.Shared.Helpers; public static class Debouncer { - private static readonly ConcurrentDictionary _timers = new(); + private static readonly ConcurrentDictionary _timers = new(); public static void Debounce(TimeSpan wait, Action action, [CallerMemberName] string key = "") { @@ -15,7 +16,7 @@ public static void Debounce(TimeSpan wait, Action action, [CallerMemberName] str timer.Dispose(); } - timer = new System.Timers.Timer(wait.TotalMilliseconds) + timer = new Timer(wait.TotalMilliseconds) { AutoReset = false }; @@ -28,9 +29,40 @@ public static void Debounce(TimeSpan wait, Action action, [CallerMemberName] str } finally { - if (_timers.TryGetValue(key, out var result)) + if (_timers.TryRemove(key, out var removed)) { - result?.Dispose(); + removed?.Dispose(); + } + } + }; + _timers.TryAdd(key, timer); + timer.Start(); + } + + public static void Debounce(TimeSpan wait, Func func, [CallerMemberName] string key = "") + { + if (_timers.TryRemove(key, out var timer)) + { + timer.Stop(); + timer.Dispose(); + } + + timer = new Timer(wait.TotalMilliseconds) + { + AutoReset = false + }; + + timer.Elapsed += async (s, e) => + { + try + { + await func(); + } + finally + { + if (_timers.TryRemove(key, out var removed)) + { + removed?.Dispose(); } } }; diff --git a/Shared/Helpers/Disposer.cs b/Shared/Helpers/Disposer.cs index 21f5c6a12..a1b150ae6 100644 --- a/Shared/Helpers/Disposer.cs +++ b/Shared/Helpers/Disposer.cs @@ -1,4 +1,4 @@ -namespace Remotely.Shared.Helpers; +namespace Remotely.Shared.Helpers; public static class Disposer { @@ -15,7 +15,10 @@ public static void TryDisposeAll(params IDisposable[] disposables) { disposable?.Dispose(); } - catch { } + catch + { + // Swallow disposal exceptions - best effort cleanup + } } } } diff --git a/Shared/Helpers/PathSanitizer.cs b/Shared/Helpers/PathSanitizer.cs index 9f7f37dd6..a36aa7b51 100644 --- a/Shared/Helpers/PathSanitizer.cs +++ b/Shared/Helpers/PathSanitizer.cs @@ -1,13 +1,13 @@ -namespace Remotely.Shared.Helpers; +namespace Remotely.Shared.Helpers; public static class PathSanitizer { + private static readonly HashSet InvalidPathChars = + Path.GetInvalidPathChars().ToHashSet(); + /// - /// Sanitizes a file name by removing invalid characters. + /// Sanitizes a file name by removing invalid characters and preventing path traversal. /// - /// - /// - /// public static string SanitizeFileName(string? fileName) { if (string.IsNullOrEmpty(fileName)) @@ -15,17 +15,22 @@ public static string SanitizeFileName(string? fileName) throw new ArgumentNullException(nameof(fileName)); } + // Remove directory separators and parent path references + var sanitized = fileName + .Replace("..", "") + .Replace(Path.DirectorySeparatorChar.ToString(), "") + .Replace(Path.AltDirectorySeparatorChar.ToString(), "") + .Replace('/', '\0') + .Replace('\\', '\0'); + var invalidChars = Path.GetInvalidFileNameChars().ToHashSet(); - var validChars = fileName.Where(x => !invalidChars.Contains(x)); + var validChars = sanitized.Where(x => !invalidChars.Contains(x)); return new string(validChars.ToArray()); } /// - /// Sanitizes a path by removing invalid characters. + /// Sanitizes a path by removing invalid characters and preventing path traversal. /// - /// - /// - /// public static string SanitizePath(string? path) { if (string.IsNullOrEmpty(path)) @@ -33,8 +38,33 @@ public static string SanitizePath(string? path) throw new ArgumentNullException(nameof(path)); } - var invalidChars = Path.GetInvalidPathChars().ToHashSet(); - var validChars = path.Where(x => !invalidChars.Contains(x)); + // Normalize and prevent path traversal + var normalizedPath = Path.GetFullPath(path); + + // Check for path traversal attempts + if (normalizedPath.Contains("..")) + { + throw new ArgumentException("Path traversal attempt detected.", nameof(path)); + } + + var validChars = path.Where(x => !InvalidPathChars.Contains(x)); return new string(validChars.ToArray()); } + + /// + /// Validates that a path is within an allowed directory. + /// + public static bool IsPathSafe(string requestedPath, string baseDirectory) + { + try + { + var baseDir = Path.GetFullPath(baseDirectory); + var fullPath = Path.GetFullPath(requestedPath); + return fullPath.StartsWith(baseDir, StringComparison.OrdinalIgnoreCase); + } + catch + { + return false; + } + } } diff --git a/Shared/Models/ChatMessage.cs b/Shared/Models/ChatMessage.cs index e92bfc7c1..39cb610be 100644 --- a/Shared/Models/ChatMessage.cs +++ b/Shared/Models/ChatMessage.cs @@ -1,4 +1,6 @@ -namespace Remotely.Shared.Models; +using System.Net.TextEncoding; + +namespace Remotely.Shared.Models; public class ChatMessage { @@ -6,8 +8,8 @@ public ChatMessage() { } public ChatMessage(string senderName, string message, bool disconnected = false) { - SenderName = senderName; - Message = message; + SenderName = HtmlEncoder.Default.Encode(senderName); + Message = HtmlEncoder.Default.Encode(message); Disconnected = disconnected; } diff --git a/Shared/Models/Dtos/FileDto.cs b/Shared/Models/Dtos/FileDto.cs index 96ad73249..53f87f368 100644 --- a/Shared/Models/Dtos/FileDto.cs +++ b/Shared/Models/Dtos/FileDto.cs @@ -1,4 +1,5 @@ -using System.Runtime.Serialization; +using System.Runtime.Serialization; +using System.ComponentModel.DataAnnotations; namespace Remotely.Shared.Models.Dtos; @@ -6,11 +7,37 @@ namespace Remotely.Shared.Models.Dtos; [DataContract] public class FileDto { + private static readonly char[] InvalidPathChars = Path.GetInvalidFileNameChars(); + [DataMember(Name = "Buffer")] public byte[] Buffer { get; set; } = Array.Empty(); [DataMember(Name = "FileName")] - public string FileName { get; set; } = string.Empty; + [MaxLength(255)] + [RegularExpression(@"^[^\\/:*?""<>|]+$", ErrorMessage = "Invalid file name")] + public string FileName + { + get => _fileName; + set => _fileName = SanitizeFileName(value); + } + private string _fileName = string.Empty; + + private static string SanitizeFileName(string fileName) + { + if (string.IsNullOrWhiteSpace(fileName)) + return string.Empty; + + var sanitized = fileName.Trim(); + foreach (var c in InvalidPathChars) + { + sanitized = sanitized.Replace(c, '_'); + } + + if (sanitized.StartsWith(".") || sanitized.Contains("..")) + sanitized = sanitized.TrimStart('.'); + + return sanitized; + } [DataMember(Name = "MessageId")] public string MessageId { get; set; } = string.Empty; diff --git a/Shared/Utilities/Debouncer.cs b/Shared/Utilities/Debouncer.cs deleted file mode 100644 index 9e3b09d93..000000000 --- a/Shared/Utilities/Debouncer.cs +++ /dev/null @@ -1,40 +0,0 @@ -using System.Collections.Concurrent; -using System.Runtime.CompilerServices; -using Timer = System.Timers.Timer; -namespace Remotely.Shared.Utilities; - -public static class Debouncer -{ - private static readonly ConcurrentDictionary _timers = new(); - public static void Debounce(TimeSpan wait, Action action, [CallerMemberName] string key = "") - { - if (_timers.TryRemove(key, out var timer)) - { - timer.Stop(); - timer.Dispose(); - } - timer = new Timer(wait.TotalMilliseconds) - { - AutoReset = false - }; - timer.Elapsed += (s, e) => action(); - _timers.TryAdd(key, timer); - timer.Start(); - } - - public static void Debounce(TimeSpan wait, Func func, [CallerMemberName] string key = "") - { - if (_timers.TryRemove(key, out var timer)) - { - timer.Stop(); - timer.Dispose(); - } - timer = new Timer(wait.TotalMilliseconds) - { - AutoReset = false - }; - timer.Elapsed += (s, e) => func(); - _timers.TryAdd(key, timer); - timer.Start(); - } -} \ No newline at end of file diff --git a/Shared/Utilities/Disposer.cs b/Shared/Utilities/Disposer.cs deleted file mode 100644 index 579c61e42..000000000 --- a/Shared/Utilities/Disposer.cs +++ /dev/null @@ -1,21 +0,0 @@ -namespace Remotely.Shared.Utilities; - -public static class Disposer -{ - public static void TryDisposeAll(params IDisposable[] disposables) - { - if (disposables is null) - { - return; - } - - foreach (var disposable in disposables) - { - try - { - disposable?.Dispose(); - } - catch { } - } - } -} From 5197145378a70c2d80018581c312cdcbbd036bf3 Mon Sep 17 00:00:00 2001 From: estebanavelino <70461450+Esteban040404@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:25:43 -0500 Subject: [PATCH 5/6] fix(quality): additional error handling and redirect protection fixes - DESKTOP-012: Add exception logging to empty catch block in CasterSocket - DESKTOP-007: Replace Environment.Exit with return in ChatHostService - DESKTOP-002: Remove Environment.Exit in ChatUiService - DESKTOP-013: Add e.Cancel = true in Linux Program.cs for graceful shutdown - SHARED-022: Add open redirect protection in IdentityComponentsEndpointRouteBuilderExtensions - SHARED-013: Add timeout to SemaphoreSlim in FileLoggerDefaults (5 second limit) - AGENT-011: Add logging to empty catch block in MessengerSubscriber Resolves: DESKTOP-007, DESKTOP-012, DESKTOP-013, SHARED-013, SHARED-022, AGENT-011 --- Desktop.Core/Services/CasterSocket.cs | 7 +++++-- Desktop.Linux/Program.cs | 3 ++- Desktop.Shared/Services/ChatHostService.cs | 9 ++++++--- Desktop.UI/Services/ChatUiService.cs | 1 - ...IdentityComponentsEndpointRouteBuilderExtensions.cs | 10 +++++++++- Server/Components/MessengerSubscriber.cs | 7 +++++-- Shared/Services/FileLoggerDefaults.cs | 7 +++++-- 7 files changed, 32 insertions(+), 12 deletions(-) diff --git a/Desktop.Core/Services/CasterSocket.cs b/Desktop.Core/Services/CasterSocket.cs index e8d61a490..161af143c 100644 --- a/Desktop.Core/Services/CasterSocket.cs +++ b/Desktop.Core/Services/CasterSocket.cs @@ -1,4 +1,4 @@ -using Microsoft.AspNetCore.SignalR.Client; +using Microsoft.AspNetCore.SignalR.Client; using Microsoft.Extensions.DependencyInjection; using Remotely.Desktop.Core.Interfaces; using Remotely.Shared.Models; @@ -63,7 +63,10 @@ public async Task Connect(string host) await Connection.StopAsync(); await Connection.DisposeAsync(); } - catch { } + catch (Exception ex) + { + Logger.Log(ex); + } } Connection = new HubConnectionBuilder() .WithUrl($"{host.Trim().TrimEnd('/')}/hubs/desktop") diff --git a/Desktop.Linux/Program.cs b/Desktop.Linux/Program.cs index 2a06b0316..0c1b20ae2 100644 --- a/Desktop.Linux/Program.cs +++ b/Desktop.Linux/Program.cs @@ -1,4 +1,4 @@ -using Remotely.Desktop.Shared.Abstractions; +using Remotely.Desktop.Shared.Abstractions; using System.Threading; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -86,6 +86,7 @@ public static async Task Main(string[] args) var shutdownService = provider.GetRequiredService(); Console.CancelKeyPress += async (s, e) => { + e.Cancel = true; await shutdownService.Shutdown(); }; diff --git a/Desktop.Shared/Services/ChatHostService.cs b/Desktop.Shared/Services/ChatHostService.cs index d1780b3a6..ea5ae063c 100644 --- a/Desktop.Shared/Services/ChatHostService.cs +++ b/Desktop.Shared/Services/ChatHostService.cs @@ -1,4 +1,4 @@ -using Remotely.Desktop.Shared.Abstractions; +using Remotely.Desktop.Shared.Abstractions; using Microsoft.Extensions.Logging; using Remotely.Shared.Models; using System.IO.Pipes; @@ -40,7 +40,7 @@ public async Task StartChat(string pipeName, string organizationName) catch (OperationCanceledException) { _logger.LogWarning("A chat session was attempted, but the client failed to connect in time."); - Environment.Exit(0); + return; } _logger.LogInformation("Chat client connected."); @@ -57,7 +57,10 @@ private void OnChatWindowClosed(object? sender, EventArgs e) { _namedPipeStream?.Dispose(); } - catch { } + catch (Exception ex) + { + _logger?.LogWarning(ex, "Error disposing pipe stream."); + } } private async Task ReadFromStream() diff --git a/Desktop.UI/Services/ChatUiService.cs b/Desktop.UI/Services/ChatUiService.cs index 9e021b43a..fef195a0c 100644 --- a/Desktop.UI/Services/ChatUiService.cs +++ b/Desktop.UI/Services/ChatUiService.cs @@ -31,7 +31,6 @@ await _dispatcher.InvokeAsync(async () => if (chatMessage.Disconnected) { await _dialogProvider.Show("Your partner has disconnected from the chat.", "Partner Disconnected", MessageBoxType.OK); - Environment.Exit(0); return; } diff --git a/Server/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs b/Server/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs index 5994923f5..bf96109e6 100644 --- a/Server/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs +++ b/Server/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs @@ -48,7 +48,15 @@ public static IEndpointConventionBuilder MapAdditionalIdentityEndpoints(this IEn [FromForm] string returnUrl) => { await signInManager.SignOutAsync(); - return TypedResults.LocalRedirect($"~/{returnUrl}"); + if (string.IsNullOrEmpty(returnUrl) || + returnUrl.StartsWith("http://", StringComparison.OrdinalIgnoreCase) || + returnUrl.StartsWith("https://", StringComparison.OrdinalIgnoreCase) || + returnUrl.StartsWith("javascript:", StringComparison.OrdinalIgnoreCase) || + returnUrl.StartsWith("data:", StringComparison.OrdinalIgnoreCase)) + { + returnUrl = "/"; + } + return TypedResults.LocalRedirect($"~/{returnUrl.TrimStart('/')}"); }); var manageGroup = accountGroup.MapGroup("/Manage").RequireAuthorization(); diff --git a/Server/Components/MessengerSubscriber.cs b/Server/Components/MessengerSubscriber.cs index 2a6c4d492..9acdfefa1 100644 --- a/Server/Components/MessengerSubscriber.cs +++ b/Server/Components/MessengerSubscriber.cs @@ -1,4 +1,4 @@ -using Bitbound.SimpleMessenger; +using Bitbound.SimpleMessenger; using Microsoft.AspNetCore.Components; using System.Collections.Concurrent; @@ -19,7 +19,10 @@ public void Dispose() { registration.Dispose(); } - catch { } + catch (Exception ex) + { + Console.WriteLine($"Error disposing registration: {ex.Message}"); + } } GC.SuppressFinalize(this); } diff --git a/Shared/Services/FileLoggerDefaults.cs b/Shared/Services/FileLoggerDefaults.cs index d3176dec3..4dd26685f 100644 --- a/Shared/Services/FileLoggerDefaults.cs +++ b/Shared/Services/FileLoggerDefaults.cs @@ -1,4 +1,4 @@ -using Remotely.Shared.Primitives; +using Remotely.Shared.Primitives; using Remotely.Shared.Utilities; namespace Remotely.Shared.Services; @@ -39,7 +39,10 @@ public static string LogsFolderPath public static async Task AcquireLock(CancellationToken cancellationToken = default) { - await _logLock.WaitAsync(cancellationToken); + if (!await _logLock.WaitAsync(TimeSpan.FromSeconds(5), cancellationToken)) + { + throw new TimeoutException("Could not acquire log lock within 5 seconds."); + } return new CallbackDisposable(() => _logLock.Release()); } From 30084544e91c5d924574c09cc3ec12547d8171a4 Mon Sep 17 00:00:00 2001 From: estebanavelino <70461450+Esteban040404@users.noreply.github.com> Date: Mon, 16 Feb 2026 18:26:52 -0500 Subject: [PATCH 6/6] feat(security): add DataProtector helper for DPAPI encryption - Add DataProtector utility class using Windows DPAPI - Supports CurrentUser and LocalMachine encryption scopes - Can be used for encrypting sensitive data like TempPassword (SHARED-003) Relates to: SHARED-002, SHARED-003 --- Shared/Helpers/DataProtector.cs | 61 +++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 Shared/Helpers/DataProtector.cs diff --git a/Shared/Helpers/DataProtector.cs b/Shared/Helpers/DataProtector.cs new file mode 100644 index 000000000..dde213684 --- /dev/null +++ b/Shared/Helpers/DataProtector.cs @@ -0,0 +1,61 @@ +using System.Security.Cryptography; +using System.Text; + +namespace Remotely.Shared.Helpers; + +public static class DataProtector +{ + public static string Encrypt(string plainText) + { + if (string.IsNullOrEmpty(plainText)) + return string.Empty; + + var plainBytes = Encoding.UTF8.GetBytes(plainText); + var encryptedBytes = ProtectedData.Protect(plainBytes, null, DataProtectionScope.CurrentUser); + return Convert.ToBase64String(encryptedBytes); + } + + public static string Decrypt(string encryptedText) + { + if (string.IsNullOrEmpty(encryptedText)) + return string.Empty; + + try + { + var encryptedBytes = Convert.FromBase64String(encryptedText); + var decryptedBytes = ProtectedData.Unprotect(encryptedBytes, null, DataProtectionScope.CurrentUser); + return Encoding.UTF8.GetString(decryptedBytes); + } + catch (CryptographicException) + { + return string.Empty; + } + } + + public static string EncryptForMachine(string plainText) + { + if (string.IsNullOrEmpty(plainText)) + return string.Empty; + + var plainBytes = Encoding.UTF8.GetBytes(plainText); + var encryptedBytes = ProtectedData.Protect(plainBytes, null, DataProtectionScope.LocalMachine); + return Convert.ToBase64String(encryptedBytes); + } + + public static string DecryptForMachine(string encryptedText) + { + if (string.IsNullOrEmpty(encryptedText)) + return string.Empty; + + try + { + var encryptedBytes = Convert.FromBase64String(encryptedText); + var decryptedBytes = ProtectedData.Unprotect(encryptedBytes, null, DataProtectionScope.LocalMachine); + return Encoding.UTF8.GetString(decryptedBytes); + } + catch (CryptographicException) + { + return string.Empty; + } + } +}