Conversation
WalkthroughAdds a VNC authentication client and integration test: introduces VNCClient.Connect for password auth, wires a VNC Docker resource into JavaScript integration tests (vnc-pass-brute), updates imports/aliasing for the VNC plugin, and adds an indirect go.mod dependency on github.com/alexsnet/go-vnc. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor JS as JS Runtime
participant VC as VNCClient
participant PS as ProtocolState / Dialers
participant NET as Network (fast dialer)
participant VN as go-vnc (client)
JS->>VC: Connect(ctx, host, port, password)
VC->>PS: IsHostAllowed / GetDialersWithId(executionId)
alt host denied
PS-->>VC: ErrHostDenied
VC-->>JS: (false, ErrHostDenied)
else allowed
VC->>NET: Dial TCP (fast dialer, 10s)
NET-->>VC: net.Conn / error
VC->>VN: vnclib.Connect(conn, cfg{Password})
alt auth fails
VN-->>VC: auth error
VC-->>JS: (false, nil)
else other error
VN-->>VC: error
VC-->>JS: (false, error)
else success
VN-->>VC: client
VC->>VN: Close()
VC-->>JS: (true, nil)
end
end
sequenceDiagram
autonumber
actor TR as Test Runner
participant DP as Docker Pool
participant VC as VNC Container
participant NU as nuclei
participant TM as vnc-pass-brute.yaml
TR->>DP: Start VNC container (dorowu/ubuntu-desktop-lxde-vnc, VNC_PASSWORD=mysecret)
DP-->>TR: Resource (exposes 5900/tcp -> host port)
TR->>TR: Wait ~3s and retry loop
TR->>NU: Execute vnc-pass-brute.yaml on localhost:<port>
NU->>TM: Run checks / brute attempts
TM-->>NU: Results
NU-->>TR: Results (count)
alt exactly one result
TR-->>TR: Mark test pass
else
TR-->>TR: Record error and retry until exhausted
end
TR->>DP: Purge resource (deferred)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
cmd/integration-test/javascript.go (3)
18-19: Disable VNC test on Windows/macOS like other Docker-based JS tests.Redis and SSH Docker-based tests are disabled on Windows/macOS; the VNC test should follow suit to avoid CI flakiness on non-Linux runners.
- {Path: "protocols/javascript/vnc-pass-brute.yaml", TestCase: &javascriptVncPassBrute{}}, + {Path: "protocols/javascript/vnc-pass-brute.yaml", TestCase: &javascriptVncPassBrute{}, DisableOn: func() bool { return osutils.IsWindows() || osutils.IsOSX() }},
103-134: Stabilize the VNC integration test (readiness wait, copy/paste comment).
- The image dorowu/ubuntu-desktop-lxde-vnc is heavy; 3s is often not enough for the VNC server to accept connections, causing flakes.
- Comment mentions “ssh” instead of “vnc”.
Increase the wait and fix the comment:
- //let ssh server start - time.Sleep(3 * time.Second) + // let vnc server start + time.Sleep(8 * time.Second)Optionally, make pool.Retry meaningful by returning err (current closure always returns nil and never retries within the pool.Retry loop):
- results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) - return nil + results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) + return errIf you prefer to keep the outer for-retry loop, consider dropping pool.Retry entirely to reduce nesting.
Also consider raising defaultRetry from 3 to 5 for this heavy image (outside the changed hunk).
201-218: Give the VNC container more time to live; 30s expiry is tight for a desktop+VNC image.30 seconds can expire mid-test under load/emulation (especially with Platform forced to linux/amd64 on non-amd64 runners).
- // by default expire after 30 sec - if err := vncResource.Expire(30); err != nil { + // give the desktop+vnc container enough time + if err := vncResource.Expire(120); err != nil {Optional: if CI runners are arm64, consider dropping Platform override to avoid qemu emulation overhead (align with how other tests handle this if possible).
pkg/js/libs/vnc/vnc.go (2)
51-54: Guard against missing executionId in context (to avoid panics).Direct type assertion can panic if context is not prepared by the runtime.
-func (c *VNCClient) Connect(ctx context.Context, host string, port int, password string) (bool, error) { - executionId := ctx.Value("executionId").(string) +func (c *VNCClient) Connect(ctx context.Context, host string, port int, password string) (bool, error) { + execVal := ctx.Value("executionId") + executionId, _ := execVal.(string) + if executionId == "" { + return false, fmt.Errorf("executionId missing in context") + } return connect(executionId, host, port, password) }Note: If other JS libs intentionally assume presence and prefer panic, feel free to keep the pattern for consistency; otherwise the above improves safety.
101-110: UpdateisAuthErrorto broaden error tokens; no typed sentinel errors in alexsnet/go-vncThe upstream
github.com/alexsnet/go-vnclibrary does not export a sentinel or typed error for authentication failures, so we must continue using substring matching as a fallback. To reduce false negatives, consider broadening the set of tokens and add aTODOfor future migration toerrors.Is/errors.Aswhen such typed errors become available.Suggested diff:
--- a/pkg/js/libs/vnc/vnc.go +++ b/pkg/js/libs/vnc/vnc.go @@ -107,7 +107,11 @@ func isAuthError(err error) bool { - return stringsutil.ContainsAnyI(errStr, "authentication", "auth", "password", "invalid", "failed") + // TODO: once go-vnc exports a sentinel auth error (e.g., ErrAuthFailed), switch to errors.Is/As here. + return stringsutil.ContainsAnyI(errStr, + "authentication", "auth", "password", "invalid", "failed", + "denied", "unauth", "unauthorized", "access denied")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumintegration_tests/protocols/javascript/vnc-pass-brute.yamlis excluded by!**/*.yamlpkg/js/generated/go/libvnc/vnc.gois excluded by!**/generated/**pkg/js/generated/ts/vnc.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
cmd/integration-test/javascript.go(3 hunks)go.mod(1 hunks)pkg/js/libs/vnc/vnc.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
pkg/js/libs/vnc/vnc.go (4)
pkg/js/generated/ts/vnc.ts (2)
VNCClient(44-65)Connect(62-64)pkg/js/libs/redis/redis.go (1)
Connect(63-66)pkg/protocols/common/protocolstate/headless.go (2)
IsHostAllowed(124-150)ErrHostDenied(22-22)pkg/protocols/common/protocolstate/state.go (1)
GetDialersWithId(39-45)
🔇 Additional comments (3)
cmd/integration-test/javascript.go (1)
24-27: LGTM: resource wiring is consistent with existing pattern.Adding vncResource alongside redis/ssh and reusing pool/defaultRetry matches the current test infra conventions.
pkg/js/libs/vnc/vnc.go (2)
30-39: Public VNCClient type and JS docs look good.The zero-field client aligns with other JS libs; examples match the generated TS interface.
143-143: LGTM: plugin alias switch to vncplugin.VNCPlugin is correct.Import aliasing is clear and avoids path ambiguity with other plugins.
| github.com/alecthomas/kingpin/v2 v2.4.0 // indirect | ||
| github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751 // indirect | ||
| github.com/alecthomas/units v0.0.0-20211218093645-b94a6e3cc137 // indirect | ||
| github.com/alexsnet/go-vnc v0.1.0 // indirect |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make go-vnc a direct dependency and tidy the module.
The package is imported directly in pkg/js/libs/vnc/vnc.go, so it should not be marked as indirect. This can cause drift and tooling confusion.
Apply this minimal change and then run go mod tidy:
- github.com/alexsnet/go-vnc v0.1.0 // indirect
+ github.com/alexsnet/go-vnc v0.1.0Run the following to verify and clean up:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go mod tidy -v
go mod why -m github.com/alexsnet/go-vnc || trueLength of output: 24910
Remove indirect marker for github.com/alexsnet/go-vnc and tidy module
The output of your verification confirms that pkg/js/libs/vnc/vnc.go imports github.com/alexsnet/go-vnc directly:
# github.com/alexsnet/go-vnc
github.com/projectdiscovery/nuclei/v3/pkg/js/libs/vnc
github.com/alexsnet/go-vnc
Accordingly, update your go.mod:
- github.com/alexsnet/go-vnc v0.1.0 // indirect
+ github.com/alexsnet/go-vnc v0.1.0Then run:
go mod tidyto ensure the module file is clean and accurate.
🤖 Prompt for AI Agents
In go.mod around line 147, the dependency github.com/alexsnet/go-vnc is marked
as // indirect but is imported directly by pkg/js/libs/vnc/vnc.go; remove the
indirect marker by making the dependency direct (e.g., run go get
github.com/alexsnet/go-vnc@v0.1.0 or update the requirement entry so it is not
commented as indirect) and then run go mod tidy to clean up go.mod/go.sum and
commit the updated files.
| if !protocolstate.IsHostAllowed(executionId, host) { | ||
| // host is not valid according to network policy | ||
| return false, protocolstate.ErrHostDenied.Msgf(host) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include port in network-policy check to handle IPv6 and port-scoped rules.
Passing only the host can misclassify IPv6 literals and bypass/over-block port-scoped policies. Use host:port.
- if !protocolstate.IsHostAllowed(executionId, host) {
+ addr := net.JoinHostPort(host, strconv.Itoa(port))
+ if !protocolstate.IsHostAllowed(executionId, addr) {
// host is not valid according to network policy
- return false, protocolstate.ErrHostDenied.Msgf(host)
+ return false, protocolstate.ErrHostDenied.Msgf(addr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !protocolstate.IsHostAllowed(executionId, host) { | |
| // host is not valid according to network policy | |
| return false, protocolstate.ErrHostDenied.Msgf(host) | |
| } | |
| addr := net.JoinHostPort(host, strconv.Itoa(port)) | |
| if !protocolstate.IsHostAllowed(executionId, addr) { | |
| // host is not valid according to network policy | |
| return false, protocolstate.ErrHostDenied.Msgf(addr) | |
| } |
🤖 Prompt for AI Agents
In pkg/js/libs/vnc/vnc.go around lines 61 to 65, the network-policy check
currently passes only the host which misclassifies IPv6 literals and ignores
port-scoped rules; change the check to pass the host with port (host:port)
instead — format IPv6 literals with brackets (e.g. [ipv6]:port) when composing
the address, then call protocolstate.IsHostAllowed(executionId, hostWithPort)
and return protocolstate.ErrHostDenied.Msgf(hostWithPort) on denial so policy
evaluations correctly consider both address and port.
| dialer := protocolstate.GetDialersWithId(executionId) | ||
| if dialer == nil { | ||
| return false, fmt.Errorf("dialers not initialized for %s", executionId) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nil-check Fastdialer to avoid nil deref.
Dialers may be present but Fastdialer nil during early init or in restricted contexts.
dialer := protocolstate.GetDialersWithId(executionId)
if dialer == nil {
return false, fmt.Errorf("dialers not initialized for %s", executionId)
}
+ if dialer.Fastdialer == nil {
+ return false, fmt.Errorf("fastdialer not initialized for %s", executionId)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dialer := protocolstate.GetDialersWithId(executionId) | |
| if dialer == nil { | |
| return false, fmt.Errorf("dialers not initialized for %s", executionId) | |
| } | |
| dialer := protocolstate.GetDialersWithId(executionId) | |
| if dialer == nil { | |
| return false, fmt.Errorf("dialers not initialized for %s", executionId) | |
| } | |
| if dialer.Fastdialer == nil { | |
| return false, fmt.Errorf("fastdialer not initialized for %s", executionId) | |
| } |
🤖 Prompt for AI Agents
In pkg/js/libs/vnc/vnc.go around lines 66 to 70, the code checks dialer for nil
but does not verify that dialer.Fastdialer is non-nil; add a nil-check for
dialer.Fastdialer immediately after confirming dialer exists and before any use
to avoid a nil pointer dereference, and return an appropriate error (e.g., false
and fmt.Errorf("fastdialer not initialized for %s", executionId)) or handle the
missing fastdialer case gracefully (log/propagate) consistent with surrounding
error handling.
| conn, err := dialer.Fastdialer.Dial(context.TODO(), "tcp", net.JoinHostPort(host, strconv.Itoa(port))) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| defer func() { | ||
| _ = conn.Close() | ||
| }() | ||
|
|
||
| // Set connection timeout | ||
| _ = conn.SetDeadline(time.Now().Add(10 * time.Second)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Propagate context and set a timeout for dialing; avoid context.TODO.
Use a bounded context for the dial and reuse the same for the handshake; deadline on the net.Conn alone doesn’t cancel the dialing attempt.
- conn, err := dialer.Fastdialer.Dial(context.TODO(), "tcp", net.JoinHostPort(host, strconv.Itoa(port)))
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ addr := net.JoinHostPort(host, strconv.Itoa(port))
+ conn, err := dialer.Fastdialer.Dial(ctx, "tcp", addr)
if err != nil {
return false, err
}
defer func() {
_ = conn.Close()
}()
// Set connection timeout
- _ = conn.SetDeadline(time.Now().Add(10 * time.Second))
+ _ = conn.SetDeadline(time.Now().Add(10 * time.Second))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn, err := dialer.Fastdialer.Dial(context.TODO(), "tcp", net.JoinHostPort(host, strconv.Itoa(port))) | |
| if err != nil { | |
| return false, err | |
| } | |
| defer func() { | |
| _ = conn.Close() | |
| }() | |
| // Set connection timeout | |
| _ = conn.SetDeadline(time.Now().Add(10 * time.Second)) | |
| // Propagate a 10s timeout into the dial (so the dial itself will be canceled if it hangs) | |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer cancel() | |
| addr := net.JoinHostPort(host, strconv.Itoa(port)) | |
| conn, err := dialer.Fastdialer.Dial(ctx, "tcp", addr) | |
| if err != nil { | |
| return false, err | |
| } | |
| defer func() { | |
| _ = conn.Close() | |
| }() | |
| // Also set a deadline on the established connection for the handshake/read/write | |
| _ = conn.SetDeadline(time.Now().Add(10 * time.Second)) |
🤖 Prompt for AI Agents
In pkg/js/libs/vnc/vnc.go around lines 71 to 81, replace the use of context.TODO
and the lone conn.SetDeadline with a bounded context that is used for dialing
and the subsequent handshake: create a ctx, cancel :=
context.WithTimeout(parentCtxOrBackground, 10*time.Second) and defer cancel(),
call dialer.Fastdialer.Dial(ctx, ...) so the dial can be canceled by the
timeout, reuse that same ctx when performing the VNC handshake, and if you still
set a conn deadline derive it from ctx.Deadline() (if present) rather than
hardcoding time.Now().Add(...); ensure the cancel() is deferred immediately
after ctx creation.
| // Attempt to connect and authenticate | ||
| c, err := vnclib.Connect(context.TODO(), conn, vncConfig) | ||
| if err != nil { | ||
| // Check for specific authentication errors | ||
| if isAuthError(err) { | ||
| return false, nil // Authentication failed, but connection succeeded | ||
| } | ||
| return false, err // Connection or other error | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse the same bounded context for the VNC handshake.
Avoid context.TODO so the handshake obeys the same timeout/cancellation.
- c, err := vnclib.Connect(context.TODO(), conn, vncConfig)
+ c, err := vnclib.Connect(ctx, conn, vncConfig)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Attempt to connect and authenticate | |
| c, err := vnclib.Connect(context.TODO(), conn, vncConfig) | |
| if err != nil { | |
| // Check for specific authentication errors | |
| if isAuthError(err) { | |
| return false, nil // Authentication failed, but connection succeeded | |
| } | |
| return false, err // Connection or other error | |
| } | |
| // Attempt to connect and authenticate | |
| c, err := vnclib.Connect(ctx, conn, vncConfig) | |
| if err != nil { | |
| // Check for specific authentication errors | |
| if isAuthError(err) { | |
| return false, nil // Authentication failed, but connection succeeded | |
| } | |
| return false, err // Connection or other error | |
| } |
🤖 Prompt for AI Agents
In pkg/js/libs/vnc/vnc.go around lines 85 to 93, the handshake call uses
context.TODO(), which ignores the caller's timeout/cancellation; replace
context.TODO() with the function's context parameter (ctx) so the vnclib.Connect
call uses the same bounded context; if the function currently lacks a
context.Context parameter, add one to the signature and propagate it through all
callers so the VNC connect/handshake obeys the caller's timeout and
cancellation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/integration-test/javascript.go (1)
185-196: Fix shadowed ‘pool’: tests skip and purging may not work.
Using := here creates a local pool and leaves the package-level pool nil; Execute() methods then skip due to pool == nil. Replace with assignment to the global.func init() { // uses a sensible default on windows (tcp/http) and linux/osx (socket) - pool, err := dockertest.NewPool("") + var err error + pool, err = dockertest.NewPool("")
🧹 Nitpick comments (4)
cmd/integration-test/javascript.go (4)
19-20: Gate VNC test on Windows/macOS to match other JS tests and avoid Docker/platform quirks.
Add the same DisableOn guard used by other tests.- {Path: "protocols/javascript/vnc-pass-brute.yaml", TestCase: &javascriptVncPassBrute{}}, + {Path: "protocols/javascript/vnc-pass-brute.yaml", TestCase: &javascriptVncPassBrute{}, DisableOn: func() bool { return osutils.IsWindows() || osutils.IsOSX() }},
116-136: Make dockertest Retry actually retry; fix misleading comment.
The closure always returns nil, so Retry never retries. Also the comment mentions SSH in the Oracle test.errs := []error{} for i := 0; i < defaultRetry; i++ { results := []string{} var err error - _ = pool.Retry(func() error { - //let ssh server start + err = pool.Retry(func() error { + // let oracle server start time.Sleep(3 * time.Second) - results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) - return nil + var innerErr error + results, innerErr = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) + return innerErr }) if err != nil { return err }
138-168: VNC Execute: enable real retries and fix the comment.
Same Retry issue and SSH copy-paste comment here.for i := 0; i < defaultRetry; i++ { results := []string{} var err error - _ = pool.Retry(func() error { - //let ssh server start + err = pool.Retry(func() error { + // let vnc server start time.Sleep(3 * time.Second) - results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) - return nil + var innerErr error + results, innerErr = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) + return innerErr }) if err != nil { return err }
255-271: Increase VNC container expiration to reduce flakiness.
This image is heavy and often needs >30s to be ready, especially under emulation (linux/amd64 on ARM). Give it more headroom.- // by default expire after 30 sec - if err := vncResource.Expire(30); err != nil { + // give more headroom for UI/VNC to come up + if err := vncResource.Expire(90); err != nil { log.Printf("Could not expire resource: %s", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
cmd/integration-test/javascript.go(3 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
cmd/integration-test/javascript.go
🧬 Code graph analysis (1)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (1)
cmd/integration-test/javascript.go (1)
26-27: LGTM on new resource variable.
Consistent with existing resources.
1 participant
Proposed changes
Closes #4842
Vulnerable container:
Template:
Output:
Checklist
Summary by CodeRabbit