[client, management] Add embedded VNC server - #6170
Open
lixmal wants to merge 164 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds an embedded VNC server with Noise authentication, approval prompts, platform-specific capture and input backends, VNC policy propagation, daemon and UI integration, WASM proxy support, status reporting, and management API updates. ChangesEmbedded VNC server and protocol
Estimated code review effort: 5 (Critical) | ~180 minutes Merge Risk: 🟠 High · up to This PR adds privileged remote screen viewing and input injection, but a failed temporary-access request can still leave remote access active, and build gates plus several runtime correctness issues remain unresolved; it is not merge-ready until these issues are fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the feature, records issue Full details: Docstring CoverageExplanation Docstring coverage is 55.97% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 134 functions across 59 files. (125 skipped: 18 unsupported, 107 over the file limit.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/types/account.go (1)
983-1014:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope Netbird VNC rules to the internal VNC port.
PolicyRuleProtocolNetbirdVNCis converted to plain TCP here, but no port is injected. For the common VNC case with no explicitPorts/PortRanges, this emits a portless TCP firewall rule, so a VNC-only policy ends up authorizing every TCP service on the destination peer instead of just the embedded VNC listener.🔧 Suggested fix
- protocol := rule.Protocol + effectiveRule := rule + if rule.Protocol == PolicyRuleProtocolNetbirdVNC && len(rule.Ports) == 0 && len(rule.PortRanges) == 0 { + ruleCopy := *rule + ruleCopy.Ports = []string{strconv.Itoa(vncInternalPort)} + effectiveRule = &ruleCopy + } + + protocol := effectiveRule.Protocol if protocol == PolicyRuleProtocolNetbirdSSH || protocol == PolicyRuleProtocolNetbirdVNC { protocol = PolicyRuleProtocolTCP } @@ - ruleID := rule.ID + fr.PeerIP + strconv.Itoa(direction) + - fr.Protocol + fr.Action + strings.Join(rule.Ports, ",") + ruleID := effectiveRule.ID + fr.PeerIP + strconv.Itoa(direction) + + fr.Protocol + fr.Action + strings.Join(effectiveRule.Ports, ",") @@ - if len(rule.Ports) == 0 && len(rule.PortRanges) == 0 { + if len(effectiveRule.Ports) == 0 && len(effectiveRule.PortRanges) == 0 { rules = append(rules, &fr) } else { - rules = append(rules, expandPortsAndRanges(fr, rule, targetPeer)...) + rules = append(rules, expandPortsAndRanges(fr, effectiveRule, targetPeer)...) } - rules = appendIPv6FirewallRule(rules, rulesExists, peer, targetPeer, rule, firewallRuleContext{ + rules = appendIPv6FirewallRule(rules, rulesExists, peer, targetPeer, effectiveRule, firewallRuleContext{ direction: direction, dirStr: strconv.Itoa(direction), protocolStr: string(protocol), actionStr: string(rule.Action), - portsJoined: strings.Join(rule.Ports, ","), + portsJoined: strings.Join(effectiveRule.Ports, ","), })🤖 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 `@management/server/types/account.go` around lines 983 - 1014, The code converts PolicyRuleProtocolNetbirdVNC to TCP but doesn't inject the VNC port, so a VNC-only rule with no Ports/PortRanges becomes an unscoped TCP allow; update the block handling protocol conversion to detect when protocol == PolicyRuleProtocolNetbirdVNC and both rule.Ports and rule.PortRanges are empty and then inject the internal VNC port (e.g., set rule.Ports = []string{"5900"}) before building FirewallRule/fr, generating ruleID, and calling expandPortsAndRanges and appendIPv6FirewallRule; ensure you reference PolicyRuleProtocolNetbirdVNC, rule.Ports, rule.PortRanges, expandPortsAndRanges, FirewallRule, and appendIPv6FirewallRule when making the change.
🟡 Minor comments (5)
client/vnc/server/capture_fb_freebsd.go-140-143 (1)
140-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFile descriptor 0 is valid; use a different sentinel or flag.
fd != 0is an incorrect check since 0 is a valid file descriptor (stdin). Although unlikely in practice (stdin is typically open), relying on this is fragile. Consider using-1as the "closed" sentinel or a separateclosed boolfield.🛡️ Proposed fix
Initialize
fdto-1in the struct or after close:func (c *FBCapturer) Close() { c.closeOnce.Do(func() { if c.mmap != nil { _ = unix.Munmap(c.mmap) c.mmap = nil } - if c.fd != 0 { + if c.fd >= 0 { _ = unix.Close(c.fd) - c.fd = 0 + c.fd = -1 } }) }🤖 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 `@client/vnc/server/capture_fb_freebsd.go` around lines 140 - 143, The code uses 0 as the "closed" sentinel for the file descriptor (c.fd), which is wrong because 0 is a valid FD; change the sentinel to -1 (or add a separate closed bool) by initializing c.fd to -1 in the struct/constructor and updating the close logic in capture_fb_freebsd.go to check for c.fd != -1 before calling unix.Close, then set c.fd = -1 after closing; ensure all other places that open or test c.fd use the new sentinel (c.fd == -1) consistently.client/vnc/server/input_uinput_unix.go-258-263 (1)
258-263:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explanatory comment for intentionally empty
SetClipboard.Static analysis flags the empty function body. The existing doc comment explains the rationale, but adding an inline comment satisfies the linter.
📝 Proposed fix
// SetClipboard is a no-op on the framebuffer console: there is no system // clipboard daemon. Use TypeText (Paste button) to deliver host text. -func (u *UInputInjector) SetClipboard(_ string) {} +func (u *UInputInjector) SetClipboard(_ string) { + // No system clipboard available on framebuffer console; intentionally empty. +}🤖 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 `@client/vnc/server/input_uinput_unix.go` around lines 258 - 263, The SetClipboard method currently has an empty body which static analysis flags; modify the UInputInjector.SetClipboard implementation to include a concise inline comment (e.g., "no-op: no system clipboard on framebuffer console") inside the function body to document the intentional no-op, leaving the GetClipboard unchanged; reference the UInputInjector.SetClipboard method to locate and update the function body.client/vnc/server/rfb.go-231-231 (1)
231-231:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIgnored error from
des.NewCipher.While DES key setup with an 8-byte key won't fail in practice, ignoring the error is not idiomatic Go.
🛡️ Proposed fix
- block, _ := des.NewCipher(key) + block, err := des.NewCipher(key) + if err != nil { + return nil // Should never happen with 8-byte key + }🤖 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 `@client/vnc/server/rfb.go` at line 231, Replace the ignored error usage of des.NewCipher by capturing its error (use block, err := des.NewCipher(key)), check if err != nil, and propagate or handle it instead of discarding: for example return or wrap the error (fmt.Errorf("des.NewCipher: %w", err)) from the surrounding function or log and return a suitable error; update the code around the des.NewCipher call (the block variable and its initializer) to perform this check so failures are not silently ignored.client/vnc/server/stubs.go-28-40 (1)
28-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAnnotate intentional no-op bodies to satisfy static analysis.
These empty stubs are intentional, but the current style is failing Sonar checks.
Proposed patch
-func (s *StubInputInjector) InjectKey(_ uint32, _ bool) {} +func (s *StubInputInjector) InjectKey(_ uint32, _ bool) { + // no-op: input injection unsupported on this platform +} @@ -func (s *StubInputInjector) SetClipboard(_ string) {} +func (s *StubInputInjector) SetClipboard(_ string) { + // no-op: clipboard injection unsupported on this platform +} @@ -func (s *StubInputInjector) TypeText(_ string) {} +func (s *StubInputInjector) TypeText(_ string) { + // no-op: type-text unsupported on this platform +}🤖 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 `@client/vnc/server/stubs.go` around lines 28 - 40, These stub methods (InjectKey, InjectPointer, SetClipboard, GetClipboard, TypeText) are intentionally empty but need explicit annotations for static analysis; update each method body to include a short comment like "// intentionally no-op for unsupported platforms" and reference the receiver to avoid unused symbol warnings (for example add "_ = s" in the body), and for GetClipboard return keep the empty string but add the same comment above the return to make the intent explicit to Sonar.shared/auth/jwt/token_age.go-17-17 (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix doc typo to satisfy spelling checks.
Line 17 should use “unparsable”.
🤖 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 `@shared/auth/jwt/token_age.go` at line 17, The doc comment for the maxAge symbol has a spelling error: replace "unparseable" with the correct word "unparsable" in the comment that describes maxAge's behavior so the comment reads "Returns an error if the claims are unparsable" (update the comment near the maxAge declaration).
🧹 Nitpick comments (17)
client/vnc/server/agent_windows.go (1)
564-644: ⚖️ Poor tradeoffConsider extracting helpers to reduce cognitive complexity.
Static analysis flags this method at complexity 32 (limit 20). The main loop handles session detection, agent lifecycle, backoff, and cleanup. While the logic is cohesive, extracting sub-routines like
checkAgentHealth()andtrySpawnAgent()would improve readability.🤖 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 `@client/vnc/server/agent_windows.go` around lines 564 - 644, The run method in sessionManager is too complex; extract the main responsibilities into small helper methods to reduce cognitive complexity: create detectSessionChange(m *sessionManager) to handle session ID comparison, logging and calling m.killAgent()/m.sessionID update; create checkAgentHealth(m *sessionManager) to encapsulate the agent exit detection logic (GetExitCodeProcess, lifetime/backoff calculation, updating m.spawnFailures, m.nextSpawnAt, closing handle and zeroing m.agentProc); and create trySpawnAgent(m *sessionManager) to contain the spawn logic (reapOrphanOnPort on first spawn, generateAuthToken, call spawnAgentInSession, handle ERROR_PRIVILEGE_NOT_HELD, set m.agentProc/m.agentStartedAt/m.everSpawned or clear m.authToken on error). Move only the corresponding blocks out of run and call these helpers from run while preserving existing m.mu locking semantics and behavior.client/vnc/server/capture_x11_shm_stub.go (1)
22-22: 💤 Low valueAdd comment explaining the intentional no-op.
Static analysis flagged this empty function. Adding a brief comment documents the intentional behavior for the stub.
📝 Suggested comment
-func (c *X11Capturer) closeSHM() {} +func (c *X11Capturer) closeSHM() { + // No-op: SHM is never initialized on FreeBSD. +}🤖 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 `@client/vnc/server/capture_x11_shm_stub.go` at line 22, The function X11Capturer.closeSHM is intentionally a no-op but currently lacks any comment explaining that, which tripped static analysis; add a brief explanatory comment above the closeSHM method (or inside its body) stating that this stub intentionally does nothing (e.g., "no-op for platforms without SHM cleanup" or "placeholder: SHM cleanup not required/handled elsewhere") so readers and linters understand the behavior while keeping the empty implementation.client/vnc/server/capture_windows.go (1)
397-517: ⚖️ Poor tradeoffConsider splitting
worker()to reduce complexity.Static analysis flagged this function for high cognitive complexity (35 vs 20 allowed) and line count (103 vs 100 allowed). The nested
prepCapturerandcloseCapturerclosures help, but extracting the main select loop body into a helper method (e.g.,handleCaptureRequest) would improve readability and testability.This is a suggested improvement rather than a blocking issue—the current structure is coherent and handles the inherent complexity of Windows desktop transitions.
🤖 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 `@client/vnc/server/capture_windows.go` around lines 397 - 517, The worker() function is over the complexity/length threshold; extract the select-loop request handling into a new helper (e.g., handleCaptureRequest) to reduce cognitive load and make it testable: move the logic that checks clients.Load(), handles <-c.done and case req := <-c.reqCh into handleCaptureRequest which should call existing helpers prepCapturer(), closeCapturer(), and use createCapturer()/fc.capture() and nextInitRetry/cap state via parameters or receiver access; keep setupInteractiveWindowStation(), createCapturer(), prepCapturer(), closeCapturer(), and defer closeCapturer() intact and update worker() to call the new helper from the loop.client/vnc/server/rfb.go (2)
813-813: ⚡ Quick winParameter
capshadows the built-incapfunction.Consider renaming to
limitormaxColorsfor clarity.♻️ Proposed fix
-func sampledColorCountInto(seen map[uint32]struct{}, img *image.RGBA, x, y, w, h, cap int) int { +func sampledColorCountInto(seen map[uint32]struct{}, img *image.RGBA, x, y, w, h, limit int) int { clear(seen) stride := img.Stride - step := max((w*h)/(cap*4), 1) + step := max((w*h)/(limit*4), 1) var idx int for row := 0; row < h; row++ { p := (y+row)*stride + x*4 for col := 0; col < w; col++ { if idx%step == 0 { px := *(*uint32)(unsafe.Pointer(&img.Pix[p+col*4])) seen[px&0x00ffffff] = struct{}{} - if len(seen) > cap { + if len(seen) > limit { return len(seen) } } idx++ } } return len(seen) }🤖 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 `@client/vnc/server/rfb.go` at line 813, The parameter name `cap` in sampledColorCountInto shadows Go's built-in cap function; rename it to a descriptive name like `limit` or `maxColors` in the function signature (sampledColorCountInto) and update every call site to use the new name, plus any internal references within that function, so no references rely on the built-in `cap` and the intent (maximum colors to count) is clearer.
398-415: 💤 Low value
tileIsUniformassumes consistent pixel byte order across platforms.Using
unsafe.Pointerto cast[]bytetouint32assumes the pixel layout is consistent. On big-endian systems, this could give incorrect comparisons. Sinceimage.RGBAstores pixels in a defined order (R, G, B, A at consecutive bytes), and you're comparing for equality, this should be fine as long as you're consistent—but document the assumption.🤖 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 `@client/vnc/server/rfb.go` around lines 398 - 415, tileIsUniform currently casts img.Pix bytes to uint32 via unsafe.Pointer which is platform-endian dependent; change the comparison to use a platform-independent byte-wise or defined-endian comparison instead: read the first pixel from img.Pix at index base as four consecutive bytes (img.Pix[base:base+4]) and compare subsequent pixels by comparing their four-byte slices (or by using encoding/binary with a fixed endianness) rather than *(*uint32)(unsafe.Pointer(...)); update uses in tileIsUniform to avoid unsafe.Pointer and ensure correct results on big-endian systems.client/vnc/server/input_x11.go (1)
269-275: ⚡ Quick win
clipboardEnvdoesn't inherit the full environment.Creating a minimal env with only
DISPLAYandXAUTHORITYmay cause issues with clipboard tools that depend on other environment variables (e.g., locale settings,PATHfor helper processes).♻️ Proposed fix: inherit full environment
func (x *X11InputInjector) clipboardEnv() []string { - env := []string{"DISPLAY=" + x.display} + env := os.Environ() + // Ensure DISPLAY is set correctly (override if present) + found := false + for i, e := range env { + if strings.HasPrefix(e, "DISPLAY=") { + env[i] = "DISPLAY=" + x.display + found = true + break + } + } + if !found { + env = append(env, "DISPLAY="+x.display) + } if auth := os.Getenv("XAUTHORITY"); auth != "" { - env = append(env, "XAUTHORITY="+auth) + // Already in env from os.Environ() } return env }Alternatively, keep the minimal approach but document the limitation.
🤖 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 `@client/vnc/server/input_x11.go` around lines 269 - 275, The clipboardEnv function creates a minimal environment which can break clipboard helpers; change X11InputInjector.clipboardEnv to start from the full environment (os.Environ()), then ensure/override "DISPLAY" and, if present, "XAUTHORITY" in that slice so existing vars like PATH and locale are preserved; update the function to merge/replace those keys rather than returning a two-entry slice.client/vnc/server/input_darwin.go (2)
385-428: 💤 Low value
TypeTextcognitive complexity slightly exceeds threshold.Static analysis flags complexity 22 vs limit 20. The shift key handling could be extracted to a small helper, but the current form is readable.
🤖 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 `@client/vnc/server/input_darwin.go` around lines 385 - 428, TypeText's cognitive complexity exceeds the threshold; extract the repeated shift-press/release block into a small helper to simplify logic: create a helper function (e.g., sendKeyWithOptionalShift or pressKeyWithShift) that accepts src, keycode, shift flag and calls cgEventCreateKeyboardEvent/ cgEventPost/ cfRelease for shift down, key down, key up, shift up as needed, then replace the inline shift handling in TypeText with calls to that helper while keeping keysymForASCIIRune and keysymToMacKeycode usage intact.
273-338: 💤 Low valueConsider extracting helper methods to reduce cognitive complexity.
Static analysis flags this method at complexity 25 (limit 20). The button handling and scroll logic can be extracted.
🤖 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 `@client/vnc/server/input_darwin.go` around lines 273 - 338, The InjectPointer method is over-complex; extract the movement, button transition handling, and scroll handling into small helpers to reduce cognitive complexity: create e.g. computeLogicalCoords(px, py, serverW, serverH) to return (x,y) and use it where x/y are computed, move the conditional dispatch into handlePointerMovement(src, leftDown, rightDown, x, y) that calls postMouse with kCGEventLeftMouseDragged/RightMouseDragged/MouseMoved, implement handleButtonTransitions(src, leftDown, rightDown, middleDown, wasLeft, wasRight, wasMiddle, x, y) to emit Left/Right/Other Mouse Down/Up events by calling postMouse, and extract scroll handling into handleScroll(src, scrollUp, scrollDown) to call postScroll; then replace the corresponding blocks in InjectPointer with calls to these helpers and update lastButtons at the end.client/vnc/server/input_uinput_unix.go (2)
401-401: ⚡ Quick winRemove unused parameter from
keymapByKeysym.The
_ []uint16parameter is never used. Either remove it or use it to filter the keymap.♻️ Proposed fix
-func keymapByKeysym(_ []uint16) map[uint32]uint16 { +func keymapByKeysym() map[uint32]uint16 {And update the call site at line 150:
- keysymToKey: keymapByKeysym(keymap), + keysymToKey: keymapByKeysym(),🤖 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 `@client/vnc/server/input_uinput_unix.go` at line 401, The function keymapByKeysym currently declares an unused parameter (_ []uint16); remove that parameter from the function signature to avoid unused-parameter linting, and update all call sites that pass that argument to call keymapByKeysym() with no arguments; alternatively, if the parameter was meant to filter results, implement filtering logic inside keymapByKeysym using the provided []uint16 and keep callers unchanged—locate keymapByKeysym and its callers to apply one of these two fixes (removing the parameter and adjusting callers, or using the parameter to filter the returned map).
456-548: ⚖️ Poor tradeoff
keyForRuneduplicates letter/digit mappings defined inkeymapByKeysym.Both functions define the same letter-to-keycode mappings. Consider extracting a shared
letterToKeycodemap to avoid divergence.🤖 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 `@client/vnc/server/input_uinput_unix.go` around lines 456 - 548, keyForRune duplicates the letter/digit keycode mappings already defined in keymapByKeysym; extract a shared map (e.g., letterToKeycode) containing the rune->keycode entries and use it from both keyForRune and keymapByKeysym (and reuse the nums slice for digits), updating keyForRune to lookups against that shared map and fall back to existing switch cases for punctuation/shifted variants so the mappings stay centralized and avoid divergence.client/vnc/server/server.go (2)
444-480: ⚖️ Poor tradeoffConsider extracting ModeSession handling into a helper method.
Static analysis suggests reducing the case clause from 23 lines to at most 10. This improves readability and testability.
🤖 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 `@client/vnc/server/server.go` around lines 444 - 480, Extract the ModeSession branch into a helper on the server type that performs the validation, vmgr lookup/create, logging and returns the capturer, injector, updated connLog and a cleanup function (or an error to trigger rejectConnection). Specifically, move the code that checks s.vmgr, header.username, calls s.vmgr.GetOrCreate, obtains vs.Capturer()/vs.Injector(), calls vs.ClientConnect(), sets up the deferred vs.ClientDisconnect(), and enriches connLog into a helper (e.g., func (s *Server) initSessionResources(header Header, conn net.Conn, connLog log.Entry) (capturer Capturer, injector Injector, connLog log.Entry, cleanup func(), err error)), then replace the ModeSession case with a call to that helper, handle error by calling rejectConnection/connLog.Warn as before, and defer the returned cleanup to preserve disconnect behavior; keep all original symbols (s.vmgr, rejectConnection, codeMessage, RejectCode*, GetOrCreate, vs.ClientConnect/ClientDisconnect, capturer/injector) and the same log messages.
316-331: 💤 Low value
acceptLoopdoesn't cleanly exit when context is cancelled.The
Accept()call blocks until a new connection arrives or the listener is closed. Whens.ctxis cancelled, the loop only exits ifAcceptreturns an error. If the listener is not closed, the goroutine may hang.The
Stop()method does close the listener, so this is safe as long asStop()is always called. Consider documenting this dependency or adding a comment.🤖 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 `@client/vnc/server/server.go` around lines 316 - 331, acceptLoop can hang if Accept() is blocked and s.ctx is cancelled, so ensure the listener gets closed when the context is done; at the start of Server.acceptLoop() spawn a short goroutine that waits on s.ctx.Done() and calls s.listener.Close() (and add a comment that Stop() also closes the listener) so Accept() unblocks and the loop exits cleanly; reference acceptLoop, s.listener.Accept, s.ctx.Done and Stop to locate where to add the goroutine and comment.client/vnc/server/input_windows.go (1)
173-179: 💤 Low valueSilent drop of pointer events when queue is full.
The non-blocking send silently drops events. While the comment explains this is intentional for mouse coalescing, consider logging at trace level for debugging purposes.
🤖 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 `@client/vnc/server/input_windows.go` around lines 173 - 179, InjectPointer currently drops inputCmd silently when w.ch is full; update WindowsInputInjector.InjectPointer to keep the non-blocking behavior but add a trace-level log in the default branch that records the dropped inputCmd (or at least buttonMask,x,y and serverW,serverH) to aid debugging. Locate InjectPointer and the channel w.ch and emit a trace/debug message (using the existing logger on the WindowsInputInjector instance, e.g., w.logger or the package tracing/log facility present in the repo) inside the select's default branch so behavior is unchanged but drops are observable.client/vnc/server/server_windows.go (1)
85-137: ⚡ Quick winSplit
startSASListenerinto smaller helpers to clear complexity gate.This function is currently tripping the cognitive complexity check; extracting setup and wait-loop pieces will make it pass and easier to maintain.
Refactor shape
-func startSASListener(ctx context.Context) { - ... -} +func startSASListener(ctx context.Context) { + ev, err := prepareSASEvent() + if err != nil { + log.Warnf("SAS listener setup: %v", err) + return + } + log.Info("SAS listener ready (Session 0)") + go runSASLoop(ctx, ev) +}🤖 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 `@client/vnc/server/server_windows.go` around lines 85 - 137, The startSASListener function is too complex and should be split into smaller helpers: extract the setup portion (procSendSAS.Find check, enableSoftwareSAS call, UTF16PtrFromString(sasEventName), sasSecurityAttributes(), and windows.CreateEvent) into a helper like createSASEvent or initSASEvent that returns (windows.Handle, error), and extract the goroutine wait/dispatch loop into a helper like runSASListenerLoop(ctx, ev) that handles CloseHandle(ev), the recover defer, the WaitForSingleObject polling and the procSendSAS.Call logic; keep all existing log messages and error handling, ensure createSASEvent returns nil/error on failure so startSASListener simply calls it, and ensure runSASListenerLoop receives the event handle and context so startSASListener just wires them together.client/vnc/server/server_test.go (1)
46-48: ⚡ Quick winAdd socket deadlines to avoid hanging test runs.
These tests can block forever if the handshake stalls. Set a short deadline right after
net.Dialin each case (or in a helper) to keep CI deterministic.Proposed patch
import ( "encoding/binary" "image" "io" "net" "net/netip" "testing" + "time" @@ conn, err := net.Dial("tcp", addr.String()) require.NoError(t, err) + require.NoError(t, conn.SetDeadline(time.Now().Add(5*time.Second))) defer conn.Close()Also applies to: 84-86, 118-120
🤖 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 `@client/vnc/server/server_test.go` around lines 46 - 48, The tests call net.Dial and then use conn but do not set any socket deadlines, so a stalled handshake can hang CI; after each net.Dial (e.g., where conn is returned in server_test.go) call conn.SetDeadline(time.Now().Add(2*time.Second)) (or a similar short timeout) immediately to bound read/write waits and fail fast; apply this change to the other net.Dial occurrences referenced (the conn in the blocks around lines 84-86 and 118-120) or centralize into a helper that sets the deadline on the returned conn.management/server/types/networkmap_components.go (2)
183-255: 🏗️ Heavy liftRefactor
getPeerConnectionResourcesinto smaller phases to reduce complexity risk.Line 183 currently mixes peer resolution, direction handling, resource generation, and auth collection in one path. Splitting this into helpers will make policy behavior safer to evolve and should address the current complexity gate failure.
♻️ Suggested decomposition sketch
func (c *NetworkMapComponents) getPeerConnectionResources(targetPeerID string) peerConnectionResult { + // 1) resolve rule peers + membership + // 2) generate firewall/peer resources + // 3) collect protocol-specific auth state } + +// resolveRulePeers(...) +// applyRuleDirections(...) +// applyProtocolAuthorization(...)🤖 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 `@management/server/types/networkmap_components.go` around lines 183 - 255, The getPeerConnectionResources function is doing peer resolution, direction handling/resource generation and auth collection all inline which increases complexity; refactor it into clear phases and helper methods: (1) extract peer-resolution logic into helpers that call getPeerFromResource and getAllPeersFromGroups to produce sourcePeers/destinationPeers and booleans (peerInSources/peerInDestinations), (2) move directional resource generation into a helper that invokes connResourcesGenerator(rule) and generateResources(rule, peers, direction) for IN/OUT and bidirectional cases, (3) separate auth collection into a helper that handles collectAuthorizedUsers, vncAuthorizedUsers, legacy SSH via policyRuleImpliesLegacySSH and getAllowedUserIDs and sets sshEnabled/authorizedUsers, and (4) keep final accumulation via the existing getAccumulatedResources call; create small functions named like resolveRulePeers, applyRuleDirectionResources, and collectRuleAuth to replace the inline blocks in getPeerConnectionResources.
259-289: ⚡ Quick winExtract branch handlers from
collectAuthorizedUsersfor clarity and testability.Line 259 is just over the complexity threshold; extracting per-mode handlers (
AuthorizedGroups,AuthorizedUser, default) should simplify this without behavior change.🤖 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 `@management/server/types/networkmap_components.go` around lines 259 - 289, The collectAuthorizedUsers function is doing three distinct modes (AuthorizedGroups, AuthorizedUser, default) which pushes complexity; refactor by extracting each branch into small helper methods (e.g., collectAuthorizedGroups(rule *PolicyRule, target map[string]map[string]struct{}), collectAuthorizedUser(rule *PolicyRule, target map[string]map[string]struct{}), and collectAuthorizedDefault(target map[string]map[string]struct{})) and update collectAuthorizedUsers to simply switch on the same conditions and call the appropriate helper; ensure the helpers preserve existing behavior: use c.GroupIDToUserIDs when handling AuthorizedGroups (including populating localUsers with auth.Wildcard when empty and mapping userIDs into target[localUser]), set target[auth.Wildcard][rule.AuthorizedUser] in the AuthorizedUser handler, and return c.getAllowedUserIDs() into target[auth.Wildcard] in the default handler so behavior and state (GroupIDToUserIDs, auth.Wildcard, getAllowedUserIDs) remain unchanged.
🤖 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 `@client/cmd/vnc_agent.go`:
- Around line 43-45: The code currently disables auth (srv.SetDisableAuth(true))
and sets the agent token from NB_VNC_AGENT_TOKEN (srv.SetAgentToken(...))
without verifying the env var; add a check that reads
os.Getenv("NB_VNC_AGENT_TOKEN") into a variable, verify it's non-empty, and only
then call srv.SetDisableAuth(true) and srv.SetAgentToken(token); if the token is
empty, return an error / log and exit (or leave auth enabled) so the loopback
server isn't left unguarded; reference srv.SetDisableAuth and srv.SetAgentToken
when making the change.
In `@client/internal/engine_vnc_console_linux.go`:
- Around line 22-26: The current code closes poller and returns an error when
vncserver.NewUInputInjector(w, h) fails, which disables VNC capture; instead
preserve framebuffer capture by falling back to a view-only (stub) input
injector. Change the error path in the block that calls
vncserver.NewUInputInjector so that on error you do not return nils: keep the
poller open, log or warn about uinput init failure, instantiate the library's
view-only/stub input implementation (use the existing stub input constructor in
your vncserver package) and assign it to inj so the rest of the code continues
using the real framebuffer with stubbed input, then continue returning the
normal non-error results. Ensure you reference vncserver.NewUInputInjector and
the inj variable when making the change.
In `@client/internal/engine.go`:
- Around line 1385-1388: Currently VNC auth is only updated when
networkMap.GetVncAuth() returns non-nil, which preserves stale auth when
management clears it; change the logic in the block around
networkMap.GetVncAuth() so that you always call e.updateVNCServerAuth(vncAuth)
(passing the nil through when GetVncAuth() is nil) instead of guarding the call
with an if non-nil check, ensuring the VNC server auth state is explicitly
cleared when management sends nil.
In `@client/vnc/server/agent_windows.go`:
- Around line 314-347: injectEnvVar currently returns uintptr to the first
element of a locally allocated newBlock which can be GC'd; change injectEnvVar
to return the created []uint16 slice (or []uint16 and its uintptr) instead of
just uintptr so the backing array is kept alive, then in spawnAgentInSession
capture and hold that slice (e.g. envSlice := injectEnvVar(...)) until after
CreateProcessAsUser completes; update callers to use
uintptr(unsafe.Pointer(&envSlice[0])) when calling CreateProcessAsUser and
ensure newBlock/entryUTF16 are not referenced after moving to the caller.
In `@client/vnc/server/input_darwin.go`:
- Around line 134-149: wakeDisplay currently does a read-modify-write on
userActivityID outside a single critical section causing lost updates; to fix,
acquire pmMu around the whole sequence: check pmAssertionNameCFStr/pm mu
presence, lock pmMu, copy userActivityID into local id, call
iopmAssertionDeclareUserActivity(pmAssertionNameCFStr, kIOPMUserActiveLocal,
&id) while still holding pmMu, then update userActivityID = id and unlock;
reference wakeDisplay, pmMu, userActivityID, iopmAssertionDeclareUserActivity,
pmAssertionNameCFStr, and kIOPMUserActiveLocal to locate the change.
In `@client/vnc/server/input_uinput_unix.go`:
- Around line 47-55: Remove the explicit padding field from the inputEvent
struct: delete the `_pad [4]byte` member so the struct definition for inputEvent
(used by emit()) relies on Go's natural alignment and matches the kernel's
24-byte struct input_event; update any references if necessary but do not add
manual padding or change emit() — just remove `_pad` to prevent writing the
extra 8 bytes to /dev/uinput.
In `@client/vnc/server/input_windows.go`:
- Around line 136-140: NewWindowsInputInjector spawns a goroutine that ranges
over WindowsInputInjector.ch and never ends; add a Close method on
WindowsInputInjector that safely closes the ch channel (and optionally guards
double-close with a sync.Once or check) so the loop() goroutine can exit when
the channel is closed. Update any callers to call w.Close() when done; reference
WindowsInputInjector, NewWindowsInputInjector, ch and loop to locate where to
implement the Close method.
In `@client/vnc/server/server.go`:
- Line 615: The code creates opts using
nbjwt.WithAudience(s.jwtConfig.Audiences[0]) which will panic if
s.jwtConfig.Audiences is empty; update the logic in server.go around the opts
construction (where opts is declared) to first check len(s.jwtConfig.Audiences)
> 0 and only include nbjwt.WithAudience(...) when an audience exists, otherwise
either omit the WithAudience option or supply a safe default/return an error;
ensure references to s.jwtConfig.Audiences and nbjwt.WithAudience are the ones
you modify.
In `@client/vnc/server/session.go`:
- Around line 248-255: The read deadline is being cleared immediately after
reading the message type byte (s.conn.SetDeadline / io.ReadFull on msgType),
allowing handlers to block indefinitely while reading the rest of the payload;
change the logic so the deadline is kept active for the entire message read: set
the deadline before reading the type and the payload, only clear or extend the
deadline after the full payload has been successfully read and processed (or on
error), and apply the same fix to the subsequent read blocks referenced around
the 257-287 region to ensure every io.ReadFull call for payload data uses the
active deadline until completion.
- Around line 46-55: The negotiated state fields (pf, useZlib, useHextile,
useTight, zlib, tight) are written by messageLoop and read by encoderLoop
concurrently; fix by protecting those fields with a sync.RWMutex (or by
atomically snapshotting them once per frame) — add a mutex (e.g. session.mu) and
take mu.Lock() in messageLoop when mutating
pf/useZlib/useHextile/useTight/zlib/tight and take mu.RLock() in encoderLoop and
encode/send paths (or copy the values under RLock into local variables at the
start of each frame) so reads and writes are synchronized; update all places
referenced (messageLoop, encoderLoop and encode/send code paths) to use the new
locking/snapshot pattern.
In `@client/vnc/server/shutdown_state.go`:
- Around line 61-66: The current code treats any error reading
/proc/<pid>/cmdline as proof of ownership by returning true; instead, when
os.ReadFile(fmt.Sprintf("/proc/%d/cmdline", pid)) fails (cmdline variable), do
not assume ownership—return false (or otherwise indicate "cannot verify") and
optionally log/debug the read error; keep the existing successful-path
comparison of cmdline when the file is readable, and if cross-platform support
is needed add a platform-specific ownership check rather than defaulting to
true.
In `@client/vnc/server/swizzle.go`:
- Around line 17-18: The code creates dp and sp by taking &dst[0] and &src[0]
which will panic for empty or too-short slices; add a guard at the start of the
function (or just before these lines) that returns early when n == 0 or len(dst)
< 4 or len(src) < 4 (or generally when len(dst) < n*4 || len(src) < n*4) to
avoid indexing &dst[0]/&src[0], then proceed to build dp :=
unsafe.Slice((*uint32)(unsafe.Pointer(&dst[0])), n) and sp :=
unsafe.Slice((*uint32)(unsafe.Pointer(&src[0])), n).
In `@client/vnc/server/virtual_x11.go`:
- Around line 270-287: The code writes a predictable root-owned file into /tmp
using confPath and os.WriteFile; replace this with a secure temporary file
creation (e.g., use os.CreateTemp/TempFile) to generate an unpredictable
filename, write the conf contents to that temp file (conf variable) with
restrictive permissions (owner-only), ensure the file is closed and optionally
fsync/atomically renamed into place if needed, and remove the temp file when
finished to avoid symlink attacks; update the logic around confPath and the
os.WriteFile call to use the temp file API and explicit permission handling.
In `@client/vnc/testpage/index.html`:
- Around line 31-32: The HTML currently assigns a real-looking default to
SETUP_KEY which leaks credentials; change the logic in the code that reads
params to remove the hardcoded default (the const SETUP_KEY = ... line) so
SETUP_KEY must come explicitly from params.get('setup_key') or fall back to a
non-sensitive placeholder like 'REPLACE_WITH_SETUP_KEY' and add a runtime check
that throws or shows an error if setup_key is missing; update any references to
SETUP_KEY accordingly (search for SETUP_KEY and params usage in this file) to
ensure the page requires an explicit setup_key input rather than using the
embedded value.
- Around line 73-76: The catch block currently only logs the connect error and
sets this.readyState = 3, leaving consumers unaware; update the catch path (the
catch handling around the proxy connect where addLog and this.readyState are
used) to emit both an error and a close event so consumers can react: after
logging and before/after setting this.readyState, dispatch an ErrorEvent (or
call this.onerror if present) carrying the error info and dispatch a CloseEvent
(or call this.onclose) with an appropriate abnormal closure code (e.g. 1006) and
reason derived from err.message so listeners receive both error and close
notifications.
In `@client/wasm/cmd/main.go`:
- Around line 405-407: The code assigns sessionID from args[5].Int() without
range checking, so negative JS numbers wrap when cast to uint32; update the
block that reads args[5] (the branch using js.TypeNumber and the sessionID
variable) to first read the value into a signed integer (e.g., v :=
int64(args[5].Int())), then validate v >= 0 and v <= 0xFFFFFFFF (or use a typed
constant for uint32 max), and only then set sessionID = uint32(v); if the value
is out of range, return an error or reject the argument (log/throw) instead of
casting.
In `@client/wasm/internal/vnc/proxy.go`:
- Around line 234-237: When sendSessionHeader (p.sendSessionHeader) fails, the
browser-side websocket must be closed so noVNC receives a disconnect; update the
error branch that now logs and calls p.cleanupConnection(conn) to also close the
browser socket (e.g., call conn.ws.Close() or conn.browserConn.Close(), checking
for nil and logging any close error) before returning so both sides are cleaned
up.
- Around line 254-280: In sendSessionHeader, the 2-byte length fields for
dest.username and dest.jwt can silently truncate inputs >65535; validate
len(dest.username) and len(dest.jwt) <= 0xFFFF before building hdr and return a
clear error if exceeded (do not mask/truncate), using the
vncDestination.username and vncDestination.jwt fields to locate the check; this
ensures the 2-byte encoding is safe and prevents broken framing.
In `@shared/auth/jwt/token_age.go`:
- Around line 19-23: CheckTokenAge currently dereferences token without a nil
guard which can panic; add an explicit nil check at the start of CheckTokenAge
(e.g., if token == nil) and return a descriptive error (including context such
as from UserIDFromToken(token) if safe, or omit user id when token is nil) so
callers receive a validation error instead of a runtime crash; update any tests
calling CheckTokenAge to cover the nil-token case.
---
Outside diff comments:
In `@management/server/types/account.go`:
- Around line 983-1014: The code converts PolicyRuleProtocolNetbirdVNC to TCP
but doesn't inject the VNC port, so a VNC-only rule with no Ports/PortRanges
becomes an unscoped TCP allow; update the block handling protocol conversion to
detect when protocol == PolicyRuleProtocolNetbirdVNC and both rule.Ports and
rule.PortRanges are empty and then inject the internal VNC port (e.g., set
rule.Ports = []string{"5900"}) before building FirewallRule/fr, generating
ruleID, and calling expandPortsAndRanges and appendIPv6FirewallRule; ensure you
reference PolicyRuleProtocolNetbirdVNC, rule.Ports, rule.PortRanges,
expandPortsAndRanges, FirewallRule, and appendIPv6FirewallRule when making the
change.
---
Minor comments:
In `@client/vnc/server/capture_fb_freebsd.go`:
- Around line 140-143: The code uses 0 as the "closed" sentinel for the file
descriptor (c.fd), which is wrong because 0 is a valid FD; change the sentinel
to -1 (or add a separate closed bool) by initializing c.fd to -1 in the
struct/constructor and updating the close logic in capture_fb_freebsd.go to
check for c.fd != -1 before calling unix.Close, then set c.fd = -1 after
closing; ensure all other places that open or test c.fd use the new sentinel
(c.fd == -1) consistently.
In `@client/vnc/server/input_uinput_unix.go`:
- Around line 258-263: The SetClipboard method currently has an empty body which
static analysis flags; modify the UInputInjector.SetClipboard implementation to
include a concise inline comment (e.g., "no-op: no system clipboard on
framebuffer console") inside the function body to document the intentional
no-op, leaving the GetClipboard unchanged; reference the
UInputInjector.SetClipboard method to locate and update the function body.
In `@client/vnc/server/rfb.go`:
- Line 231: Replace the ignored error usage of des.NewCipher by capturing its
error (use block, err := des.NewCipher(key)), check if err != nil, and propagate
or handle it instead of discarding: for example return or wrap the error
(fmt.Errorf("des.NewCipher: %w", err)) from the surrounding function or log and
return a suitable error; update the code around the des.NewCipher call (the
block variable and its initializer) to perform this check so failures are not
silently ignored.
In `@client/vnc/server/stubs.go`:
- Around line 28-40: These stub methods (InjectKey, InjectPointer, SetClipboard,
GetClipboard, TypeText) are intentionally empty but need explicit annotations
for static analysis; update each method body to include a short comment like "//
intentionally no-op for unsupported platforms" and reference the receiver to
avoid unused symbol warnings (for example add "_ = s" in the body), and for
GetClipboard return keep the empty string but add the same comment above the
return to make the intent explicit to Sonar.
In `@shared/auth/jwt/token_age.go`:
- Line 17: The doc comment for the maxAge symbol has a spelling error: replace
"unparseable" with the correct word "unparsable" in the comment that describes
maxAge's behavior so the comment reads "Returns an error if the claims are
unparsable" (update the comment near the maxAge declaration).
---
Nitpick comments:
In `@client/vnc/server/agent_windows.go`:
- Around line 564-644: The run method in sessionManager is too complex; extract
the main responsibilities into small helper methods to reduce cognitive
complexity: create detectSessionChange(m *sessionManager) to handle session ID
comparison, logging and calling m.killAgent()/m.sessionID update; create
checkAgentHealth(m *sessionManager) to encapsulate the agent exit detection
logic (GetExitCodeProcess, lifetime/backoff calculation, updating
m.spawnFailures, m.nextSpawnAt, closing handle and zeroing m.agentProc); and
create trySpawnAgent(m *sessionManager) to contain the spawn logic
(reapOrphanOnPort on first spawn, generateAuthToken, call spawnAgentInSession,
handle ERROR_PRIVILEGE_NOT_HELD, set m.agentProc/m.agentStartedAt/m.everSpawned
or clear m.authToken on error). Move only the corresponding blocks out of run
and call these helpers from run while preserving existing m.mu locking semantics
and behavior.
In `@client/vnc/server/capture_windows.go`:
- Around line 397-517: The worker() function is over the complexity/length
threshold; extract the select-loop request handling into a new helper (e.g.,
handleCaptureRequest) to reduce cognitive load and make it testable: move the
logic that checks clients.Load(), handles <-c.done and case req := <-c.reqCh
into handleCaptureRequest which should call existing helpers prepCapturer(),
closeCapturer(), and use createCapturer()/fc.capture() and nextInitRetry/cap
state via parameters or receiver access; keep setupInteractiveWindowStation(),
createCapturer(), prepCapturer(), closeCapturer(), and defer closeCapturer()
intact and update worker() to call the new helper from the loop.
In `@client/vnc/server/capture_x11_shm_stub.go`:
- Line 22: The function X11Capturer.closeSHM is intentionally a no-op but
currently lacks any comment explaining that, which tripped static analysis; add
a brief explanatory comment above the closeSHM method (or inside its body)
stating that this stub intentionally does nothing (e.g., "no-op for platforms
without SHM cleanup" or "placeholder: SHM cleanup not required/handled
elsewhere") so readers and linters understand the behavior while keeping the
empty implementation.
In `@client/vnc/server/input_darwin.go`:
- Around line 385-428: TypeText's cognitive complexity exceeds the threshold;
extract the repeated shift-press/release block into a small helper to simplify
logic: create a helper function (e.g., sendKeyWithOptionalShift or
pressKeyWithShift) that accepts src, keycode, shift flag and calls
cgEventCreateKeyboardEvent/ cgEventPost/ cfRelease for shift down, key down, key
up, shift up as needed, then replace the inline shift handling in TypeText with
calls to that helper while keeping keysymForASCIIRune and keysymToMacKeycode
usage intact.
- Around line 273-338: The InjectPointer method is over-complex; extract the
movement, button transition handling, and scroll handling into small helpers to
reduce cognitive complexity: create e.g. computeLogicalCoords(px, py, serverW,
serverH) to return (x,y) and use it where x/y are computed, move the conditional
dispatch into handlePointerMovement(src, leftDown, rightDown, x, y) that calls
postMouse with kCGEventLeftMouseDragged/RightMouseDragged/MouseMoved, implement
handleButtonTransitions(src, leftDown, rightDown, middleDown, wasLeft, wasRight,
wasMiddle, x, y) to emit Left/Right/Other Mouse Down/Up events by calling
postMouse, and extract scroll handling into handleScroll(src, scrollUp,
scrollDown) to call postScroll; then replace the corresponding blocks in
InjectPointer with calls to these helpers and update lastButtons at the end.
In `@client/vnc/server/input_uinput_unix.go`:
- Line 401: The function keymapByKeysym currently declares an unused parameter
(_ []uint16); remove that parameter from the function signature to avoid
unused-parameter linting, and update all call sites that pass that argument to
call keymapByKeysym() with no arguments; alternatively, if the parameter was
meant to filter results, implement filtering logic inside keymapByKeysym using
the provided []uint16 and keep callers unchanged—locate keymapByKeysym and its
callers to apply one of these two fixes (removing the parameter and adjusting
callers, or using the parameter to filter the returned map).
- Around line 456-548: keyForRune duplicates the letter/digit keycode mappings
already defined in keymapByKeysym; extract a shared map (e.g., letterToKeycode)
containing the rune->keycode entries and use it from both keyForRune and
keymapByKeysym (and reuse the nums slice for digits), updating keyForRune to
lookups against that shared map and fall back to existing switch cases for
punctuation/shifted variants so the mappings stay centralized and avoid
divergence.
In `@client/vnc/server/input_windows.go`:
- Around line 173-179: InjectPointer currently drops inputCmd silently when w.ch
is full; update WindowsInputInjector.InjectPointer to keep the non-blocking
behavior but add a trace-level log in the default branch that records the
dropped inputCmd (or at least buttonMask,x,y and serverW,serverH) to aid
debugging. Locate InjectPointer and the channel w.ch and emit a trace/debug
message (using the existing logger on the WindowsInputInjector instance, e.g.,
w.logger or the package tracing/log facility present in the repo) inside the
select's default branch so behavior is unchanged but drops are observable.
In `@client/vnc/server/input_x11.go`:
- Around line 269-275: The clipboardEnv function creates a minimal environment
which can break clipboard helpers; change X11InputInjector.clipboardEnv to start
from the full environment (os.Environ()), then ensure/override "DISPLAY" and, if
present, "XAUTHORITY" in that slice so existing vars like PATH and locale are
preserved; update the function to merge/replace those keys rather than returning
a two-entry slice.
In `@client/vnc/server/rfb.go`:
- Line 813: The parameter name `cap` in sampledColorCountInto shadows Go's
built-in cap function; rename it to a descriptive name like `limit` or
`maxColors` in the function signature (sampledColorCountInto) and update every
call site to use the new name, plus any internal references within that
function, so no references rely on the built-in `cap` and the intent (maximum
colors to count) is clearer.
- Around line 398-415: tileIsUniform currently casts img.Pix bytes to uint32 via
unsafe.Pointer which is platform-endian dependent; change the comparison to use
a platform-independent byte-wise or defined-endian comparison instead: read the
first pixel from img.Pix at index base as four consecutive bytes
(img.Pix[base:base+4]) and compare subsequent pixels by comparing their
four-byte slices (or by using encoding/binary with a fixed endianness) rather
than *(*uint32)(unsafe.Pointer(...)); update uses in tileIsUniform to avoid
unsafe.Pointer and ensure correct results on big-endian systems.
In `@client/vnc/server/server_test.go`:
- Around line 46-48: The tests call net.Dial and then use conn but do not set
any socket deadlines, so a stalled handshake can hang CI; after each net.Dial
(e.g., where conn is returned in server_test.go) call
conn.SetDeadline(time.Now().Add(2*time.Second)) (or a similar short timeout)
immediately to bound read/write waits and fail fast; apply this change to the
other net.Dial occurrences referenced (the conn in the blocks around lines 84-86
and 118-120) or centralize into a helper that sets the deadline on the returned
conn.
In `@client/vnc/server/server_windows.go`:
- Around line 85-137: The startSASListener function is too complex and should be
split into smaller helpers: extract the setup portion (procSendSAS.Find check,
enableSoftwareSAS call, UTF16PtrFromString(sasEventName),
sasSecurityAttributes(), and windows.CreateEvent) into a helper like
createSASEvent or initSASEvent that returns (windows.Handle, error), and extract
the goroutine wait/dispatch loop into a helper like runSASListenerLoop(ctx, ev)
that handles CloseHandle(ev), the recover defer, the WaitForSingleObject polling
and the procSendSAS.Call logic; keep all existing log messages and error
handling, ensure createSASEvent returns nil/error on failure so startSASListener
simply calls it, and ensure runSASListenerLoop receives the event handle and
context so startSASListener just wires them together.
In `@client/vnc/server/server.go`:
- Around line 444-480: Extract the ModeSession branch into a helper on the
server type that performs the validation, vmgr lookup/create, logging and
returns the capturer, injector, updated connLog and a cleanup function (or an
error to trigger rejectConnection). Specifically, move the code that checks
s.vmgr, header.username, calls s.vmgr.GetOrCreate, obtains
vs.Capturer()/vs.Injector(), calls vs.ClientConnect(), sets up the deferred
vs.ClientDisconnect(), and enriches connLog into a helper (e.g., func (s
*Server) initSessionResources(header Header, conn net.Conn, connLog log.Entry)
(capturer Capturer, injector Injector, connLog log.Entry, cleanup func(), err
error)), then replace the ModeSession case with a call to that helper, handle
error by calling rejectConnection/connLog.Warn as before, and defer the returned
cleanup to preserve disconnect behavior; keep all original symbols (s.vmgr,
rejectConnection, codeMessage, RejectCode*, GetOrCreate,
vs.ClientConnect/ClientDisconnect, capturer/injector) and the same log messages.
- Around line 316-331: acceptLoop can hang if Accept() is blocked and s.ctx is
cancelled, so ensure the listener gets closed when the context is done; at the
start of Server.acceptLoop() spawn a short goroutine that waits on s.ctx.Done()
and calls s.listener.Close() (and add a comment that Stop() also closes the
listener) so Accept() unblocks and the loop exits cleanly; reference acceptLoop,
s.listener.Accept, s.ctx.Done and Stop to locate where to add the goroutine and
comment.
In `@management/server/types/networkmap_components.go`:
- Around line 183-255: The getPeerConnectionResources function is doing peer
resolution, direction handling/resource generation and auth collection all
inline which increases complexity; refactor it into clear phases and helper
methods: (1) extract peer-resolution logic into helpers that call
getPeerFromResource and getAllPeersFromGroups to produce
sourcePeers/destinationPeers and booleans (peerInSources/peerInDestinations),
(2) move directional resource generation into a helper that invokes
connResourcesGenerator(rule) and generateResources(rule, peers, direction) for
IN/OUT and bidirectional cases, (3) separate auth collection into a helper that
handles collectAuthorizedUsers, vncAuthorizedUsers, legacy SSH via
policyRuleImpliesLegacySSH and getAllowedUserIDs and sets
sshEnabled/authorizedUsers, and (4) keep final accumulation via the existing
getAccumulatedResources call; create small functions named like
resolveRulePeers, applyRuleDirectionResources, and collectRuleAuth to replace
the inline blocks in getPeerConnectionResources.
- Around line 259-289: The collectAuthorizedUsers function is doing three
distinct modes (AuthorizedGroups, AuthorizedUser, default) which pushes
complexity; refactor by extracting each branch into small helper methods (e.g.,
collectAuthorizedGroups(rule *PolicyRule, target
map[string]map[string]struct{}), collectAuthorizedUser(rule *PolicyRule, target
map[string]map[string]struct{}), and collectAuthorizedDefault(target
map[string]map[string]struct{})) and update collectAuthorizedUsers to simply
switch on the same conditions and call the appropriate helper; ensure the
helpers preserve existing behavior: use c.GroupIDToUserIDs when handling
AuthorizedGroups (including populating localUsers with auth.Wildcard when empty
and mapping userIDs into target[localUser]), set
target[auth.Wildcard][rule.AuthorizedUser] in the AuthorizedUser handler, and
return c.getAllowedUserIDs() into target[auth.Wildcard] in the default handler
so behavior and state (GroupIDToUserIDs, auth.Wildcard, getAllowedUserIDs)
remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
client/vnc/server/session.go (1)
46-55:⚠️ Potential issue | 🟡 Minor | ⚖️ Poor tradeoffData race between
messageLoopandencoderLoopon encoding state.The comment at lines 46-49 claims ordering invariant makes locks unnecessary, but RFB spec allows
SetPixelFormatandSetEncodingsat any time during the session. If a client sendsSetEncodingswhileencoderLoopis encoding a frame, the concurrent read/write topf,useZlib,useHextile,useTight,zlib, andtightis a data race.In practice, well-behaved clients only send these during initial negotiation, so this may be acceptable for the initial implementation. Consider adding a mutex or snapshotting encoding config atomically per frame if robustness against misbehaving clients is needed.
Also applies to: 291-338, 507-577
🤖 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 `@client/vnc/server/session.go` around lines 46 - 55, The encoding state fields (pf, useZlib, useHextile, useTight, zlib, tight) are concurrently written by messageLoop (handling SetPixelFormat/SetEncodings) and read by encoderLoop causing a data race; fix by introducing a synchronization strategy: either add a mutex (e.g., encodingMu) and lock it around all writes in messageLoop and reads in encoderLoop, or atomically snapshot the encoding config into a local struct at the start of each frame in encoderLoop (copy pf, useZlib/useHextile/useTight and pointers/state like zlib/tight) so encoderLoop uses the snapshot without further locking; update both messageLoop and encoderLoop to use the chosen approach and ensure SetPixelFormat/SetEncodings handlers modify the shared fields under the same mutex or update the shared struct atomically.
🧹 Nitpick comments (7)
client/vnc/server/rfb.go (4)
720-729: 💤 Low valueUnused
pfparameter inencodeTightFill.The
pfparameter is assigned to blank identifier on line 721 and never used. Per the Tight spec, Fill always uses 24-bit RGB regardless of pixel format, which the code correctly implements. Consider removing the unused parameter to reduce function signature complexity.♻️ Proposed fix
-func encodeTightFill(pf clientPixelFormat, x, y, w, h int, r, g, b byte) []byte { - _ = pf +func encodeTightFill(x, y, w, h int, r, g, b byte) []byte { buf := make([]byte, 12+1+3)Update the call site in
encodeTightRectaccordingly.🤖 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 `@client/vnc/server/rfb.go` around lines 720 - 729, The encodeTightFill function currently takes an unused clientPixelFormat parameter (pf) which is discarded with the blank identifier; remove the pf parameter from encodeTightFill's signature and update any callers (notably encodeTightRect) to call encodeTightFill without the pf argument so the function signature matches its 24-bit RGB-only behavior per the Tight spec.
754-755: 💤 Low valueUnused
pfparameter inencodeTightBasic.The Tight encoding uses 24-bit TPIXEL format and ignores the negotiated pixel format. Consider removing
pffrom the signature.🤖 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 `@client/vnc/server/rfb.go` around lines 754 - 755, The parameter pf in function encodeTightBasic is unused; remove pf from the function signature (change func encodeTightBasic(img *image.RGBA, x, y, w, h int, t *tightState) []byte) and update every call site that passes a clientPixelFormat so they call the new signature; also update any references in rfb.go that mention encodeTightBasic to match the new parameter list and run go build/tests to ensure no remaining references remain.
733-734: 💤 Low valueUnused
pfparameter inencodeTightJPEG.Similar to
encodeTightFill, thepfparameter is unused here.🤖 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 `@client/vnc/server/rfb.go` around lines 733 - 734, The encodeTightJPEG function currently declares a clientPixelFormat parameter pf which is unused (it only has a no-op "_ = pf"), mirroring encodeTightFill; fix this by either removing the pf parameter from encodeTightJPEG's signature if callers don’t need it, or rename it to the blank identifier (e.g., _ clientPixelFormat) and delete the "_ = pf" line so the compiler and linters stop flagging an unused variable; update any callers if you remove the parameter and ensure consistency with encodeTightFill.
817-836: 💤 Low valueParameter name
capshadows the builtincapfunction.Renaming the parameter avoids shadowing the builtin and improves readability.
♻️ Proposed fix
-func sampledColorCountInto(seen map[uint32]struct{}, img *image.RGBA, x, y, w, h, cap int) int { +func sampledColorCountInto(seen map[uint32]struct{}, img *image.RGBA, x, y, w, h, maxColors int) int { clear(seen) stride := img.Stride - step := max((w*h)/(cap*4), 1) + step := max((w*h)/(maxColors*4), 1) var idx int for row := 0; row < h; row++ { p := (y+row)*stride + x*4 for col := 0; col < w; col++ { if idx%step == 0 { px := *(*uint32)(unsafe.Pointer(&img.Pix[p+col*4])) seen[px&0x00ffffff] = struct{}{} - if len(seen) > cap { + if len(seen) > maxColors { return len(seen) } }🤖 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 `@client/vnc/server/rfb.go` around lines 817 - 836, The parameter name cap in sampledColorCountInto shadows the builtin cap; rename it (e.g., to limit or maxColors) in the sampledColorCountInto function signature and update all uses inside the function (the step calculation and the len(seen) comparison) to the new name to avoid shadowing and improve readability.client/vnc/server/virtual_x11.go (1)
358-364: ⚖️ Poor tradeoffProcess termination waits 200ms between SIGTERM and SIGKILL.
The fixed 200ms sleep between SIGTERM and SIGKILL (lines 361 and 435) may be too short for desktop sessions to cleanly shut down, or too long if the process is already gone. Consider using
Process.Waitwith a timeout instead of fixed sleep.This is a minor ergonomic improvement rather than a correctness issue.
Also applies to: 432-438
🤖 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 `@client/vnc/server/virtual_x11.go` around lines 358 - 364, Replace the fixed 200ms sleep between SIGTERM and SIGKILL in VirtualSession.stopXvfb (and the similar block around lines 432-438) with waiting for the process to exit using Process.Wait with a timeout: after sending SIGTERM to -vs.xvfb.Process.Pid, start a goroutine that calls vs.xvfb.Process.Wait(), use select with time.After(desiredTimeout) to either return when Wait finishes or fall through to send SIGKILL if the timeout expires, and handle the case where the process is already gone (nil/ErrProcessDone) to avoid unnecessary signals.client/wasm/cmd/main.go (1)
367-424: ⚖️ Poor tradeoffConsider extracting parameter parsing to reduce complexity.
The
createVNCProxyMethodfunction has high cognitive complexity due to parsing 8 optional parameters. While the current implementation is correct, extracting parameter parsing into a helper struct/function would improve readability.🤖 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 `@client/wasm/cmd/main.go` around lines 367 - 424, createVNCProxyMethod is doing heavy inline parsing of up to 8 optional JS args which increases cognitive complexity; extract that logic into a dedicated helper (e.g., parseVNCProxyArgs or a vncProxyParams struct with a FromJS(args []js.Value) error method) that validates and converts hostname, port, mode, username, jwtToken, sessionID, width, height and returns typed values (or an error) so createVNCProxyMethod simply calls the parser, checks error, then calls vnc.NewVNCProxy(client).CreateProxy(...); update references to sessionID/width/height types and maintain the same validation rules currently in the function.client/vnc/server/server_windows.go (1)
148-157: 💤 Low value
UTF16PtrFromStringerror ignored on line 149.The error from
windows.UTF16PtrFromString(name)is discarded with_. While this function only fails if the string contains embedded NULs (which privilege names don't), explicitly handling it would be more robust.♻️ Proposed fix
- namePtr, _ := windows.UTF16PtrFromString(name) + namePtr, err := windows.UTF16PtrFromString(name) + if err != nil { + return err + }🤖 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 `@client/vnc/server/server_windows.go` around lines 148 - 157, The call to windows.UTF16PtrFromString(name) currently ignores its error; update the code around the UTF16PtrFromString call so you capture the returned error (e.g., namePtr, err := windows.UTF16PtrFromString(name)), check it, and return it if non-nil before calling windows.LookupPrivilegeValue; ensure this change is applied in the same function that declares luid, namePtr and calls LookupPrivilegeValue and AdjustTokenPrivileges so the pointer is only used when valid.
🤖 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 `@client/internal/engine.go`:
- Around line 1385-1387: The call to
e.updateVNCServerAuth(networkMap.GetVncAuth()) is currently inside the branch
guarded by RemotePeersIsEmpty, so VNC auth updates (including explicit clears
via nil) are skipped when management sends an empty peer list; move the
e.updateVNCServerAuth(...) invocation out of that conditional so it runs
unconditionally after networkMap is obtained (i.e., call updateVNCServerAuth
with networkMap.GetVncAuth() regardless of the RemotePeersIsEmpty branch) to
ensure auth rotations/clears are always applied locally.
In `@client/vnc/server/agent_windows.go`:
- Around line 358-374: The code currently continues launching the child when
procCreateEnvironmentBlock.Call fails (r == 0), leading to a missing
NB_VNC_AGENT_TOKEN; modify the logic around procCreateEnvironmentBlock,
envBlock, procDestroyEnvironmentBlock and injectEnvVar so that when
procCreateEnvironmentBlock indicates failure you abort the spawn and return an
error instead of proceeding; only call procDestroyEnvironmentBlock (defer) and
call injectEnvVar(agentTokenEnvVar, authToken) when procCreateEnvironmentBlock
succeeded, and ensure CreateProcessAsUser is not called if the environment block
could not be created.
In `@client/vnc/server/capture_fb_linux.go`:
- Around line 191-200: The Close method treats fd==0 as invalid and skips
closing a legitimate descriptor; update cleanup logic in FBCapturer.Close to
consider any non-negative fd as valid (check c.fd >= 0) and close it, and ensure
the FBCapturer.fd field is initialized to an invalid sentinel (e.g. -1) where
declared so the new check works correctly; after closing set c.fd back to the
sentinel. Reference: FBCapturer.Close, c.fd, c.mmap, unix.Close, unix.Munmap,
and closeOnce.Do.
In `@client/vnc/server/input_darwin.go`:
- Around line 151-167: The process-global idle-sleep assertion needs a reference
count so multiple injectors don't race-clearing it: add a pm-refcounter (e.g.,
preventSleepRefcount int) protected by pmMu and update holdPreventIdleSleep() to
increment the counter and only call IOPMAssertionCreateWithName when the counter
transitions from 0->1 (setting preventSleepID and preventSleepHeld), and modify
the counterpart release function(s) (the code around the release paths
referenced at lines ~221-224 and ~441-443) to decrement the counter under pmMu
and only call the IOPMAssertionRelease when the counter transitions to 0; keep
existing pmMu locking and existing variables (preventSleepID, preventSleepHeld)
semantics but base create/release on the refcount.
In `@client/vnc/server/input_uinput_unix.go`:
- Around line 299-307: The Close method in UInputInjector incorrectly treats
fd==0 as "closed", so if unix.Open returns 0 the device won't be destroyed;
update the code to treat negative values as the closed sentinel instead: ensure
UInputInjector.fd is initialized to -1 when created, change the Close logic in
UInputInjector.Close to check for fd >= 0 before calling ioctl/Close (using
uiDevDestroy and unix.Close), and set fd back to -1 after closing; reference the
fields/methods closeOnce, mu, u.fd, and the uiDevDestroy ioctl in your changes.
In `@client/vnc/server/input_windows.go`:
- Around line 145-149: The Close implementation for WindowsInputInjector
currently closes the work channel (w.ch) which causes late calls to
InjectKey/InjectPointer/SetClipboard/TypeText to panic with "send on closed
channel"; instead add a separate closed signal (e.g., w.closed chan struct{} or
an atomic boolean field) and ensure Close only signals shutdown via that signal
(using close(w.closed) or set atomic flag inside WindowsInputInjector.Close),
leaving w.ch open or drained by the worker; update all enqueue points in
InjectKey, InjectPointer, SetClipboard, TypeText (and other places noted) to
check the closed signal before attempting to send (use a non-blocking/select
send that drops the work if closed) so callers simply return without sending
into a closed queue; keep closeOnce for idempotence and ensure the worker
goroutine exits when it sees the closed signal.
- Around line 203-214: The code sets keyeventfScanCode for extended keys which
makes Windows ignore the wVk value; instead, when keysym2VK returns
extended==true set the KEYEVENTF_EXTENDEDKEY flag (e.g. keyeventfExtendedKey or
similar constant) rather than keyeventfScanCode so sendKeyInput(vk, 0, flags)
preserves wVk; update the flags logic in the block that currently uses
keyeventfScanCode (and keep keyeventfKeyUp handling) so extended keys use
KEYEVENTF_EXTENDEDKEY.
In `@client/wasm/internal/vnc/proxy.go`:
- Around line 321-328: When the TCP-side read loop hits a non-EOF error (the
branch handling err != io.EOF), also cancel the connection's local context so
cleanupConnection won't hang waiting on conn.ctx.Done() if the JS close callback
never fires; after calling conn.wsHandlers.Call("close", ...) invoke the
connection's cancel function (e.g. conn.cancel or conn.cancelFunc) or otherwise
close the local context associated with conn.ctx so connectToVNC unblocks and
cleanupConnection runs. Ensure you reference the same conn object used in this
read loop (conn.ctx, conn.wsHandlers) when wiring the cancel invocation.
- Around line 103-138: The Promise executor created with js.FuncOf in the
Promise.New call is never released and leaks; wrap that executor in a js.Func
variable, and ensure you call its Release() after the goroutine finishes (i.e.,
after resolve.Invoke(proxyURL)) so the callback is freed; keep the inner
handlerFn and pendingHandlers logic as-is (references: js.FuncOf for the Promise
executor, resolve.Invoke, handlerFn, p.handleWebSocketConnection, and
p.pendingHandlers) and release only the executor func (not the handlerFn stored
in the global handle map).
- Around line 288-289: The current write of the session header uses a single
call to conn.Write(hdr) which may allow partial writes; replace it with a loop
that repeatedly writes the remaining bytes until all len(hdr) bytes are sent or
an error occurs. Locate the code that calls conn.Write(hdr) (the hdr variable
and the conn.Write call) and implement a write-all loop that advances the offset
by n on each successful Write, returning any write error immediately and only
proceeding once the full header has been written before starting RFB.
In `@management/server/types/account.go`:
- Around line 983-985: The current branch unconditionally rewrites
PolicyRuleProtocolNetbirdVNC to PolicyRuleProtocolTCP which expands VNC-only
rules to all TCP; change the condition so PolicyRuleProtocolNetbirdVNC is
rewritten to PolicyRuleProtocolTCP only when the rule targets the embedded VNC
port (e.g. compare the rule's port field to vncInternalPort) and leave
NetbirdVNC untouched otherwise; update the if that checks
PolicyRuleProtocolNetbirdSSH/PolicyRuleProtocolNetbirdVNC to only remap
NetbirdVNC when rule.Port == vncInternalPort (keep the existing unconditional
remap for PolicyRuleProtocolNetbirdSSH) and preserve the Protocol value for
other cases.
In `@management/server/types/networkmap_components.go`:
- Around line 307-310: The component-path generator currently widens
PolicyRuleProtocolNetbirdVNC to TCP when a rule has no explicit ports (same
issue as in connResourcesGenerator); update the code that sets protocol and
ports (the block that assigns protocol := rule.Protocol) to apply the same
internal-port clamp used in connResourcesGenerator: when protocol ==
PolicyRuleProtocolNetbirdVNC and rule.Ports is empty, keep the VNC-specific port
mapping instead of treating it as unrestricted TCP (i.e., set the effective port
list to the VNC port(s) or a clamped internal port rather than leaving it open),
using the same helper/logic as connResourcesGenerator to locate where to change
behavior.
---
Duplicate comments:
In `@client/vnc/server/session.go`:
- Around line 46-55: The encoding state fields (pf, useZlib, useHextile,
useTight, zlib, tight) are concurrently written by messageLoop (handling
SetPixelFormat/SetEncodings) and read by encoderLoop causing a data race; fix by
introducing a synchronization strategy: either add a mutex (e.g., encodingMu)
and lock it around all writes in messageLoop and reads in encoderLoop, or
atomically snapshot the encoding config into a local struct at the start of each
frame in encoderLoop (copy pf, useZlib/useHextile/useTight and pointers/state
like zlib/tight) so encoderLoop uses the snapshot without further locking;
update both messageLoop and encoderLoop to use the chosen approach and ensure
SetPixelFormat/SetEncodings handlers modify the shared fields under the same
mutex or update the shared struct atomically.
---
Nitpick comments:
In `@client/vnc/server/rfb.go`:
- Around line 720-729: The encodeTightFill function currently takes an unused
clientPixelFormat parameter (pf) which is discarded with the blank identifier;
remove the pf parameter from encodeTightFill's signature and update any callers
(notably encodeTightRect) to call encodeTightFill without the pf argument so the
function signature matches its 24-bit RGB-only behavior per the Tight spec.
- Around line 754-755: The parameter pf in function encodeTightBasic is unused;
remove pf from the function signature (change func encodeTightBasic(img
*image.RGBA, x, y, w, h int, t *tightState) []byte) and update every call site
that passes a clientPixelFormat so they call the new signature; also update any
references in rfb.go that mention encodeTightBasic to match the new parameter
list and run go build/tests to ensure no remaining references remain.
- Around line 733-734: The encodeTightJPEG function currently declares a
clientPixelFormat parameter pf which is unused (it only has a no-op "_ = pf"),
mirroring encodeTightFill; fix this by either removing the pf parameter from
encodeTightJPEG's signature if callers don’t need it, or rename it to the blank
identifier (e.g., _ clientPixelFormat) and delete the "_ = pf" line so the
compiler and linters stop flagging an unused variable; update any callers if you
remove the parameter and ensure consistency with encodeTightFill.
- Around line 817-836: The parameter name cap in sampledColorCountInto shadows
the builtin cap; rename it (e.g., to limit or maxColors) in the
sampledColorCountInto function signature and update all uses inside the function
(the step calculation and the len(seen) comparison) to the new name to avoid
shadowing and improve readability.
In `@client/vnc/server/server_windows.go`:
- Around line 148-157: The call to windows.UTF16PtrFromString(name) currently
ignores its error; update the code around the UTF16PtrFromString call so you
capture the returned error (e.g., namePtr, err :=
windows.UTF16PtrFromString(name)), check it, and return it if non-nil before
calling windows.LookupPrivilegeValue; ensure this change is applied in the same
function that declares luid, namePtr and calls LookupPrivilegeValue and
AdjustTokenPrivileges so the pointer is only used when valid.
In `@client/vnc/server/virtual_x11.go`:
- Around line 358-364: Replace the fixed 200ms sleep between SIGTERM and SIGKILL
in VirtualSession.stopXvfb (and the similar block around lines 432-438) with
waiting for the process to exit using Process.Wait with a timeout: after sending
SIGTERM to -vs.xvfb.Process.Pid, start a goroutine that calls
vs.xvfb.Process.Wait(), use select with time.After(desiredTimeout) to either
return when Wait finishes or fall through to send SIGKILL if the timeout
expires, and handle the case where the process is already gone
(nil/ErrProcessDone) to avoid unnecessary signals.
In `@client/wasm/cmd/main.go`:
- Around line 367-424: createVNCProxyMethod is doing heavy inline parsing of up
to 8 optional JS args which increases cognitive complexity; extract that logic
into a dedicated helper (e.g., parseVNCProxyArgs or a vncProxyParams struct with
a FromJS(args []js.Value) error method) that validates and converts hostname,
port, mode, username, jwtToken, sessionID, width, height and returns typed
values (or an error) so createVNCProxyMethod simply calls the parser, checks
error, then calls vnc.NewVNCProxy(client).CreateProxy(...); update references to
sessionID/width/height types and maintain the same validation rules currently in
the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5322e52-f910-4a94-b635-a462b8172a28
⛔ Files ignored due to path filters (4)
client/proto/daemon.pb.gois excluded by!**/*.pb.gogo.sumis excluded by!**/*.sumshared/management/proto/management.pb.gois excluded by!**/*.pb.goshared/management/proto/management_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (80)
.github/workflows/wasm-build-validation.ymlclient/cmd/ssh.goclient/cmd/up.goclient/cmd/vnc_agent.goclient/cmd/vnc_flags.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/debug/debug.goclient/internal/engine.goclient/internal/engine_vnc.goclient/internal/engine_vnc_console_freebsd.goclient/internal/engine_vnc_console_linux.goclient/internal/engine_vnc_darwin.goclient/internal/engine_vnc_stub.goclient/internal/engine_vnc_windows.goclient/internal/engine_vnc_x11.goclient/internal/profilemanager/config.goclient/internal/statemanager/manager.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.goclient/ssh/server/executor_windows.goclient/ssh/server/server.goclient/status/status.goclient/status/status_test.goclient/system/info.goclient/ui/client_ui.goclient/ui/const.goclient/ui/event_handler.goclient/vnc/server/agent_windows.goclient/vnc/server/capture_darwin.goclient/vnc/server/capture_dxgi_windows.goclient/vnc/server/capture_fb_freebsd.goclient/vnc/server/capture_fb_linux.goclient/vnc/server/capture_fb_unix.goclient/vnc/server/capture_windows.goclient/vnc/server/capture_x11.goclient/vnc/server/capture_x11_shm_linux.goclient/vnc/server/capture_x11_shm_stub.goclient/vnc/server/coalesce_test.goclient/vnc/server/hextile_test.goclient/vnc/server/input_darwin.goclient/vnc/server/input_uinput_unix.goclient/vnc/server/input_windows.goclient/vnc/server/input_x11.goclient/vnc/server/keysym_typetext.goclient/vnc/server/rfb.goclient/vnc/server/rfb_bench_test.goclient/vnc/server/server.goclient/vnc/server/server_darwin.goclient/vnc/server/server_stub.goclient/vnc/server/server_test.goclient/vnc/server/server_windows.goclient/vnc/server/server_x11.goclient/vnc/server/session.goclient/vnc/server/shutdown_state.goclient/vnc/server/stubs.goclient/vnc/server/swizzle.goclient/vnc/server/tight_test.goclient/vnc/server/virtual_x11.goclient/wasm/cmd/main.goclient/wasm/internal/rdp/cert_validation.goclient/wasm/internal/rdp/rdcleanpath_handlers.goclient/wasm/internal/vnc/proxy.gogo.modmanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/management_proto_test.gomanagement/server/peer/peer.gomanagement/server/policy_test.gomanagement/server/types/account.gomanagement/server/types/network.gomanagement/server/types/networkmap_components.gomanagement/server/types/policy.goshared/auth/jwt/token_age.goshared/management/client/grpc.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/management.proto
💤 Files with no reviewable changes (2)
- client/wasm/internal/rdp/cert_validation.go
- client/wasm/internal/rdp/rdcleanpath_handlers.go
✅ Files skipped from review due to trivial changes (4)
- client/internal/engine_vnc_stub.go
- client/ssh/server/executor_windows.go
- client/cmd/vnc_flags.go
- shared/management/http/api/types.gen.go
🚧 Files skipped from review as they are similar to previous changes (55)
- client/internal/debug/debug.go
- client/ui/const.go
- client/internal/statemanager/manager.go
- management/server/management_proto_test.go
- management/internals/shared/grpc/server.go
- client/ui/event_handler.go
- client/vnc/server/keysym_typetext.go
- client/status/status_test.go
- client/vnc/server/tight_test.go
- client/vnc/server/capture_dxgi_windows.go
- client/proto/daemon.proto
- client/vnc/server/server_darwin.go
- shared/management/http/api/openapi.yml
- client/cmd/ssh.go
- client/internal/engine_vnc_console_freebsd.go
- management/server/types/policy.go
- management/server/types/network.go
- client/vnc/server/server_x11.go
- client/internal/auth/auth.go
- shared/management/proto/management.proto
- client/vnc/server/capture_x11_shm_stub.go
- client/vnc/server/swizzle.go
- client/internal/engine_vnc_darwin.go
- client/internal/engine_vnc_console_linux.go
- client/vnc/server/stubs.go
- management/server/peer/peer.go
- client/ui/client_ui.go
- client/internal/profilemanager/config.go
- client/vnc/server/capture_fb_freebsd.go
- client/system/info.go
- management/server/http/handlers/peers/peers_handler.go
- client/vnc/server/capture_x11_shm_linux.go
- client/internal/engine_vnc_windows.go
- client/server/server.go
- client/vnc/server/server_test.go
- client/status/status.go
- shared/management/client/grpc.go
- client/cmd/up.go
- client/vnc/server/capture_darwin.go
- client/cmd/vnc_agent.go
- client/ssh/server/server.go
- management/internals/shared/grpc/conversion.go
- client/vnc/server/hextile_test.go
- client/vnc/server/rfb_bench_test.go
- client/internal/connect.go
- shared/auth/jwt/token_age.go
- client/server/setconfig_test.go
- client/internal/engine_vnc_x11.go
- management/server/policy_test.go
- go.mod
- client/vnc/server/capture_fb_unix.go
- client/vnc/server/coalesce_test.go
- client/internal/engine_vnc.go
- client/vnc/server/capture_x11.go
- client/vnc/server/input_x11.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@client/vnc/server/capture_x11.go`:
- Around line 381-394: Close currently only closes p.done and capturer but does
not mark the X11Poller as permanently closed, so ensureCapturerLocked can
recreate resources later; add a permanent closed flag on X11Poller (e.g.,
p.closed bool) that you set to true under p.mu in X11Poller.Close, then update
ensureCapturerLocked to check p.closed and return an error instead of creating a
new X11Capturer when closed, and propagate that error from Capture, Width,
Height (and any other callers) so after Close those calls fail as the comment
promises.
- Around line 181-186: NewX11Capturer currently calls detectX11Display() before
honoring the explicit display argument, which may mutate DISPLAY/XAUTHORITY and
cause auth to point to the wrong server; change the flow so detectX11Display is
only invoked when the caller did not supply a display (i.e., when display ==
""), or update detectX11Display to accept a target display and only
auto-detect/auth for that specific display rather than globally mutating env
vars; refer to NewX11Capturer and detectX11Display and ensure any env changes
are scoped to the requested display (or skipped entirely when an explicit
display is provided).
In `@client/vnc/server/rfb.go`:
- Around line 684-693: encodeTightRect and its tight subencoders
(encodeTightFill, encodeTightJPEG, encodeTightBasic and any internal tight
packing routines) currently emit fixed 24-bit RGB regardless of the negotiated
clientPixelFormat (pf); update the tight path to respect pf by packing pixels
through the clientPixelFormat before emitting: either convert the image/tile to
the client's pixel format (bitsPerPixel, depth, bigEndian, trueColor fields) and
then run the existing tight encoders, or modify the tight subencoder routines to
use pf when writing pixel bytes (including channel order and
bytes-per-pixel/endianness), and ensure headers/metadata reflect the negotiated
bpp/depth so clients decode correctly.
In `@management/server/types/policy_authorized_users.go`:
- Around line 56-58: The code path inside the policyRuleImpliesLegacySSH &&
targetPeerSSHEnabled branch currently assigns
state.authorizedUsers[auth.Wildcard] = cb.getAllowedUserIDs(), which replaces
any previously added users; change this to merge/union the allowed user IDs into
the existing set instead. Locate the block handling policyRuleImpliesLegacySSH
(the branch where state.sshEnabled is set) and replace the direct assignment
with logic that ensures state.authorizedUsers[auth.Wildcard] exists (initialize
if nil/empty) and then add/union each ID from cb.getAllowedUserIDs() into the
existing collection so earlier matching rules are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d056db7f-92a7-40c8-963b-51901bd9ac49
⛔ Files ignored due to path filters (4)
client/proto/daemon.pb.gois excluded by!**/*.pb.gogo.sumis excluded by!**/*.sumshared/management/proto/management.pb.gois excluded by!**/*.pb.goshared/management/proto/management_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (82)
.github/workflows/wasm-build-validation.ymlclient/cmd/ssh.goclient/cmd/up.goclient/cmd/vnc_agent.goclient/cmd/vnc_flags.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/debug/debug.goclient/internal/engine.goclient/internal/engine_vnc.goclient/internal/engine_vnc_console_freebsd.goclient/internal/engine_vnc_console_linux.goclient/internal/engine_vnc_darwin.goclient/internal/engine_vnc_stub.goclient/internal/engine_vnc_windows.goclient/internal/engine_vnc_x11.goclient/internal/profilemanager/config.goclient/internal/statemanager/manager.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.goclient/ssh/server/executor_windows.goclient/ssh/server/server.goclient/status/status.goclient/status/status_test.goclient/system/info.goclient/ui/client_ui.goclient/ui/const.goclient/ui/event_handler.goclient/vnc/server/agent_windows.goclient/vnc/server/capture_darwin.goclient/vnc/server/capture_dxgi_windows.goclient/vnc/server/capture_fb_freebsd.goclient/vnc/server/capture_fb_linux.goclient/vnc/server/capture_fb_unix.goclient/vnc/server/capture_windows.goclient/vnc/server/capture_x11.goclient/vnc/server/capture_x11_shm_linux.goclient/vnc/server/capture_x11_shm_stub.goclient/vnc/server/coalesce_test.goclient/vnc/server/hextile_test.goclient/vnc/server/input_darwin.goclient/vnc/server/input_uinput_unix.goclient/vnc/server/input_windows.goclient/vnc/server/input_x11.goclient/vnc/server/keysym_typetext.goclient/vnc/server/rfb.goclient/vnc/server/rfb_bench_test.goclient/vnc/server/server.goclient/vnc/server/server_darwin.goclient/vnc/server/server_stub.goclient/vnc/server/server_test.goclient/vnc/server/server_windows.goclient/vnc/server/server_x11.goclient/vnc/server/session.goclient/vnc/server/shutdown_state.goclient/vnc/server/stubs.goclient/vnc/server/swizzle.goclient/vnc/server/tight_test.goclient/vnc/server/virtual_x11.goclient/wasm/cmd/main.goclient/wasm/internal/rdp/cert_validation.goclient/wasm/internal/rdp/rdcleanpath.goclient/wasm/internal/rdp/rdcleanpath_handlers.goclient/wasm/internal/vnc/proxy.gogo.modmanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/management_proto_test.gomanagement/server/peer/peer.gomanagement/server/policy_test.gomanagement/server/types/account.gomanagement/server/types/network.gomanagement/server/types/networkmap_components.gomanagement/server/types/policy.gomanagement/server/types/policy_authorized_users.goshared/auth/jwt/token_age.goshared/management/client/grpc.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/management.proto
💤 Files with no reviewable changes (3)
- client/wasm/internal/rdp/cert_validation.go
- client/wasm/internal/rdp/rdcleanpath.go
- client/wasm/internal/rdp/rdcleanpath_handlers.go
✅ Files skipped from review due to trivial changes (5)
- client/internal/engine_vnc_windows.go
- client/vnc/server/stubs.go
- client/internal/statemanager/manager.go
- client/status/status_test.go
- shared/management/http/api/types.gen.go
🚧 Files skipped from review as they are similar to previous changes (70)
- client/ssh/server/executor_windows.go
- client/internal/connect.go
- client/internal/auth/auth.go
- client/ui/const.go
- client/internal/engine_vnc_darwin.go
- shared/management/http/api/openapi.yml
- client/internal/engine_vnc_x11.go
- client/internal/engine_vnc_stub.go
- shared/management/client/grpc.go
- management/internals/shared/grpc/server.go
- client/cmd/vnc_flags.go
- client/vnc/server/keysym_typetext.go
- management/server/types/policy.go
- client/server/setconfig_test.go
- client/vnc/server/server_x11.go
- go.mod
- client/ui/event_handler.go
- client/vnc/server/swizzle.go
- client/vnc/server/capture_fb_linux.go
- .github/workflows/wasm-build-validation.yml
- client/proto/daemon.proto
- client/system/info.go
- client/vnc/server/hextile_test.go
- client/vnc/server/server_stub.go
- client/status/status.go
- client/vnc/server/server_darwin.go
- shared/auth/jwt/token_age.go
- client/vnc/server/capture_x11_shm_stub.go
- client/cmd/ssh.go
- client/wasm/cmd/main.go
- client/internal/debug/debug.go
- client/vnc/server/shutdown_state.go
- client/internal/profilemanager/config.go
- client/ui/client_ui.go
- client/vnc/server/capture_fb_freebsd.go
- client/ssh/server/server.go
- client/vnc/server/server_test.go
- client/vnc/server/coalesce_test.go
- client/vnc/server/capture_x11_shm_linux.go
- client/cmd/up.go
- client/internal/engine_vnc_console_linux.go
- client/server/server.go
- management/server/peer/peer.go
- client/internal/engine_vnc_console_freebsd.go
- management/server/policy_test.go
- management/internals/shared/grpc/conversion.go
- shared/management/proto/management.proto
- client/vnc/server/capture_darwin.go
- client/vnc/server/capture_fb_unix.go
- management/server/http/handlers/peers/peers_handler.go
- management/server/types/account.go
- client/vnc/server/capture_windows.go
- client/vnc/server/server.go
- management/server/management_proto_test.go
- client/vnc/server/capture_dxgi_windows.go
- client/vnc/server/server_windows.go
- client/wasm/internal/vnc/proxy.go
- client/cmd/vnc_agent.go
- client/vnc/server/input_x11.go
- client/vnc/server/session.go
- client/vnc/server/agent_windows.go
- client/vnc/server/virtual_x11.go
- client/vnc/server/input_windows.go
- client/vnc/server/rfb_bench_test.go
- client/vnc/server/tight_test.go
- client/internal/engine_vnc.go
- client/vnc/server/input_darwin.go
- management/server/types/networkmap_components.go
- client/internal/engine.go
- client/vnc/server/input_uinput_unix.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
client/vnc/server/input_darwin.go (1)
149-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefcount the global idle-sleep assertion.
The assertion state is process-global, but every injector acquires it (line 221) and every
Close()releases it (line 444). With two concurrent VNC sessions, the first disconnect clears the assertion while the second session is still active, allowing the display to go idle mid-session.🔒 Proposed fix: add a reference counter
var ( pmMu sync.Mutex userActivityID uint32 preventSleepID uint32 preventSleepHeld bool + preventSleepRefCount int ) func holdPreventIdleSleep() { if iopmAssertionCreateWithName == nil || pmPreventIdleDisplayCFStr == 0 || pmAssertionNameCFStr == 0 { return } pmMu.Lock() defer pmMu.Unlock() - if preventSleepHeld { + preventSleepRefCount++ + if preventSleepRefCount > 1 { + // Already held by another session return } var id uint32 r := iopmAssertionCreateWithName(pmPreventIdleDisplayCFStr, kIOPMAssertionLevelOn, pmAssertionNameCFStr, &id) if r != 0 { log.Debugf("IOPMAssertionCreateWithName returned %d", r) + preventSleepRefCount-- return } preventSleepID = id preventSleepHeld = true } func releasePreventIdleSleep() { if iopmAssertionRelease == nil { return } pmMu.Lock() defer pmMu.Unlock() - if !preventSleepHeld { + if preventSleepRefCount <= 0 { return } + preventSleepRefCount-- + if preventSleepRefCount > 0 { + // Other sessions still active + return + } if r := iopmAssertionRelease(preventSleepID); r != 0 { log.Debugf("IOPMAssertionRelease returned %d", r) } preventSleepHeld = false preventSleepID = 0 }Also applies to: 221-221, 443-445
🤖 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 `@client/vnc/server/input_darwin.go` around lines 149 - 185, holdPreventIdleSleep and releasePreventIdleSleep must be changed to use a process-global reference counter instead of a boolean so multiple sessions don't yank the assertion: add a guarded integer (e.g. preventSleepRef) protected by pmMu, increment it in holdPreventIdleSleep and only call iopmAssertionCreateWithName when the counter transitions from 0→1 (then set preventSleepID and preventSleepHeld), and in releasePreventIdleSleep decrement the counter and only call iopmAssertionRelease when the counter transitions to 0 (then clear preventSleepID/preventSleepHeld); keep existing nil checks for the iopm symbols and error logging behavior, and ensure all access to preventSleepRef, preventSleepHeld and preventSleepID is done under pmMu.
🧹 Nitpick comments (4)
client/vnc/server/capture_windows.go (1)
397-517: ⚖️ Poor tradeoffConsider extracting
prepCapturerandcloseCaptureras methods.SonarCloud flags cognitive complexity (35) and line count (103). The nested closures
createCapturer,prepCapturer, andcloseCapturercould be extracted as private methods onDesktopCapturerto reduce complexity while keeping the state management cohesive. This is optional since the current structure keeps related logic together.🤖 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 `@client/vnc/server/capture_windows.go` around lines 397 - 517, The worker function has high cognitive complexity due to nested closures (createCapturer, prepCapturer, closeCapturer) inside DesktopCapturer.worker; extract these closures into private methods on DesktopCapturer (e.g., desktop.createCapturer(), desktop.prepCapturer(), desktop.closeCapturer()) that capture/operate on the same receiver fields (c, cap, nextInitRetry, lastDesktop, desktopFails) or accept/return the necessary state so the logic and state transitions remain identical, update DesktopCapturer.worker to call these methods and remove the nested functions to reduce complexity and line count while preserving behavior.client/vnc/server/capture_fb_linux.go (1)
204-219: 💤 Low valueConsider grouping channel shift parameters into a struct.
The 9 parameters flagged by SonarCloud are functional but could be reduced by passing channel offsets as a small struct. This is optional since the function is internal and only called in one place.
🤖 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 `@client/vnc/server/capture_fb_linux.go` around lines 204 - 219, The swizzleFB32 function has many parameters for channel shifts—replace the separate rShift, gShift, bShift uint32 parameters with a small struct (e.g., type ChannelShifts struct { R, G, B uint32 }) and update swizzleFB32 signature to accept this struct (swizzleFB32(..., shifts ChannelShifts)), then adjust the function body to use shifts.R/ shifts.G/ shifts.B and update the single call site accordingly; this reduces parameter count and keeps behavior unchanged.client/wasm/cmd/main.go (1)
367-424: 💤 Low valueParameter validation is thorough; complexity is acceptable.
The
createVNCProxyMethodhandles 8 optional parameters with type and range validation. SonarCloud's complexity flag (36) reflects the necessary validation logic. Consider extracting into aparseVNCProxyArgshelper if more parameters are added in the future.🤖 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 `@client/wasm/cmd/main.go` around lines 367 - 424, The function createVNCProxyMethod is flagged for complexity due to extensive CLI/JS arg parsing; extract the argument parsing into a new helper parseVNCProxyArgs that accepts []js.Value and returns (hostname string, port string, mode string, username string, jwtToken string, sessionID uint32, width uint16, height uint16, err error). Move all type/range checks and defaulting logic into parseVNCProxyArgs, have it return a descriptive error for invalid input, then update createVNCProxyMethod to call parseVNCProxyArgs, reject the promise on error, and pass the returned values into vnc.NewVNCProxy(client) and proxy.CreateProxy; keep function names createVNCProxyMethod, parseVNCProxyArgs, vnc.NewVNCProxy, and proxy.CreateProxy as references.client/vnc/server/input_darwin.go (1)
271-337: 💤 Low valueSonarCloud flags cognitive complexity 23; consider extracting button-handling logic.
The
InjectPointermethod handles coordinate scaling, button state diffing, and scroll events in one function. Extracting the button-state diff into a helper would reduce complexity. This is optional since the current flow is readable.🤖 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 `@client/vnc/server/input_darwin.go` around lines 271 - 337, The InjectPointer method is doing coordinate scaling, button-state diffing, and scroll handling, causing high cognitive complexity; extract the button diff and event dispatch into a helper (e.g., a new method on MacInputInjector like handleButtonState(buttonMask uint8, prevMask uint8, src C.CGEventSourceRef, x, y float64)) that compares m.lastButtons with buttonMask and calls postMouse/postScroll accordingly (handling left/right/middle down/up and mapping to kCGEvent* and kCGMouseButton* constants), then call that helper from InjectPointer and update m.lastButtons there to reduce logic in InjectPointer. Ensure the helper uses postMouse and postScroll and preserves the existing drag/move behavior decisions made in InjectPointer.
🤖 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 `@management/server/types/account.go`:
- Around line 908-911: The peer-resource branch bypasses group-based validation
(validatedPeersMap and postureChecks) by directly calling a.getPeerFromResource;
update the ResourceTypePeer path so it applies the same filters as the group
path—either extend getPeerFromResource to accept validatedPeersMap,
postureChecks, ctx and peerID (and perform the same validation logic), or call
a.getPeerFromResource then run the same validation/posture check logic (using
validatedPeersMap, postureChecks, peerID and ctx) before returning the peer;
ensure the final behavior matches a.getAllPeersFromGroups filtering so
single-peer resources cannot bypass validation.
In `@management/server/types/policy_authorized_users.go`:
- Around line 47-59: The code currently returns early when peerInDestinations is
false, so for bidirectional rules the source side never gets SSH/VNC auth
collected; change the logic to handle both directions: don't return
immediately—compute whether the peer is in destinations OR sources (or check the
rule's bidirectional nature from emitRuleDirections), and when the peer is in
the source side for a bidirectional rule also call cb.collectSSHUsers(rule,
state.authorizedUsers) and cb.collectVNCUsers(rule, state.vncAuthorizedUsers)
and set state.sshEnabled/ state.authorizedUsers[auth.Wildcard] (using
cb.getAllowedUserIDs()) the same way you do for the destination side so
source-side peers receive the same SSH/VNC authorization metadata.
---
Duplicate comments:
In `@client/vnc/server/input_darwin.go`:
- Around line 149-185: holdPreventIdleSleep and releasePreventIdleSleep must be
changed to use a process-global reference counter instead of a boolean so
multiple sessions don't yank the assertion: add a guarded integer (e.g.
preventSleepRef) protected by pmMu, increment it in holdPreventIdleSleep and
only call iopmAssertionCreateWithName when the counter transitions from 0→1
(then set preventSleepID and preventSleepHeld), and in releasePreventIdleSleep
decrement the counter and only call iopmAssertionRelease when the counter
transitions to 0 (then clear preventSleepID/preventSleepHeld); keep existing nil
checks for the iopm symbols and error logging behavior, and ensure all access to
preventSleepRef, preventSleepHeld and preventSleepID is done under pmMu.
---
Nitpick comments:
In `@client/vnc/server/capture_fb_linux.go`:
- Around line 204-219: The swizzleFB32 function has many parameters for channel
shifts—replace the separate rShift, gShift, bShift uint32 parameters with a
small struct (e.g., type ChannelShifts struct { R, G, B uint32 }) and update
swizzleFB32 signature to accept this struct (swizzleFB32(..., shifts
ChannelShifts)), then adjust the function body to use shifts.R/ shifts.G/
shifts.B and update the single call site accordingly; this reduces parameter
count and keeps behavior unchanged.
In `@client/vnc/server/capture_windows.go`:
- Around line 397-517: The worker function has high cognitive complexity due to
nested closures (createCapturer, prepCapturer, closeCapturer) inside
DesktopCapturer.worker; extract these closures into private methods on
DesktopCapturer (e.g., desktop.createCapturer(), desktop.prepCapturer(),
desktop.closeCapturer()) that capture/operate on the same receiver fields (c,
cap, nextInitRetry, lastDesktop, desktopFails) or accept/return the necessary
state so the logic and state transitions remain identical, update
DesktopCapturer.worker to call these methods and remove the nested functions to
reduce complexity and line count while preserving behavior.
In `@client/vnc/server/input_darwin.go`:
- Around line 271-337: The InjectPointer method is doing coordinate scaling,
button-state diffing, and scroll handling, causing high cognitive complexity;
extract the button diff and event dispatch into a helper (e.g., a new method on
MacInputInjector like handleButtonState(buttonMask uint8, prevMask uint8, src
C.CGEventSourceRef, x, y float64)) that compares m.lastButtons with buttonMask
and calls postMouse/postScroll accordingly (handling left/right/middle down/up
and mapping to kCGEvent* and kCGMouseButton* constants), then call that helper
from InjectPointer and update m.lastButtons there to reduce logic in
InjectPointer. Ensure the helper uses postMouse and postScroll and preserves the
existing drag/move behavior decisions made in InjectPointer.
In `@client/wasm/cmd/main.go`:
- Around line 367-424: The function createVNCProxyMethod is flagged for
complexity due to extensive CLI/JS arg parsing; extract the argument parsing
into a new helper parseVNCProxyArgs that accepts []js.Value and returns
(hostname string, port string, mode string, username string, jwtToken string,
sessionID uint32, width uint16, height uint16, err error). Move all type/range
checks and defaulting logic into parseVNCProxyArgs, have it return a descriptive
error for invalid input, then update createVNCProxyMethod to call
parseVNCProxyArgs, reject the promise on error, and pass the returned values
into vnc.NewVNCProxy(client) and proxy.CreateProxy; keep function names
createVNCProxyMethod, parseVNCProxyArgs, vnc.NewVNCProxy, and proxy.CreateProxy
as references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d30ed1d5-3d17-4363-a5d6-46da54feafb5
⛔ Files ignored due to path filters (4)
client/proto/daemon.pb.gois excluded by!**/*.pb.gogo.sumis excluded by!**/*.sumshared/management/proto/management.pb.gois excluded by!**/*.pb.goshared/management/proto/management_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (82)
.github/workflows/wasm-build-validation.ymlclient/cmd/ssh.goclient/cmd/up.goclient/cmd/vnc_agent.goclient/cmd/vnc_flags.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/debug/debug.goclient/internal/engine.goclient/internal/engine_vnc.goclient/internal/engine_vnc_console_freebsd.goclient/internal/engine_vnc_console_linux.goclient/internal/engine_vnc_darwin.goclient/internal/engine_vnc_stub.goclient/internal/engine_vnc_windows.goclient/internal/engine_vnc_x11.goclient/internal/profilemanager/config.goclient/internal/statemanager/manager.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.goclient/ssh/server/executor_windows.goclient/ssh/server/server.goclient/status/status.goclient/status/status_test.goclient/system/info.goclient/ui/client_ui.goclient/ui/const.goclient/ui/event_handler.goclient/vnc/server/agent_windows.goclient/vnc/server/capture_darwin.goclient/vnc/server/capture_dxgi_windows.goclient/vnc/server/capture_fb_freebsd.goclient/vnc/server/capture_fb_linux.goclient/vnc/server/capture_fb_unix.goclient/vnc/server/capture_windows.goclient/vnc/server/capture_x11.goclient/vnc/server/capture_x11_shm_linux.goclient/vnc/server/capture_x11_shm_stub.goclient/vnc/server/coalesce_test.goclient/vnc/server/hextile_test.goclient/vnc/server/input_darwin.goclient/vnc/server/input_uinput_unix.goclient/vnc/server/input_windows.goclient/vnc/server/input_x11.goclient/vnc/server/keysym_typetext.goclient/vnc/server/rfb.goclient/vnc/server/rfb_bench_test.goclient/vnc/server/server.goclient/vnc/server/server_darwin.goclient/vnc/server/server_stub.goclient/vnc/server/server_test.goclient/vnc/server/server_windows.goclient/vnc/server/server_x11.goclient/vnc/server/session.goclient/vnc/server/shutdown_state.goclient/vnc/server/stubs.goclient/vnc/server/swizzle.goclient/vnc/server/tight_test.goclient/vnc/server/virtual_x11.goclient/wasm/cmd/main.goclient/wasm/internal/rdp/cert_validation.goclient/wasm/internal/rdp/rdcleanpath.goclient/wasm/internal/rdp/rdcleanpath_handlers.goclient/wasm/internal/vnc/proxy.gogo.modmanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/management_proto_test.gomanagement/server/peer/peer.gomanagement/server/policy_test.gomanagement/server/types/account.gomanagement/server/types/network.gomanagement/server/types/networkmap_components.gomanagement/server/types/policy.gomanagement/server/types/policy_authorized_users.goshared/auth/jwt/token_age.goshared/management/client/grpc.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/management.proto
💤 Files with no reviewable changes (3)
- client/wasm/internal/rdp/rdcleanpath.go
- client/wasm/internal/rdp/rdcleanpath_handlers.go
- client/wasm/internal/rdp/cert_validation.go
✅ Files skipped from review due to trivial changes (6)
- client/ssh/server/executor_windows.go
- client/ui/const.go
- client/internal/auth/auth.go
- client/internal/engine_vnc_windows.go
- client/internal/statemanager/manager.go
- .github/workflows/wasm-build-validation.yml
🚧 Files skipped from review as they are similar to previous changes (55)
- client/internal/engine_vnc_console_linux.go
- client/vnc/server/capture_x11_shm_stub.go
- shared/management/client/grpc.go
- management/internals/shared/grpc/server.go
- client/cmd/vnc_flags.go
- client/internal/connect.go
- client/internal/engine_vnc_stub.go
- client/internal/debug/debug.go
- client/internal/engine_vnc_console_freebsd.go
- go.mod
- management/server/types/network.go
- client/ssh/server/server.go
- client/vnc/server/server_test.go
- management/server/types/policy.go
- management/server/peer/peer.go
- management/internals/shared/grpc/conversion.go
- shared/auth/jwt/token_age.go
- client/system/info.go
- shared/management/proto/management.proto
- client/vnc/server/server_darwin.go
- client/internal/engine_vnc_x11.go
- client/cmd/ssh.go
- management/server/management_proto_test.go
- client/internal/profilemanager/config.go
- client/vnc/server/capture_fb_freebsd.go
- client/cmd/up.go
- client/server/setconfig_test.go
- shared/management/http/api/openapi.yml
- client/vnc/server/server_x11.go
- management/server/http/handlers/peers/peers_handler.go
- client/vnc/server/keysym_typetext.go
- client/ui/client_ui.go
- client/server/server.go
- client/status/status_test.go
- client/status/status.go
- client/internal/engine_vnc_darwin.go
- client/ui/event_handler.go
- client/vnc/server/hextile_test.go
- client/vnc/server/capture_fb_unix.go
- client/vnc/server/capture_dxgi_windows.go
- shared/management/http/api/types.gen.go
- management/server/policy_test.go
- client/proto/daemon.proto
- management/server/types/networkmap_components.go
- client/internal/engine.go
- client/vnc/server/input_x11.go
- client/cmd/vnc_agent.go
- client/vnc/server/tight_test.go
- client/vnc/server/capture_x11.go
- client/vnc/server/shutdown_state.go
- client/vnc/server/rfb_bench_test.go
- client/vnc/server/coalesce_test.go
- client/vnc/server/input_windows.go
- client/internal/engine_vnc.go
- client/vnc/server/session.go
lixmal
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
client/vnc/server/input_darwin.go (1)
151-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefcount the process-global idle-sleep assertion.
preventSleepHeld/preventSleepIDare global, but each injector acquires them and eachClose()releases them. With two VNC sessions, the first disconnect clears the assertion while the second session is still active, so the display can go idle mid-session.Also applies to: 221-224, 443-445
🤖 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 `@client/vnc/server/input_darwin.go` around lines 151 - 184, The global idle-sleep assertion is not refcounted so one Close() can drop it while other sessions still need it; update holdPreventIdleSleep and releasePreventIdleSleep to maintain an integer refcount (e.g., preventSleepRefCount) protected by pmMu: in holdPreventIdleSleep increment the refcount and only call iopmAssertionCreateWithName (set preventSleepID/preventSleepHeld) when refcount transitions 0->1, and in releasePreventIdleSleep decrement the refcount and only call iopmAssertionRelease (and clear preventSleepID/preventSleepHeld) when refcount transitions 1->0; keep existing nil checks for iopmAssertionCreateWithName/iopmAssertionRelease and preserve logging on nonzero return codes.management/server/types/account.go (1)
908-911:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPeer-resource resolution still bypasses the normal peer filters.
This branch returns the target peer directly, so single-peer resources skip
validatedPeersMapand source posture checks that the group path applies. A peer that should have been filtered out can still match when referenced as a resource.🤖 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 `@management/server/types/account.go` around lines 908 - 911, The branch that returns a single peer when resource.Type == ResourceTypePeer bypasses the normal filtering pipeline (validatedPeersMap and postureChecks); update the logic so the single-peer path enforces the same validations as getAllPeersFromGroups: either have a.getPeerFromResource consult validatedPeersMap and run postureChecks/group membership before returning, or replace the direct return with a call into a.getAllPeersFromGroups (or a shared helper) that filters the single peer through validatedPeersMap and postureChecks; ensure references to ResourceTypePeer, a.getPeerFromResource, a.getAllPeersFromGroups, validatedPeersMap, and postureChecks are used so the returned peer cannot circumvent group/path/posture filtering.client/vnc/server/rfb.go (1)
701-710:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTight still bypasses the negotiated pixel format.
encodeTightFill,encodeTightJPEG, andencodeTightBasicalways emit 24-bit RGB, so clients that requested 16bpp or different channel shifts/endian will decode Tight rectangles incorrectly. Please either pack Tight pixels throughpfor fall back to a pixel-format-aware encoder when the negotiated format is not Tight-compatible.Also applies to: 738-825
🤖 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 `@client/vnc/server/rfb.go` around lines 701 - 710, encodeTightRect is emitting 24-bit RGB unconditionally (via encodeTightFill, encodeTightJPEG, encodeTightBasic) which ignores the negotiated clientPixelFormat (pf); update the tight encoders to respect pf by either (a) changing encodeTightFill, encodeTightJPEG, and encodeTightBasic signatures to accept pf (and tightState) and emit packed pixels according to pf.bitsPerPixel, pf.depth, pf.red/green/blueShift and endianness, or (b) convert the image into a temporary buffer already packed to pf before calling the existing encoders and fall back to a pixel-format-aware encoder when the format cannot be expressed in 24bpp; ensure sampledColorCountInto/tightJPEGMin checks still operate on source RGBA but that final output bytes match the clientPixelFormat semantics.
🧹 Nitpick comments (1)
management/server/types/policy_authorized_users.go (1)
34-44: ⚖️ Poor tradeoffConsider grouping parameters to reduce function signature complexity.
Static analysis flags 9 parameters (exceeds the recommended 7). The peer-related parameters could be grouped into a struct:
type peerResolutionContext struct { sourcePeers []*nbpeer.Peer destPeers []*nbpeer.Peer peerInSources bool peerInDestinations bool }This would reduce the signature to 6 parameters and improve readability at call sites.
🤖 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 `@management/server/types/policy_authorized_users.go` around lines 34 - 44, The applyResolvedRuleToState function signature is too long; define a new struct (e.g., peerResolutionContext) containing sourcePeers []*nbpeer.Peer, destPeers []*nbpeer.Peer, peerInSources bool, and peerInDestinations bool, then replace those four parameters in applyResolvedRuleToState with a single ctx *peerResolutionContext parameter; update all call sites that invoke applyResolvedRuleToState to construct and pass the new context, keeping the other parameters (rule, targetPeerSSHEnabled, generateResources, cb, state) unchanged and adjust any uses inside applyResolvedRuleToState to reference ctx fields.
🤖 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 `@client/vnc/server/capture_fb_linux.go`:
- Around line 191-200: Close() currently unmaps and closes resources without
acquiring the same mutex used by CaptureInto(), which can race on c.mmap; modify
Close() (the closeOnce.Do closure) to acquire c.mu (same lock used in
CaptureInto()) around reads/writes to c.mmap and c.fd so Munmap and Close happen
while holding c.mu, ensuring Close() is serialized with CaptureInto(); keep the
closeOnce.Do semantics but move/unify the resource cleanup under c.mu to prevent
unmapping during an in-flight capture.
In `@client/vnc/server/capture_windows.go`:
- Around line 320-345: Width() and Height() currently call Capture()
synchronously when w/h == 0 which can block because Capture() sends on the
unbuffered reqCh while the worker may be parked in waitForClient(); instead,
avoid blocking by triggering a non-blocking request: replace the synchronous
call with a non-blocking send to c.reqCh (e.g. select { case c.reqCh <-
struct{}{}: default: }) or otherwise fire the request in a goroutine that
performs a non-blocking send, then re-read c.w / c.h under the mutex and return;
update the logic in Width(), Height() (and any helper Capture() invocation) to
reference c.reqCh, c.w, c.h and c.mu accordingly.
In `@client/vnc/server/input_uinput_unix.go`:
- Around line 221-222: The current mapping for absXVal and absYVal uses
serverW/serverH as the divisor which makes the max RFB coordinate never map to
screenW-1/screenH-1; change the scaling to use (serverW-1) and (serverH-1) as
denominators so the range 0..serverW-1 maps inclusively to 0..u.screenW-1:
update the expressions for absXVal and absYVal (the lines computing int32(x *
(u.screenW - 1) / serverW) and int32(y * (u.screenH - 1) / serverH)) to use
(serverW - 1) and (serverH - 1) respectively, and add guards for serverW <= 1 /
serverH <= 1 to avoid division-by-zero (fallback to 0 or clamp as appropriate).
- Around line 1-28: The build tag in input_uinput_unix.go currently includes
freebsd but the file contains Linux-specific uinput ioctl numbers and device
semantics (see constants uiDevCreate, uiDevSetup, uiSetKeyBit, uiSetAbsBit,
uinputAbsSize and the use of /dev/uinput), so narrow the build tag to Linux-only
by removing "freebsd" (i.e. use the Linux && !android tag) or alternatively
provide a separate FreeBSD-specific implementation/back-end; update the file’s
build constraint accordingly and ensure FreeBSD-specific code does not compile
this Linux uinput file.
In `@client/vnc/server/server_windows.go`:
- Around line 56-72: The enableSoftwareSAS function is persisting a machine-wide
policy by creating/modifying
HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\Policies\System\SoftwareSASGeneration
without restoring the original value; change this so we either treat
SoftwareSASGeneration as an explicit admin prerequisite (skip writing it and
document it) or snapshot the existing value in enableSoftwareSAS (read via
registry.CreateKey/Key.GetDWordValue), store it in a package-level variable, and
restore it on shutdown via a new disableSoftwareSAS cleanup function that calls
Key.SetDWordValue or deletes the value as appropriate; update startup/shutdown
flow to call these helpers and ensure errors are logged, not silently leaving
the system changed.
In `@client/vnc/server/server.go`:
- Around line 617-641: The current logic silently ignores oversized JWT length
values causing stream desync; update the JWT parsing block (variables jwtLenBuf,
jwtLen, jwtToken) to explicitly reject/return an error when jwtLen is >= 8192
(or otherwise out of expected bounds) instead of skipping reading the bytes, so
the connection/handshake fails fast and the subsequent reads for sessionID
(sidBuf, sessionID) and geometry (geomBuf, width, height) remain synchronized.
In `@client/vnc/server/virtual_x11.go`:
- Around line 250-254: The current Xvfb/Xorg-dummy invocations in the virtual
display startup (the exec.Command assigned to vs.xvfb and the similar command
later around the other block) use the "-ac" flag which disables X11 access
control; remove "-ac" and instead generate and apply an xauth cookie for the
session user: create a temporary Xauthority (or use XAUTHORITY env) and run
xauth to add a cookie for the display before starting Xvfb/Xorg-dummy, ensure
the child process inherits XAUTHORITY and only that per-session authority is
granted (do not use global /tmp sockets without auth), and mirror the same
change for the other command block referenced (around the second invocation).
In `@client/wasm/cmd/main.go`:
- Around line 421-445: The argument parsing currently silently defaults invalid
values; validate and reject bad inputs instead: ensure p.mode (args[2]) is
explicitly "attach" or "session" and if not, return/throw an error to the
caller; validate p.sessionID (args[5]) is within 0..0xFFFFFFFF and reject if out
of range; validate p.width and p.height (args[6], args[7]) are within 1..0xFFFF
and reject if out of range; when rejecting, do not fall back to
defaults—propagate an error/throw a JS exception so callers know the input was
invalid. Use the existing p.* fields (p.mode, p.username, p.jwt, p.sessionID,
p.width, p.height) to locate the code to change.
In `@client/wasm/internal/vnc/proxy.go`:
- Around line 85-87: The address for the VNC destination is constructed
incorrectly using fmt.Sprintf in VNCProxy.CreateProxy; replace the
fmt.Sprintf("%s:%s", hostname, port) usage with net.JoinHostPort(hostname, port)
so IPv6 literals are handled correctly (update the address variable in the
CreateProxy method and add the net import if missing).
---
Duplicate comments:
In `@client/vnc/server/input_darwin.go`:
- Around line 151-184: The global idle-sleep assertion is not refcounted so one
Close() can drop it while other sessions still need it; update
holdPreventIdleSleep and releasePreventIdleSleep to maintain an integer refcount
(e.g., preventSleepRefCount) protected by pmMu: in holdPreventIdleSleep
increment the refcount and only call iopmAssertionCreateWithName (set
preventSleepID/preventSleepHeld) when refcount transitions 0->1, and in
releasePreventIdleSleep decrement the refcount and only call
iopmAssertionRelease (and clear preventSleepID/preventSleepHeld) when refcount
transitions 1->0; keep existing nil checks for
iopmAssertionCreateWithName/iopmAssertionRelease and preserve logging on nonzero
return codes.
In `@client/vnc/server/rfb.go`:
- Around line 701-710: encodeTightRect is emitting 24-bit RGB unconditionally
(via encodeTightFill, encodeTightJPEG, encodeTightBasic) which ignores the
negotiated clientPixelFormat (pf); update the tight encoders to respect pf by
either (a) changing encodeTightFill, encodeTightJPEG, and encodeTightBasic
signatures to accept pf (and tightState) and emit packed pixels according to
pf.bitsPerPixel, pf.depth, pf.red/green/blueShift and endianness, or (b) convert
the image into a temporary buffer already packed to pf before calling the
existing encoders and fall back to a pixel-format-aware encoder when the format
cannot be expressed in 24bpp; ensure sampledColorCountInto/tightJPEGMin checks
still operate on source RGBA but that final output bytes match the
clientPixelFormat semantics.
In `@management/server/types/account.go`:
- Around line 908-911: The branch that returns a single peer when resource.Type
== ResourceTypePeer bypasses the normal filtering pipeline (validatedPeersMap
and postureChecks); update the logic so the single-peer path enforces the same
validations as getAllPeersFromGroups: either have a.getPeerFromResource consult
validatedPeersMap and run postureChecks/group membership before returning, or
replace the direct return with a call into a.getAllPeersFromGroups (or a shared
helper) that filters the single peer through validatedPeersMap and
postureChecks; ensure references to ResourceTypePeer, a.getPeerFromResource,
a.getAllPeersFromGroups, validatedPeersMap, and postureChecks are used so the
returned peer cannot circumvent group/path/posture filtering.
---
Nitpick comments:
In `@management/server/types/policy_authorized_users.go`:
- Around line 34-44: The applyResolvedRuleToState function signature is too
long; define a new struct (e.g., peerResolutionContext) containing sourcePeers
[]*nbpeer.Peer, destPeers []*nbpeer.Peer, peerInSources bool, and
peerInDestinations bool, then replace those four parameters in
applyResolvedRuleToState with a single ctx *peerResolutionContext parameter;
update all call sites that invoke applyResolvedRuleToState to construct and pass
the new context, keeping the other parameters (rule, targetPeerSSHEnabled,
generateResources, cb, state) unchanged and adjust any uses inside
applyResolvedRuleToState to reference ctx fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e82289e-6ace-4a57-ac88-78584e860d84
⛔ Files ignored due to path filters (4)
client/proto/daemon.pb.gois excluded by!**/*.pb.gogo.sumis excluded by!**/*.sumshared/management/proto/management.pb.gois excluded by!**/*.pb.goshared/management/proto/management_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (79)
.github/workflows/wasm-build-validation.ymlclient/cmd/up.goclient/cmd/vnc_agent.goclient/cmd/vnc_flags.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/debug/debug.goclient/internal/debug/debug_test.goclient/internal/engine.goclient/internal/engine_vnc.goclient/internal/engine_vnc_console_freebsd.goclient/internal/engine_vnc_console_linux.goclient/internal/engine_vnc_darwin.goclient/internal/engine_vnc_stub.goclient/internal/engine_vnc_windows.goclient/internal/engine_vnc_x11.goclient/internal/profilemanager/config.goclient/internal/statemanager/manager.goclient/proto/daemon.protoclient/server/server.goclient/server/setconfig_test.goclient/ssh/server/executor_windows.goclient/ssh/server/server.goclient/status/status.goclient/status/status_test.goclient/system/info.goclient/ui/client_ui.goclient/ui/const.goclient/ui/event_handler.goclient/vnc/server/agent_windows.goclient/vnc/server/capture_darwin.goclient/vnc/server/capture_dxgi_windows.goclient/vnc/server/capture_fb_freebsd.goclient/vnc/server/capture_fb_linux.goclient/vnc/server/capture_fb_unix.goclient/vnc/server/capture_windows.goclient/vnc/server/capture_x11.goclient/vnc/server/capture_x11_shm_linux.goclient/vnc/server/capture_x11_shm_stub.goclient/vnc/server/coalesce_test.goclient/vnc/server/hextile_test.goclient/vnc/server/input_darwin.goclient/vnc/server/input_uinput_unix.goclient/vnc/server/input_windows.goclient/vnc/server/input_x11.goclient/vnc/server/keysym_typetext.goclient/vnc/server/rfb.goclient/vnc/server/rfb_bench_test.goclient/vnc/server/server.goclient/vnc/server/server_darwin.goclient/vnc/server/server_stub.goclient/vnc/server/server_test.goclient/vnc/server/server_windows.goclient/vnc/server/server_x11.goclient/vnc/server/session.goclient/vnc/server/shutdown_state.goclient/vnc/server/stubs.goclient/vnc/server/swizzle.goclient/vnc/server/tight_test.goclient/vnc/server/virtual_x11.goclient/wasm/cmd/main.goclient/wasm/internal/vnc/proxy.gogo.modmanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/server.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/management_proto_test.gomanagement/server/peer/peer.gomanagement/server/policy_test.gomanagement/server/types/account.gomanagement/server/types/network.gomanagement/server/types/networkmap_components.gomanagement/server/types/policy.gomanagement/server/types/policy_authorized_users.goshared/auth/jwt/token_age.goshared/management/client/grpc.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.goshared/management/proto/management.proto
✅ Files skipped from review due to trivial changes (6)
- client/ui/const.go
- client/internal/statemanager/manager.go
- client/vnc/server/stubs.go
- client/ssh/server/executor_windows.go
- client/status/status_test.go
- shared/management/http/api/types.gen.go
🚧 Files skipped from review as they are similar to previous changes (55)
- client/vnc/server/capture_x11_shm_stub.go
- management/server/types/network.go
- client/internal/engine_vnc_console_linux.go
- shared/management/client/grpc.go
- client/internal/engine_vnc_windows.go
- client/cmd/vnc_flags.go
- management/server/http/handlers/peers/peers_handler.go
- client/system/info.go
- client/internal/debug/debug.go
- client/internal/engine_vnc_stub.go
- client/vnc/server/coalesce_test.go
- client/ui/event_handler.go
- client/internal/engine_vnc_console_freebsd.go
- client/server/server.go
- management/server/types/policy.go
- client/internal/engine_vnc_x11.go
- client/internal/profilemanager/config.go
- client/vnc/server/server_darwin.go
- client/vnc/server/server_x11.go
- management/internals/shared/grpc/server.go
- client/server/setconfig_test.go
- client/internal/connect.go
- client/vnc/server/keysym_typetext.go
- shared/management/proto/management.proto
- client/vnc/server/swizzle.go
- client/ui/client_ui.go
- client/ssh/server/server.go
- management/internals/shared/grpc/conversion.go
- client/internal/engine_vnc_darwin.go
- client/vnc/server/capture_dxgi_windows.go
- .github/workflows/wasm-build-validation.yml
- client/vnc/server/capture_fb_unix.go
- management/server/peer/peer.go
- go.mod
- management/server/types/networkmap_components.go
- client/status/status.go
- client/vnc/server/server_stub.go
- client/vnc/server/capture_x11_shm_linux.go
- client/vnc/server/shutdown_state.go
- client/proto/daemon.proto
- client/vnc/server/server_test.go
- client/vnc/server/hextile_test.go
- shared/auth/jwt/token_age.go
- management/server/policy_test.go
- client/internal/engine.go
- client/vnc/server/rfb_bench_test.go
- client/vnc/server/agent_windows.go
- client/vnc/server/input_windows.go
- client/vnc/server/capture_fb_freebsd.go
- client/vnc/server/input_x11.go
- client/cmd/vnc_agent.go
- client/vnc/server/capture_x11.go
- client/vnc/server/tight_test.go
- client/internal/engine_vnc.go
- client/vnc/server/session.go
lixmal
force-pushed
the
embedded-vnc
branch
3 times, most recently
from
May 16, 2026 14:35
55d0637 to
bfb7840
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…, close the handler-drain race
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…me from tearing down the capturer Claude-Session: https://claude.ai/code/session_01QKDYfH4WKLbpNQHccpVo3P
There was a problem hiding this comment.
All reported issues were addressed across 17 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…der one claim Claude-Session: https://claude.ai/code/session_01QKDYfH4WKLbpNQHccpVo3P
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/vnc/server/server.go">
<violation number="1" location="client/vnc/server/server.go:1304">
P2: When `Accept` returns a temporary error outside these three errno values, `acceptLoop` exits and stops accepting later VNC connections. Treat temporary `net.Error` failures as retryable before terminating the listener.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // livelock: Android has been seen returning EINVAL from accept4 for the life of | ||
| // an otherwise healthy listening socket. | ||
| func acceptRetryable(err error) bool { | ||
| return errors.Is(err, syscall.ECONNABORTED) || |
There was a problem hiding this comment.
P2: When Accept returns a temporary error outside these three errno values, acceptLoop exits and stops accepting later VNC connections. Treat temporary net.Error failures as retryable before terminating the listener.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/vnc/server/server.go, line 1304:
<comment>When `Accept` returns a temporary error outside these three errno values, `acceptLoop` exits and stops accepting later VNC connections. Treat temporary `net.Error` failures as retryable before terminating the listener.</comment>
<file context>
@@ -1231,3 +1278,30 @@ func modeString(m byte) string {
+// livelock: Android has been seen returning EINVAL from accept4 for the life of
+// an otherwise healthy listening socket.
+func acceptRetryable(err error) bool {
+ return errors.Is(err, syscall.ECONNABORTED) ||
+ errors.Is(err, syscall.EMFILE) ||
+ errors.Is(err, syscall.ENFILE)
</file context>
|
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/internal/engine.go">
<violation number="1" location="client/internal/engine.go:2580">
P2: restartVNCListeners rebuilds the server with empty auth, dropping the management-pushed authorized users and session pubkeys. Unlike restartSSHListeners, which carries AuthConfig over precisely because a rebuilt server starts with an empty authorizer, the VNC restart re-runs startVNCServer without restoring VNC auth, so after a TUN renewal previously-authorized VNC peers are refused until the next network-map sync re-applies it via updateVNCServerAuth. Read and carry over the server's current VNCAuth config across the restart (or re-apply it after startVNCServer).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| func (e *Engine) overlayRebinds() []overlayRebind { | ||
| return []overlayRebind{ | ||
| e.restartSSHListeners, | ||
| e.restartVNCListeners, |
There was a problem hiding this comment.
P2: restartVNCListeners rebuilds the server with empty auth, dropping the management-pushed authorized users and session pubkeys. Unlike restartSSHListeners, which carries AuthConfig over precisely because a rebuilt server starts with an empty authorizer, the VNC restart re-runs startVNCServer without restoring VNC auth, so after a TUN renewal previously-authorized VNC peers are refused until the next network-map sync re-applies it via updateVNCServerAuth. Read and carry over the server's current VNCAuth config across the restart (or re-apply it after startVNCServer).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/internal/engine.go, line 2580:
<comment>restartVNCListeners rebuilds the server with empty auth, dropping the management-pushed authorized users and session pubkeys. Unlike restartSSHListeners, which carries AuthConfig over precisely because a rebuilt server starts with an empty authorizer, the VNC restart re-runs startVNCServer without restoring VNC auth, so after a TUN renewal previously-authorized VNC peers are refused until the next network-map sync re-applies it via updateVNCServerAuth. Read and carry over the server's current VNCAuth config across the restart (or re-apply it after startVNCServer).</comment>
<file context>
@@ -2577,6 +2577,7 @@ func (e *Engine) rebindOverlayListeners() {
func (e *Engine) overlayRebinds() []overlayRebind {
return []overlayRebind{
e.restartSSHListeners,
+ e.restartVNCListeners,
e.restartDNSForwarder,
}
</file context>
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
3 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.
Describe your changes
Adds an opt-in embedded VNC server to the client so peers can be remoted into without installing or running external VNC software. Access is gated per-peer with a "VNC enabled" toggle and per-policy ACLs, with browser-side connections going over the existing WireGuard tunnel.
Highlights:
vnc-agentinto the active WTS session viaCreateProcessAsUser; macOS spawns one vialaunchctl asuserinto the console user's launchd domain. The agents are recycled on session change (logout, fast user switch).vnc-agentsubcommand powering both the Windows and macOS user-session workers.Issue ticket number and link
#6135
Stack
Checklist
Documentation
Select exactly one:
Docs will follow in a separate PR.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit