Add a new matcher (nth argument of a call to foo is literal) to Tsetse.
This literalness constraint is useful as a proxy for "no user data should be in that argument", for instance for security purposes.
PiperOrigin-RevId: 261307072
diff --git a/internal/tsetse/rules/conformance_pattern_rule.ts b/internal/tsetse/rules/conformance_pattern_rule.ts
index 677490d..0cbfe71 100644
--- a/internal/tsetse/rules/conformance_pattern_rule.ts
+++ b/internal/tsetse/rules/conformance_pattern_rule.ts
@@ -3,11 +3,13 @@
import {AbstractRule} from '../rule';
import {Fixer} from '../util/fixer';
import {Config, PatternKind} from '../util/pattern_config';
+import {CallNonConstantArgumentEngine} from '../util/pattern_engines/name_call_non_constant_argument';
import {NameEngine} from '../util/pattern_engines/name_engine';
import {PatternEngine} from '../util/pattern_engines/pattern_engine';
import {PropertyNonConstantWriteEngine} from '../util/pattern_engines/property_non_constant_write_engine';
import {PropertyWriteEngine} from '../util/pattern_engines/property_write_engine';
+
/**
* Builds a Rule that matches a certain pattern, given as parameter, and
* that can additionally run a suggested fix generator on the matches.
@@ -32,6 +34,9 @@
case PatternKind.BANNED_NAME:
this.engine = new NameEngine(config, fixer);
break;
+ case PatternKind.BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT:
+ this.engine = new CallNonConstantArgumentEngine(config, fixer);
+ break;
default:
throw new Error('Config type not recognized, or not implemented yet.');
}
diff --git a/internal/tsetse/tests/ban_conformance_pattern/name_call_non_constant_argument_test.ts b/internal/tsetse/tests/ban_conformance_pattern/name_call_non_constant_argument_test.ts
new file mode 100644
index 0000000..44c9a31
--- /dev/null
+++ b/internal/tsetse/tests/ban_conformance_pattern/name_call_non_constant_argument_test.ts
@@ -0,0 +1,116 @@
+import 'jasmine';
+import {ConformancePatternRule, PatternKind} from '../../rules/conformance_pattern_rule';
+import {compileAndCheck, customMatchers} from '../../util/testing/test_support';
+
+describe('BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT', () => {
+ const rule = new ConformancePatternRule({
+ errorMessage: 'do not call bar.foo with non-literal 1st arg',
+ kind: PatternKind.BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT,
+ values: ['bar:0']
+ });
+
+ it('matches simple examples', () => {
+ const sources = [
+ `export function bar(x:any, y:any) {}`,
+ `import * as foo from './file_0'; ` +
+ `foo.bar(1, 1); foo.bar(window.name, 1);`,
+ ];
+ const results = compileAndCheck(rule, ...sources);
+
+ expect(results.length).toBe(1);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `foo.bar(window.name, 1)`,
+ messageText: 'do not call bar.foo with non-literal 1st arg'
+ });
+ });
+
+ it('looks at the right position', () => {
+ const sources = [
+ `export function bar(x:any, y:any) {}`,
+ `import * as foo from './file_0'; foo.bar(1, window.name);`,
+ ];
+ const results = compileAndCheck(rule, ...sources);
+
+ expect(results.length).toBe(0);
+ });
+
+ it('looks at the right position', () => {
+ const rule = new ConformancePatternRule({
+ errorMessage: 'non-literal arg',
+ kind: PatternKind.BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT,
+ values: ['aaa:1', 'bbb:0']
+ });
+
+ const sources = [
+ `export function aaa(x:any, y:any) {}; export function bbb(x:any) {}`,
+ `import * as foo from './file_0'; ` +
+ `foo.aaa(1, window.name); foo.bbb(window.name);`,
+ ];
+ const results = compileAndCheck(rule, ...sources);
+
+ expect(results.length).toBe(2);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `foo.aaa(1, window.name)`,
+ });
+ expect(results[1]).toBeFailureMatching({
+ matchedCode: `foo.bbb(window.name)`,
+ });
+ });
+
+ it('supports static methods', () => {
+ const rule = new ConformancePatternRule({
+ errorMessage: 'non-literal arg',
+ kind: PatternKind.BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT,
+ values: ['Car.buildFromParts:0']
+ });
+
+ const sources = [
+ `export class Car { static buildFromParts(name:string):void {}; }`,
+ `import {Car} from './file_0';\n` +
+ `Car.buildFromParts(window.name);\n` +
+ `Car.buildFromParts('hello');`,
+ ];
+ const results = compileAndCheck(rule, ...sources);
+
+ expect(results.length).toBe(1);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `Car.buildFromParts(window.name)`,
+ });
+ });
+
+ it('supports ambient global methods', () => {
+ const rule = new ConformancePatternRule({
+ errorMessage: 'non-literal arg',
+ kind: PatternKind.BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT,
+ values: ['URL.createObjectURL:0']
+ });
+
+ const sources = [`URL.createObjectURL(window.name);\n`];
+ const results = compileAndCheck(rule, ...sources);
+
+ expect(results.length).toBe(1);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `URL.createObjectURL(window.name)`,
+ });
+ });
+
+ it('supports ambient global methods', () => {
+ const rule = new ConformancePatternRule({
+ errorMessage: 'non-literal arg',
+ kind: PatternKind.BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT,
+ values: ['eval:0']
+ });
+
+ const sources = [`eval(window.name);\n`];
+ const results = compileAndCheck(rule, ...sources);
+
+ expect(results.length).toBe(1);
+ expect(results[0]).toBeFailureMatching({
+ matchedCode: `eval(window.name)`,
+ });
+ });
+});
+
+beforeEach(() => {
+ jasmine.addMatchers(customMatchers);
+});
diff --git a/internal/tsetse/util/match_symbol.ts b/internal/tsetse/util/match_symbol.ts
index c60195b..eb588e8 100644
--- a/internal/tsetse/util/match_symbol.ts
+++ b/internal/tsetse/util/match_symbol.ts
@@ -15,6 +15,11 @@
* Note that this isn't smart about subclasses and types: to write a check, we
* strongly suggest finding the expected symbol in externs to find the object
* name on which the symbol was initially defined.
+ *
+ * TODO(rjamet): add a file-based optional filter, since FQNs tell you where
+ * your imported symbols were initially defined. That would let us be more
+ * specific in matches (say, you want to ban the fromLiteral in foo.ts but not
+ * the one from bar.ts).
*/
export class AbsoluteMatcher {
/**
@@ -32,10 +37,11 @@
// on `foo`. To avoid any confusion, throw there if we see `prototype` in
// the spec: that way, it's obvious that you're not trying to match
// properties.
- if (this.bannedName.includes('.prototype')) {
+ if (this.bannedName.match('.prototype.')) {
throw new Error(
'Your pattern includes a .prototype, but the AbsoluteMatcher is ' +
- 'meant for non-object matches. Use the PropertyMatcher instead.');
+ 'meant for non-object matches. Use the PropertyMatcher instead, or ' +
+ 'the Property-based PatternKinds.');
}
}
diff --git a/internal/tsetse/util/pattern_config.ts b/internal/tsetse/util/pattern_config.ts
index 21e9c25..bc907b1 100644
--- a/internal/tsetse/util/pattern_config.ts
+++ b/internal/tsetse/util/pattern_config.ts
@@ -7,7 +7,9 @@
export const enum PatternKind {
BANNED_NAME = 'banned-name',
BANNED_PROPERTY_WRITE = 'banned-property-write',
- BANNED_PROPERTY_NON_CONSTANT_WRITE = 'banned-property-non-constant-write'
+ BANNED_PROPERTY_NON_CONSTANT_WRITE = 'banned-property-non-constant-write',
+ // Not from JSConformance.
+ BANNED_NAME_CALL_NON_CONSTANT_ARGUMENT = 'banned-call-non-constant-argument'
}
/**
@@ -19,8 +21,8 @@
/**
* Values have a pattern-specific syntax.
*
- * TODO(rjamet): We'll document them, but for now see each patternKind's tests
- * for examples.
+ * TODO(rjamet): We'll document them, but for now see each patternKind's
+ * tests for examples.
*/
values: string[];
@@ -32,13 +34,13 @@
}
/**
- * A whitelist entry, corresponding to a logical whitelisting rule. Use these to
- * distinguish between various logical reasons for whitelisting something: for
- * instance, tie these to particular bugs that needed whitelisting, per legacy
- * project, manually reviewed entries, and so on.
+ * A whitelist entry, corresponding to a logical whitelisting rule. Use these
+ * to distinguish between various logical reasons for whitelisting something:
+ * for instance, tie these to particular bugs that needed whitelisting, per
+ * legacy project, manually reviewed entries, and so on.
*
- * Whitelists are based on the file paths provided by the TS compiler, with both
- * regexp-based checks and prefix-based checks.
+ * Whitelists are based on the file paths provided by the TS compiler, with
+ * both regexp-based checks and prefix-based checks.
*
*
* Follows the logic in
diff --git a/internal/tsetse/util/pattern_engines/name_call_non_constant_argument.ts b/internal/tsetse/util/pattern_engines/name_call_non_constant_argument.ts
new file mode 100644
index 0000000..1af786d
--- /dev/null
+++ b/internal/tsetse/util/pattern_engines/name_call_non_constant_argument.ts
@@ -0,0 +1,86 @@
+import * as ts from 'typescript';
+import {Checker} from '../../checker';
+import {ErrorCode} from '../../error_code';
+import {debugLog} from '../ast_tools';
+import {Fixer} from '../fixer';
+import {isLiteral} from '../is_literal';
+import {AbsoluteMatcher} from '../match_symbol';
+import {Config} from '../pattern_config';
+import {PatternEngine} from './pattern_engine';
+
+/**
+ * The engine for BANNED_CALL_NON_CONSTANT_ARGUMENT.
+ *
+ * This takes any amount of (functionName, argument) position pairs, separated
+ * by a colon. The first part matches symbols that were defined on the global
+ * scope, and their fields, without going through a prototype chain.
+ *
+ * For instance, "URL.createObjectURL:0" will target any createObjectURL-named
+ * call on a URL-named object (like the ambient URL declared in lib.dom.d.ts),
+ * or "Car.buildFromParts:1" will match any buildFromParts reached from a
+ * Car-named symbol, including a hypothetical class with a static member
+ * function "buildFromParts" that lives in its own module.
+ */
+export class CallNonConstantArgumentEngine extends PatternEngine {
+ private readonly matchers: Array<[AbsoluteMatcher, number]> = [];
+
+ constructor(config: Config, fixer?: Fixer) {
+ super(config, fixer);
+ for (const v of config.values) {
+ const [matcherSpec, strPosition] = v.split(':', 2);
+ if (!matcherSpec || !strPosition.match('^\\d+$')) {
+ throw new Error('Couldn\'t parse values');
+ }
+ const position = Number(strPosition);
+ this.matchers.push([new AbsoluteMatcher(matcherSpec), position]);
+ }
+ }
+
+ register(checker: Checker) {
+ checker.on(
+ ts.SyntaxKind.CallExpression, this.checkAndFilterResults.bind(this),
+ ErrorCode.CONFORMANCE_PATTERN);
+ }
+
+ check(tc: ts.TypeChecker, n: ts.Node): ts.Node|undefined {
+ if (!ts.isCallExpression(n)) {
+ debugLog(`Should not happen: node is not a CallExpression`);
+ return;
+ }
+ debugLog(`inspecting ${n.getText().trim()}`);
+
+ /**
+ * Inspects a particular CallExpression to see if it calls the target
+ * function with a non-literal parameter in the target position. Returns
+ * that CallExpression if `n` matches the search, undefined otherwise.
+ */
+ function checkIndividual(
+ n: ts.CallExpression, m: [AbsoluteMatcher, number]): ts.CallExpression|
+ undefined {
+ if (!m[0].matches(n.expression, tc)) {
+ debugLog(`Wrong symbol, not ${m[0].bannedName}`);
+ return;
+ }
+ if (n.arguments.length < m[1]) {
+ debugLog(`Good symbol, not enough arguments to match (got ${
+ n.arguments.length}, want ${m[1]})`);
+ return;
+ }
+ if (isLiteral(tc, n.arguments[m[1]])) {
+ debugLog(`Good symbol, argument literal`);
+ return;
+ }
+ debugLog(`Match. Reporting failure (boundaries: ${n.getStart()}, ${
+ n.getEnd()}] on node [${n.getText()}]`);
+ return n;
+ }
+
+ for (const m of this.matchers) {
+ // The first matching matcher will be used.
+ const r = checkIndividual(n, m);
+ if (r) return r;
+ }
+ // No match.
+ return;
+ }
+}
