diff --git a/change-notes/1.19/analysis-javascript.md b/change-notes/1.19/analysis-javascript.md index 50f14ac7e121..fabd63430f50 100644 --- a/change-notes/1.19/analysis-javascript.md +++ b/change-notes/1.19/analysis-javascript.md @@ -44,6 +44,7 @@ | Conflicting HTML element attributes | Lower severity | The severity of this rule has been revised to "warning". | | Duplicate 'if' condition | Lower severity | The severity of this rule has been revised to "warning". | | Duplicate switch case | Lower severity | The severity of this rule has been revised to "warning". | +| Inconsistent use of 'new' | Simpler result presentation | This rule now only shows one call with `new` and one without. | | Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. | | Missing 'this' qualifier | Fewer false-positive results | This rule now recognizes additional intentional calls to global functions. | | Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. | diff --git a/javascript/ql/src/LanguageFeatures/InconsistentNew.ql b/javascript/ql/src/LanguageFeatures/InconsistentNew.ql index 05ab926cc2bd..a9fa57cfce67 100644 --- a/javascript/ql/src/LanguageFeatures/InconsistentNew.ql +++ b/javascript/ql/src/LanguageFeatures/InconsistentNew.ql @@ -56,26 +56,60 @@ predicate calls(DataFlow::InvokeNode cs, Function callee, int imprecision) { /** * Gets a function that may be invoked at `cs`, preferring callees that - * are less likely to be derived due to analysis imprecision. + * are less likely to be derived due to analysis imprecision and excluding + * whitelisted call sites and callees. Additionally, `isNew` is bound to + * `true` if `cs` is a `new` expression, and to `false` otherwise. */ -Function getALikelyCallee(DataFlow::InvokeNode cs) { - calls(cs, result, min(int p | calls(cs, _, p))) +Function getALikelyCallee(DataFlow::InvokeNode cs, boolean isNew) { + calls(cs, result, min(int p | calls(cs, _, p))) and + not cs.isUncertain() and + not whitelistedCall(cs) and + not whitelistedCallee(result) and + (cs instanceof DataFlow::NewNode and isNew = true + or + cs instanceof DataFlow::CallNode and isNew = false) +} + +/** + * Holds if `f` should be whitelisted, either because it guards against + * inconsistent `new` or we do not want to report it. + */ +predicate whitelistedCallee(Function f) { + // externs are special, so don't flag them + f.inExternsFile() or + // illegal constructor calls are flagged by query 'Illegal invocation', + // so don't flag them + f instanceof Constructor or + // if `f` itself guards against missing `new`, don't flag it + guardsAgainstMissingNew(f) +} + +/** + * Holds if `call` should be whitelisted because it cannot cause problems + * with inconsistent `new`. + */ +predicate whitelistedCall(DataFlow::CallNode call) { + // super constructor calls behave more like `new`, so don't flag them + call.asExpr() instanceof SuperCall or + // don't flag if there is a receiver object + exists(call.getReceiver()) +} + +/** + * Get the `new` or call (depending on whether `isNew` is true or false) of `f` + * that comes first under a lexicographical ordering by file path, start line + * and start column. + */ +DataFlow::InvokeNode getFirstInvocation(Function f, boolean isNew) { + result = min(DataFlow::InvokeNode invk, string path, int line, int col | + f = getALikelyCallee(invk, isNew) and invk.hasLocationInfo(path, line, col, _, _) | + invk order by path, line, col + ) } from Function f, DataFlow::NewNode new, DataFlow::CallNode call -where // externs are special, so don't flag them - not f.inExternsFile() and - // illegal constructor calls are flagged by query 'Illegal invocation', - // so don't flag them - not f instanceof Constructor and - f = getALikelyCallee(new) and - f = getALikelyCallee(call) and - not guardsAgainstMissingNew(f) and - not new.isUncertain() and - not call.isUncertain() and - // super constructor calls behave more like `new`, so don't flag them - not call.asExpr() instanceof SuperCall and - // don't flag if there is a receiver object - not exists(call.getReceiver()) -select (FirstLineOf)f, capitalize(f.describe()) + " is invoked as a constructor here $@, " + - "and as a normal function here $@.", new, new.toString(), call, call.toString() +where new = getFirstInvocation(f, true) and + call = getFirstInvocation(f, false) +select (FirstLineOf)f, capitalize(f.describe()) + " is sometimes invoked as a constructor " + + "(for example $@), and sometimes as a normal function (for example $@).", + new, "here", call, "here" diff --git a/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/InconsistentNew.expected b/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/InconsistentNew.expected index 698459cf12ff..0c03a59fd223 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/InconsistentNew.expected +++ b/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/InconsistentNew.expected @@ -1,2 +1,2 @@ -| m.js:1:8:1:22 | functio ... = x;\\n} | Function A is invoked as a constructor here $@, and as a normal function here $@. | c1.js:2:1:2:9 | new A(42) | new A(42) | c2.js:2:1:2:5 | A(23) | A(23) | -| tst.js:1:1:1:22 | functio ... = y;\\n} | Function Point is invoked as a constructor here $@, and as a normal function here $@. | tst.js:6:1:6:17 | new Point(23, 42) | new Point(23, 42) | tst.js:7:1:7:13 | Point(56, 72) | Point(56, 72) | +| m.js:1:8:1:22 | functio ... = x;\\n} | Function A is sometimes invoked as a constructor (for example $@), and sometimes as a normal function (for example $@). | c1.js:2:1:2:9 | new A(42) | here | c2.js:2:1:2:5 | A(23) | here | +| tst.js:1:1:1:22 | functio ... = y;\\n} | Function Point is sometimes invoked as a constructor (for example $@), and sometimes as a normal function (for example $@). | tst.js:6:1:6:17 | new Point(23, 42) | here | tst.js:7:1:7:13 | Point(56, 72) | here | diff --git a/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/tst.js b/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/tst.js index fbcc8666b5ef..56af21411b67 100644 --- a/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/tst.js +++ b/javascript/ql/test/query-tests/LanguageFeatures/InconsistentNew/tst.js @@ -63,3 +63,6 @@ C(); // NOT OK, but flagged by IllegalInvocation new A(42); A.call({}, 23); })(); + +new Point(42, 23); // NOT OK, but not flagged since line 6 above was already flagged +Point(56, 72); // NOT OK, but not flagged since line 7 above was already flagged