Feature/local build and release scripts - #3413
Open
lakshamanr wants to merge 7 commits into
Open
Conversation
Author
|
n |
- build-and-run.bat: locates MSBuild via vswhere, builds mRemoteNG.csproj (Debug|x64) and launches mRemoteNG.exe for local dev iteration. - release-msi.bat: builds the app (Release|x64) and packages the existing WiX v3 installer project (mRemoteNGInstaller/Installer) into mRemoteNG-Installer.msi, overriding WixTargetsPath/WixCATargetsPath and SolutionDir so the build works standalone (outside the .sln) and with VS2026's relocated MSBuildExtensionsPath32. - nuget.config: clears NuGet sources to nuget.org only, so restores don't depend on unrelated private feeds that may be configured globally. - mRemoteNG.csproj: guard the T4 TextTemplating targets import with Exists(...) since Properties/AssemblyInfo.cs is already generated and checked in; avoids requiring the VS Text Template Transformation component on machines that only need to build, not regenerate it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restores the fixed pixel resolution options (800x600 through 3840x2160) that were removed from RDPResolutions in commit cf1709b, and adds two larger options (5120x2880, 7680x4320) for high-DPI displays. - RDPResolutions.cs: re-add Res800x600...Res3840x2160 plus new Res5120x2880/Res7680x4320 members. - RdpExtensions.cs: restore GetResolutionRectangle() to parse the fixed size out of the enum member name. - RdpProtocol.cs SetResolution(): add a default case for fixed resolutions. Keeps the control docked (Fill) and just sets DesktopWidth/DesktopHeight; the RDP ActiveX control's own native scrollbars handle a session larger than the visible panel. Undocking and manually resizing the control (like FitToWindow does) causes a blank/partially-rendered session because the AxHost wrapper can't propagate a large one-shot Size jump to the RDP control's internal rendering surface. - RdpProtocol8.cs DoResizeControl(): update comment; fixed resolutions are skipped the same way FitToWindow already was, no dynamic resize. - Tests: cover GetResolutionRectangle() for fixed/mode values, CSV/XML serialization round-trip, and DataTableDeserializer for the restored enum members. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…sion) Adds a UseMultiMon connection property, following the existing pattern used for RDP display toggles (e.g. CacheBitmaps): wired through AbstractConnectionRecord/ConnectionInfo, inheritance, XML/CSV serialization, and localization. RdpProtocol.SetResolution() honors it ahead of the resolution switch so it overrides SmartSize/FitToWindow/Fullscreen, forcing true full screen and enabling IMsRdpClientNonScriptable5.UseMultimon (RDP client 8.1+, older clients fall back to single-monitor size). A fixed pixel resolution (e.g. Res7680x4320) is still honored exactly for DesktopWidth/DesktopHeight rather than being overridden by the local monitors' combined bounds; only Fullscreen/SmartSize/FitToWindow (which have no explicit WxH of their own) fall back to SystemInformation.VirtualScreen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a 'Show Password' item to the property grid right-click menu that reveals the plaintext value of password fields (Password, RDGatewayPassword, VNCProxyPassword). Enabled only when the selected grid item is a password property with a non-empty value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use all my monitors now hands the RDP connection off to mstsc.exe with 'use multimon:i:1' (via a temp .rdp + staged Credential Manager entry), because the embedded RDP ActiveX control can only go fullscreen on a single monitor. This spans every physical monitor like the built-in Remote Desktop client. Adds a CustomResolution WidthxHeight connection property (single-monitor) that overrides the Resolution dropdown when set, wired through AbstractConnectionRecord/inheritance, XML+CSV serialization, defaults and localization. RdpProtocol.SetResolution honors it; RdpExtensions.TryParseCustomResolution validates against RDP's 200-8192px limits. Adds RdpCustomResolutionTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three issues prevented copying from local to a remote RDP session even with Redirect Clipboard enabled: - SetRedirection() ran SetDriveRedirection() before the RedirectClipboard assignment inside the same try/catch, so a drive-redirection failure (common pre-connect when enumerating DriveCollection for Local/Custom drives) swallowed the exception and silently skipped enabling clipboard redirection. Clipboard is now set first and drive redirection is isolated in its own try. - frmMain joined the legacy SetClipboardViewer chain and forwarded WM_DRAWCLIPBOARD/WM_CHANGECBCHAIN with wParam/lParam swapped, corrupting the clipboard-viewer chain and interfering with the RDP control's clipboard monitor. Replaced with the non-chaining AddClipboardFormatListener / WM_CLIPBOARDUPDATE API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
If a previous mRemoteNG-Installer.msi is registered by Windows Installer, light.exe fails with LGHT0001 (access denied). Now the script tries a plain del first; if the file is still present it re-launches an elevated cmd to force-delete it, then aborts with a clear message if deletion still fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR Summary by QodoAdd local build/MSI scripts and expand RDP resolution + multimon support
AI Description
Diagram
High-Level Assessment
Files changed (34)
|
Code Review by Qodo
1. Cmdkey leaks RDP password
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+37
to
+40
| bool stagedCredential = !string.IsNullOrEmpty(user) && !string.IsNullOrEmpty(password); | ||
| if (stagedCredential) | ||
| RunCmdKey("/generic:TERMSRV/" + host, "/user:" + (string.IsNullOrEmpty(domain) ? user : domain + "\\" + user), "/pass:" + password); | ||
|
|
There was a problem hiding this comment.
1. Cmdkey leaks rdp password 🐞 Bug ⛨ Security
RdpExternalMultimonLauncher stages RDP credentials by invoking cmdkey.exe with the password in the /pass: command-line argument, exposing the plaintext password to local process inspection. If anything throws after staging but before the cleanup task runs, the TERMSRV credential can persist in Windows Credential Manager longer than intended.
Agent Prompt
### Issue description
`RdpExternalMultimonLauncher` passes the RDP password to `cmdkey.exe` via `/pass:<password>`, which exposes the secret in process arguments. Cleanup of the staged credential only happens in a background task after `mstsc.Start()`, so failures in between can leave a persistent cached credential.
### Issue Context
The repo already uses direct Win32 credential APIs for deletion (`CredDelete`) in `RdpCredentialCacheCleaner`, so staging should follow the same pattern (e.g., `CredWrite`) instead of spawning `cmdkey.exe`.
### Fix
- Replace `cmdkey.exe` usage with an in-process Win32 credential write (`CredWriteW`) to create/update the `TERMSRV/<hostname>` entry without putting secrets on a command line.
- Ensure staged credentials are removed on *all* failure paths:
- Track whether staging succeeded.
- Use `try/finally` around `mstsc.Start()` and cleanup scheduling.
- If `mstsc` fails to start, immediately delete the staged credential.
- Keep temp `.rdp` cleanup best-effort, but ensure credential cleanup is best-effort on every path.
### Fix Focus Areas
- mRemoteNG/Connection/Protocol/RDP/RdpExternalMultimonLauncher.cs[23-63]
- mRemoteNG/Connection/Protocol/RDP/RdpCredentialCacheCleaner.cs[33-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+99
to
+102
| if (connectionInfo.Protocol == ProtocolType.RDP && connectionInfo.UseMultiMon) | ||
| { | ||
| Protocol.RDP.RdpExternalMultimonLauncher.Launch(connectionInfo); | ||
| return; |
There was a problem hiding this comment.
2. Multimon skips postextapp 🐞 Bug ≡ Correctness
When UseMultiMon is enabled, ConnectionInitiator.OpenConnection returns after launching mstsc.exe, bypassing the normal ProtocolBase lifecycle. As a result, PostExtApp (which is triggered from Prot_Event_Closed) will never run for multimon connections.
Agent Prompt
### Issue description
For RDP multimon sessions, `OpenConnection` launches `mstsc.exe` and returns early. This skips the embedded protocol lifecycle events (`Prot_Event_Closed`), which is where `PostExtApp` is executed, so multimon connections behave differently than normal connections.
### Issue Context
`PostExtApp` is started only inside `Prot_Event_Closed`, which will not fire for an externally launched `mstsc.exe` process.
### Fix
- Add a completion hook for external `mstsc.exe` sessions:
- Option A: in `RdpExternalMultimonLauncher.Launch`, after `mstsc.WaitForExit()`, run the configured `PostExtApp` for `connectionInfo`.
- Option B: return the `Process` (or a Task) from `Launch` and have `ConnectionInitiator` await/continue to run `PostExtApp` on completion.
- Ensure PostExtApp runs only if configured (same behavior as `Prot_Event_Closed`).
### Fix Focus Areas
- mRemoteNG/Connection/ConnectionInitiator.cs[93-103]
- mRemoteNG/Connection/ConnectionInitiator.cs[415-437]
- mRemoteNG/Connection/Protocol/RDP/RdpExternalMultimonLauncher.cs[51-58]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+723
to
+726
| GridItem gridItem = _pGrid.SelectedGridItem; | ||
| if (gridItem?.Value is string password && !string.IsNullOrEmpty(password)) | ||
| { | ||
| MessageBox.Show(password, gridItem.Label, MessageBoxButtons.OK, MessageBoxIcon.Information); |
There was a problem hiding this comment.
3. Show password exposes secrets 🐞 Bug ⛨ Security
ConfigWindow adds a "Show Password" context menu action that displays the stored password in plaintext in a MessageBox. This weakens local secret handling because any user with access to the unlocked session can reveal credentials without additional authorization or safeguards.
Agent Prompt
### Issue description
The new context-menu action reveals password values directly via `MessageBox.Show(...)` with no confirmation, re-authentication, or other guardrails.
### Issue Context
The menu is enabled for properties marked with `PasswordPropertyTextAttribute` and then displays the raw string value.
### Fix
- Add a guard before revealing secrets, e.g.:
- Require re-authentication / master password confirmation (if the app supports it), or
- At minimum, require an explicit confirmation dialog warning about shoulder-surfing/screen capture.
- Consider showing the value in a dedicated dialog with a masked-by-default textbox and a deliberate “Reveal” toggle, rather than always showing plaintext.
### Fix Focus Areas
- mRemoteNG/UI/Window/ConfigWindow.cs[682-734]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| echo Building MSI installer... | ||
| :: ponytail: WixTargetsPath is forced because VS's own MSBuildExtensionsPath32 no longer | ||
| :: points at the classic shared "%ProgramFiles(x86)%\MSBuild" folder where WiX v3 installs its targets. | ||
| "%MSBUILD%" "%ROOT%mRemoteNGInstaller\Installer\Installer.wixproj" /t:Build /p:Configuration=Release /p:Platform=x64 /p:SolutionDir=%ROOT% "/p:WixTargetsPath=%ProgramFiles(x86)%\MSBuild\Microsoft\WiX\v3.x\Wix.targets" "/p:WixCATargetsPath=%ProgramFiles(x86)%\MSBuild\Microsoft\WiX\v3.x\wix.ca.targets" /nologo /v:minimal |
There was a problem hiding this comment.
4. Msbuild args unquoted 🐞 Bug ☼ Reliability
release-msi.bat passes /p:SolutionDir=%ROOT% to MSBuild without quotes, so builds from paths containing spaces will mis-parse the argument and fail or behave incorrectly.
Agent Prompt
### Issue description
The MSI build command sets `SolutionDir` without quoting `%ROOT%`, which can contain spaces (derived from `%~dp0`). This breaks MSBuild argument parsing.
### Fix
- Quote the property value:
- Change `/p:SolutionDir=%ROOT%` to `/p:SolutionDir="%ROOT%"`.
- (Optional) Apply the same quoting discipline to any other path-valued properties you add later.
### Fix Focus Areas
- release-msi.bat[6-7]
- release-msi.bat[51-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
lakshamanr
force-pushed
the
feature/local-build-and-release-scripts
branch
from
August 7, 2026 15:13
d1cc6d4 to
7cd55b5
Compare
|
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
2 participants
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.
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: