fix: improve XSS context analyzer for edge cases#7089
Open
MekonMAC wants to merge 2 commits intoprojectdiscovery:devfrom
Open
fix: improve XSS context analyzer for edge cases#7089MekonMAC wants to merge 2 commits intoprojectdiscovery:devfrom
MekonMAC wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
Implements an XSS context analyzer that integrates with nuclei's fuzzing pipeline. The analyzer: - Injects a canary string with XSS-critical characters (<>"'/) to detect reflection and character survival in HTTP responses - Uses golang.org/x/net/html tokenizer to classify reflection context into 8 types: HTMLText, Attribute, AttributeUnquoted, Script, ScriptString, Style, HTMLComment, and None - Selects context-appropriate XSS payloads filtered by which special characters survive server-side encoding - Replays payloads and verifies unencoded reflection to confirm XSS - Reports CSP header presence as a note on exploitability New files: pkg/fuzz/analyzers/xss/types.go - context types and constants pkg/fuzz/analyzers/xss/context.go - HTML tokenizer context detection pkg/fuzz/analyzers/xss/analyzer.go - main analyzer with replay logic pkg/fuzz/analyzers/xss/context_test.go - comprehensive tests Modified files: pkg/fuzz/analyzers/analyzers.go - add response fields to Options pkg/protocols/http/http.go - register xss analyzer via import pkg/protocols/http/request.go - pass response data to analyzer pkg/protocols/http/request_fuzz.go - init nil Parameters map Closes projectdiscovery#5838
This commit addresses four context-classification issues: 1. javascript: URIs in href/src/action attributes are now correctly classified as ContextScript instead of ContextAttribute 2. <script type="application/json"> and other non-executable script types (ld+json, templates, importmap) are now classified as ContextNone instead of executable script context 3. Marker reflection detection is now case-insensitive, catching transformed reflections where server may uppercase/lowercase 4. srcdoc attributes are now treated as HTML injection context (ContextHTMLText) since they allow full HTML content Includes comprehensive test coverage for all new behaviors. Fixes projectdiscovery#7086
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
Contributor
WalkthroughThis pull request introduces a complete XSS context analyzer for the fuzzing engine, including thread-safe random utilities, HTTP response tracking in Options, HTML reflection detection, context classification, and payload selection logic. The analyzer registers under "xss_context" and performs context-aware XSS verification. Changes
Sequence DiagramsequenceDiagram
participant Fuzzer as Fuzzer Engine
participant XSSAnalyzer as XSS Analyzer
participant HTMLParser as HTML Parser
participant PayloadSelector as Payload Selector
participant Replayer as Request Replayer
participant Server as Target Server
Fuzzer->>XSSAnalyzer: ApplyInitialTransformation([XSS_CANARY])
XSSAnalyzer->>XSSAnalyzer: Generate unique canary + contextual chars
XSSAnalyzer-->>Fuzzer: Return transformed payload
Fuzzer->>Fuzzer: Send fuzzed request with canary
Fuzzer->>Server: HTTP Request
Server-->>Fuzzer: Response (body + headers + status)
Fuzzer->>XSSAnalyzer: Analyze(options with response)
XSSAnalyzer->>HTMLParser: DetectReflections(body, canary)
HTMLParser->>HTMLParser: Tokenize HTML, track tag stack
HTMLParser->>HTMLParser: Classify reflection contexts
HTMLParser-->>XSSAnalyzer: []ReflectionInfo
XSSAnalyzer->>XSSAnalyzer: BestReflection(reflections)
XSSAnalyzer->>PayloadSelector: selectPayloads(context, characterSet)
PayloadSelector-->>XSSAnalyzer: Payload candidates
loop For each payload
XSSAnalyzer->>Replayer: replayAndVerify(payload)
Replayer->>Server: Inject payload, send request
Server-->>Replayer: Response
Replayer->>Replayer: Check payload reflection
Replayer-->>XSSAnalyzer: (bool, result string, error)
end
XSSAnalyzer-->>Fuzzer: (vulnerable, details, error)
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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🧹 Nitpick comments (1)
pkg/fuzz/analyzers/xss/analyzer.go (1)
218-227: Character survival detection depends on strict character ordering.The detection assumes characters appear in the exact order
canary + "<>"'/"fromcanaryChars. For example,GreaterThancheckscanary+"<>"which requires both<and>to survive in that exact sequence. If a server filters only<, the>won't be detected independently.This is acceptable given the canary injection includes all characters together, but the logic could be clearer:
♻️ Consider explicit character checks (optional)
func detectCharacterSurvival(body string, canary string) CharacterSet { + // Characters are appended as: canary + `<>"'/` + // We check for progressive survival since they're injected together return CharacterSet{ LessThan: strings.Contains(body, canary+"<"), - GreaterThan: strings.Contains(body, canary+"<>") || strings.Contains(body, canary+">"), - DoubleQuote: strings.Contains(body, canary+`<>"`), - SingleQuote: strings.Contains(body, canary+`<>"'`), - ForwardSlash: strings.Contains(body, canary+canaryChars), // full canary+chars survived + GreaterThan: strings.Contains(body, canary+"<>"), + DoubleQuote: strings.Contains(body, canary+`<>"`), + SingleQuote: strings.Contains(body, canary+`<>"'`), + ForwardSlash: strings.Contains(body, canary+canaryChars), } }🤖 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 218 - 227, detectCharacterSurvival currently infers survival by looking for specific multi-character sequences (e.g., GreaterThan uses canary+"<>"), which misses independently-surviving characters; change the checks in detectCharacterSurvival to test each XSS-critical character separately (e.g., check body for canary+"<", canary+">", canary+`"`, canary+"'", and canary+"/") so each CharacterSet field (LessThan, GreaterThan, DoubleQuote, SingleQuote, ForwardSlash) is set by an independent strings.Contains call rather than relying on combined-order patterns.
🤖 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 79-82: The early canary presence check uses a case-sensitive
strings.Contains(body, canary) which mismatches the case-insensitive logic in
DetectReflections; update the check in the function (same block that currently
uses strings.Contains) to perform a case-insensitive containment test (e.g.,
compare normalized lowercase versions of body and canary or otherwise use a
case-insensitive contains) so the initial guard and DetectReflections use the
same matching behavior.
---
Nitpick comments:
In `@pkg/fuzz/analyzers/xss/analyzer.go`:
- Around line 218-227: detectCharacterSurvival currently infers survival by
looking for specific multi-character sequences (e.g., GreaterThan uses
canary+"<>"), which misses independently-surviving characters; change the checks
in detectCharacterSurvival to test each XSS-critical character separately (e.g.,
check body for canary+"<", canary+">", canary+`"`, canary+"'", and canary+"/")
so each CharacterSet field (LessThan, GreaterThan, DoubleQuote, SingleQuote,
ForwardSlash) is set by an independent strings.Contains call rather than relying
on combined-order patterns.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/fuzz/analyzers/analyzers.gopkg/fuzz/analyzers/xss/analyzer.gopkg/fuzz/analyzers/xss/context.gopkg/fuzz/analyzers/xss/context_test.gopkg/fuzz/analyzers/xss/types.gopkg/protocols/http/http.gopkg/protocols/http/request.gopkg/protocols/http/request_fuzz.go
Comment on lines
+79
to
+82
| // Check if canary is reflected at all | ||
| if !strings.Contains(body, canary) { | ||
| return false, "", nil | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistency between case-sensitive canary check here and case-insensitive in DetectReflections.
Line 80 uses strings.Contains(body, canary) (case-sensitive), but DetectReflections at Line 88 uses case-insensitive matching. If the server transforms the canary to uppercase, this check fails early and returns false, even though DetectReflections would have found the reflection.
Consider aligning the case sensitivity:
🔧 Proposed fix
// Check if canary is reflected at all
- if !strings.Contains(body, canary) {
+ if !strings.Contains(strings.ToLower(body), strings.ToLower(canary)) {
return false, "", 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
| // Check if canary is reflected at all | |
| if !strings.Contains(body, canary) { | |
| return false, "", nil | |
| } | |
| // Check if canary is reflected at all | |
| if !strings.Contains(strings.ToLower(body), strings.ToLower(canary)) { | |
| return false, "", 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 79 - 82, The early canary
presence check uses a case-sensitive strings.Contains(body, canary) which
mismatches the case-insensitive logic in DetectReflections; update the check in
the function (same block that currently uses strings.Contains) to perform a
case-insensitive containment test (e.g., compare normalized lowercase versions
of body and canary or otherwise use a case-insensitive contains) so the initial
guard and DetectReflections use the same matching behavior.
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.
Summary
This PR addresses the four context-classification edge cases reported in #7086, improving the accuracy of the XSS context analyzer introduced in #7076.
Changes
javascript: URIs — Attributes like
href="javascript:..."are now correctly classified asContextScriptinstead ofContextAttribute. Applies tohref,src,action,formaction,xlink:href,data, andposterattributes.Non-executable script types —
<script type="application/json">,application/ld+json,text/template,importmap, and similar non-executable script types are now classified asContextNoneinstead of executable script context.Case-insensitive marker detection — The initial marker presence check is now case-insensitive, catching transformed reflections where the server may uppercase or lowercase the input.
srcdoc attributes — The
srcdocattribute is now treated asContextHTMLText(HTML injection context) since it allows full HTML content in iframes.Test Plan
Before/After
<a href="javascript:alert(canary)"><script type="application/json">canary</script>CANARY, marker:canary<iframe srcdoc="<script>canary">Fixes #7086
🤖 Generated with OpenClaw
Summary by CodeRabbit
Release Notes
New Features
Improvements