feat: single address field and port mode selector for the port scan - #161
Open
jafin wants to merge 37 commits into
Open
feat: single address field and port mode selector for the port scan#161jafin wants to merge 37 commits into
jafin wants to merge 37 commits into
Conversation
Replace == string comparisons with string.Equals(..., StringComparison.Ordinal) to satisfy MA0006, and drop a duplicate 'using System;' (CS0105).
Nullability: align IRegistryRead.GetValue with its nullable implementation (CS8766); use [AllowNull] on the Control.Text overrides in MrngIpTextBox and CommandButton (CS8765/CS8764); match the base TypeConverter.CanConvertFrom signature (CS8604); return string.Empty rather than default from the RDM CSV tuple, which callers already test with IsNullOrEmpty (CS8619); add null guards in DataTableSerializer, PortableSettingsInitializer, RdpProtocol8, PuttyBase and XmlConnectionsDeserializer; drop a [NotNull] on an unassigned parameter (CS8777). Analyzers: Environment.CurrentManagedThreadId (CA1840); System.Threading.Lock (MA0158); CultureInfo-aware StringBuilder.Append (CA1305); explicit discards on BCrypt cleanup results (CA1806); log instead of swallowing in a best-effort hit-test (RCS1075). Dead code: remove the unreferenced FrmMain.UserInterfaceResize event and the BackupPage._frmMain field, whose only uses sit in a commented-out block; the never-raised ICredentialRecord.PropertyChanged on PlaceholderCredentialRecord keeps its normal semantics behind a scoped CS0067 suppression. Pkcs5S2KeyGenerator moves to the static Rfc2898DeriveBytes.Pbkdf2 overload, clearing SYSLIB0060 and CA5379. SHA1 is retained deliberately - it is required to decrypt existing connection files - and the derived bytes are unchanged.
The package is supplied by the targeted framework, so the explicit PackageReference only produced NU1510; no source in the repo uses the System.DirectoryServices namespace. Removing it forced a real recompile of ExternalConnectors, which surfaced an unused designer field (CS0169).
SupportedOSPlatformVersion was pinned at 10.0.26100.0 - stricter than mRemoteNG.csproj's 10.0.17763.0 - so the test host refused to run on anything older than Windows 11 24H2, reporting 'Only supported on Windows10.0.26100.0' and zero executed tests. Aligning it with the app lets the suite run on 23H2 (10.0.22631) and older; no CA1416 warnings result. Also set Nullable=annotations so tests may use 'T?' without enabling flow analysis warnings across the project (CS8632).
No project code uses ZstdSharp — it arrives transitively via MySql.Data, which declares ZstdSharp.Port >= 0.8.6 on every target framework for MySQL protocol zstd compression. The direct PackageReference in the app, test, and spec projects added nothing. Keep the central PackageVersion at 0.8.7 so transitive pinning still bumps the resolved version above MySql.Data's floor; without it NuGet would silently fall back to 0.8.6. Verified: ZstdSharp.Port/0.8.7 still resolves in project.assets.json and mRemoteNG.deps.json, and ZstdSharp.dll still ships in the build output.
Picking a colour for a connection's Color or TabColor failed with "Property value is not valid" / "Object of type 'System.Drawing.Color' cannot be converted to type 'System.String'", and the colour was not set. Both properties are strings but were annotated with the stock ColorEditor, which returns a System.Drawing.Color. The PropertyGrid commits an editor's return value straight through PropertyDescriptor.SetValue and never runs the TypeConverter on that path, so the assignment threw. TabColorConverter only covered the text-entry path, which is why the error survived it. - Add ColorStringEditor, wrapping ColorEditor for string-backed colour properties: it feeds the editor a Color and translates the picked colour back to the stored string form. It also restores the swatch preview, which was blank because ColorEditor cannot paint a string. - Point Color and TabColor at the new editor. - Return the standard-values dropdown entries as strings; picking one from the list failed in exactly the same way. - Convert Color.Empty to an empty string instead of "#00000000" so clearing a colour leaves the property unset.
TabColorConverter maps text it cannot parse to Color.Empty rather than throwing, so a legacy or hand-edited entry such as "not-a-color" reached the wrapped ColorEditor as an empty colour. Dismissing the picker echoed that empty colour straight back, and it was then stored as an empty string, silently discarding the original value. Track whether the stored value actually describes a colour, and keep the value untouched when it does not and no colour was picked. A genuinely unset value - null or blank - still round-trips to an empty string.
Dismissing the "are you sure you want to close" prompt left the tab open but switched the active connection to the tab on its left. DockPanelSuite's TryCloseTab calls SelectClosestPane straight after DockPane.CloseContent without checking whether the content actually closed, and it sets ActiveContent to Tabs[index - 1]. EnableSelectClosestOnClose defaults to true and connDock uses DocumentStyle.DockingWindow whenever more than one tab is open, so both of its guards pass. The selection therefore moved even though ConnectionTab.OnFormClosing had cancelled the close, which is why it only showed up on tabs other than the first. Wrap the close so the previous selection is restored when the tab is still displaying afterwards. Keying off DisplayingContents rather than a cancel flag covers every refusal - the confirmation prompt, a protocol veto, KeepTabsOpenAfterDisconnect - and leaves a successful close alone.
…le timeout Thread.Interrupt only unblocks interruptible waits, so a wedged UI thread survived the timeout, and because it was a foreground thread it could hold the whole test run open. The test body is now posted onto an STA thread running Application.Run(new ApplicationContext()) and calls Application.ExitThread in a finally, so the loop unwinds as soon as the body finishes. On timeout the loop is asked to exit and joined again briefly before failing. The thread is also marked IsBackground as a last-resort net: even if it never pumps again it cannot keep the process alive. This helper is shared by the whole fixture, so MiddleClick_ClosesSpecificTab_NotAll gets the same treatment.
Closing a PuTTY session from the tab's X asked the user to confirm twice
and then left the tab behind:
- mRemoteNG posts WM_CLOSE to the PuTTY window, which answers with its own
warn-on-close box ("Are you sure you want to close this session?") on top
of the confirmation mRemoteNG had already shown. The graceful close now
polls while waiting for the process to exit and acknowledges that box on
the user's behalf. The match is deliberately narrow - owning process,
dialog class #32770 and an "Exit Confirmation" title - so the host key
Security Alert and the settings dialog stay under user control.
- KeepTabsOpenAfterDisconnect (on by default) cancelled the form close for
every close path, so an explicitly closed tab stayed in place showing the
reconnect panel. That option now applies only to a disconnect request:
the tab context menu's Disconnect entry sets the new disconnectOnly flag,
while the tab's X and Ctrl+W always take the tab down. HandleProtocolClosed
no longer revives a tab that is already disposing.
Clearing the search box left the tree in a broken state: folders that were expanded before filtering came back collapsed, and the tree could throw an invalid-index exception from EnsureVisible. Two compounding defects in RemoveFilter(): ApplyFilter stored ExpandedObjects as the pre-filter state, but that getter returns a live view over the tree model's expansion map rather than a snapshot. The ExpandAll() that follows rewrote the same map, and the ExpandedObjects setter clears the map before enumerating the value it was handed - the very collection it just emptied - so nothing was restored. Assigning ExpandedObjects also only updates the model's map; the branch structure and row list are not rebuilt. RemoveFilter rebuilt first (via ResetColumnFiltering) and mutated the map afterwards, leaving expansion state and row indexes reading from different sources. That mismatch is what let IndexOf hand EnsureVisible a row past the end of the list. The pre-filter state is now materialized into a list, and RemoveFilter uses RebuildAll to restore expansion, selection and the row list together.
Clearing the filter runs three passes: UseFiltering and the column filter reset each trigger UpdateFiltering, then the tree is rebuilt to restore the pre-filter expansion state. Each pass repainted and re-ran the column auto-resize, which measures the text of every visible row. Hold painting across the whole method and suspend the per-pass auto-resize, doing it once at the end against the restored rows. The early return when there is no saved expansion state became a conditional so that final resize runs on every path. Freeze()/Unfreeze() would not help here: TreeListView is a virtual list, and VirtualObjectListView.BuildList(bool) overrides the base without the "if (Frozen) return" guard, so freezing suspends painting while adding one more BuildList in DoUnfreeze. Collapsing the two UpdateFiltering passes into one would mean changing the vendored ObjectListView filtering workflow, which is out of scope here. No behavior change; the existing filter regression tests cover it.
Pull in the package bumps from upstream v1.78.2-dev range f85c02f..2ab748e: - AWSSDK.Core 4.0.100.1 -> 4.0.100.9 - AWSSDK.EC2 4.0.102 -> 4.0.109 - BouncyCastle.Cryptography 2.6.2 -> 2.7.0 - Cucumber.Messages 32.2.0 -> 34.2.1 - Microsoft.Web.WebView2 1.0.4022.49 -> 1.0.4129.50 - MySql.Data 9.6.0 -> 26.7.0 MySql.Data 26.x raises its transitive floors, so align the whole System.*/Microsoft.Extensions 10.0.x band from 10.0.5 to 10.0.10 to match upstream and satisfy those floors. The explicit ZstdSharp.Port central pin is dropped: MySql.Data 26.7.0 now requires >= 0.8.8 directly, and central transitive pinning resolves it without a manual entry. (ReportGenerator from the same range is not adopted; dev does not declare that package.)
Port the hardened SSH launch path from the upstream PR branch (mRemoteNG#3411) onto dev. The Terminal protocol built "cmd.exe /K ssh <user>@<host>" by concatenating the unsanitized Hostname/Username connection fields into a single argument string that cmd.exe then re-parsed, allowing arbitrary command execution via shell metacharacters (& | < > ^) from a malicious .xml connections file — automatic when "Reconnect to previously opened sessions" is enabled. Launch ssh.exe directly instead so no shell interprets the arguments: - BuildSshArguments validates hostname/username and rejects whitespace, control characters, a leading '-', and double quotes (the latter because CommandLineToArgvW strips quotes, so "-oProxyCommand=..." would otherwise reach ssh.exe as an option — argument injection). - Validation errors do not echo the attacker-controlled value (avoids log/UI injection via control characters). - FindSshExe locates ssh.exe (System32\OpenSSH then PATH), normalizing PATH segments (strip quotes, skip empty/non-rooted, expand %VAR%). - Localhost check uses OrdinalIgnoreCase instead of culture-sensitive ToLower(); local sessions keep COMSPEC with a fixed "/K". Adds ProtocolTerminalTests (27 cases): valid targets incl. IPv6, the issue payload, shell metacharacters, whitespace, leading-dash and quoted option-injection variants, and sanitized error messages. 27/27 pass.
- coverlet.collector 8.0.0 -> 10.0.1 - FlaUI.Core / UIA3 4.0.0 -> 5.0.0 - Gherkin 39.0.0 -> 42.0.1 - Meziantou.Analyzer 2.0.194 -> 3.0.139 - Microsoft.NET.Test.Sdk 18.3.0 -> 18.8.1 - NSubstitute 5.3.0 -> 6.0.0 - NUnit 4.5.1 -> 4.6.1 - NUnit3TestAdapter 6.1.0 -> 6.2.0 - Roslynator.Analyzers 4.12.11 -> 4.15.0 Build clean; full suite 6437 passed. The one deterministic failure (StartupConnectionPathReturnsSavedPathWhenItIsTheSoleCandidate) is a pre-existing environmental test — it fails identically on the prior package set — and is unrelated to these updates.
The port scan tool's First/Last IP inputs were a four-octet control that only accepted IPv4. Two changes: - Input: MrngIpTextBox is now a single-line text box, so a full address can be typed or pasted in one field (no per-octet tabbing). Validation moves to IPAddress.TryParse; the two endpoints must be the same address family. The fields are widened to fit an IPv6 address. - Scanner: PortScanner enumerated the range with 32-bit uint math and threw "Only IPv4 addresses are supported". It now uses BigInteger over the address bytes, so both IPv4 and IPv6 ranges enumerate correctly (unsigned, big-endian; carries across group boundaries). The 65,536-address cap is kept — every address is pinged, so a range must stay small (this matters most for IPv6). Mixed IPv4/IPv6 endpoints are rejected. Tests: add IPv6 range ordering, group-boundary carry, over-limit, and mixed-family cases to PortScannerTests.
StartScan set _scanning = true and switched the button to "Stop" before constructing the PortScanner. Constructing it validates/enumerates the range and can throw (range over the 65,536 limit, mixed IPv4/IPv6 endpoints). The exception was only logged, and the scanning state was never reset — so the button stuck on "Stop" and the tool appeared to do nothing (e.g. First 192.168.0.1 / Last 192.169.0.254 is 65,789 addresses, just over the cap). Now the scanner is built before entering the scanning state; argument errors surface a clear warning (message panel + a dialog) and leave the button on "Scan". Cleaned up the range/family exception messages so they read well when shown to the user (no "(Parameter …)" suffix, thousands separators).
The scan engine fanned out a Ping.SendAsync over the entire range at once, and each completion did a synchronous, blocking TcpClient connect per port using the OS default timeout (~21s for filtered ports), ignoring the configured timeout. Results were marshalled to the UI with a blocking Invoke, so worker threads piled up on the UI pump — the window froze and Stop lagged. Stop could only cancel pings, not the in-flight blocking connects, so work continued after it was clicked. Rewritten as an async pipeline: - Parallel.ForEachAsync bounds concurrency (MaxConcurrentHosts = 64) instead of firing thousands of pings/sockets at once. - Ping, reverse-DNS and TCP connect are all awaited; each port connect uses ConnectAsync with a linked timeout so it honours the user's timeout and can be cancelled immediately (no 21s hangs). A host's ports are probed concurrently. - StopScan cancels a CancellationTokenSource, so pings AND connects stop promptly; ScanComplete is always raised once at the end. - The window marshals results with BeginInvoke (non-blocking), keeping the UI and the Stop button responsive under a flood of results. Range enumeration, constructors, events and IsPortOpen are unchanged; PortScannerTests remain green.
Running a full build plus the full suite after every edit costs ~4 minutes and is wasted on most changes. Add a "Verification Effort" section mapping change types to the verification they warrant (nothing for docs, targeted compile/filter for small edits, full build + suite for dependency and cross-project changes), and reference it from the issue-fix workflow and session discipline rules. Also records that a build failing only on file-copy locks (a running mRemoteNG.exe holding bin\) is not a code failure, and keeps the hard rule that verification is never faked or skipped where it is warranted.
The port controls had a checkbox per field ("First Port" and "Last Port")
on separate rows, which didn't make sense: a range needs both ends, and the
two boxes could be left half-configured (only one ticked), where the scanner
silently substituted the other value.
Replaced with one "Port Range" checkbox and a single inline row:
[ ] Port Range Start Port [1] to End Port [65535]
The checkbox enables/disables both numerics and their labels together, and
selects between "scan this range" and the default well-known protocol
ports. portStart now defaults to 1 (was 0, which the scanner rewrote to the
end port, scanning a single port). The layout drops from six rows to five.
- First IP / Last IP are now a single row: "IP Range [first] - [last]". Both fields are 265px, enough for a full-length IPv6 address (39 chars, ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff). lblStartIP becomes the "IP Range" caption and lblEndIP the "-" separator, so no controls are orphaned. - The Scan button moves out of the far-right stretch column and sits next to the timeout field, so it is close to the controls it acts on. - The grid drops from five rows to four (IP range, port range, custom ports, timeout + scan).
Investigated the remaining UI hang during a scan. Three causes, all in the result-reporting path rather than the scan itself: 1. Per-host logging on the hot path. Every host raised BeginHostScan and HostScanned, whose handlers called MessageCollector.AddMessage. The collector runs its writer chain inline on the calling thread, and the log writer does synchronous file I/O — so 64 scan workers serialised on the log appender's lock, and the UI thread (which handled HostScanned) blocked on that same lock once per host. The per-host messages are gone. 2. One cross-thread post and one list rebuild per host. Results are now queued from the workers with no marshalling and drained onto the results list every 200ms by a UI timer, in a single AddObjects batch inside BeginUpdate/EndUpdate. Progress updates come from the same flush. Stop and ScanComplete flush once more so nothing is lost. 3. MessageCollector mutated a plain List<IMessage> from many threads at once — a genuine race now that the scanner is parallel. Added a lock, and replaced the repeated RemoveAt(0) trim (which shifts the whole list per message once the 10,000 cap is hit) with a single RemoveRange. Also adds a "Set common ports" button beside the custom ports box that fills it with the usual service ports (FTP/SSH/Telnet/SMTP/DNS/HTTP(S), Windows RPC/NetBIOS/SMB, LDAP(S), IMAP/POP3, databases, RDP, VNC, WinRM, 8080/8443, Elasticsearch, MongoDB).
The concurrency limit was a hard-coded 64. PortScanner now takes it as a constructor argument (defaulting to DefaultConcurrentHosts = 64) and clamps it to MinConcurrentHosts..MaxConcurrentHosts (1..128), so an out-of-range value can never fan out unbounded. The port scan window exposes it as a "Parallel scans" spinner on the timeout row, min 1, max 128, default 64, with a tooltip explaining when to lower it. The UI value is passed to every PortScanner construction path.
pnlMain reserved a fixed 159px for the settings panel, sized back when pnlIp had six rows. Consolidating the IP range and port range onto single lines cut pnlIp to four rows (~110px), leaving ~46px of empty space above the progress bar. Row height is now 116px — the four rows plus pnlIp's 3px margins.
The scan target is now one field that accepts a single address, an explicit range (192.168.1.1 - 192.168.1.254) or a CIDR block (192.168.1.0/24), in IPv4 or IPv6 form, replacing the separate start/end address boxes. Parsing lives in Tools/IpRangeParser so it is unit testable, and a failed parse now reports why instead of the generic "cannot start scan" warning. The port controls become a three-way choice - common ports, all ports, or a custom list - replacing the port-range checkbox, the start/end spinners and the "set common ports" button. The custom list accepts ports and ranges (22, 80, 443, 3389, 8000-8100) via Tools/PortListParser, and is only editable while Custom is selected. Port 513 (rlogin) joins the common set so every protocol column in the results list is still populated in that mode.
PR Summary by QodoPort scan: single target field (IPv4/IPv6/range/CIDR) + port mode selector
AI Description
Diagram
High-Level Assessment
Files changed (53)
|
There was a problem hiding this comment.
Pull request overview
This PR primarily modernizes the Port Scan feature by collapsing scan-target input into a single IPv4/IPv6 range field, replacing port-range controls with a port-mode selector, and moving parsing/validation into dedicated, unit-tested helpers. It also includes several additional UI reliability/security fixes outside Port Scan (tab-close behavior, connection-tree filtering, Terminal protocol SSH launch hardening, etc.).
Changes:
- Rework Port Scan UI + logic: single IP/range/CIDR input, port mode selection (common/all/custom), new
IpRangeParser/PortListParser, and updatedPortScanner(IPv6 + concurrency + improved cancellation). - Improve UI correctness/perf in related areas: tab closing selection behavior, disconnect/close semantics, connection-tree filter expansion restore, and batched port-scan result updates.
- Add/adjust test coverage and supporting utilities (new parsers/tests, color editor support for string-backed colors, MessageCollector thread-safety improvements), plus dependency/project housekeeping.
Reviewed changes
Copilot reviewed 51 out of 53 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mRemoteNGTests/UI/Tabs/DockPaneStripNGTests.cs | Stronger STA/message-pump test harness + new regression test |
| mRemoteNGTests/UI/Tabs/ConnectionTabCloseTests.cs | New tests for disconnect-vs-close tab semantics |
| mRemoteNGTests/UI/Controls/ConnectionTreeExpansionTests.cs | New tests for filter removal restoring expansion/rows |
| mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs | New tests for string-backed color editor behavior |
| mRemoteNGTests/Tools/TabColorConverterTests.cs | Adds coverage for Color.Empty conversion |
| mRemoteNGTests/Tools/PortScannerTests.cs | Adds IPv6 range + mixed-family tests |
| mRemoteNGTests/Tools/PortListParserTests.cs | New unit tests for custom port list parsing |
| mRemoteNGTests/Tools/IpRangeParserTests.cs | New unit tests for IP/range/CIDR parsing |
| mRemoteNGTests/mRemoteNGTests.csproj | Nullable annotations + OS platform version adjustment + package cleanup |
| mRemoteNGTests/Connection/Protocol/PuttyBaseTests.cs | Adds coverage for PuTTY exit-confirmation detection |
| mRemoteNGTests/Connection/Protocol/ProtocolTerminalTests.cs | New regression tests for SSH argument sanitization |
| mRemoteNGTests/Config/Serializers/MiscSerializers/MobaXTermSessionDeserializerTests.cs | Ordinal string comparisons for deterministic tests |
| mRemoteNGTests/Config/Serializers/MiscSerializers/MicrosoftRdClientBackupDeserializerTests.cs | Ordinal comparisons in tests |
| mRemoteNGTests/Config/Serializers/ConnectionSerializers/Json/JsonConnectionsSerializerTests.cs | Removes duplicate using |
| mRemoteNGSpecs/mRemoteNGSpecs.csproj | Package cleanup |
| mRemoteNG/UI/Window/PortScanWindow.Designer.cs | New Port Scan layout: range textbox + port mode + parallelism control |
| mRemoteNG/UI/Window/PortScanWindow.cs | New parsing + port mode logic + batched UI updates for results |
| mRemoteNG/UI/Window/ConnectionWindow.cs | Mark disconnect-only closes; avoid reviving closing tabs |
| mRemoteNG/UI/TaskDialog/CommandButton.cs | Nullability annotation for Text override |
| mRemoteNG/UI/Tabs/DockPaneStripNG.cs | Preserve active tab when a close is cancelled |
| mRemoteNG/UI/Tabs/ConnectionTab.cs | Distinguish disconnect-only close from true tab close |
| mRemoteNG/UI/Forms/OptionsPages/BackupPage.cs | Removes unused field |
| mRemoteNG/UI/Forms/frmMain.cs | Safer logging + culture-aware title formatting + removes unused event |
| mRemoteNG/UI/Controls/mrngIpTextBox.cs | Replace IPv4-octet control with single-field IP text box |
| mRemoteNG/UI/Controls/ConnectionTree/ConnectionTree.cs | Snapshot/restore expansion state + rebuild rows after filter removal |
| mRemoteNG/UI/Controls/ConnectionInfoPropertyGrid/ColorStringEditor.cs | New editor for string-backed color properties |
| mRemoteNG/UI/Controls/Adapters/CredentialRecordListAdaptor.cs | Nullability annotation adjustment |
| mRemoteNG/Tools/WindowsRegistry/IRegistryRead.cs | API nullability tweak for registry reads |
| mRemoteNG/Tools/ScanHost.cs | Fix trailing delimiter by using string.Join |
| mRemoteNG/Tools/PortScanner.cs | Async/concurrent scanning + IPv6 range support + cancellation improvements |
| mRemoteNG/Tools/PortListParser.cs | New port list/range parser for custom ports |
| mRemoteNG/Tools/MiscTools.cs | Converter correctness + standard values returned as strings |
| mRemoteNG/Tools/IpRangeParser.cs | New parser for address/range/CIDR input (IPv4/IPv6) |
| mRemoteNG/Security/KeyDerivation/Pkcs5S2KeyGenerator.cs | Use built-in PBKDF2 API |
| mRemoteNG/mRemoteNG.csproj | Package cleanup |
| mRemoteNG/Messages/MessageCollector.cs | Locking + trimming efficiency under multi-threaded message floods |
| mRemoteNG/Credential/PlaceholderCredentialRecord.cs | Suppress unused event warning for immutable placeholder |
| mRemoteNG/Connection/Protocol/VNC/VncDesHelper.cs | Ignore cleanup return codes explicitly |
| mRemoteNG/Connection/Protocol/Terminal/Connection.Protocol.Terminal.cs | Launch ssh.exe directly + argument injection hardening |
| mRemoteNG/Connection/Protocol/RDP/RdpProtocol8.cs | Safer logging with null-conditional hostname |
| mRemoteNG/Connection/Protocol/PuttyBase.cs | Auto-dismiss PuTTY warn-on-close confirmation + constants |
| mRemoteNG/Connection/AbstractConnectionRecord.cs | Switch to string-aware color editor for string properties |
| mRemoteNG/Config/Settings/Providers/PortableSettingsInitializer.cs | Null-forgiveness for provider initialization call |
| mRemoteNG/Config/Serializers/ConnectionSerializers/Xml/XmlConnectionsDeserializer.cs | Null-safe decrypt assignment |
| mRemoteNG/Config/Serializers/ConnectionSerializers/Sql/DataTableSerializer.cs | Safer primary key column lookup |
| mRemoteNG/Config/Serializers/ConnectionSerializers/Csv/RemoteDesktopManager/CsvConnectionsDeserializerRdmFormat.cs | Avoid default string in tuple return |
| mRemoteNG/Config/Connections/Multiuser/RemoteConnectionsSyncronizer.cs | Use System.Threading.Lock |
| mRemoteNG/App/NativeMethods.cs | Add GetWindowText P/Invoke |
| mRemoteNG/App/DevLog.cs | Use Lock + Environment.CurrentManagedThreadId |
| ExternalConnectors/ExternalConnectors.csproj | Package reference cleanup |
| ExternalConnectors/CPS/CPSConnectionForm.Designer.cs | Removes unused label field |
| Directory.Packages.props | Dependency version updates + remove ZstdSharp.Port |
| CLAUDE.md | Update verification guidance section |
Files not reviewed (2)
- ExternalConnectors/CPS/CPSConnectionForm.Designer.cs: Generated file
- mRemoteNG/UI/Window/PortScanWindow.Designer.cs: Generated file
Comment on lines
+239
to
+243
| bool[] portResults = await Task.WhenAll( | ||
| _ports.Select(port => IsPortOpenAsync(ipAddress, port, token))).ConfigureAwait(false); | ||
|
|
||
| for (int i = 0; i < _ports.Count; i++) | ||
| { |
Comment on lines
+121
to
+148
| // One field takes a single address, an explicit range or a CIDR block (IPv4 or IPv6). | ||
| lblStartIP.Text = "IP / Range / CIDR"; | ||
| txtIpRange.ToolTipText = IpRangeParser.SyntaxHint; | ||
| txtIpRange.PlaceholderText = "192.168.1.1 | 192.168.1.1 - 192.168.1.254 | 192.168.1.0/24"; | ||
| btnScan.Text = Language._Scan; | ||
| btnImport.Text = Language._Import; | ||
| lblOnlyImport.Text = Language.ProtocolToImport; | ||
| clmHostIP.Text = "IP Address"; | ||
| clmHostName.Text = "Hostname"; | ||
| clmOpenPorts.Text = Language.OpenPorts; | ||
| clmClosedPorts.Text = Language.ClosedPorts; | ||
| ngCheckFirstPort.Text = Language.FirstPort; | ||
| ngCheckLastPort.Text = Language.LastPort; | ||
| lblCustomPorts.Text = "Custom ports (e.g. 22,80,443):"; | ||
| lblPorts.Text = "Ports"; | ||
| rdoCommonPorts.Text = "Common ports"; | ||
| rdoAllPorts.Text = "All ports"; | ||
| rdoCustomPorts.Text = "Custom"; | ||
| txtCustomPorts.PlaceholderText = "22, 80, 443, 3389, 8000-8100"; | ||
| portScanToolTip.SetToolTip(rdoCommonPorts, | ||
| "Probe the commonly used service ports only:" + Environment.NewLine + | ||
| string.Join(", ", CommonPorts)); | ||
| portScanToolTip.SetToolTip(rdoAllPorts, | ||
| $"Probe every port from {PortListParser.MinPort} to {PortListParser.MaxPort}. " + | ||
| "This is thorough but slow."); | ||
| portScanToolTip.SetToolTip(rdoCustomPorts, | ||
| "Probe a list of ports you specify, e.g. 22, 80, 443, 3389, 8000-8100"); | ||
| lblParallelScans.Text = "Parallel scans"; | ||
| portScanToolTip.SetToolTip(numericParallelScans, | ||
| $"How many hosts are probed at once ({PortScanner.MinConcurrentHosts}-{PortScanner.MaxConcurrentHosts}). " + | ||
| "Lower this if the scan saturates your network or machine."); |
Comment on lines
+54
to
+58
| private readonly ConcurrentQueue<ScanHost> _pendingHosts = new(); | ||
| private readonly System.Windows.Forms.Timer _scanResultsTimer = new() { Interval = 200 }; | ||
| private int _scannedHostCount; | ||
| private int _totalHostCount; | ||
|
|
Comment on lines
236
to
246
| CloseProtocolSafe(); | ||
| // Protocol close handler (HandleProtocolClosed) will show closed state. | ||
| // Cancel form close so the tab stays open with Connect button (#61). | ||
| if (Properties.OptionsTabsPanelsPage.Default.KeepTabsOpenAfterDisconnect) | ||
| if (KeepTabOpenAfterDisconnect) | ||
| e.Cancel = true; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| CloseProtocolSafe(); | ||
| if (Properties.OptionsTabsPanelsPage.Default.KeepTabsOpenAfterDisconnect) | ||
| if (KeepTabOpenAfterDisconnect) | ||
| e.Cancel = true; | ||
| } |
Code Review by Qodo
1. Unbounded port concurrency
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+8
to
+9
| <PackageVersion Include="AWSSDK.Core" Version="4.0.100.9" /> | ||
| <PackageVersion Include="AWSSDK.EC2" Version="4.0.109" /> |
There was a problem hiding this comment.
1. Modified directory.packages.props 📘 Rule violation § Compliance
This PR changes a protected infrastructure file (Directory.Packages.props), which is disallowed. Keeping this change in the PR can block compliant merging and introduces risk to the repository-wide dependency baseline.
Agent Prompt
## Issue description
This PR modifies `Directory.Packages.props`, which is listed as a protected infrastructure file and must not be changed in this PR.
## Issue Context
The compliance checklist forbids any modifications to protected infra files, regardless of change size.
## Fix Focus Areas
- Directory.Packages.props[8-12]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| [TestCase("192.168.1.1")] | ||
| [TestCase(" 192.168.1.1 ")] | ||
| [TestCase("2001:db8::1")] | ||
| public void SingleAddressYieldsAOneAddressRange(string input) |
There was a problem hiding this comment.
4. New tests violate naming pattern 📘 Rule violation ▣ Testability
Several newly added/modified test method names do not follow the required MethodName_Scenario_ExpectedBehavior pattern. This reduces consistency and can hinder test discoverability and filtering conventions.
Agent Prompt
## Issue description
New tests use method names that do not follow the required `MethodName_Scenario_ExpectedBehavior` naming convention.
## Issue Context
The repo’s compliance rules require exactly three PascalCase segments separated by two underscores for test methods.
## Fix Focus Areas
- mRemoteNGTests/Tools/IpRangeParserTests.cs[10-16]
- mRemoteNGTests/UI/Controls/ColorStringEditorTests.cs[34-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+239
to
+240
| bool[] portResults = await Task.WhenAll( | ||
| _ports.Select(port => IsPortOpenAsync(ipAddress, port, token))).ConfigureAwait(false); |
There was a problem hiding this comment.
5. Unbounded port concurrency 🐞 Bug ☼ Reliability
PortScanner.ScanHostAsync probes all ports with Task.WhenAll over _ports, which creates one TCP connect task/socket per port concurrently. In “All ports” mode this can flood the OS with tens of thousands of concurrent connects per host (and millions overall), leading to resource exhaustion and scans that hang or fail.
Agent Prompt
## Issue description
`PortScanner.ScanHostAsync` currently launches **one async connect per port at once** using `Task.WhenAll(_ports.Select(...))`. When the UI selects **All ports (1-65535)**, this can create tens of thousands of simultaneous socket connects per host, overwhelming ephemeral ports / socket limits / memory and causing the scan to hang or fail.
## Issue Context
- UI can pass a 65,535-length port list via `PortListParser.AllPorts()`.
- Host concurrency is bounded (`_maxConcurrentHosts`), but **port concurrency per host is not bounded**.
## Fix Focus Areas
- mRemoteNG/Tools/PortScanner.cs[190-256]
- mRemoteNG/Tools/PortScanner.cs[258-280]
- mRemoteNG/UI/Window/PortScanWindow.cs[258-275]
## Suggested fix
- Replace the `Task.WhenAll(_ports.Select(...))` fan-out with a bounded-concurrency approach, e.g.:
- Use `Parallel.ForEachAsync(_ports, new ParallelOptions { MaxDegreeOfParallelism = <small number>, CancellationToken = token }, ...)` and store results into a pre-sized `bool[]` or update `scanHost` under a lock.
- Or use a `SemaphoreSlim` to cap concurrent `ConnectAsync` operations.
- Consider a single global limiter across hosts+ports (so `hosts * ports` cannot explode).
- Ensure cancellation still aborts promptly and timeouts are preserved.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
354
to
+357
| if (InvokeRequired) | ||
| { | ||
| Invoke(new PortScannerScanComplete(PortScanner_ScanComplete), new object[] {hosts}); | ||
| if (IsHandleCreated) | ||
| BeginInvoke(new PortScannerScanComplete(PortScanner_ScanComplete), hosts); |
There was a problem hiding this comment.
6. Timer cleanup skipped 🐞 Bug ☼ Reliability
PortScanWindow starts a WinForms Timer for batched UI updates, but PortScanner_ScanComplete returns early when IsHandleCreated is false, skipping the timer stop/flush and leaving scan state/timer running. Closing/destroying the window during a scan can therefore leak the timer/handlers and keep ticking against disposed controls.
Agent Prompt
## Issue description
`PortScanWindow` introduces `_scanResultsTimer` and starts it during scans, but scan completion cleanup can be skipped when the form handle is not available (`IsHandleCreated == false`), leaving the timer running and the scan not explicitly cancelled when the window is closing/disposed.
## Issue Context
- `_scanResultsTimer` is a field-owned `System.Windows.Forms.Timer` and is started in `StartScan()`.
- In `PortScanner_ScanComplete`, when invoked from a worker thread, the code only marshals back to the UI thread if `IsHandleCreated` is true; otherwise it returns without stopping the timer.
## Fix Focus Areas
- mRemoteNG/UI/Window/PortScanWindow.cs[27-58]
- mRemoteNG/UI/Window/PortScanWindow.cs[163-216]
- mRemoteNG/UI/Window/PortScanWindow.cs[352-369]
## Suggested fix
- Ensure timer and scan are always cleaned up on window teardown:
- Override `Dispose(bool disposing)` or handle `FormClosed`/`HandleDestroyed` to call `StopScan()` (or `_portScanner?.StopScan()`), `_scanResultsTimer.Stop()`, detach the Tick handler, and `Dispose()` the timer.
- Alternatively, create the timer in the `components` container (so it is disposed automatically) and still stop/cancel scans on close.
- In `PortScanner_ScanComplete`, consider safe cleanup when the handle is gone (e.g., rely on Dispose/FormClosed to stop the timer) rather than returning without any shutdown path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ApplyLanguage() assigned 13 hard-coded English strings, and the parsers and
scanner built their error messages from interpolated literals that
PortScanWindow shows verbatim in a MessageBox. Non-English cultures got a
largely untranslated panel and English validation errors.
Move the whole panel into Language.resx: labels, column headers, the two
placeholders, the four tooltips and the five validation/reporting templates.
The tool layer reads them directly, matching the existing mRemoteNG.Tools ->
Resources.Language dependency used by ScanHost, MiscTools and ExternalTool.
Three notes on key choices:
- PortListParser.SyntaxHint maps to a new PortScanPortListHint rather than
PortScanCustomPortsHint; the latter holds the radio tooltip wording, which
is a different sentence and wrong for a validation error.
- Hostname already exists but its value carries a trailing colon, so it is
unusable as a column header. PortScanHostname added instead.
- The parameterised tooltips take {0}/{1} fed from PortListParser.MinPort /
MaxPort and PortScanner.MinConcurrentHosts / MaxConcurrentHosts, keeping the
bounds code-driven while the sentence stays translatable.
Both SyntaxHint members now resolve to resources instead of duplicating their
text, which turns them from const into static properties - resource lookups
are not compile-time constants. No caller relied on const.
The IEnumerable<int> constructor took the port list on trust. A null list threw an opaque ArgumentNullException from List.AddRange, an empty list produced a scan that pinged every host in the range and probed nothing, and an out-of-range value only surfaced much later as a failure inside the per-port TcpClient connect - by which time the scan was already running and the UI had switched the button to "Stop". Validate up front instead: reject null, reject an empty list, and reject any port outside PortListParser.MinPort..MaxPort. All three throw ArgumentException or a subclass, so PortScanWindow.StartScan already catches them and reports a localized, actionable message without starting the scan. The sequence is materialized once before validating, since it may be lazy and validating separately from AddRange would otherwise enumerate it twice. The older (port1, port2, checkDefaultPortsOnly) constructor is left alone: it has no callers in the repo and treats port 0 as "unspecified", a convention the same validation would break.
The (port1, port2, checkDefaultPortsOnly) overload accepted any int pair, so a negative or above-65535 endpoint produced a port list that could only fail later inside the per-port TcpClient connect. Validation runs after the "0 means unspecified" rule has been applied, so (0, 3389) still resolves to a single-port scan as before, and before the loop expands the range, so an absurd endpoint cannot allocate a million entries first. Both endpoints go through the same ValidatePort helper as the IEnumerable<int> overload, giving identical localized messages. port1/port2 are unused when checkDefaultPortsOnly is set, so that mode stays unvalidated rather than throwing on arguments it ignores.
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.
What
Two input areas on the Port Scan panel collapse into one control each.
Scan target — one field replaces the separate start/end address boxes and accepts, in IPv4 or IPv6 form:
192.168.1.1,2001:db8::1192.168.1.1 - 192.168.1.254(spaces optional, endpoints in either order)192.168.1.0/24,2001:db8::/120Ports — a three-way choice replaces the
Port Rangecheckbox, the start/end spinners and the Set common ports button:The custom list accepts individual ports and ranges, deduplicated and sorted. The field is only editable while Custom is selected, so the port options cannot be left half-configured. Port 513 (rlogin) joins the common set, so every protocol column in the results list is still populated in that mode.
Notes
Tools/IpRangeParserandTools/PortListParserrather than in the window, so both are unit tested.", "after the last port.Verification
ConnectionsServiceStartupPathTests.StartupConnectionPathReturnsSavedPathWhenItIsTheSoleCandidate, is a pre-existing environment-dependent failure in this working copy (it resolves aconfCons.xmlfrom the local tree) and is unrelated to these changes.IpRangeParserTests,PortListParserTests.