Open
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Comment |
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:
WalkthroughAdds an exported XSSContextAnalyzer that appends a canary ("pd_xss"), issues the rebuilt HTTP request, reads up to 4MB of the response, tokenizes the HTML, and detects canary reflections in attribute values or text nodes; registers the analyzer and adds unit tests covering several cases. Changes
Sequence DiagramsequenceDiagram
participant Test
participant Analyzer as XSSContextAnalyzer
participant HttpClient
participant Server
participant Tokenizer as HTMLTokenizer
Test->>Analyzer: Analyze(options)
activate Analyzer
Analyzer->>Analyzer: ApplyInitialTransformation(data -> data + "pd_xss")
Analyzer->>HttpClient: Do(rebuilt request)
HttpClient->>Server: HTTP request
Server-->>HttpClient: HTTP response
HttpClient-->>Analyzer: response
Analyzer->>Analyzer: Read body (limit 4MB)
Analyzer->>Tokenizer: Tokenize HTML
Tokenizer-->>Analyzer: token stream
loop scan tokens
Analyzer->>Analyzer: check attributes / text for canary
end
alt attribute match
Analyzer-->>Test: (true, "attr:<attr-key>:<tag>", nil)
else text match
Analyzer-->>Test: (true, "text:<text>", nil)
else no match
Analyzer-->>Test: (true, "reflected:unknown", nil)
end
deactivate Analyzer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pkg/fuzz/analyzers/xss_analyzer_test.go (2)
67-69:MockComponent.Rebuild()uses raw string concatenation without URL-encoding the value.If a future test uses a payload with
&,=,#, or spaces, the reconstructed URL will be malformed and the test will break in a confusing way. Useurl.Valuesfor correctness.Proposed fix
+import "net/url" + func (m *MockComponent) Rebuild() (*retryablehttp.Request, error) { - return retryablehttp.NewRequest("GET", fmt.Sprintf("%s?%s=%s", m.URL, m.Key, m.CurrentValue), nil) + params := url.Values{} + params.Set(m.Key, m.CurrentValue) + return retryablehttp.NewRequest("GET", m.URL+"?"+params.Encode(), nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 67 - 69, MockComponent.Rebuild builds the request URL by raw string concatenation which fails for values containing &, =, #, or spaces; update the Rebuild method to construct the query using url.Values (or url.QueryEscape) and append the encoded query to m.URL so that m.Key and m.CurrentValue are properly URL-encoded before calling retryablehttp.NewRequest; ensure you reference the existing symbols (MockComponent.Rebuild, m.URL, m.Key, m.CurrentValue) and return the same retryablehttp.NewRequest(...) result after encoding.
15-47: Test coverage is limited to a single happy-path scenario.The test only covers text-node reflection. To gain confidence, consider adding at least:
- Attribute reflection — server echoes payload inside a tag attribute.
- Negative case — server returns a page that does not reflect the input; assert
matched == false.- Error propagation — e.g., unreachable server, verify error is returned.
These are important because the analyzer has multiple code paths (attribute check, text check, error handling, no-match).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 15 - 47, Add two new subtests inside TestXSSContextAnalyzer_Analyze to cover attribute reflection, negative/no-match, and error propagation: (1) "reflection-in-attribute" — spin up an httptest.Server whose handler echoes the query value into an HTML attribute (e.g., <div data-val="%s">), build Options with fuzz.GeneratedRequest and MockComponent like the existing case, call XSSContextAnalyzer.Analyze and assert matched==true, message mentions "attribute" (or attribute-related text), and verify opts.FuzzGenerated.Component.(*MockComponent).CurrentValue restored to original; (2) "no-reflection" — server returns static HTML that does not include the query value, call Analyze and assert matched==false (and no error), plus component restored; (3) "error-propagation" — use an unreachable URL or create and Close() an httptest.Server before sending the request, call Analyze and assert an error is returned; reuse the same Options/fuzz.GeneratedRequest/MockComponent setup and check CurrentValue restoration in each case to ensure cleanup logic in XSSContextAnalyzer.Analyze works.pkg/fuzz/analyzers/xss_analyzer.go (2)
20-77: The "context-aware" XSS analysis only covers attribute values and text nodes — important HTML contexts are missing.For a "context-aware" XSS analyzer, reflections inside
<script>blocks, HTML comments (CommentToken), and<style>blocks are critical XSS injection vectors that are not detected here. The current implementation would silently miss reflections in those contexts, potentially producing false negatives for real-world XSS.This doesn't need to block the initial merge, but should be tracked — at minimum,
html.CommentTokenis easy to add now.Minimal addition for comment token handling
case html.TextToken: if strings.Contains(token.Data, payload) { return true, "reflection in html text node", nil } + case html.CommentToken: + if strings.Contains(token.Data, payload) { + return true, "reflection in html comment", nil + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 20 - 77, The Analyze method of XSSContextAnalyzer only checks html.StartTagToken/html.SelfClosingTagToken and html.TextToken; add handling for html.CommentToken and contexts for script/style blocks: inspect html.CommentToken for payload reflections and treat html.StartTagToken with token.Data == "script" or "style" so you parse subsequent html.TextToken (or html.RawTextToken if applicable) within those tags for payload occurrences; update the tokenizer switch in Analyze (referencing tokenizer, tokenType, token, payload) to return the same true/description pattern when reflections are found in comments, <script> content, or <style> content.
29-31: Silently discarding the restore error could leave the component in a mutated state.Per the Component contract, the value must be restored after mutation. If the deferred
SetValuefails, downstream code will operate on stale/mutated state. Consider at least logging the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 29 - 31, The deferred restore currently discards any error from gr.Component.SetValue(gr.Key, gr.Value) which can leave the component mutated; update the deferred func to capture the returned error and at minimum log it (instead of assigning to `_`). Specifically, change the defer block around gr.Component.SetValue to something like capturing err := gr.Component.SetValue(...) and if err != nil call the package or component logger (or log.Printf) with context including gr.Key and gr.Value so failures to restore are visible; keep the same deferred semantics and use the same symbols gr.Component.SetValue, gr.Key and gr.Value to locate the change.
🤖 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/fuzz/analyzers/xss_analyzer_test.go`:
- Line 27: The test discards the error returned by retryablehttp.NewRequest and
may panic later; update the call that creates req (retryablehttp.NewRequest) to
capture the error and assert it with require.NoError(t, err) (or the existing
test assertion helper) immediately after creation so failures produce clear
diagnostics rather than a nil-pointer panic.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Around line 38-42: The call rebuilt.WithContext(rebuilt.Context()) is a no-op;
change the request to use a real upstream context (or drop WithContext
entirely). Replace the current call to
options.HttpClient.Do(rebuilt.WithContext(rebuilt.Context())) by either
propagating the caller/options context (e.g., rebuilt =
rebuilt.WithContext(options.Context or options.Ctx) and then
options.HttpClient.Do(rebuilt)) or simply call options.HttpClient.Do(rebuilt) if
no propagation is needed; update the code paths around options.HttpClient.Do and
rebuilt to use the chosen context propagation approach.
- Around line 45-48: The code currently calls io.ReadAll(resp.Body) (producing
bodyBytes) which can OOM on large HTTP responses; replace that call with reading
from io.LimitReader(resp.Body, maxBodySize+1) using a defined constant (e.g.,
maxBodySize) so the read is capped, then detect if the result length >
maxBodySize to treat as "too large" and return an error or truncated result.
Update the logic around bodyBytes and err to use the LimitReader, close
resp.Body as before, and ensure any downstream code that expects the full body
handles the truncated-case consistently.
- Around line 16-18: The ApplyInitialTransformation function currently appends
the hardcoded canary "pd_xss", causing false positives; modify
XSSContextAnalyzer.ApplyInitialTransformation to call the shared
randStringBytesMask() (from analyzers.go) to generate a per-request randomized
canary, append that generated string to data, and return it, and ensure the test
harness captures and exposes the generated canary value so reflection checks
verify against the dynamic payload.
---
Nitpick comments:
In `@pkg/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 67-69: MockComponent.Rebuild builds the request URL by raw string
concatenation which fails for values containing &, =, #, or spaces; update the
Rebuild method to construct the query using url.Values (or url.QueryEscape) and
append the encoded query to m.URL so that m.Key and m.CurrentValue are properly
URL-encoded before calling retryablehttp.NewRequest; ensure you reference the
existing symbols (MockComponent.Rebuild, m.URL, m.Key, m.CurrentValue) and
return the same retryablehttp.NewRequest(...) result after encoding.
- Around line 15-47: Add two new subtests inside TestXSSContextAnalyzer_Analyze
to cover attribute reflection, negative/no-match, and error propagation: (1)
"reflection-in-attribute" — spin up an httptest.Server whose handler echoes the
query value into an HTML attribute (e.g., <div data-val="%s">), build Options
with fuzz.GeneratedRequest and MockComponent like the existing case, call
XSSContextAnalyzer.Analyze and assert matched==true, message mentions
"attribute" (or attribute-related text), and verify
opts.FuzzGenerated.Component.(*MockComponent).CurrentValue restored to original;
(2) "no-reflection" — server returns static HTML that does not include the query
value, call Analyze and assert matched==false (and no error), plus component
restored; (3) "error-propagation" — use an unreachable URL or create and Close()
an httptest.Server before sending the request, call Analyze and assert an error
is returned; reuse the same Options/fuzz.GeneratedRequest/MockComponent setup
and check CurrentValue restoration in each case to ensure cleanup logic in
XSSContextAnalyzer.Analyze works.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Around line 20-77: The Analyze method of XSSContextAnalyzer only checks
html.StartTagToken/html.SelfClosingTagToken and html.TextToken; add handling for
html.CommentToken and contexts for script/style blocks: inspect
html.CommentToken for payload reflections and treat html.StartTagToken with
token.Data == "script" or "style" so you parse subsequent html.TextToken (or
html.RawTextToken if applicable) within those tags for payload occurrences;
update the tokenizer switch in Analyze (referencing tokenizer, tokenType, token,
payload) to return the same true/description pattern when reflections are found
in comments, <script> content, or <style> content.
- Around line 29-31: The deferred restore currently discards any error from
gr.Component.SetValue(gr.Key, gr.Value) which can leave the component mutated;
update the deferred func to capture the returned error and at minimum log it
(instead of assigning to `_`). Specifically, change the defer block around
gr.Component.SetValue to something like capturing err :=
gr.Component.SetValue(...) and if err != nil call the package or component
logger (or log.Printf) with context including gr.Key and gr.Value so failures to
restore are visible; keep the same deferred semantics and use the same symbols
gr.Component.SetValue, gr.Key and gr.Value to locate the change.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/fuzz/analyzers/xss_analyzer_test.go (1)
16-45: Add coverage for comment reflections.The analyzer now detects comment reflections; a small subtest will prevent regressions.
✅ Suggested test
t.Run("reflection-in-attribute", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "<input value='%s'>", r.URL.Query().Get("q")) })) defer server.Close() runTest(t, analyzer, server.URL, "attribute") }) + t.Run("reflection-in-comment", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, "<!--%s-->", r.URL.Query().Get("q")) + })) + defer server.Close() + runTest(t, analyzer, server.URL, "comment") + }) + t.Run("no-reflection", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, "<html>safe content</html>") }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 16 - 45, Add a new subtest to TestXSSContextAnalyzer_Analyze that verifies comment reflections are detected: create an httptest server handler that writes the query param inside an HTML comment (e.g., "<!--%s-->"), defer Close, and call runTest(t, analyzer, server.URL, "comment") like the existing text/attribute cases; alternatively, mimic the "no-reflection" flow by using setupOptions and invoking analyzer.Analyze(opts) and asserting matched == true and err == nil to ensure XSSContextAnalyzer detects comment context reflections.pkg/fuzz/analyzers/xss_analyzer.go (1)
49-55: Avoid silent truncation of large bodies.The current cap prevents OOM, but it silently truncates responses, which can hide reflections. Consider detecting overflow (e.g., read
maxBodySize+1) and return a clear error (or at least log) when exceeded.🔧 Suggested adjustment
- const maxBodySize = 4 * 1024 * 1024 - bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxBodySize)) + const maxBodySize = 4 * 1024 * 1024 + bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxBodySize+1)) if err != nil { return false, "", err } + if len(bodyBytes) > maxBodySize { + return false, "", fmt.Errorf("response body exceeds %d bytes", maxBodySize) + } body := string(bodyBytes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 49 - 55, The current read using maxBodySize with io.LimitReader silently truncates large responses and may hide reflections; change the read call to use maxBodySize+1 (use io.LimitReader(resp.Body, maxBodySize+1) and read into bodyBytes), then if len(bodyBytes) > maxBodySize return a clear error (or log a specific truncation error) instead of proceeding; update the code around maxBodySize, io.LimitReader, bodyBytes and body (in the XSS analysis function in xss_analyzer.go) so callers receive an explicit error when the response exceeds the cap.
🤖 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/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 16-45: Add a new subtest to TestXSSContextAnalyzer_Analyze that
verifies comment reflections are detected: create an httptest server handler
that writes the query param inside an HTML comment (e.g., "<!--%s-->"), defer
Close, and call runTest(t, analyzer, server.URL, "comment") like the existing
text/attribute cases; alternatively, mimic the "no-reflection" flow by using
setupOptions and invoking analyzer.Analyze(opts) and asserting matched == true
and err == nil to ensure XSSContextAnalyzer detects comment context reflections.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Around line 49-55: The current read using maxBodySize with io.LimitReader
silently truncates large responses and may hide reflections; change the read
call to use maxBodySize+1 (use io.LimitReader(resp.Body, maxBodySize+1) and read
into bodyBytes), then if len(bodyBytes) > maxBodySize return a clear error (or
log a specific truncation error) instead of proceeding; update the code around
maxBodySize, io.LimitReader, bodyBytes and body (in the XSS analysis function in
xss_analyzer.go) so callers receive an explicit error when the response exceeds
the cap.
… full test coverage
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/fuzz/analyzers/xss_analyzer.go (2)
64-64: Avoid the[]byte→string→io.Readerdouble conversion.
strings.NewReader(string(bodyBytes))allocates a new string copy unnecessarily.bytes.NewReaderaccepts the slice directly.♻️ Proposed fix
- return a.analyzeContent(strings.NewReader(string(bodyBytes)), payload) + return a.analyzeContent(bytes.NewReader(bodyBytes), payload)And add
"bytes"to the import block (remove"strings"if it is no longer used elsewhere in this file).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` at line 64, Replace the unnecessary []byte→string conversion when calling analyzeContent by passing a bytes.Reader directly: change the call in the function that currently does return a.analyzeContent(strings.NewReader(string(bodyBytes)), payload) to use bytes.NewReader(bodyBytes) instead; also update the import block to add "bytes" and remove "strings" if it is no longer used elsewhere in this file so imports remain correct.
40-41: Replace stdliblog.Printfwith the project'sgologger.The project uses
github.com/projectdiscovery/gologgerthroughout. Using stdlibloghere is inconsistent;gologger.Print()is the appropriate call for stderr-bound error output.♻️ Proposed fix
-import ( - "fmt" - "io" - "log" - "strings" - ... -) +import ( + "fmt" + "io" + "strings" + ... + "github.com/projectdiscovery/gologger" +)- log.Printf("[%s] critical: failed to restore component state: %v", XSSAnalyzerName, err) + gologger.Print().Msgf("[%s] critical: failed to restore component state: %v", XSSAnalyzerName, err)Based on learnings: "In projectdiscovery/gologger, the Print() function writes to stderr and Silent() writes to stdout … ensure code uses Print() for error/output going to stderr."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 40 - 41, Replace the stdlib call log.Printf(...) with the project's gologger by removing the "log" import and using github.com/projectdiscovery/gologger.Print to emit the same formatted error message (keep the existing formatting and XSSAnalyzerName/err values); update imports to add gologger and remove the stdlib log import so the gologger.Print call is used where the current log.Printf("[%s] critical: failed to restore component state: %v", XSSAnalyzerName, err) appears.pkg/fuzz/analyzers/xss_analyzer_test.go (1)
53-60: Fragile server handler — safe case identified by name string comparison.The condition
tt.name != "No Reflection (Safe)"couples server behavior to the test name, making it easy for a future case to silently misbehave. Since the safe-caseresponseBodycontains no%sverbs,fmt.Fprintfis already a no-op for it. Simplify to always usefmt.Fprintf:♻️ Proposed fix
- if tt.expectedMatch && tt.name != "No Reflection (Safe)" { - fmt.Fprintf(w, tt.responseBody, val) - } else { - fmt.Fprint(w, tt.responseBody) - } + fmt.Fprintf(w, tt.responseBody, val)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 53 - 60, The test server handler is fragile because it switches behavior based on tt.name; instead always use fmt.Fprintf(w, tt.responseBody, val) in the httptest server handler so the safe case (whose tt.responseBody contains no %s) remains safe and you avoid coupling logic to tt.name — update the anonymous handler used to create server := httptest.NewServer(http.HandlerFunc(...)) to call fmt.Fprintf unconditionally and remove the tt.name check.
🤖 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/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 38-43: The "HTML Comment Reflection" unit test is broken because
the server uses fmt.Fprintf(w, "", val) so the payload is never reflected;
update the test case in xss_analyzer_test.go for the subtest named "HTML Comment
Reflection" to use an HTML comment format string (e.g. "<!--%s-->") as the
responseBody so the payload is actually embedded and the analyzer can detect the
reflection (leave expectedMatch and expectedMsg as-is).
- Line 78: Rename the helper function setupSeniorOptions to setupServerOptions
and update any call sites (e.g., the test that currently calls
setupSeniorOptions) to use the new name; ensure the function signature (*Options
parameters) and all references in pkg/fuzz/analyzers/xss_analyzer_test.go match
the renamed identifier so tests compile and run.
- Around line 99-105: MockComponent.Rebuild is discarding errors from url.Parse
and retryablehttp.NewRequest causing nil requests and opaque panics; update
Rebuild to capture and return both errors instead of using "_"—validate the
parsed URL (from url.Parse) and return an error if parsing fails, then call
retryablehttp.NewRequest and return any error it returns, so Rebuild returns
(*retryablehttp.Request, error) with meaningful errors propagated to callers.
---
Nitpick comments:
In `@pkg/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 53-60: The test server handler is fragile because it switches
behavior based on tt.name; instead always use fmt.Fprintf(w, tt.responseBody,
val) in the httptest server handler so the safe case (whose tt.responseBody
contains no %s) remains safe and you avoid coupling logic to tt.name — update
the anonymous handler used to create server :=
httptest.NewServer(http.HandlerFunc(...)) to call fmt.Fprintf unconditionally
and remove the tt.name check.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Line 64: Replace the unnecessary []byte→string conversion when calling
analyzeContent by passing a bytes.Reader directly: change the call in the
function that currently does return
a.analyzeContent(strings.NewReader(string(bodyBytes)), payload) to use
bytes.NewReader(bodyBytes) instead; also update the import block to add "bytes"
and remove "strings" if it is no longer used elsewhere in this file so imports
remain correct.
- Around line 40-41: Replace the stdlib call log.Printf(...) with the project's
gologger by removing the "log" import and using
github.com/projectdiscovery/gologger.Print to emit the same formatted error
message (keep the existing formatting and XSSAnalyzerName/err values); update
imports to add gologger and remove the stdlib log import so the gologger.Print
call is used where the current log.Printf("[%s] critical: failed to restore
component state: %v", XSSAnalyzerName, err) appears.
Comment on lines
38
to
43
| { | ||
| name: "HTML Comment Reflection", | ||
| responseBody: "", | ||
| expectedMatch: true, | ||
| expectedMsg: "comment", | ||
| }, |
Contributor
There was a problem hiding this comment.
"HTML Comment Reflection" test case is broken — payload is never reflected.
responseBody is "". The server handler calls fmt.Fprintf(w, "", val): an empty format string ignores val, so the response body is empty. The analyzer returns matched=false but the test asserts expectedMatch=true, so this subtest will fail.
Provide the HTML comment template as the format string so the payload is actually embedded:
🐛 Proposed fix
{
name: "HTML Comment Reflection",
- responseBody: "",
+ responseBody: "<!-- %s -->",
expectedMatch: true,
expectedMsg: "comment",
},📝 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
| { | |
| name: "HTML Comment Reflection", | |
| responseBody: "", | |
| expectedMatch: true, | |
| expectedMsg: "comment", | |
| }, | |
| { | |
| name: "HTML Comment Reflection", | |
| responseBody: "<!-- %s -->", | |
| expectedMatch: true, | |
| expectedMsg: "comment", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 38 - 43, The "HTML
Comment Reflection" unit test is broken because the server uses fmt.Fprintf(w,
"", val) so the payload is never reflected; update the test case in
xss_analyzer_test.go for the subtest named "HTML Comment Reflection" to use an
HTML comment format string (e.g. "<!--%s-->") as the responseBody so the payload
is actually embedded and the analyzer can detect the reflection (leave
expectedMatch and expectedMsg as-is).
Comment on lines
99
to
105
| func (m *MockComponent) Rebuild() (*retryablehttp.Request, error) { | ||
| u, _ := url.Parse(m.URL) | ||
| q := u.Query() | ||
| q.Set(m.Key, m.CurrentValue) | ||
| u.RawQuery = q.Encode() | ||
| return retryablehttp.NewRequest("GET", u.String(), nil) | ||
| } |
Contributor
There was a problem hiding this comment.
Discarded errors in MockComponent.Rebuild can cause opaque panics.
Both url.Parse and retryablehttp.NewRequest errors are silently ignored with _. If either fails (e.g., malformed URL due to a future test change), the returned nil request will cause an unclear panic downstream.
♻️ Proposed fix
func (m *MockComponent) Rebuild() (*retryablehttp.Request, error) {
- u, _ := url.Parse(m.URL)
+ u, err := url.Parse(m.URL)
+ if err != nil {
+ return nil, fmt.Errorf("MockComponent: invalid URL %q: %w", m.URL, err)
+ }
q := u.Query()
q.Set(m.Key, m.CurrentValue)
u.RawQuery = q.Encode()
- return retryablehttp.NewRequest("GET", u.String(), nil)
+ req, err := retryablehttp.NewRequest("GET", u.String(), nil)
+ if err != nil {
+ return nil, fmt.Errorf("MockComponent: failed to build request: %w", err)
+ }
+ return req, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 99 - 105,
MockComponent.Rebuild is discarding errors from url.Parse and
retryablehttp.NewRequest causing nil requests and opaque panics; update Rebuild
to capture and return both errors instead of using "_"—validate the parsed URL
(from url.Parse) and return an error if parsing fails, then call
retryablehttp.NewRequest and return any error it returns, so Rebuild returns
(*retryablehttp.Request, error) with meaningful errors propagated to callers.
Author
|
@dogancanbakir I have Create one New update Final check please |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/fuzz/analyzers/xss_analyzer.go (2)
16-18:⚠️ Potential issue | 🟠 MajorUse a per-request canary and source it from fuzzed value, not a hardcoded literal.
Hardcoding
"pd_xss"makes matching brittle and can produce false positives. Generate a unique canary in transformation and inAnalyzeread the effective value fromoptions.FuzzGenerated.Value.Suggested fix
func (a *XSSContextAnalyzer) ApplyInitialTransformation(data string, params map[string]interface{}) string { - return data + "pd_xss" + return data + randStringBytesMask(8) } @@ - canary := "pd_xss" + canary := options.FuzzGenerated.Value + if canary == "" { + return false, "", nil + }Based on learnings: "extract the canary from FuzzGenerated.Value (the final reflected string) ... ensures compatibility across all fuzzing modes, including KV-mode."
Also applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 16 - 18, The XSSContextAnalyzer currently hardcodes the canary string in ApplyInitialTransformation ("pd_xss"), which is brittle; update ApplyInitialTransformation (on XSSContextAnalyzer) to generate or embed a per-request canary derived from the fuzz input instead of a literal, and change Analyze to read the effective canary from options.FuzzGenerated.Value (the final reflected string) for matching; locate ApplyInitialTransformation and Analyze in XSSContextAnalyzer, create/use a generated canary value passed through the fuzz options (options.FuzzGenerated.Value) and use that dynamic value for both transformation and verification so KV- and other fuzzing modes work correctly.
31-31:⚠️ Potential issue | 🟠 MajorAvoid unbounded body reads and handle read errors.
Line 31 ignores
io.ReadAllerrors and reads without a cap, which can hurt analyzer stability on large responses.Suggested fix
import ( + "fmt" "io" "strings" @@ - bodyBytes, _ := io.ReadAll(resp.Body) + const maxBodySize = 4 * 1024 * 1024 // 4MB + bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxBodySize+1)) + if err != nil { + return false, "", err + } + if len(bodyBytes) > maxBodySize { + return false, "", fmt.Errorf("response body exceeds %d bytes", maxBodySize) + }#!/bin/bash # Verify unbounded/unchecked response body read in current file rg -nP 'io\.ReadAll\(|LimitReader|bodyBytes,\s*_' pkg/fuzz/analyzers/xss_analyzer.go -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` at line 31, Replace the unchecked unbounded read of resp.Body (the line using io.ReadAll and variable bodyBytes) with a capped read and proper error handling: wrap resp.Body with io.LimitReader using a defined maxResponseBodyBytes constant (e.g., maxResponseBodyBytes) and call io.ReadAll on that reader, then check and handle the returned error instead of discarding it; ensure the function that uses bodyBytes (the xss analyzer code that reads resp.Body) also defers/closes resp.Body appropriately and handles the case where the read was truncated due to the limit.pkg/fuzz/analyzers/xss_analyzer_test.go (1)
61-64:⚠️ Potential issue | 🟡 MinorDon’t discard request-construction errors in test setup.
Line 63 ignores the
retryablehttp.NewRequesterror, which can lead to nil-request panics and opaque failures.Suggested fix
-func setupTestOptions(targetURL string) *Options { +func setupTestOptions(t *testing.T, targetURL string) *Options { + t.Helper() client := retryablehttp.NewClient(retryablehttp.DefaultOptionsSingle) - req, _ := retryablehttp.NewRequest("GET", targetURL, nil) + req, err := retryablehttp.NewRequest("GET", targetURL, nil) + require.NoError(t, err) return &Options{- opts := setupTestOptions(server.URL) + opts := setupTestOptions(t, server.URL) @@ - opts := setupTestOptions(server.URL) + opts := setupTestOptions(t, server.URL) @@ - opts := setupTestOptions(server.URL) + opts := setupTestOptions(t, server.URL)#!/bin/bash # Verify discarded request-construction errors in test helper rg -nP 'retryablehttp\.NewRequest\(.+\)\s*$|,\s*_\s*:=' pkg/fuzz/analyzers/xss_analyzer_test.go -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 61 - 64, The test helper setupTestOptions currently ignores the error returned by retryablehttp.NewRequest, which can produce nil-request panics; update setupTestOptions to capture (req, err := retryablehttp.NewRequest(...)) and handle err instead of discarding it—either return nil and an error from setupTestOptions or panic/log.Fatalf with a clear message; ensure the req variable is only used when err == nil and mention the symbols involved: setupTestOptions, Options, retryablehttp.NewRequest, and req.
🤖 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/fuzz/analyzers/xss_analyzer.go`:
- Line 40: The inTag boolean in xss_analyzer.go loses nesting context — replace
it with an integer depth counter (e.g., tagDepth) and update logic in the
functions that parse tags (the code around the current inTag usage and the block
referenced at lines 52-63) so you increment tagDepth when encountering a start
tag (including when encountering '<' followed by a name), decrement tagDepth on
end tags ('</...>'), handle self-closing tags by not incrementing or by
immediately decrementing, and change all checks from "inTag" to "tagDepth > 0"
so nested elements are tracked correctly and text reflections are classified
accurately.
- Around line 21-25: The code checks options and options.FuzzGenerated.Request
but calls options.HttpClient.Do(...) without guarding options.HttpClient; add a
nil check for options.HttpClient before invoking Do (e.g., if options.HttpClient
== nil { return false, "", nil } or return a suitable error) so the call to
options.HttpClient.Do(request) cannot nil-deref; update the logic around the
resp, err := options.HttpClient.Do(options.FuzzGenerated.Request) site to
perform this check and handle the nil-client case consistently with the existing
return values.
---
Duplicate comments:
In `@pkg/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 61-64: The test helper setupTestOptions currently ignores the
error returned by retryablehttp.NewRequest, which can produce nil-request
panics; update setupTestOptions to capture (req, err :=
retryablehttp.NewRequest(...)) and handle err instead of discarding it—either
return nil and an error from setupTestOptions or panic/log.Fatalf with a clear
message; ensure the req variable is only used when err == nil and mention the
symbols involved: setupTestOptions, Options, retryablehttp.NewRequest, and req.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Around line 16-18: The XSSContextAnalyzer currently hardcodes the canary
string in ApplyInitialTransformation ("pd_xss"), which is brittle; update
ApplyInitialTransformation (on XSSContextAnalyzer) to generate or embed a
per-request canary derived from the fuzz input instead of a literal, and change
Analyze to read the effective canary from options.FuzzGenerated.Value (the final
reflected string) for matching; locate ApplyInitialTransformation and Analyze in
XSSContextAnalyzer, create/use a generated canary value passed through the fuzz
options (options.FuzzGenerated.Value) and use that dynamic value for both
transformation and verification so KV- and other fuzzing modes work correctly.
- Line 31: Replace the unchecked unbounded read of resp.Body (the line using
io.ReadAll and variable bodyBytes) with a capped read and proper error handling:
wrap resp.Body with io.LimitReader using a defined maxResponseBodyBytes constant
(e.g., maxResponseBodyBytes) and call io.ReadAll on that reader, then check and
handle the returned error instead of discarding it; ensure the function that
uses bodyBytes (the xss analyzer code that reads resp.Body) also defers/closes
resp.Body appropriately and handles the case where the read was truncated due to
the limit.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/fuzz/analyzers/xss_analyzer.go (2)
32-34:⚠️ Potential issue | 🟠 MajorUnbounded
io.ReadAllwith discarded error, and canary hard-coded instead of extracted fromFuzzGenerated.Value.Two issues here:
io.ReadAllhas no size cap and its error is silently discarded. In a fuzzing context the target may return arbitrarily large bodies — this can OOM the process. Useio.LimitReaderand handle the error.Canary should be derived from
FuzzGenerated.Value, not re-declared as a literal. This ensures compatibility across all fuzzing modes (including KV-mode) and keeps the canary source consistent with the transformation step.Proposed fix
- bodyBytes, _ := io.ReadAll(resp.Body) + const maxBodySize = 4 * 1024 * 1024 // 4 MB + bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxBodySize)) + if err != nil { + return false, "", err + } body := string(bodyBytes) - canary := "pd_xss" + canary := options.FuzzGenerated.Value // extract canary from the fuzzed valueBased on learnings: "In Go code related to fuzz analyzers (e.g., XSS analyzer in pkg/fuzz/analyzers), extract the canary from FuzzGenerated.Value (the final reflected string) rather than from GeneratedRequest.OriginalPayload."
(unbounded ReadAll was flagged previously)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 32 - 34, Replace the unbounded, ignored-error read and the hard-coded canary: use io.LimitReader on resp.Body (e.g., wrap resp.Body with io.LimitReader using a safe max size) and call io.ReadAll (or io.ReadFull) capturing and handling the error instead of discarding it, then convert to string into the existing body variable; and derive the canary from the fuzz result (use FuzzGenerated.Value / the final reflected string) rather than re-declaring canary = "pd_xss" so the analyzer uses the actual generated payload (not GeneratedRequest.OriginalPayload or a literal). Ensure any resources (resp.Body) are closed as currently done and return/log errors as appropriate.
16-18: Still using hardcoded"pd_xss"canary — high false-positive risk.This was flagged in a prior review round and marked as addressed, but the code still appends a static literal. A six-character constant can naturally appear in responses. Use
randStringBytesMask()(already available inanalyzers.go) to generate a unique canary per transformation, and store it soAnalyzecan retrieve it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 16 - 18, The ApplyInitialTransformation currently appends a hardcoded "pd_xss" canary; change it to generate a unique canary with randStringBytesMask() (from analyzers.go), append that canary to the returned string, and store it on the XSSContextAnalyzer instance (e.g., add a field like lastCanary or canary) so the Analyze method can retrieve and match it; update Analyze to read that stored canary when checking for the marker instead of expecting the static literal.pkg/fuzz/analyzers/xss_analyzer_test.go (1)
61-70:⚠️ Potential issue | 🟠 MajorDiscarded error from
retryablehttp.NewRequestand missingFuzzGenerated.Value.Two issues in this helper:
The error from
retryablehttp.NewRequeston Line 63 is silently discarded — if it fails,reqis nil and the test panics with an opaque message. Userequire.NoError(you'd need to accept*testing.T).
FuzzGenerated.Valueis never set. Once the analyzer is corrected to extract the canary fromValue(per the learning about canary extraction), all three subtests will fail because the canary will be an empty string.Proposed fix
-func setupTestOptions(targetURL string) *Options { +func setupTestOptions(t *testing.T, targetURL string) *Options { + t.Helper() client := retryablehttp.NewClient(retryablehttp.DefaultOptionsSingle) - req, _ := retryablehttp.NewRequest("GET", targetURL, nil) + req, err := retryablehttp.NewRequest("GET", targetURL, nil) + require.NoError(t, err) return &Options{ HttpClient: client, FuzzGenerated: fuzz.GeneratedRequest{ Request: req, + Value: "pd_xss", // canary the analyzer should look for }, } }Update call sites accordingly:
- opts := setupTestOptions(server.URL) + opts := setupTestOptions(t, server.URL)(discarded error was flagged previously)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 61 - 70, In setupTestOptions, stop discarding the error from retryablehttp.NewRequest by changing the signature to accept *testing.T and using require.NoError(t, err) after creating req (so a failed request fails the test clearly); also populate the returned Options.FuzzGenerated.Value with the expected canary string (e.g., the same payload used in FuzzGenerated.Request) so the analyzer can extract the canary; update all call sites of setupTestOptions to pass the *testing.T argument.
🧹 Nitpick comments (1)
pkg/fuzz/analyzers/xss_analyzer_test.go (1)
14-58: Tests only cover happy paths — consider adding negative/edge-case subtests.All three subtests expect
found == true. Consider adding at least:
- A subtest where the response does not contain the canary → verifies
found == false.- A subtest with
niloptions → verifies the nil guard returns(false, "", nil)without panic.This would exercise the early-return paths in
Analyzeand improve confidence in the guard clauses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 14 - 58, Add two negative subtests inside TestXSSContextAnalyzer_Analyze to cover early-return paths: (1) "no-canary-detection" should start an httptest server that responds without the canary value, call setupTestOptions(server.URL) and assert analyzer.Analyze(opts) returns found == false (and context == ""), and (2) "nil-options-guard" should call analyzer.Analyze(nil) and assert it returns (false, "", nil) to ensure the nil guard in XSSContextAnalyzer.Analyze is exercised; use the existing analyzer variable and require.NoError/require.False/require.Equal assertions similar to the other subtests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 61-70: In setupTestOptions, stop discarding the error from
retryablehttp.NewRequest by changing the signature to accept *testing.T and
using require.NoError(t, err) after creating req (so a failed request fails the
test clearly); also populate the returned Options.FuzzGenerated.Value with the
expected canary string (e.g., the same payload used in FuzzGenerated.Request) so
the analyzer can extract the canary; update all call sites of setupTestOptions
to pass the *testing.T argument.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Around line 32-34: Replace the unbounded, ignored-error read and the
hard-coded canary: use io.LimitReader on resp.Body (e.g., wrap resp.Body with
io.LimitReader using a safe max size) and call io.ReadAll (or io.ReadFull)
capturing and handling the error instead of discarding it, then convert to
string into the existing body variable; and derive the canary from the fuzz
result (use FuzzGenerated.Value / the final reflected string) rather than
re-declaring canary = "pd_xss" so the analyzer uses the actual generated payload
(not GeneratedRequest.OriginalPayload or a literal). Ensure any resources
(resp.Body) are closed as currently done and return/log errors as appropriate.
- Around line 16-18: The ApplyInitialTransformation currently appends a
hardcoded "pd_xss" canary; change it to generate a unique canary with
randStringBytesMask() (from analyzers.go), append that canary to the returned
string, and store it on the XSSContextAnalyzer instance (e.g., add a field like
lastCanary or canary) so the Analyze method can retrieve and match it; update
Analyze to read that stored canary when checking for the marker instead of
expecting the static literal.
---
Nitpick comments:
In `@pkg/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 14-58: Add two negative subtests inside
TestXSSContextAnalyzer_Analyze to cover early-return paths: (1)
"no-canary-detection" should start an httptest server that responds without the
canary value, call setupTestOptions(server.URL) and assert
analyzer.Analyze(opts) returns found == false (and context == ""), and (2)
"nil-options-guard" should call analyzer.Analyze(nil) and assert it returns
(false, "", nil) to ensure the nil guard in XSSContextAnalyzer.Analyze is
exercised; use the existing analyzer variable and
require.NoError/require.False/require.Equal assertions similar to the other
subtests.
…ies, and negative tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/fuzz/analyzers/xss_analyzer.go (1)
16-18: Remove tool-specific review history from code comments.The note referencing reviewer feedback is non-functional and will age poorly in production code comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 16 - 18, Remove the non-functional reviewer note from the comment above ApplyInitialTransformation; leave a concise description of purpose only (e.g., "ApplyInitialTransformation appends a marker to track reflection.") and do not include any tool- or reviewer-specific history or names such as "CodeRabbit" or references to past suggestions; ensure remaining comment may still reference project pattern like FuzzGenerated.Value in Analyze if needed for context.pkg/fuzz/analyzers/xss_analyzer_test.go (1)
14-47: Add subtests for attribute-context and unknown-fallback branches.Current tests validate text detection and no-reflection/nil guards, but they don’t cover the
attr:*path or the"reflected:unknown"fallback returned when reflection exists outside recognized contexts.✅ Suggested additions
func TestXSSContextAnalyzer_Analyze(t *testing.T) { analyzer := &XSSContextAnalyzer{} @@ t.Run("nil-options-guard", func(t *testing.T) { found, _, err := analyzer.Analyze(nil) require.NoError(t, err) require.False(t, found) }) + + t.Run("attribute-context-detection", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `<html><body><img src="pd_xss" /></body></html>`) + })) + defer server.Close() + + opts := setupTestOptions(t, server.URL) + found, context, err := analyzer.Analyze(opts) + require.NoError(t, err) + require.True(t, found) + require.Equal(t, "attr:src:img", context) + }) + + t.Run("unknown-context-fallback", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Reflection outside any HTML tag text node + fmt.Fprint(w, "pd_xss") + })) + defer server.Close() + + opts := setupTestOptions(t, server.URL) + found, context, err := analyzer.Analyze(opts) + require.NoError(t, err) + require.True(t, found) + require.Equal(t, "reflected:unknown", context) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fuzz/analyzers/xss_analyzer_test.go` around lines 14 - 47, Add two subtests to TestXSSContextAnalyzer_Analyze that exercise the attribute-context and unknown-fallback branches of XSSContextAnalyzer.Analyze: 1) "attr-context-detection" — spin up an httptest server that returns HTML with the canary reflected inside an attribute (e.g., <div title="pd_xss">), call setupTestOptions(server.URL) and analyzer.Analyze(opts), assert found==true and context contains the attribute-style token (e.g., "attr:title:pd_xss" or the analyzer's attr:<name>:<canary> format). 2) "unknown-fallback-detection" — return HTML where the canary is reflected but not in a recognized text/attr/event context (e.g., inside a comment or script fragment the analyzer treats as unknown), call Analyze and assert found==true and context equals or contains "reflected:unknown". Use the existing analyzer variable and setupTestOptions helper; mirror the style of the existing subtests for server lifecycle and require assertions.
🤖 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/fuzz/analyzers/xss_analyzer.go`:
- Around line 16-21: ApplyInitialTransformation appends a fixed marker
("pd_xss") but the context detection later compares against the entire
options.FuzzGenerated.Value; update the lookup in the XSSContextAnalyzer
analysis path (the code that reads options.FuzzGenerated.Value) to match the
appended marker instead of exact equality—either extract the marker into a
shared constant used by ApplyInitialTransformation and the analyzer, or detect
the marker with strings.HasSuffix/strings.Contains on
options.FuzzGenerated.Value and fall back to previous logic; reference
ApplyInitialTransformation, XSSContextAnalyzer, and options.FuzzGenerated.Value
when making the change.
- Around line 1-92: Remove the trailing space in the package comment near the
ApplyInitialTransformation block (the comment above ApplyInitialTransformation
in XSSContextAnalyzer) and add a final newline at EOF so the file ends with a
newline; run gofmt (or manually remove the extra whitespace after "follow" in
that comment and ensure the file terminates with a newline) to satisfy
formatting for functions/types XSSContextAnalyzer and ApplyInitialTransformation
and the init registration.
---
Nitpick comments:
In `@pkg/fuzz/analyzers/xss_analyzer_test.go`:
- Around line 14-47: Add two subtests to TestXSSContextAnalyzer_Analyze that
exercise the attribute-context and unknown-fallback branches of
XSSContextAnalyzer.Analyze: 1) "attr-context-detection" — spin up an httptest
server that returns HTML with the canary reflected inside an attribute (e.g.,
<div title="pd_xss">), call setupTestOptions(server.URL) and
analyzer.Analyze(opts), assert found==true and context contains the
attribute-style token (e.g., "attr:title:pd_xss" or the analyzer's
attr:<name>:<canary> format). 2) "unknown-fallback-detection" — return HTML
where the canary is reflected but not in a recognized text/attr/event context
(e.g., inside a comment or script fragment the analyzer treats as unknown), call
Analyze and assert found==true and context equals or contains
"reflected:unknown". Use the existing analyzer variable and setupTestOptions
helper; mirror the style of the existing subtests for server lifecycle and
require assertions.
In `@pkg/fuzz/analyzers/xss_analyzer.go`:
- Around line 16-18: Remove the non-functional reviewer note from the comment
above ApplyInitialTransformation; leave a concise description of purpose only
(e.g., "ApplyInitialTransformation appends a marker to track reflection.") and
do not include any tool- or reviewer-specific history or names such as
"CodeRabbit" or references to past suggestions; ensure remaining comment may
still reference project pattern like FuzzGenerated.Value in Analyze if needed
for context.
Comment on lines
1
to
92
| package analyzers | ||
|
|
||
| import ( | ||
| "io" | ||
| "strings" | ||
|
|
||
| "golang.org/x/net/html" | ||
| ) | ||
|
|
||
| type XSSContextAnalyzer struct{} | ||
|
|
||
| func (a *XSSContextAnalyzer) Name() string { | ||
| return "xss-context" | ||
| } | ||
|
|
||
| // ApplyInitialTransformation appends a marker to track reflection. | ||
| // Note: CodeRabbit suggested unique canaries, but for now we follow | ||
| // the existing project pattern using FuzzGenerated.Value in Analyze. | ||
| func (a *XSSContextAnalyzer) ApplyInitialTransformation(data string, params map[string]interface{}) string { | ||
| return data + "pd_xss" | ||
| } | ||
|
|
||
| func (a *XSSContextAnalyzer) Analyze(options *Options) (bool, string, error) { | ||
| if options == nil || options.HttpClient == nil || options.FuzzGenerated.Request == nil { | ||
| return false, "", nil | ||
| } | ||
|
|
||
| resp, err := options.HttpClient.Do(options.FuzzGenerated.Request) | ||
| if err != nil { | ||
| return false, "", err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| // Limit response reading to 4MB to prevent Out Of Memory (OOM) issues | ||
| const maxBodySize = 4 * 1024 * 1024 | ||
| bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxBodySize)) | ||
| if err != nil { | ||
| return false, "", err | ||
| } | ||
|
|
||
| body := string(bodyBytes) | ||
| // Use the actual fuzzed value as the canary instead of a hardcoded string | ||
| canary := options.FuzzGenerated.Value | ||
| if canary == "" { | ||
| canary = "pd_xss" // Fallback if Value is not set | ||
| } | ||
|
|
||
| if !strings.Contains(body, canary) { | ||
| return false, "", nil | ||
| } | ||
|
|
||
| tokenizer := html.NewTokenizer(strings.NewReader(body)) | ||
| tagDepth := 0 | ||
|
|
||
| for { | ||
| tokenType := tokenizer.Next() | ||
| if tokenType == html.ErrorToken { | ||
| break | ||
| } | ||
|
|
||
| token := tokenizer.Token() | ||
| switch tokenType { | ||
| case html.StartTagToken: | ||
| tagDepth++ | ||
| for _, attr := range token.Attr { | ||
| if strings.Contains(attr.Val, canary) { | ||
| return true, "attr:" + attr.Key + ":" + token.Data, nil | ||
| } | ||
| } | ||
| case html.SelfClosingTagToken: | ||
| for _, attr := range token.Attr { | ||
| if strings.Contains(attr.Val, canary) { | ||
| return true, "attr:" + attr.Key + ":" + token.Data, nil | ||
| } | ||
| } | ||
| case html.EndTagToken: | ||
| if tagDepth > 0 { | ||
| tagDepth-- | ||
| } | ||
| case html.TextToken: | ||
| if tagDepth > 0 && strings.Contains(token.Data, canary) { | ||
| return true, "text:" + token.Data, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true, "reflected:unknown", nil | ||
| } | ||
|
|
||
| func init() { | ||
| RegisterAnalyzer("xss-context", &XSSContextAnalyzer{}) | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1/2] gofmt check (should print nothing if properly formatted)"
gofmt -l pkg/fuzz/analyzers/xss_analyzer.go 2>&1 || true
echo ""
echo "[2/2] go vet check"
go vet ./pkg/fuzz/analyzers/... 2>&1 || trueRepository: projectdiscovery/nuclei
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1/1] gofmt check on specific file"
if gofmt -l pkg/fuzz/analyzers/xss_analyzer.go 2>&1 | grep -q .; then
echo "File needs formatting"
gofmt -d pkg/fuzz/analyzers/xss_analyzer.go
else
echo "File is properly formatted"
fiRepository: projectdiscovery/nuclei
Length of output: 851
Fix Go formatting violations in this file.
The code does not meet gofmt standards. Two issues found:
- Line 17: trailing whitespace after "follow" in the comment
- Line 92: missing newline at end of file
Run gofmt -w pkg/fuzz/analyzers/xss_analyzer.go to fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fuzz/analyzers/xss_analyzer.go` around lines 1 - 92, Remove the trailing
space in the package comment near the ApplyInitialTransformation block (the
comment above ApplyInitialTransformation in XSSContextAnalyzer) and add a final
newline at EOF so the file ends with a newline; run gofmt (or manually remove
the extra whitespace after "follow" in that comment and ensure the file
terminates with a newline) to satisfy formatting for functions/types
XSSContextAnalyzer and ApplyInitialTransformation and the init registration.
Author
|
@Mzack9999 waiting your response |
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.
The XSS Context Analyzer identifies the exact HTML environment where a fuzzed payload is reflected. This allows the fuzzer to adapt its strategy based on whether the injection lands inside an attribute, a text node, or an unknown area.
How it works:
Memory Safety: The analyzer implements protection using io.LimitReader, capping the response body processing at 4MB to prevent Out-Of-Memory (OOM) errors on large targets.
State-Aware Tokenization: Utilizing the golang.org/x/net/html tokenizer, the analyzer tracks nesting levels with a tagDepth counter. This ensures reflections are only classified as "text" if they reside within a valid HTML element.
Context Mapping:
Attribute Context (attr:name:tag): Detected when the canary is found within an attribute value (e.g., returns attr:value:input).
Text Context (text:content): Detected when the canary is reflected as a text node inside an element (e.g.,
Fallback Context (reflected:unknown): Triggered if the canary is present but not mapped to a specific HTML structure.
Dynamic Matching: It uses a shared xssCanaryMarker and checks the FuzzGenerated.Value to ensure the detection matches the actual string injected during the fuzzing process.
Usage Tutorial
To utilize this analyzer, reference it in the analyzers section of your Nuclei fuzzer template.
YAML
id: xss-context-fuzzing
info:
name: XSS Context-Aware Fuzzing
author: Eliene-byte
severity: info
http:
method: GET
path:
fuzzing:
type: replace
mode: multiple
keys:
fuzz:
analyzers:
matchers:
type: word
part: xss-context
words:
Bash
nuclei -t xss-context-fuzzing.yaml -u http://example.com/search
3. Interpreting Results:
The analyzer provides structured output strings that help guide payload selection:
If output is attr:value:input: The payload is inside an attribute. Recommendation: Use quotes (") to break the context.
If output is text:pd_xss: The payload is in plain text. Recommendation: Use HTML tags like![]()
or <script>.
If output is reflected:unknown: The reflection is in an unmapped area (e.g., a comment or script block).