Merge remote-tracking branch 'origin/main' into js/quality/loop_shift

This commit is contained in:
Napalys Klicius
2025-06-19 10:21:52 +02:00
737 changed files with 98256 additions and 20372 deletions

View File

@@ -0,0 +1,9 @@
ql/javascript/ql/src/Declarations/IneffectiveParameterType.ql
ql/javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.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/LanguageFeatures/TemplateSyntaxInStringLiteral.ql
ql/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql
ql/javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql
ql/javascript/ql/src/RegExp/RegExpAlwaysMatches.ql

View File

@@ -1,7 +1,9 @@
ql/javascript/ql/src/Declarations/IneffectiveParameterType.ql
ql/javascript/ql/src/Declarations/SuspiciousMethodNameDeclaration.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/LanguageFeatures/TemplateSyntaxInStringLiteral.ql
ql/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql
ql/javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql
ql/javascript/ql/src/RegExp/RegExpAlwaysMatches.ql

View File

@@ -2,7 +2,7 @@ import runs_on
import pytest
from query_suites import *
well_known_query_suites = ['javascript-code-quality.qls', 'javascript-security-and-quality.qls', 'javascript-security-extended.qls', 'javascript-code-scanning.qls']
well_known_query_suites = ['javascript-code-quality.qls', 'javascript-code-quality-extended.qls', 'javascript-security-and-quality.qls', 'javascript-security-extended.qls', 'javascript-code-scanning.qls']
@runs_on.posix
@pytest.mark.parametrize("query_suite", well_known_query_suites)

View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Improved taint tracking through calls to `serialize-javascript`.

View File

@@ -33,8 +33,7 @@ private class PlainJsonParserCall extends JsonParserCall {
callee = DataFlow::moduleImport("parse-json") or
callee = DataFlow::moduleImport("json-parse-better-errors") or
callee = DataFlow::moduleImport("json-safe-parse") or
callee = AngularJS::angular().getAPropertyRead("fromJson") or
callee = DataFlow::moduleImport("serialize-javascript")
callee = AngularJS::angular().getAPropertyRead("fromJson")
)
}

View File

