JS: Improve support for NodeJS `pipe` callbacks by MathiasVP · Pull Request #22765 · github/codeql

MathiasVP · 2026-10-06T19:13:34Z

Somewhat vibe-coded, but I think it looks reasonable (after lots of cleanup).

Copilot

Copilot review overview

🟡 Changes recommended

Standard writable hooks and several supported stream inputs or filesystem variants are not modeled correctly.

Review effort: Balanced
Findings: 5

Open (5)

What changed in this PR

Adds Node.js stream data-flow modeling for Readable.from() and piped destinations.

Changes:

Models readable stream factories, fluent methods, and implicit pipe writes.
Adds stream pipe flow tests.
Documents the analysis improvement.

File	Description
`NodeJSLib.qll`	Adds stream and pipe flow modeling.
`StreamPipeDataFlow.ql`	Defines inline flow assertions.
`StreamPipeDataFlow.expected`	Stores expected test results.
`stream-pipe.js`	Provides stream flow test cases.
`2026-10-07-nodejs-stream-pipe.md`	Adds the change note.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Constructor-option callbacks remain unsupported, and some previously recognized file-stream reads can lose their data node.

2 open findings

5 resolved since last review

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Chained pipes are recognized but do not propagate transformed chunks to downstream destinations.

2 open findings

2 resolved since last review

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Readable.from(Map) entries currently lose key and value flow before reaching pipe callbacks.

1 open finding

Model Map entries emitted by Readable.from · New

2 resolved since last review

🧠 Review effort: Balanced

Copilot

🔵 Needs a closer look

Override detection can suppress valid stream hooks for imported, conditional, or later-assigned callbacks.

0 open findings

1 resolved since last review

Model Map entries emitted by Readable.from

Previously missed (1)

In code that hasn't changed since last review

Conditional or late overrides incorrectly suppress native write hooks

javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll:910

This existential assignment check suppresses the native implementation hook even when the assignment is conditional, unreachable, or occurs after the pipe call. In those executions the inherited write still invokes _write/_transform, so the new model loses real flow. Only suppress the hook for an override known to be active at this call; otherwise model both targets conservatively.

This issue also appears on line 940 of the same file.

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Stream recognition misses PassThrough flows and incorrectly treats every pipe destination as readable.

3 open findings

🧠 Review effort: Balanced

+  private predicate streamConstructor(EarlyStageNode node) {
+    exists(EarlyStageNode base |
+      base = getAStreamModuleNode() and
+      memberRead(base, ["Readable", "Duplex", "Transform"], node)


+---
+category: minorAnalysis
+---
+* Improved Node.js modeling of `Readable.from(...)` and related methods.


Copilot

🟡 Changes recommended

Definite instance method overrides currently produce impossible flows through bypassed stream hooks.

4 open findings

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

MathiasVP added the JS label Oct 6, 2026

MathiasVP force-pushed the js-better-nodejs-callback-support branch from 28aa544 to 620f2a8 Compare October 7, 2026 10:25

MathiasVP marked this pull request as ready for review October 7, 2026 10:30

MathiasVP requested a review from a team as a code owner October 7, 2026 10:30

Copilot AI balanced review requested due to automatic review settings October 7, 2026 10:30

Copilot started reviewing on behalf of MathiasVP October 7, 2026 10:31 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

MathiasVP marked this pull request as draft October 7, 2026 10:42

github-actions Bot added the documentation label Oct 7, 2026

MathiasVP force-pushed the js-better-nodejs-callback-support branch from 620f2a8 to 18c8abc Compare October 7, 2026 15:20

MathiasVP requested a balanced review from Copilot October 7, 2026 15:20

Copilot started reviewing on behalf of MathiasVP October 7, 2026 15:21 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll Outdated

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll Outdated

github-advanced-security Bot found potential problems Oct 7, 2026

View reviewed changes

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll Fixed

MathiasVP force-pushed the js-better-nodejs-callback-support branch from 18c8abc to 7e88967 Compare October 7, 2026 17:43

MathiasVP requested a balanced review from Copilot October 7, 2026 17:43

Copilot started reviewing on behalf of MathiasVP October 7, 2026 17:44 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll

MathiasVP force-pushed the js-better-nodejs-callback-support branch from 7e88967 to 4b0d118 Compare October 7, 2026 18:25

MathiasVP requested a balanced review from Copilot October 7, 2026 18:25

Copilot started reviewing on behalf of MathiasVP October 7, 2026 18:26 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll

MathiasVP requested a balanced review from Copilot October 7, 2026 19:16

Copilot started reviewing on behalf of MathiasVP October 7, 2026 19:17 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

MathiasVP force-pushed the js-better-nodejs-callback-support branch from fd1da79 to 9c48dd2 Compare October 7, 2026 19:25

MathiasVP requested a balanced review from Copilot October 7, 2026 19:25

Copilot started reviewing on behalf of MathiasVP October 7, 2026 19:26 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

MathiasVP mentioned this pull request Oct 8, 2026

JS: Fix Map content flow #22781

Open

MathiasVP added 20 commits October 8, 2026 15:11

JS: Add a failing test.

1ad9bfc

JS: Fix map flow.

371a812

JS: Accept test changes.

8cc93eb

JS: Add tests.

40e4a65

JS: Support NodeJS pipe callbacks and related APIs.

d2b8011

JS: Accept test changes.

a7c5b20

JS: Add change note.

67a9f41

JS: Add tests with missing flow.

ee8c5b0

JS: Add missing flow steps and accept test changes.

cf49e42

JS: Add tests with missing flow.

1f7898a

JS: model more implicit Node.js stream writes.

6942252

JS: Accept test changes.

16ecdfb

JS: Add missing flow steps.

2b32811

JS: Accept test changes.

06b0511

JS: Add tests with missing flow.

ee38449

JS: Add missing flow steps.

b659d5a

JS: Accept test changes.

4dcdadb

JS: Add tests with missing flow.

77c3953

JS: Add missing content flow.

2eaf2fc

JS: Accept test changes.

90dc38c

MathiasVP force-pushed the js-better-nodejs-callback-support branch from 9c48dd2 to 90dc38c Compare October 8, 2026 14:12

MathiasVP requested a balanced review from Copilot October 8, 2026 14:13

Copilot started reviewing on behalf of MathiasVP October 8, 2026 14:14 View session

Copilot AI reviewed Oct 8, 2026

View reviewed changes

Comment thread javascript/ql/lib/semmle/javascript/frameworks/NodeJSLib.qll

Conversation

MathiasVP commented Oct 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants