fix(js): propagate proxy, timeout, and custom headers to JS templates#7281
Open
eyupcanakman wants to merge 2 commits intoprojectdiscovery:devfrom
Open
fix(js): propagate proxy, timeout, and custom headers to JS templates#7281eyupcanakman wants to merge 2 commits intoprojectdiscovery:devfrom
eyupcanakman wants to merge 2 commits intoprojectdiscovery:devfrom
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(135,206,250,0.5)
participant JS as JavaScript Runtime
end
rect rgba(144,238,144,0.5)
participant Pool as Runtime Pool
end
rect rgba(255,228,181,0.5)
participant Net as net library
end
rect rgba(255,182,193,0.5)
participant Proxy as HTTP CONNECT Proxy
end
rect rgba(221,160,221,0.5)
participant Target as Remote Target
end
JS->>Pool: ExecuteWithOptions { ProxyURL, timeoutVariants }
Pool->>JS: inject context values (proxyURL, timeoutVariants)
JS->>Net: Open/OpenTLS(ctx, address)
Net->>Net: getTimeoutFromContext(ctx)
alt proxyURL present (http/https)
Net->>Proxy: dialHTTPProxy CONNECT target (with optional BasicAuth)
Proxy-->>Net: 200 OK (tunnel established)
Net->>Target: (over tunnel) TLS handshake / TCP I/O
else no proxy
Net->>Target: direct dial (respect timeout)
end
Net-->>JS: return net.Conn or error
Pool->>JS: cleanup context values (proxyURL, timeoutVariants)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/js/libs/net/net.go (1)
106-115: Silent fallback to direct connection when proxy URL parsing fails.If
url.Parse(proxyURL)returns an error, the code silently falls through to direct dialing without logging or notifying the user. This could mask configuration issues where the user expects traffic to go through a proxy but it doesn't.Consider logging a warning when proxy URL parsing fails so users are aware their proxy configuration is being ignored.
💡 Suggested enhancement for Open (similar change needed in OpenTLS)
// check for HTTP proxy in context if proxyURL, ok := ctx.Value("proxyURL").(string); ok && proxyURL != "" && protocol == "tcp" { parsed, err := url.Parse(proxyURL) + if err != nil { + // Log and fall through to direct connection + // Consider: gologger.Warning().Msgf("invalid proxy URL %q, using direct connection: %v", proxyURL, err) + } if err == nil && (parsed.Scheme == "http" || parsed.Scheme == "https") {Also applies to: 146-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/libs/net/net.go` around lines 106 - 115, The code silently falls back to direct dialing when url.Parse(proxyURL) fails; modify the block that checks ctx.Value("proxyURL") in the TCP proxy branch to detect the parse error and emit a warning (using the existing logger or processLogger) including the proxyURL and the parse error before continuing to the direct dial fallback; update the same pattern in the corresponding OpenTLS branch (the code around dialHTTPProxy and NetConn creation) so both Open and OpenTLS warn on parse failure rather than failing silently.pkg/protocols/javascript/js.go (1)
396-404: Inconsistent indentation on line 396.Line 396 has unusual leading whitespace before
result, err :=that doesn't match the surrounding code style.💅 Suggested fix
- result, err := request.options.JsCompiler.ExecuteWithOptions(request.preConditionCompiled, argsCopy, + result, err := request.options.JsCompiler.ExecuteWithOptions(request.preConditionCompiled, argsCopy,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/javascript/js.go` around lines 396 - 404, The indentation for the call that assigns result and err (the line starting with "result, err :=" in the block invoking request.options.JsCompiler.ExecuteWithOptions) is misaligned; align that line with the surrounding block so it matches the indentation of the subsequent argument lines and the enclosing function/method (look for ExecuteWithOptions invocation inside the method where request.preConditionCompiled and argsCopy are passed). Ensure the entire call uses consistent indentation (same indent level for the "result, err :=" line and the following argument lines) to match project style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/js/libs/net/net.go`:
- Around line 106-115: The code silently falls back to direct dialing when
url.Parse(proxyURL) fails; modify the block that checks ctx.Value("proxyURL") in
the TCP proxy branch to detect the parse error and emit a warning (using the
existing logger or processLogger) including the proxyURL and the parse error
before continuing to the direct dial fallback; update the same pattern in the
corresponding OpenTLS branch (the code around dialHTTPProxy and NetConn
creation) so both Open and OpenTLS warn on parse failure rather than failing
silently.
In `@pkg/protocols/javascript/js.go`:
- Around line 396-404: The indentation for the call that assigns result and err
(the line starting with "result, err :=" in the block invoking
request.options.JsCompiler.ExecuteWithOptions) is misaligned; align that line
with the surrounding block so it matches the indentation of the subsequent
argument lines and the enclosing function/method (look for ExecuteWithOptions
invocation inside the method where request.preConditionCompiled and argsCopy are
passed). Ensure the entire call uses consistent indentation (same indent level
for the "result, err :=" line and the following argument lines) to match project
style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e48f834-ad14-4dfb-af02-852afd33cfe1
📒 Files selected for processing (5)
pkg/js/compiler/compiler.gopkg/js/compiler/pool.gopkg/js/libs/net/net.gopkg/js/libs/net/net_test.gopkg/protocols/javascript/js.go
eyupcanakman
force-pushed
the
fix/js-template-flags-6645
branch
from
7b2d48a to
07f21d8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/protocols/javascript/js.go (1)
157-162:⚠️ Potential issue | 🟡 MinorInit block does not receive ProxyURL unlike other execution paths.
The
Initexecution (lines 157-162) passesTimeoutVariantsbut notProxyURL, while pre-condition (line 402) and request execution (line 579) both passProxyURL. If Init code makes network calls vianet.Open/OpenTLS, those connections won't use the configured proxy.This may be intentional (Init runs once during template compilation, typically for payload preparation), but worth verifying whether Init should also support proxy routing for consistency.
Proposed fix if proxy support is needed in Init
opts := &compiler.ExecuteOptions{ ExecutionId: request.options.Options.ExecutionId, TimeoutVariants: request.options.Options.GetTimeouts(), + ProxyURL: request.options.Options.AliveHttpProxy, Source: &request.Init, Context: context.Background(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/javascript/js.go` around lines 157 - 162, Init execution builds compiler.ExecuteOptions without setting ProxyURL, so Init network calls won't use the configured proxy; update the code that constructs compiler.ExecuteOptions for the Init run (the opts variable in the Init path) to include the same ProxyURL assignment as other paths (e.g., set ProxyURL: request.options.Options.GetProxyURL() or use request.options.Options.ProxyURL) so that compiler.ExecuteOptions used by Init includes ProxyURL just like the pre-condition and request execution paths; modify the block that creates opts (where ExecutionId, TimeoutVariants, Source, Context are set) to also populate ProxyURL from the request options.
🧹 Nitpick comments (1)
pkg/protocols/javascript/js.go (1)
396-404: Minor formatting inconsistency.Line 396 appears to have inconsistent indentation (extra leading whitespace before
result, err :=). Consider runninggo fmt ./...to ensure consistent formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/javascript/js.go` around lines 396 - 404, The call to request.options.JsCompiler.ExecuteWithOptions in js.go has inconsistent leading whitespace before "result, err :="; fix by normalizing indentation to align with surrounding code (remove the extra leading space) and run go fmt (go fmt ./...) to ensure consistent formatting across the file; look for the ExecuteWithOptions invocation in the same function where request.preConditionCompiled, argsCopy and compiler.ExecuteOptions are passed to locate and correct the indentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/libs/net/net.go`:
- Around line 71-85: The code currently discards any bytes buffered by
bufio.NewReader(proxyConn) after parsing the CONNECT response, causing data
loss; change the return to wrap proxyConn in a new bufferedConn that preserves
the bufio.Reader's buffered bytes. Implement a bufferedConn type that holds the
original net.Conn and the bufio.Reader (or an io.Reader created from the
reader's buffer + conn), have its Read first drain the reader's buffered bytes
and then read from the underlying conn, delegate Close/SetDeadline/etc. to the
underlying connection, and return that bufferedConn from the function instead of
proxyConn so no bytes are lost after http.ReadResponse.
---
Outside diff comments:
In `@pkg/protocols/javascript/js.go`:
- Around line 157-162: Init execution builds compiler.ExecuteOptions without
setting ProxyURL, so Init network calls won't use the configured proxy; update
the code that constructs compiler.ExecuteOptions for the Init run (the opts
variable in the Init path) to include the same ProxyURL assignment as other
paths (e.g., set ProxyURL: request.options.Options.GetProxyURL() or use
request.options.Options.ProxyURL) so that compiler.ExecuteOptions used by Init
includes ProxyURL just like the pre-condition and request execution paths;
modify the block that creates opts (where ExecutionId, TimeoutVariants, Source,
Context are set) to also populate ProxyURL from the request options.
---
Nitpick comments:
In `@pkg/protocols/javascript/js.go`:
- Around line 396-404: The call to request.options.JsCompiler.ExecuteWithOptions
in js.go has inconsistent leading whitespace before "result, err :="; fix by
normalizing indentation to align with surrounding code (remove the extra leading
space) and run go fmt (go fmt ./...) to ensure consistent formatting across the
file; look for the ExecuteWithOptions invocation in the same function where
request.preConditionCompiled, argsCopy and compiler.ExecuteOptions are passed to
locate and correct the indentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63b76d89-fc75-4d50-a6eb-490f0ec376e1
📒 Files selected for processing (5)
pkg/js/compiler/compiler.gopkg/js/compiler/pool.gopkg/js/libs/net/net.gopkg/js/libs/net/net_test.gopkg/protocols/javascript/js.go
✅ Files skipped from review due to trivial changes (1)
- pkg/js/libs/net/net_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/js/compiler/compiler.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/js/libs/net/net_test.go (1)
19-44: Consider testing the actual implementation function.These tests validate the context value pattern and URL parsing behavior, but they don't exercise the actual
proxyURLFromContextfunction (or equivalent) fromnet.go. While this documents the expected pattern, consider adding a test that calls the real function to ensure it handles these cases correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/libs/net/net_test.go` around lines 19 - 44, Add a unit test that calls the actual proxyURLFromContext function (and optionally dialHTTPProxy where relevant) instead of only asserting on raw context values and url.Parse; specifically, create contexts that contain "proxyURL" with "http://proxy.example.com:8080", an empty context, and a "socks5://proxy.example.com:1080" context and assert proxyURLFromContext(ctx) returns the expected results (the http URL and true, empty/false for missing, and either empty/false or no-op for the socks5 case) and that dialHTTPProxy is not triggered for a socks scheme; reference proxyURLFromContext and dialHTTPProxy to locate the implementations and ensure the tests exercise their real behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/js/libs/net/net_test.go`:
- Around line 19-44: Add a unit test that calls the actual proxyURLFromContext
function (and optionally dialHTTPProxy where relevant) instead of only asserting
on raw context values and url.Parse; specifically, create contexts that contain
"proxyURL" with "http://proxy.example.com:8080", an empty context, and a
"socks5://proxy.example.com:1080" context and assert proxyURLFromContext(ctx)
returns the expected results (the http URL and true, empty/false for missing,
and either empty/false or no-op for the socks5 case) and that dialHTTPProxy is
not triggered for a socks scheme; reference proxyURLFromContext and
dialHTTPProxy to locate the implementations and ensure the tests exercise their
real behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14e89dde-0a04-4c9a-b244-f5cdb8dcba9f
📒 Files selected for processing (4)
pkg/js/compiler/compiler.gopkg/js/libs/net/net.gopkg/js/libs/net/net_test.gopkg/protocols/javascript/js.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/js/libs/net/net.go
- pkg/protocols/javascript/js.go
eyupcanakman
force-pushed
the
fix/js-template-flags-6645
branch
from
c0f4973 to
8f0568c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/libs/net/net.go`:
- Around line 127-140: The warning logs in the proxy handling code currently
print proxyURL verbatim (the ctx.Value("proxyURL") usage inside the TCP branch
and the matching OpenTLS branch), which can leak credentials; create a small
helper function (e.g., redactProxyURL(proxyURL string) string) that parses the
URL, strips or masks url.User (replace password with "***" or remove userinfo
entirely) and returns a safe string, then call that helper in the
gologger.Warning().Msgf calls instead of printing proxyURL directly; update both
the tcp branch around dialHTTPProxy and the OpenTLS proxy handling to use the
same helper so credential-containing proxy URLs are never logged.
- Around line 39-69: The dialHTTPProxy function currently treats all proxy URLs
as plaintext and always writes the CONNECT request directly to proxyConn; when
parsed.Scheme == "https" you must TLS-wrap the proxy connection first and use
default port 443 (instead of 8080) for host:port resolution. Update
dialHTTPProxy to: detect parsed.Scheme, set proxyAddr default port 443 for
"https" and 8080 otherwise, call dial to obtain proxyConn, and if parsed.Scheme
== "https" perform a tls.Client handshake (using parsed.Hostname() for
ServerName) before calling connectReq.Write; ensure Proxy-Authorization header
is still set on connectReq and that deadlines and error handling (Close on
error) remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 079d41ea-0884-4fd9-bc5d-81fca741081f
📒 Files selected for processing (5)
pkg/js/compiler/compiler.gopkg/js/compiler/pool.gopkg/js/libs/net/net.gopkg/js/libs/net/net_test.gopkg/protocols/javascript/js.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/protocols/javascript/js.go
Neo - PR Security ReviewNo security issues found Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Fixes #6645
JS protocol templates ignored --proxy, --timeout, and -H flags because these values were not forwarded to the JS execution context. This forwards them through ExecuteOptions into the JS runtime so net.Open/OpenTLS can route connections through an HTTP CONNECT proxy and respect the configured timeout. Custom headers are exposed as a template variable.
Proof
Checklist
Summary by CodeRabbit
New Features
Tests