add nuclei/http js client - #7568
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Goja-backed JavaScript HTTP client with redirects, cookies, headers, limits, host policy, response handling, NTLM helpers, compiler registration, unit tests, and integration tests for HTTP templates. ChangesJavaScript HTTP capabilities
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JSTemplate
participant HTTPClient
participant ProtocolState
participant LocalHTTPServer
JSTemplate->>HTTPClient: Request(method, URL, body)
HTTPClient->>ProtocolState: Validate execution ID and target
HTTPClient->>LocalHTTPServer: Send HTTP request
LocalHTTPServer-->>HTTPClient: Return status, headers, and body
HTTPClient-->>JSTemplate: Return Response
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/tests/integration/javascript_http_test.go (1)
51-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
Executebodies forjavascriptHTTPGetandjavascriptHTTPClientFlow.Both methods are identical apart from type name. Could share a helper, though the duplication is small and low-risk.
♻️ Optional helper extraction
+func runJSHTTPServerTest(filePath string) error { + ts := startJSHTTPServer() + defer ts.Close() + hostPort := strings.TrimPrefix(ts.URL, "http://") + results, err := runSignedNucleiTemplateAndGetResults(filePath, hostPort, debug) + if err != nil { + return err + } + return expectResultsCount(results, 1) +} + type javascriptHTTPGet struct{} func (j *javascriptHTTPGet) Execute(filePath string) error { - ts := startJSHTTPServer() - defer ts.Close() - hostPort := strings.TrimPrefix(ts.URL, "http://") - results, err := runSignedNucleiTemplateAndGetResults(filePath, hostPort, debug) - if err != nil { - return err - } - return expectResultsCount(results, 1) + return runJSHTTPServerTest(filePath) } type javascriptHTTPClientFlow struct{} func (j *javascriptHTTPClientFlow) Execute(filePath string) error { - ts := startJSHTTPServer() - defer ts.Close() - hostPort := strings.TrimPrefix(ts.URL, "http://") - results, err := runSignedNucleiTemplateAndGetResults(filePath, hostPort, debug) - if err != nil { - return err - } - return expectResultsCount(results, 1) + return runJSHTTPServerTest(filePath) }🤖 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_http_test.go` around lines 51 - 76, Extract the shared server setup, template execution, error propagation, and result-count validation from javascriptHTTPGet.Execute and javascriptHTTPClientFlow.Execute into a helper, then have both Execute methods delegate to it while preserving the existing behavior.pkg/js/libs/http/http.go (1)
369-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a typed context key instead of a raw string.
ctx.Value("executionId")uses a barestringkey;go vet/staticcheck(SA1029) flag this pattern because it risks collisions with keys from other packages. The test helperwithExecmirrors this exact pattern but explicitly suppresses the linter (//nolint:staticcheck // SA1029) — the fix belongs here at the production call site so the test doesn't need to keep mirroring an anti-pattern.♻️ Suggested typed context key
+type contextKey string + +const executionIDContextKey contextKey = "executionId" + func executionIDFrom(ctx context.Context, c *Client) string { if c != nil && c.nj != nil { if id := c.nj.ExecutionId(); id != "" { return id } } if ctx == nil { return "" } - if v := ctx.Value("executionId"); v != nil { + if v := ctx.Value(executionIDContextKey); v != nil { if id, ok := v.(string); ok { return id } } return "" }As per coding guidelines,
**/*.gochanges should pass static analysis (go vet ./...); while SA1029 is astaticcheckrule rather thango vet, it's the same category of Go static-analysis hygiene this guideline is aimed at.🤖 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/http/http.go` around lines 369 - 384, Replace the raw "executionId" key used by executionIDFrom with a package-private typed context-key symbol, and use that symbol for the ctx.Value lookup. Update the test helper withExec to use the same production key rather than relying on the linter suppression, preserving the existing execution ID fallback behavior.Source: Coding guidelines
pkg/js/libs/http/ntlm_test.go (1)
30-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend
buildType2to cover AvId 9 and a no-VERSION header.Coverage gap tied to the two
ntlm.gofindings: no test emits anAvId=9(MsvAvTargetName) pair or a 48-byte (no-VERSION) challenge, so the AV-pair-8/9 mismap and the unconditional VERSION read inparseNTLMMessagearen't caught by the suite. Consider adding aheaderLenparameter/variant tobuildType2and one test per scenario.🤖 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/http/ntlm_test.go` around lines 30 - 85, Extend buildType2 to optionally emit AvId 9 (MsvAvTargetName) and construct a 48-byte challenge without the VERSION field, while preserving the existing 56-byte behavior. Add focused tests covering AvId 9 parsing and no-VERSION header parsing so both scenarios exercise parseNTLMMessage.
🤖 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/libs/http/ntlm.go`:
- Around line 117-123: Gate the VERSION parsing in the ProductVersion extraction
logic on confirmed VERSION presence rather than len(data) alone. Use the
NTLMSSP_NEGOTIATE_VERSION bit in NegotiateFlags, or ensure offset 48 plus 8
bytes precedes the smallest non-zero targetNameOffset/targetInfoOffset, while
preserving the existing ProductVersion empty check and formatting.
- Around line 144-163: Update the AV-pair switch to treat ID 8 as
MsvAvSingleHost without decoding it as text, and add ID 9 as MsvAvTargetName
that populates info.TargetName using the existing conditional behavior. Keep the
existing handling for all other AV IDs unchanged.
---
Nitpick comments:
In `@internal/tests/integration/javascript_http_test.go`:
- Around line 51-76: Extract the shared server setup, template execution, error
propagation, and result-count validation from javascriptHTTPGet.Execute and
javascriptHTTPClientFlow.Execute into a helper, then have both Execute methods
delegate to it while preserving the existing behavior.
In `@pkg/js/libs/http/http.go`:
- Around line 369-384: Replace the raw "executionId" key used by executionIDFrom
with a package-private typed context-key symbol, and use that symbol for the
ctx.Value lookup. Update the test helper withExec to use the same production key
rather than relying on the linter suppression, preserving the existing execution
ID fallback behavior.
In `@pkg/js/libs/http/ntlm_test.go`:
- Around line 30-85: Extend buildType2 to optionally emit AvId 9
(MsvAvTargetName) and construct a 48-byte challenge without the VERSION field,
while preserving the existing 56-byte behavior. Add focused tests covering AvId
9 parsing and no-VERSION header parsing so both scenarios exercise
parseNTLMMessage.
🪄 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: f2affae5-ac6e-4b13-a723-2005f4a66345
⛔ Files ignored due to path filters (6)
internal/tests/integration/testdata/protocols/javascript/http-client-flow.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/http-denied.yamlis excluded by!**/*.yamlinternal/tests/integration/testdata/protocols/javascript/http-get.yamlis excluded by!**/*.yamlpkg/js/generated/go/libhttp/http.gois excluded by!**/generated/**pkg/js/generated/ts/http.tsis excluded by!**/generated/**pkg/js/generated/ts/index.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
internal/tests/integration/javascript_http_test.gointernal/tests/integration/javascript_test.gopkg/js/compiler/pool.gopkg/js/libs/http/http.gopkg/js/libs/http/http_test.gopkg/js/libs/http/ntlm.gopkg/js/libs/http/ntlm_test.gopkg/js/libs/http/response.go
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 `@pkg/js/libs/http/ntlm.go`:
- Around line 115-116: Update the target-info validation in the NTLM parsing
flow around parseAVPairs to perform offset-plus-length bounds checks using
uint64 arithmetic, avoiding int conversion of untrusted uint32 values. Only
convert targetInfoOffset and targetInfoLen to int after confirming the range
fits within len(data), then preserve the existing parseAVPairs behavior.
🪄 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: 2e0edf7c-e291-4745-aa6d-09697e782d7a
📒 Files selected for processing (2)
pkg/js/libs/http/ntlm.gopkg/js/libs/http/ntlm_test.go
dwisiswant0
left a comment
dwisiswant0
left a comment
There was a problem hiding this comment.
banger.
(too large didn't review so I'm just gonna ask this instead:) does this respect our sandbox network policy?
@coderabbitai answer this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline pkg/js/libs/http/http.go --items all
printf '\n--- policy-related symbols and tests ---\n'
rg -n -C 5 -i 'protocolstate|executionid|allow|deny|redirect|fastdialer|dialcontext|checkredirect' \
pkg/js/libs/http/http.go pkg/js/libs/http/http_test.go internal/tests/integration/javascript_http_test.go
printf '\n--- implementation ---\n'
sed -n '1,460p' pkg/js/libs/http/http.goLength of output: 45421
So |
3 participants
Closes #7567
Closes #4679
Add require('nuclei/http'), an httpx-shaped JS HTTP client for multi-step HTTP in code instead of YAML string soup or one-off DSL helpers.
Summary by CodeRabbit
Response(status, final URL, body, and flattened headers with case-insensitive lookup).