-
Notifications
You must be signed in to change notification settings - Fork 1.9k
JS: Improve XSS detection for serialize-javascript with tainted objects
#19771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c0fb7b6 to
71a5a6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Enhance XSS analysis to track taint through serialize-javascript calls when tainted object properties are passed, and update tests and expected outputs to flag unsafe serializations.
- Add new test cases in
tst2.jscovering object arguments and{unsafe: true} - Update two expected files to include alerts for unsafe serialization of object-based inputs
- Implement a new
SerializeJavascriptFlowStepinXss.qlland document the change
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js | Added routes testing serializeJavaScript with object props and {unsafe: true} |
| javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected | Expanded expected alerts to include taint through object serialization with unsafe flag |
| javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected | Expanded expected alerts and dataflow edges/nodes for unsafe object serialization |
| javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll | Introduced SerializeJavascriptFlowStep to propagate taint through object arguments |
| javascript/ql/lib/change-notes/2025-06-16-serialize-js.md | Documented the improved XSS detection for object properties in change notes |
Comments suppressed due to low confidence (1)
javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll:64
- [nitpick] Indent the
SerializeJavascriptFlowStepclass definition to align with the surrounding code (4 spaces) for consistent formatting.
private class SerializeJavascriptFlowStep extends DataFlow::AdditionalFlowStep {
| call = DataFlow::moduleImport("serialize-javascript").getACall() and | ||
| succ = call and | ||
| propWrite.getRhs() = pred and | ||
| propWrite.getBase().getALocalSource().flowsTo(call.getArgument(0)) |
Copilot
AI
Jun 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flow step currently propagates taint through object arguments regardless of the unsafe option. Restrict this step to only when the unsafe flag is set to true to avoid false positives on safe serializations.
| propWrite.getBase().getALocalSource().flowsTo(call.getArgument(0)) | |
| propWrite.getBase().getALocalSource().flowsTo(call.getArgument(0)) and | |
| call.getOptionArgument(1, "unsafe").mayHaveBooleanValue(true) |
| exists(DataFlow::CallNode call, DataFlow::PropWrite propWrite | | ||
| call = DataFlow::moduleImport("serialize-javascript").getACall() and | ||
| succ = call and | ||
| propWrite.getRhs() = pred and |
Copilot
AI
Jun 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flow step only handles PropWrite assignments but does not cover object literal initializers (e.g., {someProperty: p}). Add handling for object literal property initializers so inline object literals also propagate taint.
71a5a6d to
fffbc0c
Compare
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Improved XSS detection for `serialize-javascript` calls with tainted object properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your changes I don't feel this is entirely accurate.
Can you change to a more general statement that we track data through calls to serialize-javascript?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in fc0c8a8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume DCA evaluations looked good.
Enhances the XSS analysis to track taint through
serialize-javascriptcalls when tainted data is passed via object properties.