Conversation
WalkthroughJavaScript protocol handling refactored from single-port to multi-port execution. Port retrieval replaced with deduplication-enabled multi-port collection. ExecuteWithResults now iterates over ports, performing per-port preparation, validation, and error aggregation. Integration test added validating multi-port template behavior. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Runner
participant JS as JavaScript Protocol
participant Net as Network Layer
participant Req as HTTP Request
Test->>JS: ExecuteWithResults()
JS->>JS: getPorts() → [port1, port2, ...]
loop For each port
JS->>JS: executeWithResults(port, ...)
JS->>Net: UseNetworkPort(port)
JS->>JS: Validate DSL for port
JS->>Req: Prepare input/payload (per-port)
Req->>Req: Execute request on port
Req-->>JS: Results
JS->>JS: Aggregate errors & results
end
JS-->>Test: Combined results + error set
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/nuclei/ssh.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
pkg/protocols/javascript/js.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/protocols/javascript/js.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/javascript/js.go
🧠 Learnings (1)
📚 Learning: 2025-09-10T18:57:51.327Z
Learnt from: Mzack9999
PR: projectdiscovery/nuclei#6465
File: pkg/catalog/loader/loader.go:504-505
Timestamp: 2025-09-10T18:57:51.327Z
Learning: The mapsutil.SyncLockMap from projectdiscovery/utils provides thread-safe operations including Has() and Set() methods that can be used safely in concurrent goroutines without additional synchronization. The separate Has() and Set() calls are internally synchronized by the SyncLockMap implementation.
Applied to files:
pkg/protocols/javascript/js.go
🧬 Code graph analysis (1)
pkg/protocols/javascript/js.go (3)
pkg/protocols/common/contextargs/metainput.go (1)
MetaInput(18-30)pkg/protocols/protocols.go (2)
Request(308-333)OutputEventCallback(336-336)pkg/protocols/common/contextargs/contextargs.go (1)
Context(22-33)
🪛 GitHub Actions: 🔨 Tests
pkg/protocols/javascript/js.go
[error] 858-858: getExcludePorts is unused (unused)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/protocols/javascript/js.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/protocols/javascript/js.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/javascript/js.go
🧬 Code graph analysis (1)
pkg/protocols/javascript/js.go (2)
pkg/protocols/network/network.go (1)
Request(20-104)pkg/protocols/protocols.go (2)
Request(308-333)OutputEventCallback(336-336)
🔇 Additional comments (2)
pkg/protocols/javascript/js.go (2)
39-39: LGTM!The
sliceutilimport is appropriately added to support port deduplication at line 780.
137-142: LGTM!The validation correctly iterates over all ports to ensure none contain DSL expressions, maintaining the security constraint.
| // Get default port(s) if specified in template | ||
| ports := request.getPorts() | ||
|
|
||
| for _, port := range ports { | ||
| err := request.executeWithResults(port, target, dynamicValues, previous, callback) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical: Templates without Port won't execute.
When getPorts() returns an empty slice (no Port specified in template args), the loop doesn't execute and the function returns immediately. This breaks existing JavaScript templates that don't specify a Port argument.
Apply this diff to execute once with an empty port when no ports are specified:
func (request *Request) ExecuteWithResults(target *contextargs.Context, dynamicValues, previous output.InternalEvent, callback protocols.OutputEventCallback) error {
// Get default port(s) if specified in template
ports := request.getPorts()
+ if len(ports) == 0 {
+ ports = []string{""}
+ }
for _, port := range ports {
err := request.executeWithResults(port, target, dynamicValues, previous, callback)
if err != nil {
return err
}
}
return nil
}📝 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.
| // Get default port(s) if specified in template | |
| ports := request.getPorts() | |
| for _, port := range ports { | |
| err := request.executeWithResults(port, target, dynamicValues, previous, callback) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } | |
| func (request *Request) ExecuteWithResults(target *contextargs.Context, dynamicValues, previous output.InternalEvent, callback protocols.OutputEventCallback) error { | |
| // Get default port(s) if specified in template | |
| ports := request.getPorts() | |
| if len(ports) == 0 { | |
| ports = []string{""} | |
| } | |
| for _, port := range ports { | |
| err := request.executeWithResults(port, target, dynamicValues, previous, callback) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In pkg/protocols/javascript/js.go around lines 288 to 299, the current logic
returns immediately when request.getPorts() yields an empty slice, which
prevents templates without a Port arg from running; change the flow so that if
ports is empty you call request.executeWithResults once with an empty-string (or
zero-value) port (preserving the same target, dynamicValues, previous, callback)
and return any error; otherwise keep iterating over ports and return any error
from each call. Ensure you do not alter error handling semantics and that a
single execution occurs when no ports are specified.
|
@CodeRabbit create an issue for this PR. It should be about adding the capability to support multi default ports in javascript templates |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 54 @Mzack9999 I've created issue #6513 to track the capability for supporting multiple default ports in JavaScript templates. Issue: #6513 The issue includes context about this PR, backlinks, and has been assigned to you. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/integration-test/javascript.go(2 hunks)pkg/protocols/javascript/js.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/javascript/js.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
🪛 GitHub Actions: 🔨 Tests
cmd/integration-test/javascript.go
[error] 28-28: golangci-lint: 'multiPortsSShResource' is unused (unused)
🪛 GitHub Check: Lint
cmd/integration-test/javascript.go
[failure] 28-28:
var multiPortsSShResource is unused (unused)
🔇 Additional comments (1)
cmd/integration-test/javascript.go (1)
20-20: Test case registration looks correct.The new multi-ports test case follows the established pattern for test registration.
cmd/integration-test/javascript.go
Outdated
| sshResource *dockertest.Resource | ||
| oracleResource *dockertest.Resource | ||
| vncResource *dockertest.Resource | ||
| multiPortsSShResource *dockertest.Resource |
There was a problem hiding this comment.
🧩 Analysis chain
Fix the unused variable causing pipeline failure.
The multiPortsSShResource variable is declared but never used, causing the linter to fail. The javascriptMultiPortsSSH.Execute method checks sshResource instead (line 175). This needs to be resolved:
- Option 1: Remove
multiPortsSShResourceif the test should use the existingsshResource. - Option 2: Update the executor (line 175) to check
multiPortsSShResourceand initialize it in theinit()function if a separate SSH resource is needed for multi-port testing.
Based on the PR description mentioning "multiple ports" (222, 22, 2244), it appears the test should target a separate multi-port SSH resource. Please clarify whether:
- A new Docker container with multiple exposed ports should be created in
init(), or - The test should reuse the existing
sshResource
Run this script to check if the multi-ports template expects specific port configurations:
🏁 Script executed:
#!/bin/bash
# Check the multi-ports template for port configuration details
fd -e yaml -e yml 'multi-ports' --exec cat {}Length of output: 0
Let me search for the actual file contents by examining the repository structure:
Since I cannot directly access the file without knowing the repository structure and the scripts are not providing output, let me make a final request verification approach:
Clarify whether the unused SSH resource variable should be initialized and used, or removed entirely.
Based on the original review, multiPortsSShResource is declared but never used, causing the linter to fail. The test executor checks sshResource instead (line 175). Since the PR mentions testing multiple ports (222, 22, 2244), the test likely needs a separate multi-port SSH resource, but the initialization in init() is missing and the executor doesn't reference the variable.
Two options remain:
- Remove
multiPortsSShResourceif the test should reuse the existingsshResource - Initialize and use
multiPortsSShResourceininit()and update line 175 to reference it, assuming a separate multi-port container is needed
🏁 Script executed:
#!/bin/bash
# Inspect the javascript.go file to understand variable declarations and usage
cat cmd/integration-test/javascript.go | head -60Length of output: 2239
🏁 Script executed:
#!/bin/bash
# Check the line count and find references to multiPortsSShResource and sshResource
grep -n "multiPortsSShResource\|sshResource" cmd/integration-test/javascript.goLength of output: 377
🏁 Script executed:
#!/bin/bash
# Get a broader view of the init() function and Execute method
sed -n '1,200p' cmd/integration-test/javascript.go | grep -n "init\|Execute\|sshResource\|multiPortsSShResource"Length of output: 683
🏁 Script executed:
#!/bin/bash
# Search for the javascript.go file
find . -name "javascript.go" -type f 2>/dev/null | head -5Length of output: 36
🏁 Script executed:
#!/bin/bash
# Get the full content of the javascript.go file to understand structure
find . -name "javascript.go" -type f -exec wc -l {} \; 2>/dev/nullLength of output: 40
🏁 Script executed:
#!/bin/bash
# Search for all references to SSH resource patterns in the codebase
rg -i "ssh.*Resource\|resource.*ssh" -C 3 --type go 2>/dev/null | head -100Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full Execute method for javascriptMultiPortsSSH
sed -n '174,200p' cmd/integration-test/javascript.go
Length of output: 665
🏁 Script executed:
#!/bin/bash
# Check the init() function to see how sshResource is initialized
sed -n '240,280p' cmd/integration-test/javascript.goLength of output: 1039
🏁 Script executed:
#!/bin/bash
# Check if there's any difference between the test cases for SSH fingerprint vs multi-ports
grep -A 5 "javascriptSSHServerFingerprint\|javascriptMultiPortsSSH" cmd/integration-test/javascript.go | head -20Length of output: 1050
Remove the unused multiPortsSShResource variable or initialize and use it properly.
The variable multiPortsSShResource (line 28) is declared but never initialized or referenced anywhere in the codebase, causing the linter to fail. The javascriptMultiPortsSSH.Execute() method (lines 174-200) checks sshResource instead and does not extract or configure multiple ports like the test name suggests.
Two valid resolutions:
- Remove
multiPortsSShResourceif the multi-ports test should reuse the existingsshResource - Initialize
multiPortsSShResourceininit()with the appropriate port mappings and update theExecute()method to use it (currently lines 175-200 incorrectly checksshResource)
Currently, both javascriptSSHServerFingerprint and javascriptMultiPortsSSH use the same resource, indicating incomplete implementation of the multi-port test setup.
🧰 Tools
🪛 GitHub Actions: 🔨 Tests
[error] 28-28: golangci-lint: 'multiPortsSShResource' is unused (unused)
🪛 GitHub Check: Lint
[failure] 28-28:
var multiPortsSShResource is unused (unused)
🤖 Prompt for AI Agents
In cmd/integration-test/javascript.go around line 28, the declared variable
multiPortsSShResource is unused which breaks the linter; either delete this
variable and leave the multi-ports test to reuse sshResource, or properly
implement the multi-ports resource: initialize multiPortsSShResource in init()
with the intended additional port mappings (same dockertest.Resource creation
pattern as sshResource but exposing the extra ports), and update
javascriptMultiPortsSSH.Execute() (currently checking sshResource) to reference
multiPortsSShResource and extract/configure the multiple ports for the test;
choose one approach and remove the dead declaration or wire the new resource
through init() and Execute().
| func (j *javascriptMultiPortsSSH) Execute(filePath string) error { | ||
| if sshResource == nil || pool == nil { | ||
| // skip test as redis is not running | ||
| return nil | ||
| } | ||
| finalURL := "scanme.sh" | ||
| errs := []error{} | ||
| for i := 0; i < defaultRetry; i++ { | ||
| results := []string{} | ||
| var err error | ||
| _ = pool.Retry(func() error { | ||
| //let ssh server start | ||
| time.Sleep(3 * time.Second) | ||
| results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := expectResultsCount(results, 1); err == nil { | ||
| return nil | ||
| } else { | ||
| errs = append(errs, err) | ||
| } | ||
| } | ||
| return multierr.Combine(errs...) | ||
| } |
There was a problem hiding this comment.
Multiple issues with executor implementation.
The javascriptMultiPortsSSH executor has several problems that deviate from the established pattern:
-
Line 175: Checks
sshResourceinstead ofmultiPortsSShResource, which creates inconsistency with the declared variable at line 28. -
Line 176: Copy-paste error in comment—says "redis" but should say "SSH" or "multi-port SSH".
-
Line 179: Uses hardcoded
"scanme.sh"instead of extracting a port from a Docker resource like all other executors (see lines 50-51, 82-83, 114-115, 147-148). This breaks the established pattern of testing against local Docker containers. -
Missing
defer purge(): Unlike all other executors (lines 52, 84, 116, 149), this executor doesn't clean up the Docker resource, potentially causing resource leaks.
Apply this diff to align with the established pattern (assuming you want to use the existing sshResource):
func (j *javascriptMultiPortsSSH) Execute(filePath string) error {
if sshResource == nil || pool == nil {
- // skip test as redis is not running
+ // skip test as ssh is not running
return nil
}
- finalURL := "scanme.sh"
+ tempPort := sshResource.GetPort("2222/tcp")
+ finalURL := "localhost:" + tempPort
+ defer purge(sshResource)
errs := []error{}Alternatively, if the test genuinely needs to target the external scanme.sh service (which seems inconsistent with an integration test suite using Docker), please clarify the intent and consider whether this should be a separate test type.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/integration-test/javascript.go(2 hunks)lib/example_test.go(0 hunks)pkg/operators/common/dsl/dsl.go(1 hunks)pkg/scan/events/scan_noop.go(0 hunks)pkg/scan/events/stats_build.go(0 hunks)pkg/utils/json/json.go(0 hunks)pkg/utils/json/json_fallback.go(0 hunks)
💤 Files with no reviewable changes (5)
- pkg/scan/events/scan_noop.go
- pkg/utils/json/json_fallback.go
- lib/example_test.go
- pkg/scan/events/stats_build.go
- pkg/utils/json/json.go
✅ Files skipped from review due to trivial changes (1)
- pkg/operators/common/dsl/dsl.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
🔇 Additional comments (1)
cmd/integration-test/javascript.go (1)
13-21: JS multi-ports test is correctly wired into the suiteThe new
jsTestcasesentry forprotocols/javascript/multi-ports.yamlbound tojavascriptMultiPortsSSHis consistent with the existing pattern and looks fine.
| type javascriptMultiPortsSSH struct{} | ||
|
|
||
| func (j *javascriptMultiPortsSSH) Execute(filePath string) error { | ||
| finalURL := "scanme.sh" | ||
| errs := []error{} | ||
| for i := 0; i < defaultRetry; i++ { | ||
| results := []string{} | ||
| var err error | ||
| _ = pool.Retry(func() error { | ||
| //let ssh server start | ||
| time.Sleep(3 * time.Second) | ||
| results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := expectResultsCount(results, 1); err == nil { | ||
| return nil | ||
| } else { | ||
| errs = append(errs, err) | ||
| } | ||
| } | ||
| return multierr.Combine(errs...) | ||
| } |
There was a problem hiding this comment.
Avoid using pool.Retry here – risk of nil-pointer panic and unnecessary Docker dependency
javascriptMultiPortsSSH.Execute currently calls pool.Retry but:
- There is no
pool == nilguard, unlike the other executors. If Docker is unavailable orinit()fails, the globalpoolcan benil, leading to a panic whenpool.Retryis called. - This executor does not use any Docker resource at all and only targets
scanme.sh, so it doesn’t actually need the dockertest pool; the retry loop can be implemented directly. - The comment
//let ssh server startis misleading here, since there is no local SSH container involved.
To make this robust and simpler, you can drop the pool dependency entirely and keep the retry behavior, e.g.:
func (j *javascriptMultiPortsSSH) Execute(filePath string) error {
- finalURL := "scanme.sh"
- errs := []error{}
- for i := 0; i < defaultRetry; i++ {
- results := []string{}
- var err error
- _ = pool.Retry(func() error {
- //let ssh server start
- time.Sleep(3 * time.Second)
- results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug)
- return nil
- })
- if err != nil {
- return err
- }
- if err := expectResultsCount(results, 1); err == nil {
- return nil
- } else {
- errs = append(errs, err)
- }
- }
- return multierr.Combine(errs...)
+ finalURL := "scanme.sh"
+ errs := []error{}
+ for i := 0; i < defaultRetry; i++ {
+ // give remote endpoint a brief pause between attempts
+ time.Sleep(3 * time.Second)
+
+ results, err := testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug)
+ if err != nil {
+ return err
+ }
+ if err := expectResultsCount(results, 1); err == nil {
+ return nil
+ }
+ errs = append(errs, err)
+ }
+ return multierr.Combine(errs...)
}This removes the potential nil-pointer on pool, keeps the retry logic, and avoids binding this test to Docker when it only needs a remote host.
🤖 Prompt for AI Agents
In cmd/integration-test/javascript.go around lines 171 to 195, replace the use
of pool.Retry (which can panic if pool is nil and is unnecessary for a
remote-only test) with a direct retry loop: remove the pool.Retry call and the
misleading "//let ssh server start" comment, perform up to defaultRetry
iterations where you sleep briefly (e.g., 3s) before calling
testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug), capture its
error into a local err variable, check expectResultsCount and return on success,
otherwise append the error to errs; after the loop, return
multierr.Combine(errs...) so the behavior and backoff remain but without
depending on pool or Docker.
2 participants
Proposed changes
Added support for multiple ports in the Nuclei engine for JavaScript templates
Checklist
./nuclei -u 127.0.0.1 -t ssh.yaml
Sample Template
Summary by CodeRabbit