-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.js
covering object arguments and{unsafe: true}
- Update two expected files to include alerts for unsafe serialization of object-based inputs
- Implement a new
SerializeJavascriptFlowStep
inXss.qll
and 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
SerializeJavascriptFlowStep
class 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)) |
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) |
Copilot uses AI. Check for mistakes.
exists(DataFlow::CallNode call, DataFlow::PropWrite propWrite | | ||
call = DataFlow::moduleImport("serialize-javascript").getACall() and | ||
succ = call and | ||
propWrite.getRhs() = pred and |
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.
Copilot uses AI. Check for mistakes.
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-javascript
calls when tainted data is passed via object properties.