@@ -27,6 +27,8 @@ class JsonStringifyCall extends DataFlow::CallNode {
)
or
this = Templating::getAPipeCall(["json", "dump"])
or
this = DataFlow::moduleImport("serialize-javascript").getACall()
}
/**

View File

@@ -4,50 +4,73 @@
<qhelp>
<overview>
<p>
In TypeScript the keywords <code>constructor</code> and <code>new</code> for
member declarations are used to declare constructors in classes and interfaces
respectively.
However, a member declaration with the name <code>new</code> in an interface
or <code>constructor</code> in a class, will declare an ordinary method named
<code>new</code> or <code>constructor</code> rather than a constructor.
Similarly, the keyword <code>function</code> is used to declare functions in
some contexts. However, using the name <code>function</code> for a class
or interface member declaration declares a method named <code>function</code>.
In TypeScript, certain keywords have special meanings for member declarations, and misusing them can create confusion:
</p>
<ul>
<li>In classes, use <code>constructor</code> rather than <code>new</code> to declare constructors. Using <code>new</code> within a class creates a method named "new" and not a constructor signature.</li>
<li>In interfaces, use <code>new</code> rather than <code>constructor</code> to declare constructor signatures. Using <code>constructor</code> within an interface creates a method named "constructor" and not a constructor signature.</li>
<li>Similarly, the keyword <code>function</code> is used to declare functions in some contexts. However, using the name <code>function</code> for a class or interface member declaration declares a method named "function".</li>
</ul>
<p>
When these keywords are misused, TypeScript will interpret them as regular method names rather than their intended special syntax, leading to code that may not work as expected.
</p>
</overview>
<recommendation>
<p>
Declare classes as classes and not as interfaces.
Use the keyword <code>constructor</code> to declare constructors in a class,
use the keyword <code>new</code> to declare constructors inside interfaces,
and don't use <code>function</code> when declaring a call signature in an
interface.
Consider following these guidelines for clearer code:
</p>
<ul>
<li>For classes, use <code>constructor</code> to declare constructors.</li>
<li>For interfaces, use <code>new</code> to declare constructor signatures (call signatures that create new instances).</li>
<li>Avoid accidentally creating methods named <code>function</code> by misusing the <code>function</code> keyword within class or interface declarations.</li>
</ul>
</recommendation>
<example>
<p>
The below example declares an interface <code>Point</code> with 2 fields
and a method called <code>constructor</code>. The interface does not declare
a class <code>Point</code> with a constructor, which was likely what the
developer meant to create.
The following examples show common mistakes when using these keywords:
</p>
<p>
This interface mistakenly uses <code>constructor</code>, which creates a method named "constructor" instead of a constructor signature:
</p>
<sample src="examples/SuspiciousMethodNameDeclaration.ts" />
<p>
The below example is a fixed version of the above, where the interface is
instead declared as a class, thereby describing the type the developer meant
in the first place.
Use <code>new</code> for constructor signatures in interfaces:
</p>
<sample src="examples/SuspiciousMethodNameDeclarationFixed.ts" />
<p>
This class mistakenly uses <code>new</code>, which creates a method named "new" instead of a constructor:
</p>
<sample src="examples/SuspiciousMethodNameDeclarationClass.ts" />
<p>
Use <code>constructor</code> for constructors in classes:
</p>
<sample src="examples/SuspiciousMethodNameDeclarationClassFixed.ts" />
<p>
This interface uses <code>function</code> as a method name, which declares a method named "function" rather than declaring a function:
</p>
<sample src="examples/SuspiciousMethodNameDeclarationFunction.ts" />
<p>
Use a descriptive method name instead:
</p>
<sample src="examples/SuspiciousMethodNameDeclarationFunctionFixed.ts" />
</example>
<references>
<li>TypeScript Handbook: <a href="https://www.typescriptlang.org/docs/handbook/2/classes.html#constructors">Classes - Constructors</a>.</li>
<li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/30cb20434a6b117e007a4959b2a7c16489f86069/doc/spec-ARCHIVED.md#3.8.9">Constructor Type Literals</a>.</li>
<li>TypeScript specification: <a href="https://github.com/microsoft/TypeScript/blob/30cb20434a6b117e007a4959b2a7c16489f86069/doc/spec-ARCHIVED.md#8.3.1">Constructor Parameters</a>.</li>

View File

@@ -6,7 +6,9 @@
* @problem.severity warning
* @id js/suspicious-method-name-declaration
* @precision high
* @tags correctness
* @tags quality
* reliability
* correctness
* typescript
* methods
*/

View File

