From c27157f02162a94b5b926560bb23f552d079454f Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 20 May 2025 12:45:11 +0200 Subject: [PATCH 01/37] Add `UnhandledStreamPipee` Quality query and tests to detect missing error handlers in `Node.js` streams --- .../ql/src/Quality/UnhandledStreamPipe.ql | 97 +++++++++++++ .../Quality/UnhandledStreamPipe/test.expected | 10 ++ .../Quality/UnhandledStreamPipe/test.js | 137 ++++++++++++++++++ .../Quality/UnhandledStreamPipe/test.qlref | 2 + 4 files changed, 246 insertions(+) create mode 100644 javascript/ql/src/Quality/UnhandledStreamPipe.ql create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql new file mode 100644 index 000000000000..bca1044ad1e6 --- /dev/null +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -0,0 +1,97 @@ +/** + * @id js/nodejs-stream-pipe-without-error-handling + * @name Node.js stream pipe without error handling + * @description Calling `pipe()` on a stream without error handling may silently drop errors and prevent proper propagation. + * @kind problem + * @problem.severity warning + * @precision high + * @tags quality + * frameworks/nodejs + */ + +import javascript + +/** + * A call to the `pipe` method on a Node.js stream. + */ +class PipeCall extends DataFlow::MethodCallNode { + PipeCall() { this.getMethodName() = "pipe" } + + /** Gets the source stream (receiver of the pipe call). */ + DataFlow::Node getSourceStream() { result = this.getReceiver() } + + /** Gets the destination stream (argument of the pipe call). */ + DataFlow::Node getDestinationStream() { result = this.getArgument(0) } +} + +/** + * Gets the method names used to register event handlers on Node.js streams. + * These methods are used to attach handlers for events like `error`. + */ +string getEventHandlerMethodName() { result = ["on", "once", "addListener"] } + +/** + * A call to register an event handler on a Node.js stream. + * This includes methods like `on`, `once`, and `addListener`. + */ +class StreamEventRegistration extends DataFlow::MethodCallNode { + StreamEventRegistration() { this.getMethodName() = getEventHandlerMethodName() } + + /** Gets the stream (receiver of the event handler). */ + DataFlow::Node getStream() { result = this.getReceiver() } +} + +/** + * Models flow relationships between streams and related operations. + * Connects destination streams to their corresponding pipe call nodes. + * Connects streams to their event handler registration nodes. + */ +predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) { + exists(PipeCall pipe | + streamNode = pipe.getDestinationStream() and + relatedNode = pipe + ) + or + exists(StreamEventRegistration handler | + streamNode = handler.getStream() and + relatedNode = handler + ) +} + +/** + * Gets a reference to a stream that may be the source of the given pipe call. + * Uses type back-tracking to trace stream references in the data flow. + */ +private DataFlow::SourceNode streamRef(DataFlow::TypeBackTracker t, PipeCall pipeCall) { + t.start() and + result = pipeCall.getSourceStream().getALocalSource() + or + exists(DataFlow::SourceNode prev | + prev = streamRef(t.continue(), pipeCall) and + streamFlowStep(result.getALocalUse(), prev) + ) + or + exists(DataFlow::TypeBackTracker t2 | result = streamRef(t2, pipeCall).backtrack(t2, t)) +} + +/** + * Gets a reference to a stream that may be the source of the given pipe call. + */ +private DataFlow::SourceNode streamRef(PipeCall pipeCall) { + result = streamRef(DataFlow::TypeBackTracker::end(), pipeCall) +} + +/** + * Holds if the source stream of the given pipe call has an `error` handler registered. + */ +predicate hasErrorHandlerRegistered(PipeCall pipeCall) { + exists(StreamEventRegistration handler | + handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) and + handler.getArgument(0).getStringValue() = "error" + ) +} + +from PipeCall pipeCall +where not hasErrorHandlerRegistered(pipeCall) +select pipeCall, + "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped." diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected new file mode 100644 index 000000000000..9f9866477032 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -0,0 +1,10 @@ +| test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:60:5:60:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:66:5:66:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:79:5:79:25 | s2.pipe ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:94:5:94:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:109:26:109:37 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js new file mode 100644 index 000000000000..6d7b05adb0f1 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -0,0 +1,137 @@ +function test() { + { + const stream = getStream(); + stream.pipe(destination); // $Alert + } + { + const stream = getStream(); + stream.pipe(destination); + stream.on('error', handleError); + } + { + const stream = getStream(); + stream.on('error', handleError); + stream.pipe(destination); + } + { + const stream = getStream(); + const s2 = stream; + s2.pipe(dest); // $Alert + } + { + const stream = getStream(); + stream.on('error', handleError); + const s2 = stream; + s2.pipe(dest); + } + { + const stream = getStream(); + const s2 = stream; + s2.on('error', handleError); + s2.pipe(dest); + } + { + const s = getStream().on('error', handler); + const d = getDest(); + s.pipe(d); + } + { + getStream().on('error', handler).pipe(dest); + } + { + const stream = getStream(); + stream.on('error', handleError); + const stream2 = stream.pipe(destination); + stream2.pipe(destination2); // $Alert + } + { + const stream = getStream(); + stream.on('error', handleError); + const destination = getDest(); + destination.on('error', handleError); + const stream2 = stream.pipe(destination); + const s3 = stream2; + s = s3.pipe(destination2); + } + { + const stream = getStream(); + stream.on('error', handleError); + const stream2 = stream.pipe(destination); + stream2.pipe(destination2); // $Alert + } + { // Error handler on destination instead of source + const stream = getStream(); + const dest = getDest(); + dest.on('error', handler); + stream.pipe(dest); // $Alert + } + { // Multiple aliases, error handler on one + const stream = getStream(); + const alias1 = stream; + const alias2 = alias1; + alias2.on('error', handleError); + alias1.pipe(dest); + } + { // Multiple pipes, handler after first pipe + const stream = getStream(); + const s2 = stream.pipe(destination1); + stream.on('error', handleError); + s2.pipe(destination2); // $Alert + } + { // Handler registered via .once + const stream = getStream(); + stream.once('error', handleError); + stream.pipe(dest); + } + { // Handler registered with arrow function + const stream = getStream(); + stream.on('error', (err) => handleError(err)); + stream.pipe(dest); + } + { // Handler registered for unrelated event + const stream = getStream(); + stream.on('close', handleClose); + stream.pipe(dest); // $Alert + } + { // Error handler registered after pipe, but before error + const stream = getStream(); + stream.pipe(dest); + setTimeout(() => stream.on('error', handleError), 8000); // $MISSING:Alert + } + { // Pipe in a function, error handler outside + const stream = getStream(); + function doPipe(s) { s.pipe(dest); } + stream.on('error', handleError); + doPipe(stream); + } + { // Pipe in a function, error handler not set + const stream = getStream(); + function doPipe(s) { s.pipe(dest); } // $Alert + doPipe(stream); + } + { // Dynamic event assignment + const stream = getStream(); + const event = 'error'; + stream.on(event, handleError); + stream.pipe(dest); // $SPURIOUS:Alert + } + { // Handler assigned via variable property + const stream = getStream(); + const handler = handleError; + stream.on('error', handler); + stream.pipe(dest); + } + { // Pipe with no intermediate variable, no error handler + getStream().pipe(dest); // $Alert + } + { // Handler set via .addListener synonym + const stream = getStream(); + stream.addListener('error', handleError); + stream.pipe(dest); + } + { // Handler set via .once after .pipe + const stream = getStream(); + stream.pipe(dest); + stream.once('error', handleError); + } +} diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref new file mode 100644 index 000000000000..23e2f65bab79 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref @@ -0,0 +1,2 @@ +query: Quality/UnhandledStreamPipe.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql From f39bf62fc6a32c9718a222451851dd0e40ce5d79 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 20 May 2025 13:04:55 +0200 Subject: [PATCH 02/37] test: Add edge cases for stream pipe error handling Add tests for chained stream methods and non-stream pipe objects --- .../Quality/UnhandledStreamPipe/test.expected | 7 ++++ .../Quality/UnhandledStreamPipe/test.js | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 9f9866477032..41e272f2e2d1 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -8,3 +8,10 @@ | test.js:109:26:109:37 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:139:5:139:87 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:147:5:147:28 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:151:20:151:43 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:157:47:157:74 | someVar ... ething) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:163:5:163:20 | notStream.pipe() | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:167:5:167:36 | notStre ... , arg3) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index 6d7b05adb0f1..e13608dbe2f5 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -134,4 +134,36 @@ function test() { stream.pipe(dest); stream.once('error', handleError); } + { // Long chained pipe with error handler + const stream = getStream(); + stream.pause().on('error', handleError).setEncoding('utf8').resume().pipe(writable); // $SPURIOUS:Alert + } + { // Long chained pipe without error handler + const stream = getStream(); + stream.pause().setEncoding('utf8').resume().pipe(writable); // $Alert + } + { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) + const notStream = getNotAStream(); + notStream.pipe(writable).subscribe(); // $SPURIOUS:Alert + } + { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) + const notStream = getNotAStream(); + const result = notStream.pipe(writable); // $SPURIOUS:Alert + const dealWithResult = (result) => { result.subscribe(); }; + dealWithResult(result); + } + { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) + const notStream = getNotAStream(); + const pipeIt = (someVariable) => { return someVariable.pipe(something); }; // $SPURIOUS:Alert + let x = pipeIt(notStream); + x.subscribe(); + } + { // Calling custom pipe method with no arguments + const notStream = getNotAStream(); + notStream.pipe(); // $SPURIOUS:Alert + } + { // Calling custom pipe method with more then 2 arguments + const notStream = getNotAStream(); + notStream.pipe(arg1, arg2, arg3); // $SPURIOUS:Alert + } } From ef1bde554a1e21b964b5f46b408fcfcaa3384176 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 20 May 2025 13:13:05 +0200 Subject: [PATCH 03/37] Fixed issue where streams would not be tracked via chainable methods --- .../ql/src/Quality/UnhandledStreamPipe.ql | 23 +++++++++++++------ .../Quality/UnhandledStreamPipe/test.expected | 1 - .../Quality/UnhandledStreamPipe/test.js | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index bca1044ad1e6..4f16d100021f 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -30,21 +30,29 @@ class PipeCall extends DataFlow::MethodCallNode { */ string getEventHandlerMethodName() { result = ["on", "once", "addListener"] } +/** + * Gets the method names that are chainable on Node.js streams. + */ +string getChainableStreamMethodName() { + result = + [ + "setEncoding", "pause", "resume", "unpipe", "destroy", "cork", "uncork", "setDefaultEncoding", + "off", "removeListener", getEventHandlerMethodName() + ] +} + /** * A call to register an event handler on a Node.js stream. * This includes methods like `on`, `once`, and `addListener`. */ class StreamEventRegistration extends DataFlow::MethodCallNode { StreamEventRegistration() { this.getMethodName() = getEventHandlerMethodName() } - - /** Gets the stream (receiver of the event handler). */ - DataFlow::Node getStream() { result = this.getReceiver() } } /** * Models flow relationships between streams and related operations. * Connects destination streams to their corresponding pipe call nodes. - * Connects streams to their event handler registration nodes. + * Connects streams to their chainable methods. */ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) { exists(PipeCall pipe | @@ -52,9 +60,10 @@ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) relatedNode = pipe ) or - exists(StreamEventRegistration handler | - streamNode = handler.getStream() and - relatedNode = handler + exists(DataFlow::MethodCallNode chainable | + chainable.getMethodName() = getChainableStreamMethodName() and + streamNode = chainable.getReceiver() and + relatedNode = chainable ) } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 41e272f2e2d1..776c1c07def8 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -8,7 +8,6 @@ | test.js:109:26:109:37 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:139:5:139:87 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:147:5:147:28 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:151:20:151:43 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index e13608dbe2f5..61bee5078a0c 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -136,7 +136,7 @@ function test() { } { // Long chained pipe with error handler const stream = getStream(); - stream.pause().on('error', handleError).setEncoding('utf8').resume().pipe(writable); // $SPURIOUS:Alert + stream.pause().on('error', handleError).setEncoding('utf8').resume().pipe(writable); } { // Long chained pipe without error handler const stream = getStream(); From 30f28155038726227d43443b1429934f9698bb22 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 20 May 2025 14:43:14 +0200 Subject: [PATCH 04/37] Fixed issue where a custom `pipe` method which returns non stream would be flagged by the query --- .../ql/src/Quality/UnhandledStreamPipe.ql | 46 ++++++++++++++++++- .../Quality/UnhandledStreamPipe/test.expected | 3 -- .../Quality/UnhandledStreamPipe/test.js | 6 +-- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 4f16d100021f..da2680e55314 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -41,6 +41,20 @@ string getChainableStreamMethodName() { ] } +/** + * Gets the method names that are not chainable on Node.js streams. + */ +string getNonchainableStreamMethodName() { + result = ["read", "write", "end", "pipe", "unshift", "push", "isPaused", "wrap", "emit"] +} + +/** + * Gets all method names commonly found on Node.js streams. + */ +string getStreamMethodName() { + result = [getChainableStreamMethodName(), getNonchainableStreamMethodName()] +} + /** * A call to register an event handler on a Node.js stream. * This includes methods like `on`, `once`, and `addListener`. @@ -67,6 +81,34 @@ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) ) } +/** + * Tracks the result of a pipe call as it flows through the program. + */ +private DataFlow::SourceNode pipeResultTracker(DataFlow::TypeTracker t, PipeCall pipe) { + t.start() and result = pipe + or + exists(DataFlow::TypeTracker t2 | result = pipeResultTracker(t2, pipe).track(t2, t)) +} + +/** + * Gets a reference to the result of a pipe call. + */ +private DataFlow::SourceNode pipeResultRef(PipeCall pipe) { + result = pipeResultTracker(DataFlow::TypeTracker::end(), pipe) +} + +/** + * Holds if the pipe call result is used to call a non-stream method. + * Since pipe() returns the destination stream, this finds cases where + * the destination stream is used with methods not typical of streams. + */ +predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) { + exists(DataFlow::MethodCallNode call | + call = pipeResultRef(pipeCall).getAMethodCall() and + not call.getMethodName() = getStreamMethodName() + ) +} + /** * Gets a reference to a stream that may be the source of the given pipe call. * Uses type back-tracking to trace stream references in the data flow. @@ -101,6 +143,8 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) { } from PipeCall pipeCall -where not hasErrorHandlerRegistered(pipeCall) +where + not hasErrorHandlerRegistered(pipeCall) and + not isPipeFollowedByNonStreamMethod(pipeCall) select pipeCall, "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped." diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 776c1c07def8..743b184c5152 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -9,8 +9,5 @@ | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:147:5:147:28 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:151:20:151:43 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:157:47:157:74 | someVar ... ething) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:163:5:163:20 | notStream.pipe() | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:167:5:167:36 | notStre ... , arg3) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index 61bee5078a0c..c59abb6ab1e4 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -144,17 +144,17 @@ function test() { } { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) const notStream = getNotAStream(); - notStream.pipe(writable).subscribe(); // $SPURIOUS:Alert + notStream.pipe(writable).subscribe(); } { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) const notStream = getNotAStream(); - const result = notStream.pipe(writable); // $SPURIOUS:Alert + const result = notStream.pipe(writable); const dealWithResult = (result) => { result.subscribe(); }; dealWithResult(result); } { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) const notStream = getNotAStream(); - const pipeIt = (someVariable) => { return someVariable.pipe(something); }; // $SPURIOUS:Alert + const pipeIt = (someVariable) => { return someVariable.pipe(something); }; let x = pipeIt(notStream); x.subscribe(); } From 03d1f9a7d33820082e8d036e852a6881ac94ffef Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 20 May 2025 14:45:51 +0200 Subject: [PATCH 05/37] Restrict pipe detection to calls with 1-2 arguments --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 2 +- .../query-tests/Quality/UnhandledStreamPipe/test.expected | 2 -- .../ql/test/query-tests/Quality/UnhandledStreamPipe/test.js | 4 ++-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index da2680e55314..4580e1c32938 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -15,7 +15,7 @@ import javascript * A call to the `pipe` method on a Node.js stream. */ class PipeCall extends DataFlow::MethodCallNode { - PipeCall() { this.getMethodName() = "pipe" } + PipeCall() { this.getMethodName() = "pipe" and this.getNumArgument() = [1, 2] } /** Gets the source stream (receiver of the pipe call). */ DataFlow::Node getSourceStream() { result = this.getReceiver() } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 743b184c5152..2c52c7382040 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -9,5 +9,3 @@ | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:163:5:163:20 | notStream.pipe() | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:167:5:167:36 | notStre ... , arg3) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index c59abb6ab1e4..c80cd11c16b0 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -160,10 +160,10 @@ function test() { } { // Calling custom pipe method with no arguments const notStream = getNotAStream(); - notStream.pipe(); // $SPURIOUS:Alert + notStream.pipe(); } { // Calling custom pipe method with more then 2 arguments const notStream = getNotAStream(); - notStream.pipe(arg1, arg2, arg3); // $SPURIOUS:Alert + notStream.pipe(arg1, arg2, arg3); } } From 5710f0cf51fc8876c64e62235068cc1c6fd59415 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Wed, 21 May 2025 09:42:44 +0200 Subject: [PATCH 06/37] Add test cases for non-stream field accesses and methods before and after pipe operations --- .../UnhandledStreamPipe/rxjsStreams.js | 10 +++++ .../Quality/UnhandledStreamPipe/test.expected | 17 +++++++ .../Quality/UnhandledStreamPipe/test.js | 45 +++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js new file mode 100644 index 000000000000..a074be1c1a2f --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js @@ -0,0 +1,10 @@ +import * as rx from 'rxjs'; +import * as ops from 'rxjs/operators'; + +const { of, from } = rx; +const { map, filter } = ops; + +function f(){ + of(1, 2, 3).pipe(map(x => x * 2)); // $SPURIOUS:Alert + someNonStream().pipe(map(x => x * 2)); // $SPURIOUS:Alert +} diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 2c52c7382040..a26cf2a93428 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,3 +1,5 @@ +| rxjsStreams.js:8:3:8:35 | of(1, 2 ... x * 2)) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| rxjsStreams.js:9:3:9:39 | someNon ... x * 2)) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | @@ -9,3 +11,18 @@ | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:171:17:171:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:190:17:190:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:195:17:195:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:199:5:199:22 | notStream.pipe({}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:203:5:203:26 | notStre ... ()=>{}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:207:5:207:31 | getStre ... mber()) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:207:5:207:42 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:207:5:207:53 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:207:5:207:64 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:212:5:212:23 | getStream().pipe(p) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:212:5:212:34 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:212:5:212:45 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:212:5:212:56 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index c80cd11c16b0..1cc320bd00b8 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -166,4 +166,49 @@ function test() { const notStream = getNotAStream(); notStream.pipe(arg1, arg2, arg3); } + { // Member access on a non-stream after pipe + const notStream = getNotAStream(); + const val = notStream.pipe(writable).someMember; // $SPURIOUS:Alert + } + { // Member access on a stream after pipe + const notStream = getNotAStream(); + const val = notStream.pipe(writable).readable; // $Alert + } + { // Method access on a non-stream after pipe + const notStream = getNotAStream(); + const val = notStream.pipe(writable).someMethod(); + } + { // Pipe on fs readStream + const fs = require('fs'); + const stream = fs.createReadStream('file.txt'); + const copyStream = stream; + copyStream.pipe(destination); // $Alert + } + { + const notStream = getNotAStream(); + const something = notStream.someNotStreamPropertyAccess; + const val = notStream.pipe(writable); // $SPURIOUS:Alert + } + { + const notStream = getNotAStream(); + const something = notStream.someNotStreamPropertyAccess(); + const val = notStream.pipe(writable); // $SPURIOUS:Alert + } + { + const notStream = getNotAStream(); + notStream.pipe({}); // $SPURIOUS:Alert + } + { + const notStream = getNotAStream(); + notStream.pipe(()=>{}); // $SPURIOUS:Alert + } + { + const plumber = require('gulp-plumber'); + getStream().pipe(plumber()).pipe(dest).pipe(dest).pipe(dest); // $SPURIOUS:Alert + } + { + const plumber = require('gulp-plumber'); + const p = plumber(); + getStream().pipe(p).pipe(dest).pipe(dest).pipe(dest); // $SPURIOUS:Alert + } } From 4332de464aef736dddb2616ff88868e74f9740fa Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Wed, 21 May 2025 11:28:37 +0200 Subject: [PATCH 07/37] Eliminate false positives by detecting non-stream objects returned from `pipe()` calls based on accessed properties --- .../ql/src/Quality/UnhandledStreamPipe.ql | 36 ++++++++++++++++++- .../Quality/UnhandledStreamPipe/test.expected | 7 ---- .../Quality/UnhandledStreamPipe/test.js | 2 +- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 4580e1c32938..8436aa124d04 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -48,6 +48,21 @@ string getNonchainableStreamMethodName() { result = ["read", "write", "end", "pipe", "unshift", "push", "isPaused", "wrap", "emit"] } +/** + * Gets the property names commonly found on Node.js streams. + */ +string getStreamPropertyName() { + result = + [ + "readable", "writable", "destroyed", "closed", "readableHighWaterMark", "readableLength", + "readableObjectMode", "readableEncoding", "readableFlowing", "readableEnded", "flowing", + "writableHighWaterMark", "writableLength", "writableObjectMode", "writableFinished", + "writableCorked", "writableEnded", "defaultEncoding", "allowHalfOpen", "objectMode", + "errored", "pending", "autoDestroy", "encoding", "path", "fd", "bytesRead", "bytesWritten", + "_readableState", "_writableState" + ] +} + /** * Gets all method names commonly found on Node.js streams. */ @@ -109,6 +124,25 @@ predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) { ) } +/** + * Holds if the pipe call result is used to access a property that is not typical of streams. + */ +predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) { + exists(DataFlow::PropRef propRef | + propRef = pipeResultRef(pipeCall).getAPropertyRead() and + not propRef.getPropertyName() = getStreamPropertyName() + ) +} + +/** + * Holds if the pipe call result is used in a non-stream-like way, + * either by calling non-stream methods or accessing non-stream properties. + */ +predicate isPipeFollowedByNonStreamAccess(PipeCall pipeCall) { + isPipeFollowedByNonStreamMethod(pipeCall) or + isPipeFollowedByNonStreamProperty(pipeCall) +} + /** * Gets a reference to a stream that may be the source of the given pipe call. * Uses type back-tracking to trace stream references in the data flow. @@ -145,6 +179,6 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) { from PipeCall pipeCall where not hasErrorHandlerRegistered(pipeCall) and - not isPipeFollowedByNonStreamMethod(pipeCall) + not isPipeFollowedByNonStreamAccess(pipeCall) select pipeCall, "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped." diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index a26cf2a93428..d6684328ddc0 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -11,18 +11,11 @@ | test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:171:17:171:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:190:17:190:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:195:17:195:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:199:5:199:22 | notStream.pipe({}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:203:5:203:26 | notStre ... ()=>{}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:207:5:207:31 | getStre ... mber()) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:207:5:207:42 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:207:5:207:53 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:207:5:207:64 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:212:5:212:23 | getStream().pipe(p) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:212:5:212:34 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:212:5:212:45 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:212:5:212:56 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index 1cc320bd00b8..f8c5058dc10f 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -168,7 +168,7 @@ function test() { } { // Member access on a non-stream after pipe const notStream = getNotAStream(); - const val = notStream.pipe(writable).someMember; // $SPURIOUS:Alert + const val = notStream.pipe(writable).someMember; } { // Member access on a stream after pipe const notStream = getNotAStream(); From d7f86db76c6d0bb86f35c8234f0533f1a8619c15 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 12:19:05 +0200 Subject: [PATCH 08/37] Enhance PipeCall to exclude non-function and non-object arguments in pipe method detection --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 7 ++++++- .../query-tests/Quality/UnhandledStreamPipe/test.expected | 2 -- .../test/query-tests/Quality/UnhandledStreamPipe/test.js | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 8436aa124d04..b2eab9ad62a3 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -15,7 +15,12 @@ import javascript * A call to the `pipe` method on a Node.js stream. */ class PipeCall extends DataFlow::MethodCallNode { - PipeCall() { this.getMethodName() = "pipe" and this.getNumArgument() = [1, 2] } + PipeCall() { + this.getMethodName() = "pipe" and + this.getNumArgument() = [1, 2] and + not this.getArgument(0).asExpr() instanceof Function and + not this.getArgument(0).asExpr() instanceof ObjectExpr + } /** Gets the source stream (receiver of the pipe call). */ DataFlow::Node getSourceStream() { result = this.getReceiver() } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index d6684328ddc0..8e7c4020abe0 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -15,7 +15,5 @@ | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:190:17:190:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:195:17:195:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:199:5:199:22 | notStream.pipe({}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:203:5:203:26 | notStre ... ()=>{}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:207:5:207:64 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:212:5:212:56 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index f8c5058dc10f..49f100617c5d 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -196,11 +196,11 @@ function test() { } { const notStream = getNotAStream(); - notStream.pipe({}); // $SPURIOUS:Alert + notStream.pipe({}); } { const notStream = getNotAStream(); - notStream.pipe(()=>{}); // $SPURIOUS:Alert + notStream.pipe(()=>{}); } { const plumber = require('gulp-plumber'); From 09220fce845b54d87c2701e7b5edc648c26561b4 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 12:33:36 +0200 Subject: [PATCH 09/37] Fixed issue where `pipe` calls from `rxjs` package would been identified as `pipe` calls on streams --- javascript/ql/lib/ext/rxjs.model.yml | 7 ++++++ .../ql/src/Quality/UnhandledStreamPipe.ql | 22 +++++++++++++++++-- .../UnhandledStreamPipe/rxjsStreams.js | 4 ++-- .../Quality/UnhandledStreamPipe/test.expected | 2 -- 4 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/lib/ext/rxjs.model.yml diff --git a/javascript/ql/lib/ext/rxjs.model.yml b/javascript/ql/lib/ext/rxjs.model.yml new file mode 100644 index 000000000000..73c24fd875b9 --- /dev/null +++ b/javascript/ql/lib/ext/rxjs.model.yml @@ -0,0 +1,7 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: typeModel + data: + - ["NonNodeStream", "rxjs", "Fuzzy"] + - ["NonNodeStream", "rxjs/operators", "Fuzzy"] diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index b2eab9ad62a3..5935129b610c 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -19,7 +19,8 @@ class PipeCall extends DataFlow::MethodCallNode { this.getMethodName() = "pipe" and this.getNumArgument() = [1, 2] and not this.getArgument(0).asExpr() instanceof Function and - not this.getArgument(0).asExpr() instanceof ObjectExpr + not this.getArgument(0).asExpr() instanceof ObjectExpr and + not this.getArgument(0).getALocalSource() = getNonNodeJsStreamType() } /** Gets the source stream (receiver of the pipe call). */ @@ -29,6 +30,14 @@ class PipeCall extends DataFlow::MethodCallNode { DataFlow::Node getDestinationStream() { result = this.getArgument(0) } } +/** + * Gets a reference to a value that is known to not be a Node.js stream. + * This is used to exclude pipe calls on non-stream objects from analysis. + */ +DataFlow::Node getNonNodeJsStreamType() { + result = ModelOutput::getATypeNode("NonNodeStream").asSource() +} + /** * Gets the method names used to register event handlers on Node.js streams. * These methods are used to attach handlers for events like `error`. @@ -181,9 +190,18 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) { ) } +/** + * Holds if the source or destination of the given pipe call is identified as a non-Node.js stream. + */ +predicate hasNonNodeJsStreamSource(PipeCall pipeCall) { + streamRef(pipeCall) = getNonNodeJsStreamType() or + pipeResultRef(pipeCall) = getNonNodeJsStreamType() +} + from PipeCall pipeCall where not hasErrorHandlerRegistered(pipeCall) and - not isPipeFollowedByNonStreamAccess(pipeCall) + not isPipeFollowedByNonStreamAccess(pipeCall) and + not hasNonNodeJsStreamSource(pipeCall) select pipeCall, "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped." diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js index a074be1c1a2f..0e86442dd911 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js @@ -5,6 +5,6 @@ const { of, from } = rx; const { map, filter } = ops; function f(){ - of(1, 2, 3).pipe(map(x => x * 2)); // $SPURIOUS:Alert - someNonStream().pipe(map(x => x * 2)); // $SPURIOUS:Alert + of(1, 2, 3).pipe(map(x => x * 2)); + someNonStream().pipe(map(x => x * 2)); } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 8e7c4020abe0..a0c899f50f59 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,5 +1,3 @@ -| rxjsStreams.js:8:3:8:35 | of(1, 2 ... x * 2)) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| rxjsStreams.js:9:3:9:39 | someNon ... x * 2)) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | From b1048719aa48e9261ed5f077c2eb5598746ec14a Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 12:42:56 +0200 Subject: [PATCH 10/37] Added `UnhandledStreamPipe` to `javascript-security-and-quality.qls` and `javascript-code-quality.qls` --- .../query-suite/javascript-code-quality.qls.expected | 1 + .../query-suite/javascript-security-and-quality.qls.expected | 1 + 2 files changed, 2 insertions(+) diff --git a/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected b/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected index e4620badfc82..4e1662ae7864 100644 --- a/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected +++ b/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected @@ -2,4 +2,5 @@ ql/javascript/ql/src/Declarations/IneffectiveParameterType.ql ql/javascript/ql/src/Expressions/ExprHasNoEffect.ql ql/javascript/ql/src/Expressions/MissingAwait.ql ql/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql +ql/javascript/ql/src/Quality/UnhandledStreamPipe.ql ql/javascript/ql/src/RegExp/RegExpAlwaysMatches.ql diff --git a/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected b/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected index 63f6629f7bfd..671b620c77cd 100644 --- a/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected +++ b/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected @@ -80,6 +80,7 @@ ql/javascript/ql/src/NodeJS/InvalidExport.ql ql/javascript/ql/src/NodeJS/MissingExports.ql ql/javascript/ql/src/Performance/PolynomialReDoS.ql ql/javascript/ql/src/Performance/ReDoS.ql +ql/javascript/ql/src/Quality/UnhandledStreamPipe.ql ql/javascript/ql/src/React/DirectStateMutation.ql ql/javascript/ql/src/React/InconsistentStateUpdate.ql ql/javascript/ql/src/React/UnsupportedStateUpdateInLifecycleMethod.ql From 5b1af0c0bd224b7cb7cbf72a6a6d40c64b20f839 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 13:32:25 +0200 Subject: [PATCH 11/37] Added detection of custom `gulp-plumber` sanitizer, thus one would not flag such instances. --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 9 +++++++++ .../Quality/UnhandledStreamPipe/test.expected | 2 -- .../test/query-tests/Quality/UnhandledStreamPipe/test.js | 4 ++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 5935129b610c..eadb056a424d 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -188,6 +188,15 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) { handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) and handler.getArgument(0).getStringValue() = "error" ) + or + hasPlumber(pipeCall) +} + +/** + * Holds if one of the arguments of the pipe call is a `gulp-plumber` monkey patch. + */ +predicate hasPlumber(PipeCall pipeCall) { + streamRef+(pipeCall) = API::moduleImport("gulp-plumber").getACall() } /** diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index a0c899f50f59..aac258e9c6c6 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -13,5 +13,3 @@ | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:190:17:190:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:195:17:195:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:207:5:207:64 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:212:5:212:56 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index 49f100617c5d..db10aa35fa99 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -204,11 +204,11 @@ function test() { } { const plumber = require('gulp-plumber'); - getStream().pipe(plumber()).pipe(dest).pipe(dest).pipe(dest); // $SPURIOUS:Alert + getStream().pipe(plumber()).pipe(dest).pipe(dest).pipe(dest); } { const plumber = require('gulp-plumber'); const p = plumber(); - getStream().pipe(p).pipe(dest).pipe(dest).pipe(dest); // $SPURIOUS:Alert + getStream().pipe(p).pipe(dest).pipe(dest).pipe(dest); } } From ac24fdd3480271a8880c7a39d52bc8dc2deaf158 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 13:37:04 +0200 Subject: [PATCH 12/37] Add predicate to detect non-stream-like usage in sources of pipe calls --- .../ql/src/Quality/UnhandledStreamPipe.ql | 18 ++++++++++++++++++ .../Quality/UnhandledStreamPipe/test.expected | 2 -- .../Quality/UnhandledStreamPipe/test.js | 4 ++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index eadb056a424d..d3894d128894 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -207,10 +207,28 @@ predicate hasNonNodeJsStreamSource(PipeCall pipeCall) { pipeResultRef(pipeCall) = getNonNodeJsStreamType() } +/** + * Holds if the source stream of the given pipe call is used in a non-stream-like way. + */ +private predicate hasNonStreamSourceLikeUsage(PipeCall pipeCall) { + exists(DataFlow::MethodCallNode call, string name | + call.getReceiver().getALocalSource() = streamRef(pipeCall) and + name = call.getMethodName() and + not name = getStreamMethodName() + ) + or + exists(DataFlow::PropRef propRef, string propName | + propRef.getBase().getALocalSource() = streamRef(pipeCall) and + propName = propRef.getPropertyName() and + not propName = [getStreamPropertyName(), getStreamMethodName()] + ) +} + from PipeCall pipeCall where not hasErrorHandlerRegistered(pipeCall) and not isPipeFollowedByNonStreamAccess(pipeCall) and + not hasNonStreamSourceLikeUsage(pipeCall) and not hasNonNodeJsStreamSource(pipeCall) select pipeCall, "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped." diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index aac258e9c6c6..dbbc751e7b41 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -11,5 +11,3 @@ | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:190:17:190:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:195:17:195:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index db10aa35fa99..6dd8a4ca962c 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -187,12 +187,12 @@ function test() { { const notStream = getNotAStream(); const something = notStream.someNotStreamPropertyAccess; - const val = notStream.pipe(writable); // $SPURIOUS:Alert + const val = notStream.pipe(writable); } { const notStream = getNotAStream(); const something = notStream.someNotStreamPropertyAccess(); - const val = notStream.pipe(writable); // $SPURIOUS:Alert + const val = notStream.pipe(writable); } { const notStream = getNotAStream(); From e6ae8bbde4796a2a0aa25f62dfb0b86e51eae11a Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 16:38:28 +0200 Subject: [PATCH 13/37] Added test cases where second parameter passed to `pipe` is a function and some popular library ones --- .../Quality/UnhandledStreamPipe/rxjsStreams.js | 8 ++++++++ .../query-tests/Quality/UnhandledStreamPipe/strapi.js | 5 +++++ .../query-tests/Quality/UnhandledStreamPipe/test.expected | 3 +++ .../test/query-tests/Quality/UnhandledStreamPipe/test.js | 4 ++++ 4 files changed, 20 insertions(+) create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js index 0e86442dd911..472e41dc0bb9 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js @@ -1,5 +1,7 @@ import * as rx from 'rxjs'; import * as ops from 'rxjs/operators'; +import { TestScheduler } from 'rxjs/testing'; + const { of, from } = rx; const { map, filter } = ops; @@ -7,4 +9,10 @@ const { map, filter } = ops; function f(){ of(1, 2, 3).pipe(map(x => x * 2)); someNonStream().pipe(map(x => x * 2)); + + let testScheduler = new TestScheduler(); + testScheduler.run(({x, y, z}) => { + const source = x('', {o: [a, b, c]}); + z(source.pipe(null)).toBe(expected,y,); // $SPURIOUS:Alert + }); } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js new file mode 100644 index 000000000000..2848b10132a6 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js @@ -0,0 +1,5 @@ +import { async } from '@strapi/utils'; + +const f = async () => { + const permissionsInDB = await async.pipe(strapi.db.query('x').findMany,map('y'))(); // $SPURIOUS:Alert +} diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index dbbc751e7b41..457e5e7244f0 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,3 +1,5 @@ +| rxjsStreams.js:16:7:16:23 | source.pipe(null) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| strapi.js:4:35:4:84 | async.p ... p('y')) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | @@ -11,3 +13,4 @@ | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:216:5:216:38 | notStre ... ()=>{}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index 6dd8a4ca962c..37befaf59998 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -211,4 +211,8 @@ function test() { const p = plumber(); getStream().pipe(p).pipe(dest).pipe(dest).pipe(dest); } + { + const notStream = getNotAStream(); + notStream.pipe(getStream(),()=>{}); // $SPURIOUS:Alert + } } From b10a9481f36ab830a2cf5ce2f19a9e101fd15e86 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Thu, 22 May 2025 16:41:11 +0200 Subject: [PATCH 14/37] Fixed false positives from `strapi` and `rxjs/testing` as well as when one passes function as second arg to `pipe` --- javascript/ql/lib/ext/rxjs.model.yml | 1 + javascript/ql/lib/ext/strapi.model.yml | 6 ++++++ javascript/ql/src/Quality/UnhandledStreamPipe.ql | 2 +- .../query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js | 2 +- .../test/query-tests/Quality/UnhandledStreamPipe/strapi.js | 2 +- .../query-tests/Quality/UnhandledStreamPipe/test.expected | 3 --- .../ql/test/query-tests/Quality/UnhandledStreamPipe/test.js | 2 +- 7 files changed, 11 insertions(+), 7 deletions(-) create mode 100644 javascript/ql/lib/ext/strapi.model.yml diff --git a/javascript/ql/lib/ext/rxjs.model.yml b/javascript/ql/lib/ext/rxjs.model.yml index 73c24fd875b9..0d1c26650a27 100644 --- a/javascript/ql/lib/ext/rxjs.model.yml +++ b/javascript/ql/lib/ext/rxjs.model.yml @@ -5,3 +5,4 @@ extensions: data: - ["NonNodeStream", "rxjs", "Fuzzy"] - ["NonNodeStream", "rxjs/operators", "Fuzzy"] + - ["NonNodeStream", "rxjs/testing", "Fuzzy"] diff --git a/javascript/ql/lib/ext/strapi.model.yml b/javascript/ql/lib/ext/strapi.model.yml new file mode 100644 index 000000000000..9dbe753b31bd --- /dev/null +++ b/javascript/ql/lib/ext/strapi.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: typeModel + data: + - ["NonNodeStream", "@strapi/utils", "Fuzzy"] diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index d3894d128894..d7a9868fcac3 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -18,7 +18,7 @@ class PipeCall extends DataFlow::MethodCallNode { PipeCall() { this.getMethodName() = "pipe" and this.getNumArgument() = [1, 2] and - not this.getArgument(0).asExpr() instanceof Function and + not this.getArgument([0, 1]).asExpr() instanceof Function and not this.getArgument(0).asExpr() instanceof ObjectExpr and not this.getArgument(0).getALocalSource() = getNonNodeJsStreamType() } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js index 472e41dc0bb9..fc05533d7ed4 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js @@ -13,6 +13,6 @@ function f(){ let testScheduler = new TestScheduler(); testScheduler.run(({x, y, z}) => { const source = x('', {o: [a, b, c]}); - z(source.pipe(null)).toBe(expected,y,); // $SPURIOUS:Alert + z(source.pipe(null)).toBe(expected,y,); }); } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js index 2848b10132a6..e4fd4cd4e67f 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js @@ -1,5 +1,5 @@ import { async } from '@strapi/utils'; const f = async () => { - const permissionsInDB = await async.pipe(strapi.db.query('x').findMany,map('y'))(); // $SPURIOUS:Alert + const permissionsInDB = await async.pipe(strapi.db.query('x').findMany,map('y'))(); } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 457e5e7244f0..dbbc751e7b41 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,5 +1,3 @@ -| rxjsStreams.js:16:7:16:23 | source.pipe(null) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| strapi.js:4:35:4:84 | async.p ... p('y')) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | @@ -13,4 +11,3 @@ | test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:216:5:216:38 | notStre ... ()=>{}) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index 37befaf59998..a0db4f6392e6 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -213,6 +213,6 @@ function test() { } { const notStream = getNotAStream(); - notStream.pipe(getStream(),()=>{}); // $SPURIOUS:Alert + notStream.pipe(getStream(),()=>{}); } } From 15ff7cb41a0bfbfa3da4be83e0d701a6b5003a6e Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Fri, 23 May 2025 12:30:49 +0200 Subject: [PATCH 15/37] Added more test cases which common `js` libraries uses `.pipe()` --- .../Quality/UnhandledStreamPipe/arktype.js | 3 +++ .../query-tests/Quality/UnhandledStreamPipe/execa.js | 11 +++++++++++ .../Quality/UnhandledStreamPipe/highland.js | 8 ++++++++ .../Quality/UnhandledStreamPipe/rxjsStreams.js | 4 +++- .../Quality/UnhandledStreamPipe/test.expected | 4 ++++ 5 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js new file mode 100644 index 000000000000..19d944151ab6 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js @@ -0,0 +1,3 @@ +import { type } from 'arktype'; + +type.string.pipe(Number) // $SPURIOUS:Alert diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js new file mode 100644 index 000000000000..59c4eafef59d --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js @@ -0,0 +1,11 @@ +const execa = require('execa'); + +(async () => { + const first = execa('node', ['empty.js']); + const second = execa('node', ['stdin.js']); + + first.stdout.pipe(second.stdin); // $SPURIOUS:Alert + + const {stdout} = await second; + console.log(stdout); +})(); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js new file mode 100644 index 000000000000..406ee7f44f45 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js @@ -0,0 +1,8 @@ +const highland = require('highland'); +const fs = require('fs'); + +highland(fs.createReadStream('input.txt')) + .map(line => { + if (line.length === 0) throw new Error('Empty line'); + return line; + }).pipe(fs.createWriteStream('output.txt')); // $SPURIOUS:Alert diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js index fc05533d7ed4..3bbd01ac930d 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js @@ -1,7 +1,7 @@ import * as rx from 'rxjs'; import * as ops from 'rxjs/operators'; import { TestScheduler } from 'rxjs/testing'; - +import { pluck } from "rxjs/operators/pluck"; const { of, from } = rx; const { map, filter } = ops; @@ -15,4 +15,6 @@ function f(){ const source = x('', {o: [a, b, c]}); z(source.pipe(null)).toBe(expected,y,); }); + + z.option$.pipe(pluck("x")) // $SPURIOUS:Alert } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index dbbc751e7b41..4c726175f18f 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,3 +1,7 @@ +| arktype.js:3:1:3:24 | type.st ... Number) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| execa.js:7:3:7:33 | first.s ... .stdin) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| highland.js:4:1:8:45 | highlan ... .txt')) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| rxjsStreams.js:19:3:19:28 | z.optio ... k("x")) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | From c6db32ed73b790aaf5cd094c03473386d4e7920e Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Fri, 23 May 2025 12:34:11 +0200 Subject: [PATCH 16/37] Add exceptions for `arktype`, `execa`, and `highland` to prevent them from being flagged by unhandled pipe error query --- javascript/ql/lib/ext/arktype.model.yml | 6 ++++++ javascript/ql/lib/ext/execa.model.yml | 6 ++++++ javascript/ql/lib/ext/highland.model.yml | 6 ++++++ .../test/query-tests/Quality/UnhandledStreamPipe/arktype.js | 2 +- .../test/query-tests/Quality/UnhandledStreamPipe/execa.js | 2 +- .../query-tests/Quality/UnhandledStreamPipe/highland.js | 2 +- .../query-tests/Quality/UnhandledStreamPipe/test.expected | 3 --- 7 files changed, 21 insertions(+), 6 deletions(-) create mode 100644 javascript/ql/lib/ext/arktype.model.yml create mode 100644 javascript/ql/lib/ext/execa.model.yml create mode 100644 javascript/ql/lib/ext/highland.model.yml diff --git a/javascript/ql/lib/ext/arktype.model.yml b/javascript/ql/lib/ext/arktype.model.yml new file mode 100644 index 000000000000..38cb8eb7de67 --- /dev/null +++ b/javascript/ql/lib/ext/arktype.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: typeModel + data: + - ["NonNodeStream", "arktype", "Fuzzy"] diff --git a/javascript/ql/lib/ext/execa.model.yml b/javascript/ql/lib/ext/execa.model.yml new file mode 100644 index 000000000000..1f05b575e99e --- /dev/null +++ b/javascript/ql/lib/ext/execa.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: typeModel + data: + - ["NonNodeStream", "execa", "Fuzzy"] diff --git a/javascript/ql/lib/ext/highland.model.yml b/javascript/ql/lib/ext/highland.model.yml new file mode 100644 index 000000000000..eb581bf63633 --- /dev/null +++ b/javascript/ql/lib/ext/highland.model.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/javascript-all + extensible: typeModel + data: + - ["NonNodeStream", "highland", "Fuzzy"] diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js index 19d944151ab6..cac5e57511d4 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js @@ -1,3 +1,3 @@ import { type } from 'arktype'; -type.string.pipe(Number) // $SPURIOUS:Alert +type.string.pipe(Number); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js index 59c4eafef59d..052875e849bc 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js @@ -4,7 +4,7 @@ const execa = require('execa'); const first = execa('node', ['empty.js']); const second = execa('node', ['stdin.js']); - first.stdout.pipe(second.stdin); // $SPURIOUS:Alert + first.stdout.pipe(second.stdin); const {stdout} = await second; console.log(stdout); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js index 406ee7f44f45..08ac4f8954ad 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js @@ -5,4 +5,4 @@ highland(fs.createReadStream('input.txt')) .map(line => { if (line.length === 0) throw new Error('Empty line'); return line; - }).pipe(fs.createWriteStream('output.txt')); // $SPURIOUS:Alert + }).pipe(fs.createWriteStream('output.txt')); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 4c726175f18f..6b267f502e30 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,6 +1,3 @@ -| arktype.js:3:1:3:24 | type.st ... Number) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| execa.js:7:3:7:33 | first.s ... .stdin) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| highland.js:4:1:8:45 | highlan ... .txt')) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | rxjsStreams.js:19:3:19:28 | z.optio ... k("x")) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | From 248f83c4db280918c1b1d353196fb2ed6e106d5e Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Fri, 23 May 2025 13:35:36 +0200 Subject: [PATCH 17/37] Added `qhelp` for `UnhandledStreamPipe` query --- .../ql/src/Quality/UnhandledStreamPipe.qhelp | 44 +++++++++++++++++++ .../Quality/examples/UnhandledStreamPipe.js | 6 +++ .../examples/UnhandledStreamPipeGood.js | 17 +++++++ .../UnhandledStreamPipeManualError.js | 16 +++++++ 4 files changed, 83 insertions(+) create mode 100644 javascript/ql/src/Quality/UnhandledStreamPipe.qhelp create mode 100644 javascript/ql/src/Quality/examples/UnhandledStreamPipe.js create mode 100644 javascript/ql/src/Quality/examples/UnhandledStreamPipeGood.js create mode 100644 javascript/ql/src/Quality/examples/UnhandledStreamPipeManualError.js diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp new file mode 100644 index 000000000000..677bdbd4177c --- /dev/null +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp @@ -0,0 +1,44 @@ +<!DOCTYPE qhelp PUBLIC + "-//Semmle//qhelp//EN" + "qhelp.dtd"> +<qhelp> + +<overview> +<p> +In Node.js, calling the <code>pipe()</code> method on a stream without proper error handling can lead to silent failures, where errors are dropped and not propagated downstream. This can result in unexpected behavior and make debugging difficult. It is crucial to ensure that error handling is implemented when using stream pipes to maintain the reliability of the application. +</p> +</overview> + +<recommendation> +<p> +Instead of using <code>pipe()</code> with manual error handling, prefer using the <code>pipeline</code> function from the Node.js stream module. The <code>pipeline</code> function automatically handles errors and ensures proper cleanup of resources. This approach is more robust and eliminates the risk of forgetting to handle errors. +</p> +<p> +If you must use <code>pipe()</code>, always attach an error handler to the source stream using methods like <code>on('error', handler)</code> to ensure that any errors during the streaming process are properly handled. +</p> +</recommendation> + +<example> +<p> +The following code snippet demonstrates a problematic usage of the <code>pipe()</code> method without error handling: +</p> + +<sample src="examples/UnhandledStreamPipe.js" /> + +<p> +A better approach is to use the <code>pipeline</code> function, which automatically handles errors: +</p> + +<sample src="examples/UnhandledStreamPipeGood.js" /> + +<p> +Alternatively, if you need to use <code>pipe()</code>, make sure to add error handling: +</p> + +<sample src="examples/UnhandledStreamPipeManualError.js" /> +</example> + +<references> +<li>Node.js Documentation: <a href="https://nodejs.org/api/stream.html#streampipelinestreams-callback">stream.pipeline()</a>.</li> +</references> +</qhelp> diff --git a/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js b/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js new file mode 100644 index 000000000000..d31d6936f228 --- /dev/null +++ b/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js @@ -0,0 +1,6 @@ +const fs = require('fs'); +const source = fs.createReadStream('source.txt'); +const destination = fs.createWriteStream('destination.txt'); + +// Bad: No error handling +source.pipe(destination); diff --git a/javascript/ql/src/Quality/examples/UnhandledStreamPipeGood.js b/javascript/ql/src/Quality/examples/UnhandledStreamPipeGood.js new file mode 100644 index 000000000000..08b9b2a1aab5 --- /dev/null +++ b/javascript/ql/src/Quality/examples/UnhandledStreamPipeGood.js @@ -0,0 +1,17 @@ +const { pipeline } = require('stream'); +const fs = require('fs'); +const source = fs.createReadStream('source.txt'); +const destination = fs.createWriteStream('destination.txt'); + +// Good: Using pipeline for automatic error handling +pipeline( + source, + destination, + (err) => { + if (err) { + console.error('Pipeline failed:', err); + } else { + console.log('Pipeline succeeded'); + } + } +); diff --git a/javascript/ql/src/Quality/examples/UnhandledStreamPipeManualError.js b/javascript/ql/src/Quality/examples/UnhandledStreamPipeManualError.js new file mode 100644 index 000000000000..113bc8117746 --- /dev/null +++ b/javascript/ql/src/Quality/examples/UnhandledStreamPipeManualError.js @@ -0,0 +1,16 @@ +const fs = require('fs'); +const source = fs.createReadStream('source.txt'); +const destination = fs.createWriteStream('destination.txt'); + +// Alternative Good: Manual error handling with pipe() +source.on('error', (err) => { + console.error('Source stream error:', err); + destination.destroy(err); +}); + +destination.on('error', (err) => { + console.error('Destination stream error:', err); + source.destroy(err); +}); + +source.pipe(destination); From 000e69fd487feff754d6dfce88733cef2b66e863 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Fri, 23 May 2025 13:55:40 +0200 Subject: [PATCH 18/37] Replaced fuzzy `NonNodeStream` MaD to a ql predicate to deal easier with submodules --- javascript/ql/lib/ext/arktype.model.yml | 6 ------ javascript/ql/lib/ext/execa.model.yml | 6 ------ javascript/ql/lib/ext/highland.model.yml | 6 ------ javascript/ql/lib/ext/rxjs.model.yml | 8 ------- javascript/ql/lib/ext/strapi.model.yml | 6 ------ .../ql/src/Quality/UnhandledStreamPipe.ql | 21 ++++++++++++++++++- .../UnhandledStreamPipe/rxjsStreams.js | 2 +- .../Quality/UnhandledStreamPipe/test.expected | 1 - 8 files changed, 21 insertions(+), 35 deletions(-) delete mode 100644 javascript/ql/lib/ext/arktype.model.yml delete mode 100644 javascript/ql/lib/ext/execa.model.yml delete mode 100644 javascript/ql/lib/ext/highland.model.yml delete mode 100644 javascript/ql/lib/ext/rxjs.model.yml delete mode 100644 javascript/ql/lib/ext/strapi.model.yml diff --git a/javascript/ql/lib/ext/arktype.model.yml b/javascript/ql/lib/ext/arktype.model.yml deleted file mode 100644 index 38cb8eb7de67..000000000000 --- a/javascript/ql/lib/ext/arktype.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: typeModel - data: - - ["NonNodeStream", "arktype", "Fuzzy"] diff --git a/javascript/ql/lib/ext/execa.model.yml b/javascript/ql/lib/ext/execa.model.yml deleted file mode 100644 index 1f05b575e99e..000000000000 --- a/javascript/ql/lib/ext/execa.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: typeModel - data: - - ["NonNodeStream", "execa", "Fuzzy"] diff --git a/javascript/ql/lib/ext/highland.model.yml b/javascript/ql/lib/ext/highland.model.yml deleted file mode 100644 index eb581bf63633..000000000000 --- a/javascript/ql/lib/ext/highland.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: typeModel - data: - - ["NonNodeStream", "highland", "Fuzzy"] diff --git a/javascript/ql/lib/ext/rxjs.model.yml b/javascript/ql/lib/ext/rxjs.model.yml deleted file mode 100644 index 0d1c26650a27..000000000000 --- a/javascript/ql/lib/ext/rxjs.model.yml +++ /dev/null @@ -1,8 +0,0 @@ -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: typeModel - data: - - ["NonNodeStream", "rxjs", "Fuzzy"] - - ["NonNodeStream", "rxjs/operators", "Fuzzy"] - - ["NonNodeStream", "rxjs/testing", "Fuzzy"] diff --git a/javascript/ql/lib/ext/strapi.model.yml b/javascript/ql/lib/ext/strapi.model.yml deleted file mode 100644 index 9dbe753b31bd..000000000000 --- a/javascript/ql/lib/ext/strapi.model.yml +++ /dev/null @@ -1,6 +0,0 @@ -extensions: - - addsTo: - pack: codeql/javascript-all - extensible: typeModel - data: - - ["NonNodeStream", "@strapi/utils", "Fuzzy"] diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index d7a9868fcac3..1f9bf0b8d7a3 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -35,7 +35,26 @@ class PipeCall extends DataFlow::MethodCallNode { * This is used to exclude pipe calls on non-stream objects from analysis. */ DataFlow::Node getNonNodeJsStreamType() { - result = ModelOutput::getATypeNode("NonNodeStream").asSource() + result = getNonStreamApi().getAValueReachableFromSource() +} + +//highland, arktype execa +API::Node getNonStreamApi() { + exists(string moduleName | + moduleName + .regexpMatch([ + "rxjs(|/.*)", "@strapi(|/.*)", "highland(|/.*)", "execa(|/.*)", "arktype(|/.*)" + ]) and + result = API::moduleImport(moduleName) + ) + or + result = getNonStreamApi().getAMember() + or + result = getNonStreamApi().getAParameter().getAParameter() + or + result = getNonStreamApi().getReturn() + or + result = getNonStreamApi().getPromised() } /** diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js index 3bbd01ac930d..79373b49375b 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js @@ -16,5 +16,5 @@ function f(){ z(source.pipe(null)).toBe(expected,y,); }); - z.option$.pipe(pluck("x")) // $SPURIOUS:Alert + z.option$.pipe(pluck("x")) } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 6b267f502e30..dbbc751e7b41 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -1,4 +1,3 @@ -| rxjsStreams.js:19:3:19:28 | z.optio ... k("x")) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:4:5:4:28 | stream. ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:19:5:19:17 | s2.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:45:5:45:30 | stream2 ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | From e964b175e66cf67d5f975c4f4f5d7e61a5155808 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 26 May 2025 14:23:20 +0200 Subject: [PATCH 19/37] Added `maintainability` and `error-handling` tags --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 1f9bf0b8d7a3..4f5dc1f735d5 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -6,6 +6,8 @@ * @problem.severity warning * @precision high * @tags quality + * maintainability + * error-handling * frameworks/nodejs */ From 5214cc040753511a005622801e075f465cb60a60 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 27 May 2025 09:45:37 +0200 Subject: [PATCH 20/37] Excluded `ngrx`, `datorama`, `angular`, `react` and `langchain` from stream pipe query. --- .../ql/src/Quality/UnhandledStreamPipe.ql | 3 +- .../Quality/UnhandledStreamPipe/fizz-pipe.js | 29 +++++++++++++++++++ .../Quality/UnhandledStreamPipe/langchain.ts | 15 ++++++++++ .../Quality/UnhandledStreamPipe/ngrx.ts | 17 +++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/fizz-pipe.js create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/langchain.ts create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/ngrx.ts diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 4f5dc1f735d5..913957b4c688 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -45,7 +45,8 @@ API::Node getNonStreamApi() { exists(string moduleName | moduleName .regexpMatch([ - "rxjs(|/.*)", "@strapi(|/.*)", "highland(|/.*)", "execa(|/.*)", "arktype(|/.*)" + "rxjs(|/.*)", "@strapi(|/.*)", "highland(|/.*)", "execa(|/.*)", "arktype(|/.*)", + "@ngrx(|/.*)", "@datorama(|/.*)", "@angular(|/.*)", "react.*", "@langchain(|/.*)", ]) and result = API::moduleImport(moduleName) ) diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/fizz-pipe.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/fizz-pipe.js new file mode 100644 index 000000000000..94906ee46b68 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/fizz-pipe.js @@ -0,0 +1,29 @@ +import React, { Suspense } from "react"; +import { renderToPipeableStream } from "react-dom/server"; +import { PassThrough } from "stream"; +import { act } from "react-dom/test-utils"; + + +const writable = new PassThrough(); +let output = ""; +writable.on("data", chunk => { output += chunk.toString(); }); +writable.on("end", () => { /* stream ended */ }); + +let errors = []; +let shellErrors = []; + +await act(async () => { + renderToPipeableStream( + <Suspense fallback={<Fallback />}> + <Throw /> + </Suspense>, + { + onError(err) { + errors.push(err.message); + }, + onShellError(err) { + shellErrors.push(err.message); + } + } + ).pipe(writable); +}); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/langchain.ts b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/langchain.ts new file mode 100644 index 000000000000..3203dafedf79 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/langchain.ts @@ -0,0 +1,15 @@ +import { RunnablePassthrough, RunnableSequence } from "@langchain/core/runnables"; + +const fakeRetriever = RunnablePassthrough.from((_q: string) => + Promise.resolve([{ pageContent: "Hello world." }]) +); + +const formatDocumentsAsString = (documents: { pageContent: string }[]) =>documents.map((d) => d.pageContent).join("\n\n"); + +const chain = RunnableSequence.from([ + { + context: fakeRetriever.pipe(formatDocumentsAsString), + question: new RunnablePassthrough(), + }, + "", +]); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/ngrx.ts b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/ngrx.ts new file mode 100644 index 000000000000..c72d8447bb59 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/ngrx.ts @@ -0,0 +1,17 @@ +import { Component } from '@angular/core'; +import { Store, select } from '@ngrx/store'; +import { Observable } from 'rxjs'; + +@Component({ + selector: 'minimal-example', + template: ` + <div>{{ value$ | async }}</div> + ` +}) +export class MinimalExampleComponent { + value$: Observable<any>; + + constructor(private store: Store<any>) { + this.value$ = this.store.pipe(select('someSlice')); + } +} From 5bb29b6e33ceebd152c1e14144c059190cf90d44 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Wed, 28 May 2025 17:17:43 +0200 Subject: [PATCH 21/37] Now flags only `.pipe` calls which have an error somewhere down the stream, but not on the source stream. --- .../ql/src/Quality/UnhandledStreamPipe.ql | 33 +++-- .../Quality/UnhandledStreamPipe/test.expected | 19 ++- .../Quality/UnhandledStreamPipe/test.js | 45 ++++--- .../Quality/UnhandledStreamPipe/tst.js | 113 ++++++++++++++++++ 4 files changed, 183 insertions(+), 27 deletions(-) create mode 100644 javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 913957b4c688..62d3ac67996f 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -110,8 +110,11 @@ string getStreamMethodName() { * A call to register an event handler on a Node.js stream. * This includes methods like `on`, `once`, and `addListener`. */ -class StreamEventRegistration extends DataFlow::MethodCallNode { - StreamEventRegistration() { this.getMethodName() = getEventHandlerMethodName() } +class ErrorHandlerRegistration extends DataFlow::MethodCallNode { + ErrorHandlerRegistration() { + this.getMethodName() = getEventHandlerMethodName() and + this.getArgument(0).getStringValue() = "error" + } } /** @@ -136,7 +139,12 @@ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) * Tracks the result of a pipe call as it flows through the program. */ private DataFlow::SourceNode pipeResultTracker(DataFlow::TypeTracker t, PipeCall pipe) { - t.start() and result = pipe + t.start() and result = pipe.getALocalSource() + or + exists(DataFlow::SourceNode prev | + prev = pipeResultTracker(t.continue(), pipe) and + streamFlowStep(result.getALocalUse(), prev) + ) or exists(DataFlow::TypeTracker t2 | result = pipeResultTracker(t2, pipe).track(t2, t)) } @@ -166,7 +174,7 @@ predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) { predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) { exists(DataFlow::PropRef propRef | propRef = pipeResultRef(pipeCall).getAPropertyRead() and - not propRef.getPropertyName() = getStreamPropertyName() + not propRef.getPropertyName() = [getStreamPropertyName(), getStreamMethodName()] ) } @@ -206,9 +214,8 @@ private DataFlow::SourceNode streamRef(PipeCall pipeCall) { * Holds if the source stream of the given pipe call has an `error` handler registered. */ predicate hasErrorHandlerRegistered(PipeCall pipeCall) { - exists(StreamEventRegistration handler | - handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) and - handler.getArgument(0).getStringValue() = "error" + exists(ErrorHandlerRegistration handler | + handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) ) or hasPlumber(pipeCall) @@ -218,6 +225,8 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) { * Holds if one of the arguments of the pipe call is a `gulp-plumber` monkey patch. */ predicate hasPlumber(PipeCall pipeCall) { + pipeCall.getDestinationStream().getALocalSource() = API::moduleImport("gulp-plumber").getACall() + or streamRef+(pipeCall) = API::moduleImport("gulp-plumber").getACall() } @@ -246,9 +255,19 @@ private predicate hasNonStreamSourceLikeUsage(PipeCall pipeCall) { ) } +/** + * Holds if the pipe call destination stream has an error handler registered. + */ +predicate hasErrorHandlerDownstream(PipeCall pipeCall) { + exists(ErrorHandlerRegistration handler | + handler.getReceiver().getALocalSource() = pipeResultRef(pipeCall) + ) +} + from PipeCall pipeCall where not hasErrorHandlerRegistered(pipeCall) and + hasErrorHandlerDownstream(pipeCall) and not isPipeFollowedByNonStreamAccess(pipeCall) and not hasNonStreamSourceLikeUsage(pipeCall) and not hasNonNodeJsStreamSource(pipeCall) diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index dbbc751e7b41..6d6012c0005e 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -5,9 +5,16 @@ | test.js:66:5:66:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:79:5:79:25 | s2.pipe ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:94:5:94:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:109:26:109:37 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:110:11:110:22 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:119:5:119:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:128:5:128:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:146:5:146:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:182:17:182:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| test.js:192:5:192:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:8:5:8:21 | source.pipe(gzip) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:29:5:29:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:37:21:37:56 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:44:5:44:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:59:18:59:39 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:73:5:73:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:111:5:111:26 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js index a0db4f6392e6..a253f7edf006 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js @@ -1,7 +1,7 @@ function test() { { const stream = getStream(); - stream.pipe(destination); // $Alert + stream.pipe(destination).on("error", e); // $Alert } { const stream = getStream(); @@ -16,7 +16,7 @@ function test() { { const stream = getStream(); const s2 = stream; - s2.pipe(dest); // $Alert + s2.pipe(dest).on("error", e); // $Alert } { const stream = getStream(); @@ -42,7 +42,7 @@ function test() { const stream = getStream(); stream.on('error', handleError); const stream2 = stream.pipe(destination); - stream2.pipe(destination2); // $Alert + stream2.pipe(destination2).on("error", e); // $Alert } { const stream = getStream(); @@ -57,13 +57,13 @@ function test() { const stream = getStream(); stream.on('error', handleError); const stream2 = stream.pipe(destination); - stream2.pipe(destination2); // $Alert + stream2.pipe(destination2).on("error", e); // $Alert } { // Error handler on destination instead of source const stream = getStream(); const dest = getDest(); dest.on('error', handler); - stream.pipe(dest); // $Alert + stream.pipe(dest).on("error", e); // $Alert } { // Multiple aliases, error handler on one const stream = getStream(); @@ -76,7 +76,7 @@ function test() { const stream = getStream(); const s2 = stream.pipe(destination1); stream.on('error', handleError); - s2.pipe(destination2); // $Alert + s2.pipe(destination2).on("error", e); // $Alert } { // Handler registered via .once const stream = getStream(); @@ -91,7 +91,7 @@ function test() { { // Handler registered for unrelated event const stream = getStream(); stream.on('close', handleClose); - stream.pipe(dest); // $Alert + stream.pipe(dest).on("error", e); // $Alert } { // Error handler registered after pipe, but before error const stream = getStream(); @@ -106,14 +106,17 @@ function test() { } { // Pipe in a function, error handler not set const stream = getStream(); - function doPipe(s) { s.pipe(dest); } // $Alert + function doPipe(s) { + f = s.pipe(dest); // $Alert + f.on("error", e); + } doPipe(stream); } { // Dynamic event assignment const stream = getStream(); const event = 'error'; stream.on(event, handleError); - stream.pipe(dest); // $SPURIOUS:Alert + stream.pipe(dest).on("error", e); // $SPURIOUS:Alert } { // Handler assigned via variable property const stream = getStream(); @@ -122,12 +125,12 @@ function test() { stream.pipe(dest); } { // Pipe with no intermediate variable, no error handler - getStream().pipe(dest); // $Alert + getStream().pipe(dest).on("error", e); // $Alert } { // Handler set via .addListener synonym const stream = getStream(); stream.addListener('error', handleError); - stream.pipe(dest); + stream.pipe(dest).on("error", e); } { // Handler set via .once after .pipe const stream = getStream(); @@ -140,7 +143,11 @@ function test() { } { // Long chained pipe without error handler const stream = getStream(); - stream.pause().setEncoding('utf8').resume().pipe(writable); // $Alert + stream.pause().setEncoding('utf8').resume().pipe(writable).on("error", e); // $Alert + } + { // Long chained pipe without error handler + const stream = getStream(); + stream.pause().setEncoding('utf8').on("error", e).resume().pipe(writable).on("error", e); } { // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method) const notStream = getNotAStream(); @@ -172,7 +179,7 @@ function test() { } { // Member access on a stream after pipe const notStream = getNotAStream(); - const val = notStream.pipe(writable).readable; // $Alert + const val = notStream.pipe(writable).on("error", e).readable; // $Alert } { // Method access on a non-stream after pipe const notStream = getNotAStream(); @@ -182,7 +189,7 @@ function test() { const fs = require('fs'); const stream = fs.createReadStream('file.txt'); const copyStream = stream; - copyStream.pipe(destination); // $Alert + copyStream.pipe(destination).on("error", e); // $Alert } { const notStream = getNotAStream(); @@ -211,6 +218,16 @@ function test() { const p = plumber(); getStream().pipe(p).pipe(dest).pipe(dest).pipe(dest); } + { + const plumber = require('gulp-plumber'); + const p = plumber(); + getStream().pipe(p); + } + { + const plumber = require('gulp-plumber'); + const p = plumber(); + getStream().pipe(p).pipe(dest); + } { const notStream = getNotAStream(); notStream.pipe(getStream(),()=>{}); diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js new file mode 100644 index 000000000000..01ad872a7485 --- /dev/null +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js @@ -0,0 +1,113 @@ +const fs = require('fs'); +const zlib = require('zlib'); + +function foo(){ + const source = fs.createReadStream('input.txt'); + const gzip = zlib.createGzip(); + const destination = fs.createWriteStream('output.txt.gz'); + source.pipe(gzip).pipe(destination); // $Alert + gzip.on('error', e); +} +class StreamWrapper { + constructor() { + this.outputStream = getStream(); + } +} + +function zip() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper(); + let stream = wrapper.outputStream; + stream.on('error', e); + stream.pipe(zipStream); + zipStream.on('error', e); +} + +function zip1() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper(); + wrapper.outputStream.pipe(zipStream); // $SPURIOUS:Alert + wrapper.outputStream.on('error', e); + zipStream.on('error', e); +} + +function zip2() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper(); + let outStream = wrapper.outputStream.pipe(zipStream); // $Alert + outStream.on('error', e); +} + +function zip3() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper(); + wrapper.outputStream.pipe(zipStream); // $Alert + zipStream.on('error', e); +} + +function zip3() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper(); + let source = getStream(); + source.pipe(wrapper.outputStream); // $MISSING:Alert + wrapper.outputStream.on('error', e); +} + +function zip4() { + const zipStream = createWriteStream(zipPath); + let stream = getStream(); + let output = stream.pipe(zipStream); // $Alert + output.on('error', e); +} + +class StreamWrapper2 { + constructor() { + this.outputStream = getStream(); + this.outputStream.on('error', e); + } + +} +function zip5() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper2(); + wrapper.outputStream.pipe(zipStream); // $SPURIOUS:Alert + zipStream.on('error', e); +} + +class StreamWrapper3 { + constructor() { + this.stream = getStream(); + } + pipeIt(dest) { + return this.stream.pipe(dest); + } + register_error_handler(listener) { + return this.stream.on('error', listener); + } +} + +function zip5() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper3(); + wrapper.pipeIt(zipStream); // $MISSING:Alert + zipStream.on('error', e); +} +function zip6() { + const zipStream = createWriteStream(zipPath); + let wrapper = new StreamWrapper3(); + wrapper.pipeIt(zipStream); + wrapper.register_error_handler(e); + zipStream.on('error', e); +} + +function registerErr(stream, listerner) { + stream.on('error', listerner); +} + +function zip7() { + const zipStream = createWriteStream(zipPath); + let stream = getStream(); + registerErr(stream, e); + stream.pipe(zipStream); // $SPURIOUS:Alert + zipStream.on('error', e); +} From f8f5d8f56173976fe8e828297ef05e9e0897d592 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Wed, 28 May 2025 17:18:39 +0200 Subject: [PATCH 22/37] Exclude `.pipe` detection which are in a test file. --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 62d3ac67996f..0c9e23107d64 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -12,6 +12,7 @@ */ import javascript +import semmle.javascript.filters.ClassifyFiles /** * A call to the `pipe` method on a Node.js stream. @@ -270,6 +271,7 @@ where hasErrorHandlerDownstream(pipeCall) and not isPipeFollowedByNonStreamAccess(pipeCall) and not hasNonStreamSourceLikeUsage(pipeCall) and - not hasNonNodeJsStreamSource(pipeCall) + not hasNonNodeJsStreamSource(pipeCall) and + not isTestFile(pipeCall.getFile()) select pipeCall, "Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped." From 2e2b9a9d6372272cab206277da095ef5e31ad7e3 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Wed, 28 May 2025 17:23:55 +0200 Subject: [PATCH 23/37] Make predicates private and clarify stream reference naming. --- .../ql/src/Quality/UnhandledStreamPipe.ql | 76 ++++++++++--------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 0c9e23107d64..f5d9da6bb58a 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -37,12 +37,15 @@ class PipeCall extends DataFlow::MethodCallNode { * Gets a reference to a value that is known to not be a Node.js stream. * This is used to exclude pipe calls on non-stream objects from analysis. */ -DataFlow::Node getNonNodeJsStreamType() { +private DataFlow::Node getNonNodeJsStreamType() { result = getNonStreamApi().getAValueReachableFromSource() } -//highland, arktype execa -API::Node getNonStreamApi() { +/** + * Gets API nodes from modules that are known to not provide Node.js streams. + * This includes reactive programming libraries, frontend frameworks, and other non-stream APIs. + */ +private API::Node getNonStreamApi() { exists(string moduleName | moduleName .regexpMatch([ @@ -65,12 +68,12 @@ API::Node getNonStreamApi() { * Gets the method names used to register event handlers on Node.js streams. * These methods are used to attach handlers for events like `error`. */ -string getEventHandlerMethodName() { result = ["on", "once", "addListener"] } +private string getEventHandlerMethodName() { result = ["on", "once", "addListener"] } /** * Gets the method names that are chainable on Node.js streams. */ -string getChainableStreamMethodName() { +private string getChainableStreamMethodName() { result = [ "setEncoding", "pause", "resume", "unpipe", "destroy", "cork", "uncork", "setDefaultEncoding", @@ -81,14 +84,14 @@ string getChainableStreamMethodName() { /** * Gets the method names that are not chainable on Node.js streams. */ -string getNonchainableStreamMethodName() { +private string getNonchainableStreamMethodName() { result = ["read", "write", "end", "pipe", "unshift", "push", "isPaused", "wrap", "emit"] } /** * Gets the property names commonly found on Node.js streams. */ -string getStreamPropertyName() { +private string getStreamPropertyName() { result = [ "readable", "writable", "destroyed", "closed", "readableHighWaterMark", "readableLength", @@ -103,7 +106,7 @@ string getStreamPropertyName() { /** * Gets all method names commonly found on Node.js streams. */ -string getStreamMethodName() { +private string getStreamMethodName() { result = [getChainableStreamMethodName(), getNonchainableStreamMethodName()] } @@ -123,7 +126,7 @@ class ErrorHandlerRegistration extends DataFlow::MethodCallNode { * Connects destination streams to their corresponding pipe call nodes. * Connects streams to their chainable methods. */ -predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) { +private predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) { exists(PipeCall pipe | streamNode = pipe.getDestinationStream() and relatedNode = pipe @@ -139,22 +142,22 @@ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) /** * Tracks the result of a pipe call as it flows through the program. */ -private DataFlow::SourceNode pipeResultTracker(DataFlow::TypeTracker t, PipeCall pipe) { +private DataFlow::SourceNode destinationStreamRef(DataFlow::TypeTracker t, PipeCall pipe) { t.start() and result = pipe.getALocalSource() or exists(DataFlow::SourceNode prev | - prev = pipeResultTracker(t.continue(), pipe) and + prev = destinationStreamRef(t.continue(), pipe) and streamFlowStep(result.getALocalUse(), prev) ) or - exists(DataFlow::TypeTracker t2 | result = pipeResultTracker(t2, pipe).track(t2, t)) + exists(DataFlow::TypeTracker t2 | result = destinationStreamRef(t2, pipe).track(t2, t)) } /** * Gets a reference to the result of a pipe call. */ -private DataFlow::SourceNode pipeResultRef(PipeCall pipe) { - result = pipeResultTracker(DataFlow::TypeTracker::end(), pipe) +private DataFlow::SourceNode destinationStreamRef(PipeCall pipe) { + result = destinationStreamRef(DataFlow::TypeTracker::end(), pipe) } /** @@ -162,9 +165,9 @@ private DataFlow::SourceNode pipeResultRef(PipeCall pipe) { * Since pipe() returns the destination stream, this finds cases where * the destination stream is used with methods not typical of streams. */ -predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) { +private predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) { exists(DataFlow::MethodCallNode call | - call = pipeResultRef(pipeCall).getAMethodCall() and + call = destinationStreamRef(pipeCall).getAMethodCall() and not call.getMethodName() = getStreamMethodName() ) } @@ -172,9 +175,9 @@ predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) { /** * Holds if the pipe call result is used to access a property that is not typical of streams. */ -predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) { +private predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) { exists(DataFlow::PropRef propRef | - propRef = pipeResultRef(pipeCall).getAPropertyRead() and + propRef = destinationStreamRef(pipeCall).getAPropertyRead() and not propRef.getPropertyName() = [getStreamPropertyName(), getStreamMethodName()] ) } @@ -183,7 +186,7 @@ predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) { * Holds if the pipe call result is used in a non-stream-like way, * either by calling non-stream methods or accessing non-stream properties. */ -predicate isPipeFollowedByNonStreamAccess(PipeCall pipeCall) { +private predicate isPipeFollowedByNonStreamAccess(PipeCall pipeCall) { isPipeFollowedByNonStreamMethod(pipeCall) or isPipeFollowedByNonStreamProperty(pipeCall) } @@ -192,51 +195,52 @@ predicate isPipeFollowedByNonStreamAccess(PipeCall pipeCall) { * Gets a reference to a stream that may be the source of the given pipe call. * Uses type back-tracking to trace stream references in the data flow. */ -private DataFlow::SourceNode streamRef(DataFlow::TypeBackTracker t, PipeCall pipeCall) { +private DataFlow::SourceNode sourceStreamRef(DataFlow::TypeBackTracker t, PipeCall pipeCall) { t.start() and result = pipeCall.getSourceStream().getALocalSource() or exists(DataFlow::SourceNode prev | - prev = streamRef(t.continue(), pipeCall) and + prev = sourceStreamRef(t.continue(), pipeCall) and streamFlowStep(result.getALocalUse(), prev) ) or - exists(DataFlow::TypeBackTracker t2 | result = streamRef(t2, pipeCall).backtrack(t2, t)) + exists(DataFlow::TypeBackTracker t2 | result = sourceStreamRef(t2, pipeCall).backtrack(t2, t)) } /** * Gets a reference to a stream that may be the source of the given pipe call. */ -private DataFlow::SourceNode streamRef(PipeCall pipeCall) { - result = streamRef(DataFlow::TypeBackTracker::end(), pipeCall) +private DataFlow::SourceNode sourceStreamRef(PipeCall pipeCall) { + result = sourceStreamRef(DataFlow::TypeBackTracker::end(), pipeCall) } /** * Holds if the source stream of the given pipe call has an `error` handler registered. */ -predicate hasErrorHandlerRegistered(PipeCall pipeCall) { +private predicate hasErrorHandlerRegistered(PipeCall pipeCall) { exists(ErrorHandlerRegistration handler | - handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) + handler = sourceStreamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) ) or hasPlumber(pipeCall) } /** - * Holds if one of the arguments of the pipe call is a `gulp-plumber` monkey patch. + * Holds if the pipe call uses `gulp-plumber`, which automatically handles stream errors. + * Gulp-plumber is a Node.js module that prevents pipe breaking caused by errors from gulp plugins. */ -predicate hasPlumber(PipeCall pipeCall) { +private predicate hasPlumber(PipeCall pipeCall) { pipeCall.getDestinationStream().getALocalSource() = API::moduleImport("gulp-plumber").getACall() or - streamRef+(pipeCall) = API::moduleImport("gulp-plumber").getACall() + sourceStreamRef+(pipeCall) = API::moduleImport("gulp-plumber").getACall() } /** * Holds if the source or destination of the given pipe call is identified as a non-Node.js stream. */ -predicate hasNonNodeJsStreamSource(PipeCall pipeCall) { - streamRef(pipeCall) = getNonNodeJsStreamType() or - pipeResultRef(pipeCall) = getNonNodeJsStreamType() +private predicate hasNonNodeJsStreamSource(PipeCall pipeCall) { + sourceStreamRef(pipeCall) = getNonNodeJsStreamType() or + destinationStreamRef(pipeCall) = getNonNodeJsStreamType() } /** @@ -244,13 +248,13 @@ predicate hasNonNodeJsStreamSource(PipeCall pipeCall) { */ private predicate hasNonStreamSourceLikeUsage(PipeCall pipeCall) { exists(DataFlow::MethodCallNode call, string name | - call.getReceiver().getALocalSource() = streamRef(pipeCall) and + call.getReceiver().getALocalSource() = sourceStreamRef(pipeCall) and name = call.getMethodName() and not name = getStreamMethodName() ) or exists(DataFlow::PropRef propRef, string propName | - propRef.getBase().getALocalSource() = streamRef(pipeCall) and + propRef.getBase().getALocalSource() = sourceStreamRef(pipeCall) and propName = propRef.getPropertyName() and not propName = [getStreamPropertyName(), getStreamMethodName()] ) @@ -259,9 +263,9 @@ private predicate hasNonStreamSourceLikeUsage(PipeCall pipeCall) { /** * Holds if the pipe call destination stream has an error handler registered. */ -predicate hasErrorHandlerDownstream(PipeCall pipeCall) { +private predicate hasErrorHandlerDownstream(PipeCall pipeCall) { exists(ErrorHandlerRegistration handler | - handler.getReceiver().getALocalSource() = pipeResultRef(pipeCall) + handler.getReceiver().getALocalSource() = destinationStreamRef(pipeCall) ) } From d3b2a57fbfd0e16e39482f3bfc08095580d69e5b Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Wed, 28 May 2025 17:34:16 +0200 Subject: [PATCH 24/37] Fixed ql warning `Expression can be replaced with a cast` --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index f5d9da6bb58a..2c5716ef527c 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -218,9 +218,7 @@ private DataFlow::SourceNode sourceStreamRef(PipeCall pipeCall) { * Holds if the source stream of the given pipe call has an `error` handler registered. */ private predicate hasErrorHandlerRegistered(PipeCall pipeCall) { - exists(ErrorHandlerRegistration handler | - handler = sourceStreamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) - ) + sourceStreamRef(pipeCall).getAMethodCall(_) instanceof ErrorHandlerRegistration or hasPlumber(pipeCall) } From f843cc02f6ab283c8c34836afae84e3c7b2ab6db Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Fri, 30 May 2025 18:08:04 +0200 Subject: [PATCH 25/37] Fix false positives in stream pipe analysis by improving error handler tracking via property access. --- .../ql/src/Quality/UnhandledStreamPipe.ql | 24 ++++++++++++++++--- .../Quality/UnhandledStreamPipe/test.expected | 2 +- .../Quality/UnhandledStreamPipe/tst.js | 4 ++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 2c5716ef527c..d3a0af8786e1 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -218,7 +218,17 @@ private DataFlow::SourceNode sourceStreamRef(PipeCall pipeCall) { * Holds if the source stream of the given pipe call has an `error` handler registered. */ private predicate hasErrorHandlerRegistered(PipeCall pipeCall) { - sourceStreamRef(pipeCall).getAMethodCall(_) instanceof ErrorHandlerRegistration + exists(DataFlow::Node stream | + stream = sourceStreamRef(pipeCall).getALocalUse() and + ( + stream.(DataFlow::SourceNode).getAMethodCall(_) instanceof ErrorHandlerRegistration + or + exists(DataFlow::SourceNode base, string propName | + stream = base.getAPropertyRead(propName) and + base.getAPropertyRead(propName).getAMethodCall(_) instanceof ErrorHandlerRegistration + ) + ) + ) or hasPlumber(pipeCall) } @@ -262,8 +272,16 @@ private predicate hasNonStreamSourceLikeUsage(PipeCall pipeCall) { * Holds if the pipe call destination stream has an error handler registered. */ private predicate hasErrorHandlerDownstream(PipeCall pipeCall) { - exists(ErrorHandlerRegistration handler | - handler.getReceiver().getALocalSource() = destinationStreamRef(pipeCall) + exists(DataFlow::SourceNode stream | + stream = destinationStreamRef(pipeCall) and + ( + exists(ErrorHandlerRegistration handler | handler.getReceiver().getALocalSource() = stream) + or + exists(DataFlow::SourceNode base, string propName | + stream = base.getAPropertyRead(propName) and + base.getAPropertyRead(propName).getAMethodCall(_) instanceof ErrorHandlerRegistration + ) + ) ) } diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 6d6012c0005e..99d6ed002d92 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -12,9 +12,9 @@ | test.js:182:17:182:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | test.js:192:5:192:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:8:5:8:21 | source.pipe(gzip) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| tst.js:29:5:29:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:37:21:37:56 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:44:5:44:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | +| tst.js:52:5:52:37 | source. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:59:18:59:39 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:73:5:73:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:111:5:111:26 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js index 01ad872a7485..3d962974fb89 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js @@ -26,7 +26,7 @@ function zip() { function zip1() { const zipStream = createWriteStream(zipPath); let wrapper = new StreamWrapper(); - wrapper.outputStream.pipe(zipStream); // $SPURIOUS:Alert + wrapper.outputStream.pipe(zipStream); wrapper.outputStream.on('error', e); zipStream.on('error', e); } @@ -49,7 +49,7 @@ function zip3() { const zipStream = createWriteStream(zipPath); let wrapper = new StreamWrapper(); let source = getStream(); - source.pipe(wrapper.outputStream); // $MISSING:Alert + source.pipe(wrapper.outputStream); // $Alert wrapper.outputStream.on('error', e); } From 298ef9ab129a8b90a0187716d03c5b38825227d6 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 11:01:41 +0200 Subject: [PATCH 26/37] Now able to track error handler registration via instance properties --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 7 +++++++ .../query-tests/Quality/UnhandledStreamPipe/test.expected | 1 - .../ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index d3a0af8786e1..c88a3e622238 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -227,6 +227,13 @@ private predicate hasErrorHandlerRegistered(PipeCall pipeCall) { stream = base.getAPropertyRead(propName) and base.getAPropertyRead(propName).getAMethodCall(_) instanceof ErrorHandlerRegistration ) + or + exists(DataFlow::PropWrite propWrite, DataFlow::SourceNode instance | + propWrite.getRhs().getALocalSource() = stream and + instance = propWrite.getBase().getALocalSource() and + instance.getAPropertyRead(propWrite.getPropertyName()).getAMethodCall(_) instanceof + ErrorHandlerRegistration + ) ) ) or diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected index 99d6ed002d92..1b01574112d5 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected @@ -16,5 +16,4 @@ | tst.js:44:5:44:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:52:5:52:37 | source. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:59:18:59:39 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | -| tst.js:73:5:73:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | | tst.js:111:5:111:26 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. | diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js index 3d962974fb89..46bf969255f8 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js @@ -70,7 +70,7 @@ class StreamWrapper2 { function zip5() { const zipStream = createWriteStream(zipPath); let wrapper = new StreamWrapper2(); - wrapper.outputStream.pipe(zipStream); // $SPURIOUS:Alert + wrapper.outputStream.pipe(zipStream); zipStream.on('error', e); } From 3cbc4142f06f87c28fa93db0de2fd1f1604752d9 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 17:40:06 +0200 Subject: [PATCH 27/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.ql Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index c88a3e622238..f2cdee85a213 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -242,7 +242,7 @@ private predicate hasErrorHandlerRegistered(PipeCall pipeCall) { /** * Holds if the pipe call uses `gulp-plumber`, which automatically handles stream errors. - * Gulp-plumber is a Node.js module that prevents pipe breaking caused by errors from gulp plugins. + * `gulp-plumber` returns a stream that uses monkey-patching to ensure all subsequent streams in the pipeline propagate their errors. */ private predicate hasPlumber(PipeCall pipeCall) { pipeCall.getDestinationStream().getALocalSource() = API::moduleImport("gulp-plumber").getACall() From 64f00fd0f2c8aa6dd4638240a9b04a62e95ee885 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 17:41:27 +0200 Subject: [PATCH 28/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.ql Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index f2cdee85a213..863b62900b0c 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -122,20 +122,18 @@ class ErrorHandlerRegistration extends DataFlow::MethodCallNode { } /** - * Models flow relationships between streams and related operations. - * Connects destination streams to their corresponding pipe call nodes. - * Connects streams to their chainable methods. + * Holds if the stream in `node1` will propagate to `node2`. */ -private predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode) { +private predicate streamFlowStep(DataFlow::Node node1, DataFlow::Node node2) { exists(PipeCall pipe | - streamNode = pipe.getDestinationStream() and - relatedNode = pipe + node1 = pipe.getDestinationStream() and + node2 = pipe ) or exists(DataFlow::MethodCallNode chainable | chainable.getMethodName() = getChainableStreamMethodName() and - streamNode = chainable.getReceiver() and - relatedNode = chainable + node1 = chainable.getReceiver() and + node2 = chainable ) } From abd446ae77f22279383fd2ab92f44b2ca3967b8d Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 17:41:40 +0200 Subject: [PATCH 29/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.ql Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 863b62900b0c..d024a9c5fcdd 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -1,7 +1,7 @@ /** * @id js/nodejs-stream-pipe-without-error-handling * @name Node.js stream pipe without error handling - * @description Calling `pipe()` on a stream without error handling may silently drop errors and prevent proper propagation. + * @description Calling `pipe()` on a stream without error handling will drop errors coming from the input stream * @kind problem * @problem.severity warning * @precision high From 7198372ae5325207af56e7e161923633f42ea86b Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 17:41:55 +0200 Subject: [PATCH 30/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.qhelp Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp index 677bdbd4177c..4d14734f540c 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp @@ -11,7 +11,7 @@ In Node.js, calling the <code>pipe()</code> method on a stream without proper er <recommendation> <p> -Instead of using <code>pipe()</code> with manual error handling, prefer using the <code>pipeline</code> function from the Node.js stream module. The <code>pipeline</code> function automatically handles errors and ensures proper cleanup of resources. This approach is more robust and eliminates the risk of forgetting to handle errors. +Instead of using <code>pipe()</code> with manual error handling, prefer using the <code>pipeline</code> function from the Node.js <code>stream</code> module. The <code>pipeline</code> function automatically handles errors and ensures proper cleanup of resources. This approach is more robust and eliminates the risk of forgetting to handle errors. </p> <p> If you must use <code>pipe()</code>, always attach an error handler to the source stream using methods like <code>on('error', handler)</code> to ensure that any errors during the streaming process are properly handled. From d43695c929701598bc78b19b17ba72fb78c1e2e7 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 17:42:40 +0200 Subject: [PATCH 31/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.qhelp Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp index 4d14734f540c..22c59188408e 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp @@ -5,7 +5,7 @@ <overview> <p> -In Node.js, calling the <code>pipe()</code> method on a stream without proper error handling can lead to silent failures, where errors are dropped and not propagated downstream. This can result in unexpected behavior and make debugging difficult. It is crucial to ensure that error handling is implemented when using stream pipes to maintain the reliability of the application. +In Node.js, calling the <code>pipe()</code> method on a stream without proper error handling can lead to unexplained failures, where errors are dropped and not propagated downstream. This can result in unwanted behavior and make debugging difficult. To reliably handle all errors, every stream in the pipeline must have an error handler registered. </p> </overview> From ae74edb0337f6604250bc8ed43eaf8dde8db3768 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 17:53:54 +0200 Subject: [PATCH 32/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.ql Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index d024a9c5fcdd..69280443a32e 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -141,7 +141,7 @@ private predicate streamFlowStep(DataFlow::Node node1, DataFlow::Node node2) { * Tracks the result of a pipe call as it flows through the program. */ private DataFlow::SourceNode destinationStreamRef(DataFlow::TypeTracker t, PipeCall pipe) { - t.start() and result = pipe.getALocalSource() + t.start() and result = pipe or exists(DataFlow::SourceNode prev | prev = destinationStreamRef(t.continue(), pipe) and From bf2f19da566dfe16c428144bad35a3398b6dd296 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 19:02:48 +0200 Subject: [PATCH 33/37] Update UnhandledStreamPipe.ql Address comments Co-Authored-By: Asger F <316427+asgerf@users.noreply.github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledStreamPipe.ql index 69280443a32e..c2e18e9c577a 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.ql @@ -141,11 +141,12 @@ private predicate streamFlowStep(DataFlow::Node node1, DataFlow::Node node2) { * Tracks the result of a pipe call as it flows through the program. */ private DataFlow::SourceNode destinationStreamRef(DataFlow::TypeTracker t, PipeCall pipe) { - t.start() and result = pipe + t.start() and + (result = pipe or result = pipe.getDestinationStream().getALocalSource()) or exists(DataFlow::SourceNode prev | prev = destinationStreamRef(t.continue(), pipe) and - streamFlowStep(result.getALocalUse(), prev) + streamFlowStep(prev, result) ) or exists(DataFlow::TypeTracker t2 | result = destinationStreamRef(t2, pipe).track(t2, t)) From 7993f7d8c874bbcfd1c9ea3037346cb07f4783cb Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Mon, 2 Jun 2025 19:08:33 +0200 Subject: [PATCH 34/37] Update `qhelp` example to more accurately demonstrate flagged cases --- javascript/ql/src/Quality/examples/UnhandledStreamPipe.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js b/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js index d31d6936f228..95c1661a8b9f 100644 --- a/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js +++ b/javascript/ql/src/Quality/examples/UnhandledStreamPipe.js @@ -2,5 +2,7 @@ const fs = require('fs'); const source = fs.createReadStream('source.txt'); const destination = fs.createWriteStream('destination.txt'); -// Bad: No error handling -source.pipe(destination); +// Bad: Only destination has error handling, source errors are unhandled +source.pipe(destination).on('error', (err) => { + console.error('Destination error:', err); +}); From 8ba1f3f26557a302fe73c9aab5f563bd575d2c7d Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 3 Jun 2025 13:43:45 +0200 Subject: [PATCH 35/37] Update javascript/ql/src/Quality/UnhandledStreamPipe.qhelp Co-authored-by: Asger F <asgerf@github.com> --- javascript/ql/src/Quality/UnhandledStreamPipe.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp index 22c59188408e..39de6de477d4 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp +++ b/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp @@ -14,7 +14,7 @@ In Node.js, calling the <code>pipe()</code> method on a stream without proper er Instead of using <code>pipe()</code> with manual error handling, prefer using the <code>pipeline</code> function from the Node.js <code>stream</code> module. The <code>pipeline</code> function automatically handles errors and ensures proper cleanup of resources. This approach is more robust and eliminates the risk of forgetting to handle errors. </p> <p> -If you must use <code>pipe()</code>, always attach an error handler to the source stream using methods like <code>on('error', handler)</code> to ensure that any errors during the streaming process are properly handled. +If you must use <code>pipe()</code>, always attach an error handler to the source stream using methods like <code>on('error', handler)</code> to ensure that any errors emitted by the input stream are properly handled. When multiple <code>pipe()</code> calls are chained, an error handler should be attached before each step of the pipeline. </p> </recommendation> From d1869941c298962e97a5b9d2230839bfafefbbf3 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 3 Jun 2025 13:57:10 +0200 Subject: [PATCH 36/37] Renamed `UnhandledStreamPipe.ql` to a better fitting name and ID As a side effect of merge `security-and-quality` does not contain anymore related new query. Co-Authored-By: Asger F <316427+asgerf@users.noreply.github.com> --- .../query-suite/javascript-code-quality.qls.expected | 2 +- .../query-suite/javascript-security-and-quality.qls.expected | 1 - ...dStreamPipe.qhelp => UnhandledErrorInStreamPipeline.qhelp} | 0 ...handledStreamPipe.ql => UnhandledErrorInStreamPipeline.ql} | 4 ++-- .../test/query-tests/Quality/UnhandledStreamPipe/test.qlref | 2 +- 5 files changed, 4 insertions(+), 5 deletions(-) rename javascript/ql/src/Quality/{UnhandledStreamPipe.qhelp => UnhandledErrorInStreamPipeline.qhelp} (100%) rename javascript/ql/src/Quality/{UnhandledStreamPipe.ql => UnhandledErrorInStreamPipeline.ql} (99%) diff --git a/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected b/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected index 4e1662ae7864..876b5f25fa28 100644 --- a/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected +++ b/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected @@ -2,5 +2,5 @@ ql/javascript/ql/src/Declarations/IneffectiveParameterType.ql ql/javascript/ql/src/Expressions/ExprHasNoEffect.ql ql/javascript/ql/src/Expressions/MissingAwait.ql ql/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql -ql/javascript/ql/src/Quality/UnhandledStreamPipe.ql +ql/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql ql/javascript/ql/src/RegExp/RegExpAlwaysMatches.ql diff --git a/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected b/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected index f6532eb9701c..eb4acd38e39b 100644 --- a/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected +++ b/javascript/ql/integration-tests/query-suite/javascript-security-and-quality.qls.expected @@ -80,7 +80,6 @@ ql/javascript/ql/src/NodeJS/InvalidExport.ql ql/javascript/ql/src/NodeJS/MissingExports.ql ql/javascript/ql/src/Performance/PolynomialReDoS.ql ql/javascript/ql/src/Performance/ReDoS.ql -ql/javascript/ql/src/Quality/UnhandledStreamPipe.ql ql/javascript/ql/src/React/DirectStateMutation.ql ql/javascript/ql/src/React/InconsistentStateUpdate.ql ql/javascript/ql/src/React/UnsupportedStateUpdateInLifecycleMethod.ql diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.qhelp b/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.qhelp similarity index 100% rename from javascript/ql/src/Quality/UnhandledStreamPipe.qhelp rename to javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.qhelp diff --git a/javascript/ql/src/Quality/UnhandledStreamPipe.ql b/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql similarity index 99% rename from javascript/ql/src/Quality/UnhandledStreamPipe.ql rename to javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql index c2e18e9c577a..a6142a2e6e73 100644 --- a/javascript/ql/src/Quality/UnhandledStreamPipe.ql +++ b/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql @@ -1,6 +1,6 @@ /** - * @id js/nodejs-stream-pipe-without-error-handling - * @name Node.js stream pipe without error handling + * @id js/unhandled-error-in-stream-pipeline + * @name Unhandled error in stream pipeline * @description Calling `pipe()` on a stream without error handling will drop errors coming from the input stream * @kind problem * @problem.severity warning diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref index 23e2f65bab79..0da7b1900f69 100644 --- a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref +++ b/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref @@ -1,2 +1,2 @@ -query: Quality/UnhandledStreamPipe.ql +query: Quality/UnhandledErrorInStreamPipeline.ql postprocess: utils/test/InlineExpectationsTestQuery.ql From 8521c53a408e8cb55574da8132cc7cd3bd7ddeb3 Mon Sep 17 00:00:00 2001 From: Napalys Klicius <napalys@github.com> Date: Tue, 3 Jun 2025 14:12:12 +0200 Subject: [PATCH 37/37] Renamed test directory to match the query name Co-Authored-By: Asger F <316427+asgerf@users.noreply.github.com> --- .../UnhandledErrorInStreamPipeline.expected} | 0 .../UnhandledErrorInStreamPipeline.qlref} | 0 .../arktype.js | 0 .../execa.js | 0 .../fizz-pipe.js | 0 .../highland.js | 0 .../langchain.ts | 0 .../ngrx.ts | 0 .../rxjsStreams.js | 0 .../strapi.js | 0 .../test.js | 0 .../tst.js | 0 12 files changed, 0 insertions(+), 0 deletions(-) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe/test.expected => UnhandledErrorInStreamPipeline/UnhandledErrorInStreamPipeline.expected} (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe/test.qlref => UnhandledErrorInStreamPipeline/UnhandledErrorInStreamPipeline.qlref} (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/arktype.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/execa.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/fizz-pipe.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/highland.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/langchain.ts (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/ngrx.ts (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/rxjsStreams.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/strapi.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/test.js (100%) rename javascript/ql/test/query-tests/Quality/{UnhandledStreamPipe => UnhandledErrorInStreamPipeline}/tst.js (100%) diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/UnhandledErrorInStreamPipeline.expected similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/UnhandledErrorInStreamPipeline.expected diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/UnhandledErrorInStreamPipeline.qlref similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.qlref rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/UnhandledErrorInStreamPipeline.qlref diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/arktype.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/arktype.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/arktype.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/execa.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/execa.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/execa.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/fizz-pipe.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/fizz-pipe.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/fizz-pipe.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/fizz-pipe.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/highland.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/highland.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/highland.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/langchain.ts b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/langchain.ts similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/langchain.ts rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/langchain.ts diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/ngrx.ts b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/ngrx.ts similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/ngrx.ts rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/ngrx.ts diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/rxjsStreams.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/rxjsStreams.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/rxjsStreams.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/strapi.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/strapi.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/strapi.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/test.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/test.js diff --git a/javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js b/javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/tst.js similarity index 100% rename from javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js rename to javascript/ql/test/query-tests/Quality/UnhandledErrorInStreamPipeline/tst.js