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; + } +}