@@ -1,6 +1,6 @@
declare class Point {
// BAD: Using 'constructor' in an interface creates a method, not a constructor signature
interface Point {
x: number;
y: number;
constructor(x : number, y: number);
constructor(x: number, y: number); // This is just a method named "constructor"
}

View File

@@ -0,0 +1,6 @@
// BAD: Using 'new' in a class creates a method, not a constructor
class Point {
x: number;
y: number;
new(x: number, y: number) {}; // This is just a method named "new"
}

View File

@@ -0,0 +1,9 @@
// GOOD: Using 'constructor' for constructors in classes
class Point {
x: number;
y: number;
constructor(x: number, y: number) { // This is a proper constructor
this.x = x;
this.y = y;
}
}

View File

@@ -1,4 +1,6 @@
// GOOD: Using 'new' for constructor signatures in interfaces
interface Point {
x: number;
y: number;
new(x: number, y: number): Point; // This is a proper constructor signature
}

View File

@@ -0,0 +1,4 @@
// BAD: Using 'function' as a method name is confusing
interface Calculator {
function(a: number, b: number): number; // This is just a method named "function"
}

View File

@@ -0,0 +1,4 @@
// GOOD: Using descriptive method names instead of 'function'
interface Calculator {
calculate(a: number, b: number): number; // Clear, descriptive method name
}

View File

@@ -5,7 +5,10 @@
* @problem.severity warning
* @id js/template-syntax-in-string-literal
* @precision high
* @tags correctness
* @tags quality
* reliability
* correctness
* language-features
*/
import javascript
@@ -74,8 +77,8 @@ class CandidateStringLiteral extends StringLiteral {
*/
predicate hasObjectProvidingTemplateVariables(CandidateStringLiteral lit) {
exists(DataFlow::CallNode call, DataFlow::ObjectLiteralNode obj |
call.getAnArgument().getALocalSource() = obj and
call.getAnArgument().asExpr() = lit and
call.getAnArgument() = [lit.flow(), StringConcatenation::getRoot(lit.flow())] and
obj.flowsTo(call.getAnArgument()) and
forex(string name | name = lit.getAReferencedVariable() | exists(obj.getAPropertyWrite(name)))
)
}
@@ -91,12 +94,38 @@ VarDecl getDeclIn(Variable v, Scope scope, string name, CandidateTopLevel tl) {
result.getTopLevel() = tl
}
/**
* Tracks data flow from a string literal that may flow to a replace operation.
*/
DataFlow::SourceNode trackStringWithTemplateSyntax(
CandidateStringLiteral lit, DataFlow::TypeTracker t
) {
t.start() and result = lit.flow() and exists(lit.getAReferencedVariable())
or
exists(DataFlow::TypeTracker t2 | result = trackStringWithTemplateSyntax(lit, t2).track(t2, t))
}
/**
* Gets a string literal that flows to a replace operation.
*/
DataFlow::SourceNode trackStringWithTemplateSyntax(CandidateStringLiteral lit) {
result = trackStringWithTemplateSyntax(lit, DataFlow::TypeTracker::end())
}
/**
* Holds if the string literal flows to a replace method call.
*/
predicate hasReplaceMethodCall(CandidateStringLiteral lit) {
trackStringWithTemplateSyntax(lit).getAMethodCall() instanceof StringReplaceCall
}
from CandidateStringLiteral lit, Variable v, Scope s, string name, VarDecl decl
where
decl = getDeclIn(v, s, name, lit.getTopLevel()) and
lit.getAReferencedVariable() = name and
lit.isInScope(s) and
not hasObjectProvidingTemplateVariables(lit) and
not lit.getStringValue() = "${" + name + "}"
not lit.getStringValue() = "${" + name + "}" and
not hasReplaceMethodCall(lit)
select lit, "This string is not a template literal, but appears to reference the variable $@.",
decl, v.getName()

View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Fixed false positives in the `js/template-syntax-in-string-literal` query where template syntax in string concatenation and "manual string interpolation" patterns were incorrectly flagged.

View File

@@ -0,0 +1,4 @@
---
category: queryMetadata
---
* Added `reliability` tag to the `js/suspicious-method-name-declaration` query.

View File

@@ -0,0 +1,4 @@
---
category: queryMetadata
---
* Added `reliability` and `language-features` tags to the `js/template-syntax-in-string-literal` query.

View File

@@ -0,0 +1,3 @@
- queries: .
- apply: code-quality-extended-selectors.yml
from: codeql/suite-helpers

View File

@@ -2,3 +2,14 @@
| tst.ts:16:3:16:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:37:3:37:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:48:3:48:13 | new(): Quz; | The member name 'new' does not declare a constructor, but 'constructor' does in class declarations. |
| tst.ts:60:3:60:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:64:3:64:24 | constru ... number; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |
| tst.ts:74:3:74:30 | functio ... string; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:75:3:75:30 | functio ... number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:76:3:76:24 | functio ... ): any; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:80:3:80:23 | abstrac ... : void; | The member name 'new' does not declare a constructor, but 'constructor' does in class declarations. |
| tst.ts:84:3:84:30 | abstrac ... number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:93:5:93:21 | function(): void; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:98:3:98:21 | function(): number; | The member name 'function' does not declare a function, it declares a method named 'function'. |
| tst.ts:110:3:110:24 | constru ... number; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |
| tst.ts:116:3:116:24 | constru ... number; | The member name 'constructor' does not declare a constructor in interfaces, but it does in classes. |

View File

@@ -50,3 +50,70 @@ declare class Quz {
var bla = new Foo();
var blab = new Baz();
interface X {
constructor: () => string; // Just a property, not a method.
}
type A = {
function(): number; // $ Alert
};
type B = {
constructor(): number; // $ Alert
new(): number;
};
class StaticMethods {
static function(): void {}
static new(): void {}
}
interface Overloaded {
function(x: string): string; // $Alert
function(x: number): number; // $Alert
function(x: any): any; // $Alert
}
abstract class AbstractFoo {
abstract new(): void; // $Alert
}
abstract class AbstractFooFunction {
abstract function(): number; // $Alert
}
abstract class AbstractFooConstructor {
constructor(){}
}
declare module "some-module" {
interface ModuleInterface {
function(): void; // $Alert
}
}
type Intersection = {
function(): number; // $Alert
} & {
other(): string;
};
type Union = {
new(): number;
} | {
valid(): string;
};
type Union2 = {
constructor(): number; // $Alert
} | {
valid(): string;
};
type Intersection2 = {
constructor(): number; // $Alert
} & {
other(): string;
};

View File

@@ -40,4 +40,30 @@ function foo1() {
writer.emit("Name: ${name}, Date: ${date}.", data);
writer.emit("Name: ${name}, Date: ${date}, ${foobar}", data); // $ Alert - `foobar` is not in `data`.
}
}
function a(actual, expected, description) {
assert(false, "a", description, "expected (" +
typeof expected + ") ${expected} but got (" + typeof actual + ") ${actual}", {
expected: expected,
actual: actual
});
}
function replacer(str, name) {
return str.replace("${name}", name);
}
function replacerAll(str, name) {
return str.replaceAll("${name}", name);
}
function manualInterpolation(name) {
let str = "Name: ${name}";
let result1 = replacer(str, name);
console.log(result1);
str = "Name: ${name} and again: ${name}";
let result2 = replacerAll(str, name);
console.log(result2);
}

View File

@@ -70,6 +70,8 @@
| tst2.js:76:12:76:18 | other.p | tst2.js:69:9:69:9 | p | tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to a $@. | tst2.js:69:9:69:9 | p | user-provided value |
| tst2.js:88:12:88:12 | p | tst2.js:82:9:82:9 | p | tst2.js:88:12:88:12 | p | Cross-site scripting vulnerability due to a $@. | tst2.js:82:9:82:9 | p | user-provided value |
| tst2.js:89:12:89:18 | other.p | tst2.js:82:9:82:9 | p | tst2.js:89:12:89:18 | other.p | Cross-site scripting vulnerability due to a $@. | tst2.js:82:9:82:9 | p | user-provided value |
| tst2.js:101:12:101:17 | unsafe | tst2.js:93:9:93:9 | p | tst2.js:101:12:101:17 | unsafe | Cross-site scripting vulnerability due to a $@. | tst2.js:93:9:93:9 | p | user-provided value |
| tst2.js:113:12:113:17 | unsafe | tst2.js:105:9:105:9 | p | tst2.js:113:12:113:17 | unsafe | Cross-site scripting vulnerability due to a $@. | tst2.js:105:9:105:9 | p | user-provided value |
| tst3.js:6:12:6:12 | p | tst3.js:5:9:5:9 | p | tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to a $@. | tst3.js:5:9:5:9 | p | user-provided value |
| tst3.js:12:12:12:15 | code | tst3.js:11:32:11:39 | reg.body | tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to a $@. | tst3.js:11:32:11:39 | reg.body | user-provided value |
edges
@@ -239,6 +241,22 @@ edges
| tst2.js:86:15:86:27 | sortKeys(obj) [p] | tst2.js:86:7:86:27 | other [p] | provenance | |
| tst2.js:86:24:86:26 | obj [p] | tst2.js:86:15:86:27 | sortKeys(obj) [p] | provenance | |
| tst2.js:89:12:89:16 | other [p] | tst2.js:89:12:89:18 | other.p | provenance | |
| tst2.js:93:7:93:24 | p | tst2.js:99:51:99:51 | p | provenance | |
| tst2.js:93:9:93:9 | p | tst2.js:93:7:93:24 | p | provenance | |
| tst2.js:99:7:99:69 | unsafe | tst2.js:101:12:101:17 | unsafe | provenance | |
| tst2.js:99:16:99:69 | seriali ... true}) | tst2.js:99:7:99:69 | unsafe | provenance | |
| tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | tst2.js:99:16:99:69 | seriali ... true}) | provenance | |
| tst2.js:99:51:99:51 | p | tst2.js:99:16:99:69 | seriali ... true}) | provenance | |
| tst2.js:99:51:99:51 | p | tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | provenance | |
| tst2.js:105:7:105:24 | p | tst2.js:110:28:110:28 | p | provenance | |
| tst2.js:105:9:105:9 | p | tst2.js:105:7:105:24 | p | provenance | |
| tst2.js:110:7:110:29 | obj [someProperty] | tst2.js:111:36:111:38 | obj [someProperty] | provenance | |
| tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | tst2.js:110:7:110:29 | obj [someProperty] | provenance | |
| tst2.js:110:28:110:28 | p | tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | provenance | |
| tst2.js:110:28:110:28 | p | tst2.js:111:16:111:55 | seriali ... true}) | provenance | |
| tst2.js:111:7:111:55 | unsafe | tst2.js:113:12:113:17 | unsafe | provenance | |
| tst2.js:111:16:111:55 | seriali ... true}) | tst2.js:111:7:111:55 | unsafe | provenance | |
| tst2.js:111:36:111:38 | obj [someProperty] | tst2.js:111:16:111:55 | seriali ... true}) | provenance | |
| tst3.js:5:7:5:24 | p | tst3.js:6:12:6:12 | p | provenance | |
| tst3.js:5:9:5:9 | p | tst3.js:5:7:5:24 | p | provenance | |
| tst3.js:11:9:11:74 | code | tst3.js:12:12:12:15 | code | provenance | |
@@ -457,6 +475,22 @@ nodes
| tst2.js:88:12:88:12 | p | semmle.label | p |
| tst2.js:89:12:89:16 | other [p] | semmle.label | other [p] |
| tst2.js:89:12:89:18 | other.p | semmle.label | other.p |
| tst2.js:93:7:93:24 | p | semmle.label | p |
| tst2.js:93:9:93:9 | p | semmle.label | p |
| tst2.js:99:7:99:69 | unsafe | semmle.label | unsafe |
| tst2.js:99:16:99:69 | seriali ... true}) | semmle.label | seriali ... true}) |
| tst2.js:99:36:99:52 | {someProperty: p} [someProperty] | semmle.label | {someProperty: p} [someProperty] |
| tst2.js:99:51:99:51 | p | semmle.label | p |
| tst2.js:101:12:101:17 | unsafe | semmle.label | unsafe |
| tst2.js:105:7:105:24 | p | semmle.label | p |
| tst2.js:105:9:105:9 | p | semmle.label | p |
| tst2.js:110:7:110:29 | obj [someProperty] | semmle.label | obj [someProperty] |
| tst2.js:110:13:110:29 | {someProperty: p} [someProperty] | semmle.label | {someProperty: p} [someProperty] |
| tst2.js:110:28:110:28 | p | semmle.label | p |
| tst2.js:111:7:111:55 | unsafe | semmle.label | unsafe |
| tst2.js:111:16:111:55 | seriali ... true}) | semmle.label | seriali ... true}) |
| tst2.js:111:36:111:38 | obj [someProperty] | semmle.label | obj [someProperty] |
| tst2.js:113:12:113:17 | unsafe | semmle.label | unsafe |
| tst3.js:5:7:5:24 | p | semmle.label | p |
| tst3.js:5:9:5:9 | p | semmle.label | p |
| tst3.js:6:12:6:12 | p | semmle.label | p |

