Windows RDP support (macOS production-ready) - #1
Merged
Conversation
Introduces RDP as a first-class connection type, rendering a live Windows desktop in-app. A FreeRDP 3 sidecar process (native/rdp-spike/sidecar.c) connects to the host and streams composed RGBA frames over stdout as length-prefixed NXF1 blobs; the main process (ipc/rdp.ts) parses them and forwards each frame to a <canvas> in RdpView. Output-only for now (input injection is the next milestone). - New 'rdp' ConnectionType: add-connection form (port 3389, user/pass), sidebar "Remote Desktop" section, tab/list/type icons (Monitor glyph) - openTab routes rdp -> RDP view (and folds in redis routing too) - Frame parser resyncs past stray stdout instead of tearing down; sidecar pins WLog to stderr at ERROR level so logging can't corrupt the binary frame channel (root cause of the original desync) - Dashboard now lists all connection types: SSH/untyped keep live CPU/RAM, other types render as metric-less launch cards - diag-frames.mjs: dev tool to inspect the sidecar frame stream Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The dev sidecar dynamically linked Homebrew FreeRDP (ffmpeg, X11, openssl, …) — not redistributable. Build a minimal, statically-linked FreeRDP and bundle the sidecar into the signed macOS app so it runs on a clean machine. - build-freerdp-static.sh: minimal static FreeRDP (no X11/ffmpeg/H264/audio/ redirection channels), collapsing the dep tree to system libs + OpenSSL - Makefile `rdp-sidecar-static`: links the static libs into a self-contained binary (verified: zero /opt/homebrew refs, ~7MB) - electron-builder: bundle via mac extraResources; afterPack guard (scripts/verify-sidecar.js) fails the build if the sidecar ever links non-redistributable libraries - release.yml: build + cache the static sidecar on the macOS runner - sidecarPath() is platform-aware (.exe on Windows) - sidecar.c: WLog threshold raised to ERROR (drops benign WARN console noise) - Gate the RDP connection type + "Open Remote Desktop" to macOS, the only platform shipping a sidecar today (graceful no-op elsewhere) Remaining for full cross-platform: Windows and Linux sidecar builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds end-to-end read-only RDP streaming: a C sidecar process ( ChangesRDP Streaming Feature
Sequence DiagramsequenceDiagram
participant User as User
participant RdpView as RdpView (renderer)
participant MainIPC as rdp.ts (main process)
participant Sidecar as rdp-sidecar (C process)
participant FreeRDP as FreeRDP GDI
User->>RdpView: Open RDP tab
RdpView->>MainIPC: ipcRenderer.invoke("rdp:connect", config)
MainIPC->>Sidecar: spawn rdp-sidecar host port user pass
Sidecar->>FreeRDP: freerdp_connect()
FreeRDP-->>Sidecar: EndPaint callback (primary_buffer)
Sidecar->>Sidecar: BGRA to RGBA swizzle + write NXF1 frame to stdout
Sidecar-->>MainIPC: stdout NXF1 binary frames
MainIPC->>MainIPC: drainFrames() parse and copy pixel buffer
MainIPC->>RdpView: webContents.send("rdp:frame", id, w, h, pixels)
RdpView->>RdpView: canvas.putImageData(ImageData)
User->>RdpView: Close tab
RdpView->>MainIPC: ipcRenderer.invoke("rdp:disconnect", id)
MainIPC->>Sidecar: process.kill()
MainIPC->>RdpView: webContents.send("rdp:closed", id, null)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Cross-machine handoff (next session is on Windows): documents what's done (macOS production-ready), the architecture + key files, and the step-by-step Windows sidecar plan — including the critical _setmode(stdout, _O_BINARY) gotcha, extraResources/guard/UI-gate changes, and lessons learned. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying noxed with
|
| Latest commit: |
38bc201
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1a94c666.noxed.pages.dev |
| Branch Preview URL: | https://feat-windows-rdp.noxed.pages.dev |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/store/index.ts (1)
246-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
openTabcan activate the wrong view for RDP sessions.At Line 246, dedupe only checks
sessionId, not the derivedview. If a session already has a non-RDP tab, opening an RDP session can incorrectly focus that tab instead of creating/reusing an RDP tab.Suggested fix
openTab: (session) => set((s) => { - const existing = s.tabs.find((t) => t.sessionId === session.id && t.status !== 'error' && !t.paneOf) - if (existing) return { activeTabId: existing.id, focusedPaneId: null } const isK8s = session.type === 'kubernetes' const isSftp = session.type === 'sftp' const isDb = session.type === 'database' const isRdp = session.type === 'rdp' const isRedis = session.type === 'redis' const view: TabView = isK8s ? 'k8s' : isSftp ? 'sftp' : isDb ? 'database' : isRdp ? 'rdp' : isRedis ? 'redis' : 'terminal' + const existing = s.tabs.find( + (t) => t.sessionId === session.id && t.view === view && t.status !== 'error' && !t.paneOf, + ) + if (existing) return { activeTabId: existing.id, focusedPaneId: null } const tab: Tab = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/store/index.ts` around lines 246 - 259, The deduplication check in the `openTab` function only compares sessionId but not the derived view type, which can cause the wrong tab view to be activated. Move the calculation of the view type (the ternary expression that determines whether it should be 'k8s', 'sftp', 'database', 'rdp', 'redis', or 'terminal') before the existing tab lookup, then update the find condition to also check that t.view equals the calculated view value, ensuring the correct tab type is found or created for each session.
🧹 Nitpick comments (1)
src/renderer/src/components/MainContent.tsx (1)
235-243: ⚡ Quick winConsider pausing RDP rendering when tab is inactive.
This file intentionally keeps tabs mounted; for
RdpViewthat can mean background frame decoding/painting while hidden (display: none). Passing anisActiveprop and short-circuiting paint when inactive would reduce idle CPU.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/MainContent.tsx` around lines 235 - 243, The RdpView component continues rendering and decoding frames even when its tab is inactive (hidden with display: none), consuming unnecessary CPU resources. Add an isActive prop to RdpView that indicates whether the tab is currently the active tab, passing the active status based on the current tab context. Then modify the RdpView component to short-circuit or pause frame decoding and painting operations when isActive is false to reduce idle CPU usage on background tabs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 40-43: The actions/cache step in the Cache static FreeRDP task on
line 42 uses a mutable version tag `@v4` which poses a security risk in a release
pipeline handling sensitive secrets. Replace the mutable tag `@v4` with a full
commit SHA to ensure an immutable, predictable version of the action is always
used. Look up the latest commit SHA for actions/cache and update the uses field
to reference it directly instead of the version tag.
In `@native/rdp-spike/build-freerdp-static.sh`:
- Around line 22-24: The conditional check that tests for the existence of
"$SRC/.git" only performs a git clone during the initial run. On subsequent
script executions, if the repository already exists but FREERDP_TAG has changed,
the clone step is skipped entirely, causing the script to use stale source code.
To fix this, add logic after the conditional block that fetches the latest
changes and checks out the specified FREERDP_TAG branch, ensuring the requested
tag is honored on both initial clones and subsequent runs when the repository
already exists.
In `@native/rdp-spike/diag-frames.mjs`:
- Around line 39-51: The offset advancement using off += 16 + dataLen does not
validate whether the buffer contains enough remaining bytes, allowing truncated
or corrupt frames to cause the offset to overshoot EOF undetected. Before
advancing the offset in the main loop (at the line with off += 16 + dataLen),
add a validation check to ensure that off + 16 + dataLen does not exceed
buf.length. If the check fails, log an error or warning indicating truncation
was detected and exit the loop early, preventing false success reports on the
final validation that checks if off equals buf.length.
In `@native/rdp-spike/sidecar.c`:
- Around line 191-199: The password is being passed as a command-line argument
(argv[4]) which exposes credentials to local process listing tools and crash
diagnostics. Remove the parsing of argv[4] into the 'pass' variable and update
the usage message to not include [password] as a command-line parameter.
Instead, implement secure password input by reading the password from stdin
using a function that doesn't echo the input to the terminal, or alternatively
read it from an environment variable. Make sure the argument count validation in
the condition (argc >= 6 for width/height) is adjusted accordingly to account
for the removal of the password argument.
- Around line 79-113: The emit_frame function calculates dataLen as rowBytes
multiplied by h without checking for integer overflow or validating against the
64 MiB frame size limit. Add bounds validation after calculating dataLen to
ensure it does not exceed the maximum allowed frame size and does not overflow
when cast to UINT32 for the header write operation. Check for overflow in the
rowBytes * h multiplication by verifying the result fits within size_t bounds,
then validate the final dataLen against a reasonable upper bound (64 MiB) before
proceeding with the realloc call and header construction at the end of
emit_frame.
- Around line 214-220: The freerdp_settings_set_bool call that sets
FreeRDP_IgnoreCertificate to TRUE disables server certificate validation,
creating a security vulnerability to man-in-the-middle attacks. Remove or change
this line to not disable certificate validation by default. Instead, either set
FreeRDP_IgnoreCertificate to FALSE to enable certificate validation, or make
this setting configurable through an external parameter so certificate
validation is enabled by default and can only be explicitly disabled by the user
if absolutely necessary.
In `@src/main/ipc/rdp.ts`:
- Around line 116-124: The code currently only validates that dataLen is within
MAX_FRAME_BYTES but does not validate the width and height values or check
consistency between the data length and the frame dimensions. After reading
width, height, and dataLen from the buffer, add additional validation checks to
ensure width and height are non-zero and reasonable (not exceeding maximum
display dimensions), and verify that dataLen equals width * height * 4. If any
validation fails, treat it as a false positive header match by calling
resyncToMagic and continuing to the next iteration, similar to the existing
MAX_FRAME_BYTES check.
- Around line 159-163: Remove the password from the arguments array passed to
the spawn call (which currently includes host, port, username, password, width,
and height). Instead, write the password to the stdin stream of the spawned
process after it is created. Access the stdin pipe via the proc object's stdin
property and write the password there to keep credentials out of process
arguments where they could be exposed via local process inspection.
In `@src/renderer/src/components/ConnectionManager/AddConnectionModal.tsx`:
- Around line 930-961: The RDP password fields shown when type === 'rdp' may not
be saved because the form's authType field could still be set to 'key' from a
previous connection type selection. When the connection type is switched to
'rdp', ensure that authType is automatically updated to 'password' at the same
time. Check the onChange handler for the type field (where 'rdp' is being
selected) and ensure it also updates authType to 'password' so that the save
logic includes the RDP password in the payload.
In `@src/renderer/src/components/ConnectionManager/ConnectionManager.tsx`:
- Around line 70-74: The filter menu in the ConnectionManager component is
missing options for the newly added redis and rdp connection types. Locate the
filter menu implementation (likely a list or array that defines available filter
options for connection types) and add redis and rdp as selectable filter options
alongside the existing types like ssh, sftp, database, and kubernetes. Ensure
the filter menu uses the same type identifiers (redis, rdp) that are defined in
the color and typeLabel mappings so filtering works correctly.
In `@src/renderer/src/components/Dashboard/Dashboard.tsx`:
- Around line 406-408: The onOpenRdp callback on line 406 is being exposed for
all sessions on macOS without verifying the session type, allowing non-RDP
entries to trigger RDP tab opening with mismatched data. Modify the condition
that assigns onOpenRdp to additionally check that the ctxMenu.session is
specifically an RDP session type before allowing the openRdpTab function to be
assigned, ensuring the callback is only available when both the platform is
darwin AND the session is an RDP connection.
In `@src/renderer/src/components/RDP/RdpView.tsx`:
- Around line 57-82: The RDP session can leak if unmount occurs while the
window.api.rdp.connect() call is still pending, because rdpId is assigned after
the cleanup function runs. To fix this, store a reference to the connect promise
returned by window.api.rdp.connect() in the async IIFE, and in the cleanup
function (the return statement), ensure that if the connect promise is still
pending when disposal occurs, you properly disconnect the session once it
resolves. This prevents the race condition where rdpId gets assigned after
cleanup has already checked for it. Track the connecting state and handle the
resolution of the connect promise in the cleanup handler to guarantee
disconnection regardless of timing.
In `@src/renderer/src/components/Sidebar/Sidebar.tsx`:
- Around line 248-260: The RDP section in the DraggableSection component is
rendered without verifying platform support, which conflicts with the macOS-only
RDP rollout. Add a platform check to the conditional rendering logic alongside
the existing rdpSessions.length > 0 check so that the RDP section only appears
on supported platforms. Additionally, ensure that the onOpen handler is only
passed when the platform supports RDP affordances to prevent surfacing invalid
actions for incompatible session types.
---
Outside diff comments:
In `@src/renderer/src/store/index.ts`:
- Around line 246-259: The deduplication check in the `openTab` function only
compares sessionId but not the derived view type, which can cause the wrong tab
view to be activated. Move the calculation of the view type (the ternary
expression that determines whether it should be 'k8s', 'sftp', 'database',
'rdp', 'redis', or 'terminal') before the existing tab lookup, then update the
find condition to also check that t.view equals the calculated view value,
ensuring the correct tab type is found or created for each session.
---
Nitpick comments:
In `@src/renderer/src/components/MainContent.tsx`:
- Around line 235-243: The RdpView component continues rendering and decoding
frames even when its tab is inactive (hidden with display: none), consuming
unnecessary CPU resources. Add an isActive prop to RdpView that indicates
whether the tab is currently the active tab, passing the active status based on
the current tab context. Then modify the RdpView component to short-circuit or
pause frame decoding and painting operations when isActive is false to reduce
idle CPU usage on background tabs.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d4b8538-43e5-40d9-8033-7df0e2e4a887
📒 Files selected for processing (26)
.github/workflows/release.yml.gitignoreelectron-builder.ymlnative/rdp-spike/HANDOFF.mdnative/rdp-spike/Makefilenative/rdp-spike/README.mdnative/rdp-spike/build-freerdp-static.shnative/rdp-spike/diag-frames.mjsnative/rdp-spike/sidecar.cnative/rdp-spike/spike.cscripts/verify-sidecar.jssrc/main/index.tssrc/main/ipc/rdp.tssrc/preload/index.d.tssrc/preload/index.tssrc/renderer/src/components/ConnectionManager/AddConnectionModal.tsxsrc/renderer/src/components/ConnectionManager/ConnectionManager.tsxsrc/renderer/src/components/Dashboard/Dashboard.tsxsrc/renderer/src/components/Dashboard/ServerViews.tsxsrc/renderer/src/components/MainContent.tsxsrc/renderer/src/components/RDP/RdpView.tsxsrc/renderer/src/components/ServerContextMenu.tsxsrc/renderer/src/components/Sidebar/Sidebar.tsxsrc/renderer/src/components/TabBar/TabBar.tsxsrc/renderer/src/lib/colors.tssrc/renderer/src/store/index.ts
Comment on lines
+40
to
+43
| - name: Cache static FreeRDP | ||
| if: runner.os == 'macOS' | ||
| uses: actions/cache@v4 | ||
| with: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd .github/workflows && head -50 release.yml | cat -nRepository: Meanski/noxed
Length of output: 1796
🏁 Script executed:
cat -n .github/workflows/release.yml | tail -n +40Repository: Meanski/noxed
Length of output: 6518
Pin actions/cache to a full commit SHA.
Line 42 uses a mutable tag (@v4) in a release pipeline that handles signing/notarization secrets. Pin to an immutable commit SHA.
Suggested fix
- name: Cache static FreeRDP
if: runner.os == 'macOS'
- uses: actions/cache@v4
+ uses: actions/cache@<FULL_LENGTH_COMMIT_SHA>🧰 Tools
🪛 zizmor (1.25.2)
[error] 42-42: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 42-42: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 40 - 43, The actions/cache step
in the Cache static FreeRDP task on line 42 uses a mutable version tag `@v4` which
poses a security risk in a release pipeline handling sensitive secrets. Replace
the mutable tag `@v4` with a full commit SHA to ensure an immutable, predictable
version of the action is always used. Look up the latest commit SHA for
actions/cache and update the uses field to reference it directly instead of the
version tag.
Source: Linters/SAST tools
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 10 file(s) based on 13 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken:
Lines 39–45 # arch. Cached so FreeRDP isn't recompiled on every release.
- name: Cache static FreeRDP
if: runner.os == 'macOS'
- uses: actions/cache@v4
+ uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684
with:
path: |
native/rdp-spike/.freerdp-src |
Fixed 10 file(s) based on 13 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jul 10, 2026
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
1 participant
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.
Adds RDP as a first-class connection type, rendering a live Windows desktop in-app via a bundled FreeRDP sidecar. macOS is production-ready (self-contained, signed/notarized in CI); Windows/Linux sidecars are a tracked fast-follow.
What's included
native/rdp-spike/sidecar.c) — connects to the host and streams composed RGBA frames over stdout (length-prefixedNXF1blobs);ipc/rdp.tsparses them into a<canvas>(RdpView). Output-only for now.Production packaging (macOS)
build-freerdp-static.shbuilds a minimal static FreeRDP (no X11/ffmpeg/H264/audio/redirection channels) → sidecar links only system libs (zero Homebrew refs, ~7 MB)extraResourcesbundles it into the app;afterPackguard (scripts/verify-sidecar.js) fails the build if it ever relinks non-redistributable libsrelease.ymlbuilds + caches the static sidecar on the macOS runnerVerified
npm run packbundles a self-contained, runnable sidecarRemaining (fast-follow)
extraResources+ un-gate the UI🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes