feat(js): add wmi, tsch, scmr, and dcom helper modules - #7388
Merged
Mzack9999 merged 4 commits intoMay 14, 2026
Merged
Conversation
Neo - PR Security ReviewNo security issues found Hardening Notes
Comment |
Contributor
WalkthroughIntegrates FalconOps GoExec into Nuclei: adds Auth/ExecutionOptions models, adapter Run boundary with redaction/truncation, a GoExec runner with WMI/TSCH/SCMR/DCOM handlers, Goja JS client bindings, tooling ignore/support in bindgen/tsgen, tests, and go.mod updates. ChangesGoExec Windows Execution Integration
Sequence Diagram(s)sequenceDiagram
participant JS as Goja Runtime
participant Adapter as goexec.Run
participant Runner as GoExecRunner
participant DCE as DCE/SMB client
participant Module as Module Handler
JS->>Adapter: Run(ctx, Request)
Adapter->>Adapter: normalizeTarget, WithExecutionID
Adapter->>Runner: Run(ctx, req)
Runner->>Runner: validate auth & allowlist
Runner->>DCE: build dce/smb client (if needed)
Runner->>Module: dispatch to runWMI/runTSCH/runSCMR/runDCOM
Module-->>Runner: execution result / output
Runner-->>Adapter: Result (redacted on error)
Adapter->>JS: Public() result (truncated/redacted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
dwisiswant0
changed the title
![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: 7
🧹 Nitpick comments (1)
pkg/js/libs/goexec/options_test.go (1)
5-28: ⚡ Quick winAdd coverage for struct-based merge semantics.
This test only covers the JavaScript map path. Please add a regression test for
MergeOptions(..., ExecutionOptions{...}), especially boolean override behavior.✅ Suggested test addition
func TestMergeOptionsFromJavaScriptMap(t *testing.T) { @@ } + +func TestMergeOptionsFromStructBoolOverrides(t *testing.T) { + base := DefaultExecutionOptions() + base.Output = true + base.NoDeleteOutput = true + base.NoSign = true + base.NoSeal = true + base.EPM = true + + opts := MergeOptions(base, ExecutionOptions{ + Output: false, + NoDeleteOutput: false, + NoSign: false, + NoSeal: false, + EPM: false, + }) + + if opts.Output || opts.NoDeleteOutput || opts.NoSign || opts.NoSeal || opts.EPM { + t.Fatalf("expected false bool overrides to be applied: %#v", opts) + } +}🤖 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 `@pkg/js/libs/goexec/options_test.go` around lines 5 - 28, The test file only exercises MergeOptions with a map input; add a new unit test that covers merging from a struct to verify struct-based semantics and boolean override behavior: create a test (e.g., TestMergeOptionsFromStruct) that calls MergeOptions(DefaultExecutionOptions(), ExecutionOptions{Timeout:10, Output:true, OutputMethod:"SMB", OutputTimeout:3, NoDeleteOutput:true, Directory:`C:\Temp`, Endpoint:"ncacn_np:[svcctl]", EPMFilter:"ncacn_ip_tcp:", NoSign:true, NoSeal:true, MaxOutputSize:42}) and assert fields (Timeout, Output, OutputMethod normalized, OutputTimeout, NoDeleteOutput, Directory, Endpoint, EPMFilter, NoSign, NoSeal, MaxOutputSize) match expected values, specifically checking booleans override correctly; use the same assertion style as TestMergeOptionsFromJavaScriptMap to keep consistency.
🤖 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 `@pkg/js/devtools/bindgen/generator.go`:
- Line 118: Remove the leftover debug print by deleting the
fmt.Println(directory) call from pkg/js/devtools/bindgen/generator.go (the stray
print referencing the variable "directory"); ensure no other debugging prints
remain in the surrounding function so the generator’s normal output/return
behavior is unchanged.
In `@pkg/js/libs/goexec/adapter_goexec.go`:
- Around line 295-316: The code unconditionally sets result.Cleanup.Attempted =
true even when output.NoDelete is set, which incorrectly reports cleanup as
attempted; change the logic so Attempted is true only when the output will
actually be deleted (i.e., when !output.NoDelete or when the provider's
DeleteOutputFile would be true). Locate the block that constructs output
(output.NoDelete, output.RemotePath, gosmb.OutputFileFetcher with
DeleteOutputFile) and update the assignment to result.Cleanup.Attempted to be
conditional (e.g., set Attempted = !output.NoDelete or based on the
provider.DeleteOutputFile value); ensure result.Cleanup.Artifacts still contains
output.RemotePath if you want the artifact listed regardless of deletion intent.
- Around line 363-375: The endpointAllowed function is currently parsing the
text inside "[]" instead of extracting the network host portion, allowing
bypasses (e.g., ncacn_ip_tcp:10.0.0.5[135] validated against "135"). Fix by
extracting the host portion before the "[" and after the last ":" (i.e., trim at
'[' first, then take substring after last ':' if present) and use that host
value for the empty/leading-backslash check and the call to
protocolstate.IsHostAllowed(executionID(ctx), host); keep function name
endpointAllowed and leave executionID and protocolstate.IsHostAllowed calls
unchanged.
In `@pkg/js/libs/goexec/adapter.go`:
- Around line 44-53: SetRunnerForTesting currently allows setting defaultRunner
to nil which causes a panic when Run is later called; update SetRunnerForTesting
to guard against a nil runner by rejecting nil assignments or substituting a
safe fallback (e.g., keep previous/defaultRunner) so defaultRunner is never set
to nil. Specifically, in SetRunnerForTesting(lock/unlock using runnerMu) check
the incoming runner (Runner) for nil and if nil do not overwrite defaultRunner
(or set to a known no-op Runner), and ensure the returned restore closure still
reinstates the original previous value safely; update references to
defaultRunner, SetRunnerForTesting, runnerMu, and Runner to implement this
guard.
In `@pkg/js/libs/goexec/auth.go`:
- Around line 155-160: In validate(), stop allowing multiple credential sources
when Kerberos is true: remove the special-case that exempts a.kerberos from the
missing/duplicate checks and always enforce that selected == 1; specifically
update the switch/conditions around selected, a.kerberos to return
ErrMissingAuth when selected == 0 and ErrMultipleCredentialModes when selected >
1 regardless of a.kerberos (refer to the validate() function and symbols
selected, a.kerberos, ErrMissingAuth, ErrMultipleCredentialModes).
In `@pkg/js/libs/goexec/options.go`:
- Around line 113-149: mergeStruct currently only copies boolean fields when
opts.* is true, so callers cannot clear an already-true base value; update
mergeStruct to always assign the boolean fields from opts to base (i.e., remove
the if checks) for the relevant ExecutionOptions fields so explicit false
overrides are applied. Specifically, change the handling of Output,
NoDeleteOutput, EPM, NoSign, and NoSeal in mergeStruct to unconditionally set
base.Output = opts.Output, base.NoDeleteOutput = opts.NoDeleteOutput, base.EPM =
opts.EPM, base.NoSign = opts.NoSign, and base.NoSeal = opts.NoSeal (leave
non-boolean checks like Timeout/Proxy/OutputMethod/etc. as-is).
In `@pkg/js/libs/goexec/target.go`:
- Around line 15-32: The target normalization currently trims bracketed IPv6
addresses before calling net.SplitHostPort and allows url.Parse to set an empty
host; fix by (1) after url.Parse in the block that checks
strings.Contains(target, "://") return ErrMissingTarget if parsed.Host == "" and
otherwise set target = parsed.Host, (2) remove or move the strings.Trim(target,
"[]") call so that net.SplitHostPort is called on the original target first
(this preserves inputs like "[::1]:5985"), and only trim surrounding brackets as
a fallback when SplitHostPort returns an error and the target contains no colon;
update the logic around net.SplitHostPort, target trimming, and error returns
(references: url.Parse usage, parsed.Host, net.SplitHostPort, ErrMissingTarget,
strings.Trim).
---
Nitpick comments:
In `@pkg/js/libs/goexec/options_test.go`:
- Around line 5-28: The test file only exercises MergeOptions with a map input;
add a new unit test that covers merging from a struct to verify struct-based
semantics and boolean override behavior: create a test (e.g.,
TestMergeOptionsFromStruct) that calls MergeOptions(DefaultExecutionOptions(),
ExecutionOptions{Timeout:10, Output:true, OutputMethod:"SMB", OutputTimeout:3,
NoDeleteOutput:true, Directory:`C:\Temp`, Endpoint:"ncacn_np:[svcctl]",
EPMFilter:"ncacn_ip_tcp:", NoSign:true, NoSeal:true, MaxOutputSize:42}) and
assert fields (Timeout, Output, OutputMethod normalized, OutputTimeout,
NoDeleteOutput, Directory, Endpoint, EPMFilter, NoSign, NoSeal, MaxOutputSize)
match expected values, specifically checking booleans override correctly; use
the same assertion style as TestMergeOptionsFromJavaScriptMap to keep
consistency.
🪄 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: 20ab7e09-819a-4426-9dc7-9d4314bccb6e
⛔ Files ignored due to path filters (13)
go.sumis excluded by!**/*.suminternal/tests/integration/testdata/protocols/javascript/goexec-modules.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/goexec-redaction.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/wmi-command.yamlis excluded by!**/*.yamlpkg/js/generated/go/libdcom/dcom.gois excluded by!**/generated/**pkg/js/generated/go/libscmr/scmr.gois excluded by!**/generated/**pkg/js/generated/go/libtsch/tsch.gois excluded by!**/generated/**pkg/js/generated/go/libwmi/wmi.gois excluded by!**/generated/**pkg/js/generated/ts/dcom.tsis excluded by!**/generated/**pkg/js/generated/ts/index.tsis excluded by!**/generated/**pkg/js/generated/ts/scmr.tsis excluded by!**/generated/**pkg/js/generated/ts/tsch.tsis excluded by!**/generated/**pkg/js/generated/ts/wmi.tsis excluded by!**/generated/**
📒 Files selected for processing (25)
go.modinternal/tests/integration/javascript_test.gopkg/js/compiler/pool.gopkg/js/devtools/bindgen/generator.gopkg/js/devtools/tsgen/cmd/tsgen/main.gopkg/js/devtools/tsgen/parser.gopkg/js/libs/dcom/dcom.gopkg/js/libs/goexec/.nuclei-jsgen-ignorepkg/js/libs/goexec/adapter.gopkg/js/libs/goexec/adapter_goexec.gopkg/js/libs/goexec/adapter_test.gopkg/js/libs/goexec/auth.gopkg/js/libs/goexec/auth_test.gopkg/js/libs/goexec/errors.gopkg/js/libs/goexec/options.gopkg/js/libs/goexec/options_test.gopkg/js/libs/goexec/redact.gopkg/js/libs/goexec/redact_test.gopkg/js/libs/goexec/result.gopkg/js/libs/goexec/result_test.gopkg/js/libs/goexec/target.gopkg/js/libs/scmr/scmr.gopkg/js/libs/tsch/tsch.gopkg/js/libs/wmi/wmi.gopkg/js/libs/wmi/wmi_test.go
Comment on lines
+295
to
+316
| var out bytes.Buffer | ||
| output := &upstream.ExecutionOutput{ | ||
| NoDelete: req.Options.NoDeleteOutput, | ||
| RemotePath: `C:\Windows\Temp\` + uuid.NewString(), | ||
| Timeout: time.Duration(req.Options.OutputTimeout) * time.Second, | ||
| Writer: nopWriteCloser{Writer: &out}, | ||
| } | ||
| smbClient, err := r.smbClient(ctx, req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| output.Provider = &gosmb.OutputFileFetcher{ | ||
| Client: smbClient, | ||
| Share: `ADMIN$`, | ||
| SharePath: `C:\Windows`, | ||
| File: output.RemotePath, | ||
| DeleteOutputFile: !output.NoDelete, | ||
| } | ||
| execIO.Output = output | ||
| result.OutputMethod = DefaultOutputMethod | ||
| result.Cleanup.Attempted = true | ||
| result.Cleanup.Artifacts = []string{output.RemotePath} |
Contributor
There was a problem hiding this comment.
Don't report cleanup as attempted when NoDeleteOutput keeps the artifact.
Line 315 sets result.Cleanup.Attempted = true even when DeleteOutputFile is disabled. That makes the public result claim cleanup ran although the remote output file was intentionally left behind.
Suggested fix
execIO.Output = output
result.OutputMethod = DefaultOutputMethod
- result.Cleanup.Attempted = true
+ result.Cleanup.Attempted = !output.NoDelete
result.Cleanup.Artifacts = []string{output.RemotePath}📝 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.
Suggested change
| var out bytes.Buffer | |
| output := &upstream.ExecutionOutput{ | |
| NoDelete: req.Options.NoDeleteOutput, | |
| RemotePath: `C:\Windows\Temp\` + uuid.NewString(), | |
| Timeout: time.Duration(req.Options.OutputTimeout) * time.Second, | |
| Writer: nopWriteCloser{Writer: &out}, | |
| } | |
| smbClient, err := r.smbClient(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| output.Provider = &gosmb.OutputFileFetcher{ | |
| Client: smbClient, | |
| Share: `ADMIN$`, | |
| SharePath: `C:\Windows`, | |
| File: output.RemotePath, | |
| DeleteOutputFile: !output.NoDelete, | |
| } | |
| execIO.Output = output | |
| result.OutputMethod = DefaultOutputMethod | |
| result.Cleanup.Attempted = true | |
| result.Cleanup.Artifacts = []string{output.RemotePath} | |
| var out bytes.Buffer | |
| output := &upstream.ExecutionOutput{ | |
| NoDelete: req.Options.NoDeleteOutput, | |
| RemotePath: `C:\Windows\Temp\` + uuid.NewString(), | |
| Timeout: time.Duration(req.Options.OutputTimeout) * time.Second, | |
| Writer: nopWriteCloser{Writer: &out}, | |
| } | |
| smbClient, err := r.smbClient(ctx, req) | |
| if err != nil { | |
| return nil, err | |
| } | |
| output.Provider = &gosmb.OutputFileFetcher{ | |
| Client: smbClient, | |
| Share: `ADMIN$`, | |
| SharePath: `C:\Windows`, | |
| File: output.RemotePath, | |
| DeleteOutputFile: !output.NoDelete, | |
| } | |
| execIO.Output = output | |
| result.OutputMethod = DefaultOutputMethod | |
| result.Cleanup.Attempted = !output.NoDelete | |
| result.Cleanup.Artifacts = []string{output.RemotePath} |
🤖 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 `@pkg/js/libs/goexec/adapter_goexec.go` around lines 295 - 316, The code
unconditionally sets result.Cleanup.Attempted = true even when output.NoDelete
is set, which incorrectly reports cleanup as attempted; change the logic so
Attempted is true only when the output will actually be deleted (i.e., when
!output.NoDelete or when the provider's DeleteOutputFile would be true). Locate
the block that constructs output (output.NoDelete, output.RemotePath,
gosmb.OutputFileFetcher with DeleteOutputFile) and update the assignment to
result.Cleanup.Attempted to be conditional (e.g., set Attempted =
!output.NoDelete or based on the provider.DeleteOutputFile value); ensure
result.Cleanup.Artifacts still contains output.RemotePath if you want the
artifact listed regardless of deletion intent.
Comment on lines
+44
to
+53
| func SetRunnerForTesting(runner Runner) func() { | ||
| runnerMu.Lock() | ||
| previous := defaultRunner | ||
| defaultRunner = runner | ||
| runnerMu.Unlock() | ||
| return func() { | ||
| runnerMu.Lock() | ||
| defaultRunner = previous | ||
| runnerMu.Unlock() | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against nil runner assignment in test hook.
SetRunnerForTesting(nil) makes defaultRunner nil, and Run then panics on runner.Run(...). Add a guard or fallback to keep the adapter safe under test setup mistakes.
Proposed fix
func SetRunnerForTesting(runner Runner) func() {
+ if runner == nil {
+ runner = &GoExecRunner{}
+ }
runnerMu.Lock()
previous := defaultRunner
defaultRunner = runner
runnerMu.Unlock()
return func() {🤖 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 `@pkg/js/libs/goexec/adapter.go` around lines 44 - 53, SetRunnerForTesting
currently allows setting defaultRunner to nil which causes a panic when Run is
later called; update SetRunnerForTesting to guard against a nil runner by
rejecting nil assignments or substituting a safe fallback (e.g., keep
previous/defaultRunner) so defaultRunner is never set to nil. Specifically, in
SetRunnerForTesting(lock/unlock using runnerMu) check the incoming runner
(Runner) for nil and if nil do not overwrite defaultRunner (or set to a known
no-op Runner), and ensure the returned restore closure still reinstates the
original previous value safely; update references to defaultRunner,
SetRunnerForTesting, runnerMu, and Runner to implement this guard.
Comment on lines
+113
to
+149
| func mergeStruct(base, opts ExecutionOptions) ExecutionOptions { | ||
| if opts.Timeout != 0 { | ||
| base.Timeout = opts.Timeout | ||
| } | ||
| if opts.Proxy != "" { | ||
| base.Proxy = opts.Proxy | ||
| } | ||
| if opts.Output { | ||
| base.Output = opts.Output | ||
| } | ||
| if opts.OutputMethod != "" { | ||
| base.OutputMethod = strings.ToLower(opts.OutputMethod) | ||
| } | ||
| if opts.OutputTimeout != 0 { | ||
| base.OutputTimeout = opts.OutputTimeout | ||
| } | ||
| if opts.NoDeleteOutput { | ||
| base.NoDeleteOutput = opts.NoDeleteOutput | ||
| } | ||
| if opts.Directory != "" { | ||
| base.Directory = opts.Directory | ||
| } | ||
| if opts.Endpoint != "" { | ||
| base.Endpoint = opts.Endpoint | ||
| } | ||
| if opts.EPM { | ||
| base.EPM = opts.EPM | ||
| } | ||
| if opts.EPMFilter != "" { | ||
| base.EPMFilter = opts.EPMFilter | ||
| } | ||
| if opts.NoSign { | ||
| base.NoSign = opts.NoSign | ||
| } | ||
| if opts.NoSeal { | ||
| base.NoSeal = opts.NoSeal | ||
| } |
Contributor
There was a problem hiding this comment.
mergeStruct cannot apply explicit false boolean overrides.
Lines 120, 129, 138, 144, and 147 only copy bools when true, so typed struct input cannot clear an already-enabled base option.
🐛 Proposed fix
func mergeStruct(base, opts ExecutionOptions) ExecutionOptions {
@@
- if opts.Output {
- base.Output = opts.Output
- }
+ base.Output = opts.Output
@@
- if opts.NoDeleteOutput {
- base.NoDeleteOutput = opts.NoDeleteOutput
- }
+ base.NoDeleteOutput = opts.NoDeleteOutput
@@
- if opts.EPM {
- base.EPM = opts.EPM
- }
+ base.EPM = opts.EPM
@@
- if opts.NoSign {
- base.NoSign = opts.NoSign
- }
+ base.NoSign = opts.NoSign
@@
- if opts.NoSeal {
- base.NoSeal = opts.NoSeal
- }
+ base.NoSeal = opts.NoSeal📝 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.
Suggested change
| func mergeStruct(base, opts ExecutionOptions) ExecutionOptions { | |
| if opts.Timeout != 0 { | |
| base.Timeout = opts.Timeout | |
| } | |
| if opts.Proxy != "" { | |
| base.Proxy = opts.Proxy | |
| } | |
| if opts.Output { | |
| base.Output = opts.Output | |
| } | |
| if opts.OutputMethod != "" { | |
| base.OutputMethod = strings.ToLower(opts.OutputMethod) | |
| } | |
| if opts.OutputTimeout != 0 { | |
| base.OutputTimeout = opts.OutputTimeout | |
| } | |
| if opts.NoDeleteOutput { | |
| base.NoDeleteOutput = opts.NoDeleteOutput | |
| } | |
| if opts.Directory != "" { | |
| base.Directory = opts.Directory | |
| } | |
| if opts.Endpoint != "" { | |
| base.Endpoint = opts.Endpoint | |
| } | |
| if opts.EPM { | |
| base.EPM = opts.EPM | |
| } | |
| if opts.EPMFilter != "" { | |
| base.EPMFilter = opts.EPMFilter | |
| } | |
| if opts.NoSign { | |
| base.NoSign = opts.NoSign | |
| } | |
| if opts.NoSeal { | |
| base.NoSeal = opts.NoSeal | |
| } | |
| func mergeStruct(base, opts ExecutionOptions) ExecutionOptions { | |
| if opts.Timeout != 0 { | |
| base.Timeout = opts.Timeout | |
| } | |
| if opts.Proxy != "" { | |
| base.Proxy = opts.Proxy | |
| } | |
| base.Output = opts.Output | |
| if opts.OutputMethod != "" { | |
| base.OutputMethod = strings.ToLower(opts.OutputMethod) | |
| } | |
| if opts.OutputTimeout != 0 { | |
| base.OutputTimeout = opts.OutputTimeout | |
| } | |
| base.NoDeleteOutput = opts.NoDeleteOutput | |
| if opts.Directory != "" { | |
| base.Directory = opts.Directory | |
| } | |
| if opts.Endpoint != "" { | |
| base.Endpoint = opts.Endpoint | |
| } | |
| base.EPM = opts.EPM | |
| if opts.EPMFilter != "" { | |
| base.EPMFilter = opts.EPMFilter | |
| } | |
| base.NoSign = opts.NoSign | |
| base.NoSeal = opts.NoSeal | |
| } |
🤖 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 `@pkg/js/libs/goexec/options.go` around lines 113 - 149, mergeStruct currently
only copies boolean fields when opts.* is true, so callers cannot clear an
already-true base value; update mergeStruct to always assign the boolean fields
from opts to base (i.e., remove the if checks) for the relevant ExecutionOptions
fields so explicit false overrides are applied. Specifically, change the
handling of Output, NoDeleteOutput, EPM, NoSign, and NoSeal in mergeStruct to
unconditionally set base.Output = opts.Output, base.NoDeleteOutput =
opts.NoDeleteOutput, base.EPM = opts.EPM, base.NoSign = opts.NoSign, and
base.NoSeal = opts.NoSeal (leave non-boolean checks like
Timeout/Proxy/OutputMethod/etc. as-is).
Comment on lines
+15
to
+32
| if strings.Contains(target, "://") { | ||
| parsed, err := url.Parse(target) | ||
| if err != nil { | ||
| return "", fmt.Errorf("parse target: %w", err) | ||
| } | ||
| target = parsed.Host | ||
| } | ||
| target = strings.Trim(target, "[]") | ||
| if host, port, err := net.SplitHostPort(target); err == nil { | ||
| if host == "" { | ||
| return "", ErrMissingTarget | ||
| } | ||
| if port == "" { | ||
| return host, nil | ||
| } | ||
| return net.JoinHostPort(host, port), nil | ||
| } | ||
| return target, nil |
Contributor
There was a problem hiding this comment.
Preserve bracketed IPv6 host:port during normalization.
Line 22 strips brackets before net.SplitHostPort, which breaks valid inputs like [::1]:5985 into an invalid token. Also, Line 20 can set an empty host from URL parsing and return "" without ErrMissingTarget.
🐛 Proposed fix
func normalizeTarget(target string) (string, error) {
target = strings.TrimSpace(target)
if target == "" {
return "", ErrMissingTarget
}
if strings.Contains(target, "://") {
parsed, err := url.Parse(target)
if err != nil {
return "", fmt.Errorf("parse target: %w", err)
}
target = parsed.Host
+ if target == "" {
+ return "", ErrMissingTarget
+ }
}
- target = strings.Trim(target, "[]")
if host, port, err := net.SplitHostPort(target); err == nil {
if host == "" {
return "", ErrMissingTarget
}
if port == "" {
return host, nil
}
return net.JoinHostPort(host, port), nil
}
+ if strings.HasPrefix(target, "[") && strings.HasSuffix(target, "]") {
+ target = strings.TrimPrefix(strings.TrimSuffix(target, "]"), "[")
+ }
+ if target == "" {
+ return "", ErrMissingTarget
+ }
return target, 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.
Suggested change
| if strings.Contains(target, "://") { | |
| parsed, err := url.Parse(target) | |
| if err != nil { | |
| return "", fmt.Errorf("parse target: %w", err) | |
| } | |
| target = parsed.Host | |
| } | |
| target = strings.Trim(target, "[]") | |
| if host, port, err := net.SplitHostPort(target); err == nil { | |
| if host == "" { | |
| return "", ErrMissingTarget | |
| } | |
| if port == "" { | |
| return host, nil | |
| } | |
| return net.JoinHostPort(host, port), nil | |
| } | |
| return target, nil | |
| if strings.Contains(target, "://") { | |
| parsed, err := url.Parse(target) | |
| if err != nil { | |
| return "", fmt.Errorf("parse target: %w", err) | |
| } | |
| target = parsed.Host | |
| if target == "" { | |
| return "", ErrMissingTarget | |
| } | |
| } | |
| if host, port, err := net.SplitHostPort(target); err == nil { | |
| if host == "" { | |
| return "", ErrMissingTarget | |
| } | |
| if port == "" { | |
| return host, nil | |
| } | |
| return net.JoinHostPort(host, port), nil | |
| } | |
| if strings.HasPrefix(target, "[") && strings.HasSuffix(target, "]") { | |
| target = strings.TrimPrefix(strings.TrimSuffix(target, "]"), "[") | |
| } | |
| if target == "" { | |
| return "", ErrMissingTarget | |
| } | |
| return target, nil |
🤖 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 `@pkg/js/libs/goexec/target.go` around lines 15 - 32, The target normalization
currently trims bracketed IPv6 addresses before calling net.SplitHostPort and
allows url.Parse to set an empty host; fix by (1) after url.Parse in the block
that checks strings.Contains(target, "://") return ErrMissingTarget if
parsed.Host == "" and otherwise set target = parsed.Host, (2) remove or move the
strings.Trim(target, "[]") call so that net.SplitHostPort is called on the
original target first (this preserves inputs like "[::1]:5985"), and only trim
surrounding brackets as a fallback when SplitHostPort returns an error and the
target contains no colon; update the logic around net.SplitHostPort, target
trimming, and error returns (references: url.Parse usage, parsed.Host,
net.SplitHostPort, ErrMissingTarget, strings.Trim).
![neo-by-projectdiscovery-dev[bot]](https://avatars.githubusercontent.com/in/2878468?s=60&v=4){kind=small}
Add shared auth, options, result, redaction, and adapter support for GoExec-backed JavaScript helpers, including: * `nuclei/wmi` * `nuclei/tsch` * `nuclei/scmr` * `nuclei/dcom` through generated bindings, and register them with the JS compiler. Also make JS generators to skip internal helper packages and test files. Signed-off-by: Dwi Siswanto <git@dw1.io>
Signed-off-by: Dwi Siswanto <git@dw1.io>
dwisiswant0
force-pushed
the
dwisiswant0/feat/js/add-wmi-tsch-scmr-and-dcom-helper-modules
branch
from
May 10, 2026 20:58
403c847 to
ce50479
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/js/devtools/bindgen/generator.go (1)
297-307: ⚡ Quick winHandle grouped exported
ValueSpecs instead of only index 0.This still records only
spec.Names[0]/spec.Values[0], so declarations likeconst A, B = 1, 2will silently dropBfrom the generated bindings.Proposed fix
case *ast.ValueSpec: - if !spec.Names[0].IsExported() { - continue - } - data.PackageVars[spec.Names[0].Name] = spec.Names[0].Name - if len(spec.Values) == 0 { - continue - } - if value, ok := spec.Values[0].(*ast.BasicLit); ok { - data.PackageVarsValues[spec.Names[0].Name] = value.Value - } + for i, name := range spec.Names { + if !name.IsExported() { + continue + } + data.PackageVars[name.Name] = name.Name + if i >= len(spec.Values) { + continue + } + if value, ok := spec.Values[i].(*ast.BasicLit); ok { + data.PackageVarsValues[name.Name] = value.Value + } + }🤖 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 `@pkg/js/devtools/bindgen/generator.go` around lines 297 - 307, The code only records spec.Names[0] and spec.Values[0] for an ast.ValueSpec, dropping subsequent names in a grouped declaration (e.g., const A, B = 1, 2); update the handling in the ast.ValueSpec case to iterate over all spec.Names, check each name's IsExported() and add each exported name to data.PackageVars, and for values map the corresponding spec.Values entry to data.PackageVarsValues when present and the value is an *ast.BasicLit (use the same index as the name); if there are fewer spec.Values than names, skip value mapping for those names but still record the name if exported. Ensure you reference ast.ValueSpec, spec.Names, spec.Values, data.PackageVars and data.PackageVarsValues when making the change.
🤖 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.
Nitpick comments:
In `@pkg/js/devtools/bindgen/generator.go`:
- Around line 297-307: The code only records spec.Names[0] and spec.Values[0]
for an ast.ValueSpec, dropping subsequent names in a grouped declaration (e.g.,
const A, B = 1, 2); update the handling in the ast.ValueSpec case to iterate
over all spec.Names, check each name's IsExported() and add each exported name
to data.PackageVars, and for values map the corresponding spec.Values entry to
data.PackageVarsValues when present and the value is an *ast.BasicLit (use the
same index as the name); if there are fewer spec.Values than names, skip value
mapping for those names but still record the name if exported. Ensure you
reference ast.ValueSpec, spec.Names, spec.Values, data.PackageVars and
data.PackageVarsValues when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95235ecb-c2fc-4f4a-8a42-e7f8fc948796
⛔ Files ignored due to path filters (13)
go.sumis excluded by!**/*.suminternal/tests/integration/testdata/protocols/javascript/goexec-modules.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/goexec-redaction.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/wmi-command.yamlis excluded by!**/*.yamlpkg/js/generated/go/libdcom/dcom.gois excluded by!**/generated/**pkg/js/generated/go/libscmr/scmr.gois excluded by!**/generated/**pkg/js/generated/go/libtsch/tsch.gois excluded by!**/generated/**pkg/js/generated/go/libwmi/wmi.gois excluded by!**/generated/**pkg/js/generated/ts/dcom.tsis excluded by!**/generated/**pkg/js/generated/ts/index.tsis excluded by!**/generated/**pkg/js/generated/ts/scmr.tsis excluded by!**/generated/**pkg/js/generated/ts/tsch.tsis excluded by!**/generated/**pkg/js/generated/ts/wmi.tsis excluded by!**/generated/**
📒 Files selected for processing (26)
go.modinternal/tests/integration/javascript_test.gopkg/js/compiler/pool.gopkg/js/devtools/bindgen/generator.gopkg/js/devtools/tsgen/cmd/tsgen/main.gopkg/js/devtools/tsgen/parser.gopkg/js/libs/dcom/dcom.gopkg/js/libs/goexec/.nuclei-jsgen-ignorepkg/js/libs/goexec/adapter.gopkg/js/libs/goexec/adapter_goexec.gopkg/js/libs/goexec/adapter_test.gopkg/js/libs/goexec/auth.gopkg/js/libs/goexec/auth_test.gopkg/js/libs/goexec/errors.gopkg/js/libs/goexec/options.gopkg/js/libs/goexec/options_test.gopkg/js/libs/goexec/redact.gopkg/js/libs/goexec/redact_test.gopkg/js/libs/goexec/result.gopkg/js/libs/goexec/result_test.gopkg/js/libs/goexec/target.gopkg/js/libs/goexec/target_test.gopkg/js/libs/scmr/scmr.gopkg/js/libs/tsch/tsch.gopkg/js/libs/wmi/wmi.gopkg/js/libs/wmi/wmi_test.go
✅ Files skipped from review due to trivial changes (5)
- pkg/js/libs/goexec/.nuclei-jsgen-ignore
- pkg/js/libs/goexec/redact.go
- pkg/js/libs/goexec/result_test.go
- pkg/js/libs/tsch/tsch.go
- pkg/js/libs/goexec/auth_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/js/libs/goexec/errors.go
- pkg/js/devtools/tsgen/cmd/tsgen/main.go
- pkg/js/libs/goexec/target.go
- internal/tests/integration/javascript_test.go
- pkg/js/devtools/tsgen/parser.go
- pkg/js/libs/scmr/scmr.go
- pkg/js/libs/goexec/auth.go
- pkg/js/libs/wmi/wmi.go
- pkg/js/libs/goexec/options_test.go
- pkg/js/libs/goexec/options.go
- pkg/js/libs/dcom/dcom.go
- pkg/js/libs/wmi/wmi_test.go
- pkg/js/libs/goexec/adapter.go
- go.mod
- pkg/js/compiler/pool.go
Member
Author
|
Blocker:
|
…260511073005-3d18e8a5a687 Signed-off-by: Dwi Siswanto <git@dw1.io>
Mzack9999
approved these changes
May 14, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/tests/integration/javascript_test.go`:
- Around line 305-321: newGoExecCloseListener currently panics on listen/address
errors; change its signature to newGoExecCloseListener() (*goExecCloseListener,
error) and replace panic(fmt.Errorf(...)) and the other panic with returning nil
and the wrapped error, constructing and returning the listener struct on
success. Update any callers (e.g., places in Execute paths that call
newGoExecCloseListener) to handle the error, return it up the stack, and only
defer g.Close() after the listener is successfully created; reference the
goExecCloseListener type and the newGoExecCloseListener function to find all
uses to update.
🪄 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: 5f636c5c-3f4a-493c-aec0-9f709fc34675
⛔ Files ignored due to path filters (3)
internal/tests/integration/testdata/protocols/javascript/goexec-modules.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/goexec-redaction.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/goexec-samba-ntlm.yamlis excluded by!**/*.yaml
📒 Files selected for processing (6)
internal/tests/integration/goexec_samba_local_test.gointernal/tests/integration/javascript_test.gopkg/js/libs/goexec/auth.gopkg/js/libs/goexec/auth_test.gopkg/js/libs/goexec/options.gopkg/js/libs/goexec/options_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/js/libs/goexec/auth_test.go
- pkg/js/libs/goexec/auth.go
- pkg/js/libs/goexec/options.go
Comment on lines
+305
to
+321
| func newGoExecCloseListener() *goExecCloseListener { | ||
| ln, err := net.Listen("tcp", "127.0.0.1:0") | ||
| if err != nil { | ||
| panic(fmt.Errorf("goexec listener: %w", err)) | ||
| } | ||
| host, port, err := net.SplitHostPort(ln.Addr().String()) | ||
| if err != nil { | ||
| _ = ln.Close() | ||
| panic(fmt.Errorf("goexec listener addr: %w", err)) | ||
| } | ||
| g := &goExecCloseListener{ | ||
| listener: ln, | ||
| host: host, | ||
| binding: fmt.Sprintf("ncacn_ip_tcp:%s[%s]", host, port), | ||
| } | ||
| go g.serve() | ||
| return g |
Contributor
There was a problem hiding this comment.
Return an error here instead of panicking.
newGoExecCloseListener is used from Execute paths that already return error, so panicking on listener setup failure aborts the entire integration package instead of failing just the current case. Please make this constructor return (*goExecCloseListener, error) and propagate the error from the callers.
Proposed direction
-func newGoExecCloseListener() *goExecCloseListener {
+func newGoExecCloseListener() (*goExecCloseListener, error) {
ln, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
- panic(fmt.Errorf("goexec listener: %w", err))
+ return nil, fmt.Errorf("goexec listener: %w", err)
}
host, port, err := net.SplitHostPort(ln.Addr().String())
if err != nil {
_ = ln.Close()
- panic(fmt.Errorf("goexec listener addr: %w", err))
+ return nil, fmt.Errorf("goexec listener addr: %w", err)
}
g := &goExecCloseListener{
listener: ln,
host: host,
binding: fmt.Sprintf("ncacn_ip_tcp:%s[%s]", host, port),
}
go g.serve()
- return g
+ return g, nil
}Callers would then handle the returned error before deferring Close().
🤖 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 `@internal/tests/integration/javascript_test.go` around lines 305 - 321,
newGoExecCloseListener currently panics on listen/address errors; change its
signature to newGoExecCloseListener() (*goExecCloseListener, error) and replace
panic(fmt.Errorf(...)) and the other panic with returning nil and the wrapped
error, constructing and returning the listener struct on success. Update any
callers (e.g., places in Execute paths that call newGoExecCloseListener) to
handle the error, return it up the stack, and only defer g.Close() after the
listener is successfully created; reference the goExecCloseListener type and the
newGoExecCloseListener function to find all uses to update.
Mzack9999
deleted the
dwisiswant0/feat/js/add-wmi-tsch-scmr-and-dcom-helper-modules
branch
May 14, 2026 18:53
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Add shared auth, options, result, redaction, and
adapter support for GoExec-backed JavaScript
helpers, including:
nuclei/wminuclei/tschnuclei/scmrnuclei/dcomthrough generated bindings, and register them with
the JS compiler.
Also make JS generators to skip internal helper
packages and test files.
Closes #6069
Proof
Checklist
Summary by CodeRabbit
New Features
Tests
Chores