View File

@@ -68,5 +68,7 @@
| tst2.js:76:12:76:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:69:9:69:9 | p | user-provided value |
| tst2.js:88:12:88:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:82:9:82:9 | p | user-provided value |
| tst2.js:89:12:89:18 | other.p | Cross-site scripting vulnerability due to $@. | tst2.js:82:9:82:9 | p | user-provided value |
| tst2.js:101:12:101:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:93:9:93:9 | p | user-provided value |
| tst2.js:113:12:113:17 | unsafe | Cross-site scripting vulnerability due to $@. | tst2.js:105:9:105:9 | p | user-provided value |
| tst3.js:6:12:6:12 | p | Cross-site scripting vulnerability due to $@. | tst3.js:5:9:5:9 | p | user-provided value |
| tst3.js:12:12:12:15 | code | Cross-site scripting vulnerability due to $@. | tst3.js:11:32:11:39 | reg.body | user-provided value |

View File

@@ -87,4 +87,28 @@ app.get('/baz', function(req, res) {
res.send(p); // $ Alert
res.send(other.p); // $ Alert
});
});
app.get('/baz', function(req, res) {
let { p } = req.params; // $ Source
var serialized = serializeJavaScript(p);
res.send(serialized);
var unsafe = serializeJavaScript({someProperty: p}, {unsafe: true});
res.send(unsafe); // $ Alert
});
app.get('/baz', function(req, res) {
let { p } = req.params; // $ Source
var serialized = serializeJavaScript(p);
res.send(serialized);
let obj = {someProperty: p};
var unsafe = serializeJavaScript(obj, {unsafe: true});
res.send(unsafe); // $ Alert
});