JS: Generate flow summaries from summaryModels; only generate steps as a fallback #19445
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances JavaScript data-flow analysis by using summaryModel rows for flows and falling back to step generation only when the summary library does not support a given input/output pair.
- Introduces predicates in
FlowSummaryImpl.qllto detect supported input/output stacks and flag unsupported callables. - Removes old summary-step logic from
ApiGraphModelsSpecific.qlland centralizes fallback logic inModelsAsData.qll. - Updates test expectations to reflect new fallback steps and library-only flows.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll | Added isSupportedInputStack, isSupportedOutputStack, and unsupportedCallable predicates |
| javascript/ql/lib/semmle/javascript/frameworks/data/internal/ApiGraphModelsSpecific.qll | Removed old summary-step and input/output path predicates; delegating to new fallback mechanism |
| javascript/ql/lib/semmle/javascript/frameworks/data/ModelsAsData.qll | Imported FlowSummaryPrivate, defined SummarizedCallableFromModel, and integrated fallback step logic |
| javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll | Exposed unsupportedCallable predicates for use in dataflow |
| javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected | Updated expected edges/nodes to include fallback-generated steps |
| javascript/ql/test/library-tests/frameworks/data/test.expected | Updated expected flows to reflect library-only and fallback behavior |
| javascript/ql/test/library-tests/TripleDot/underscore.string.js | Adjusted expected taint-flow annotation to match new test behavior |
Comments suppressed due to low confidence (2)
javascript/ql/lib/semmle/javascript/dataflow/internal/FlowSummaryPrivate.qll:268
- [nitpick] The predicate 'unsupportedCallable' is overloaded with arity 1 and 4, which may be confusing. Consider renaming one overload or adding documentation to clarify their distinct purposes.
predicate unsupportedCallable = Private::unsupportedCallable/1;
shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll:564
- The recursive case in
isSupportedInputStackhandles stacks beyond length 3; consider adding unit tests for deeper input stacks to ensure correct behavior.
isSupportedInputStack(s.tail()) and
| s.head() instanceof TReturnSummaryComponent and | ||
| s.tail().head() instanceof TArgumentSummaryComponent | ||
| or | ||
| // Argument[n].Parameter[n].Content.* |
There was a problem hiding this comment.
I hadn't considered the fact that we also support this case, a lambda passed in which has a side-effect on one of it's arguments.
There was a problem hiding this comment.
It's a good thing we do. We rely on this to model the Promise constructor in JavaScript.
| } | ||
|
|
||
| /** | ||
| * Holds if `s` is a valid input stack, in the sense that we generate data flow graph |
| // Argument[n].Content.* | ||
| s.length() = 2 and | ||
| s.head() instanceof TContentSummaryComponent and | ||
| s.tail().head() instanceof TArgumentSummaryComponent |
There was a problem hiding this comment.
We actually support just Argument[n] as well, for example one might model a string builder append method as input = Argument[0] and output = Argument[self] and preservesValue = false`.
There was a problem hiding this comment.
Good call. I pushed a commit that recognises this, please take a look.
Makes the JavaScript analysis generate flow summaries from
summaryModelrows, and only generate steps if the flow summary library does not support the input/output pair.