From 1d9047df0fe2fdea5bbcf34079c8aca0c7ffe57b Mon Sep 17 00:00:00 2001
From: Pei Wang <pwng@google.com>
Date: Thu, 28 Jan 2021 13:57:25 -0800
Subject: [PATCH] Avoid matching polyfilled global names in NameEngine.

Fix #22.

PiperOrigin-RevId: 354387758
Change-Id: I659f36dd0eb608aa252a908f6271a2fafd126191
---
 .../tsetse/util/absolute_matcher.ts           |  7 ------
 src/third_party/tsetse/util/ast_tools.ts      | 15 +++++++++++++
 .../util/pattern_engines/name_engine.ts       | 22 +++++++++++++++----
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/src/third_party/tsetse/util/absolute_matcher.ts b/src/third_party/tsetse/util/absolute_matcher.ts
index 6d5d25d..66ac8ce 100644
--- a/src/third_party/tsetse/util/absolute_matcher.ts
+++ b/src/third_party/tsetse/util/absolute_matcher.ts
@@ -111,13 +111,6 @@ export class AbsoluteMatcher {
     debugLog(() => `start matching ${n.getText()} in ${p?.getText()}`);
 
     if (p !== undefined) {
-      // Do not match global symbols appearing on the LHS of an assignment
-      // expression. This avoids false positives in polyfill code.
-      if (this.filePath === GLOBAL && ts.isBinaryExpression(p) &&
-          p.left === n && p.operatorToken.kind === ts.SyntaxKind.EqualsToken) {
-        return false;
-      }
-
       // Check if the node is being declared. Declaration may be imported
       // without programmer being aware of. We should not alert them about that.
       // Since import statments are also declarations, this have two notable
diff --git a/src/third_party/tsetse/util/ast_tools.ts b/src/third_party/tsetse/util/ast_tools.ts
index 98fecfc..9bf9548 100644
--- a/src/third_party/tsetse/util/ast_tools.ts
+++ b/src/third_party/tsetse/util/ast_tools.ts
@@ -70,6 +70,21 @@ export function shouldExamineNode(n: ts.Node) {
       isInStockLibraries(n));
 }
 
+/**
+ * Returns the top-level property access expression (or element access
+ * expression) that the input node is part of.
+ *
+ * For example, given an expression `c` which is part of `a['b'].c = 1;`,
+ * the function returns the whole LHS expression `a['b'].c`.
+ */
+export function walkUpPropertyAndElementAccess(n: ts.Node): ts.Node {
+  while (ts.isPropertyAccessExpression(n.parent) ||
+         ts.isElementAccessExpression(n.parent)) {
+    n = n.parent;
+  }
+  return n;
+}
+
 /**
  * Return whether the given Node is (or is in) a library included as default.
  * We currently look for a node_modules/typescript/ prefix, but this could
diff --git a/src/third_party/tsetse/util/pattern_engines/name_engine.ts b/src/third_party/tsetse/util/pattern_engines/name_engine.ts
index 7119bfd..11266ef 100644
--- a/src/third_party/tsetse/util/pattern_engines/name_engine.ts
+++ b/src/third_party/tsetse/util/pattern_engines/name_engine.ts
@@ -2,6 +2,7 @@ import * as ts from 'typescript';
 
 import {Checker} from '../../checker';
 import {AbsoluteMatcher} from '../absolute_matcher';
+import {walkUpPropertyAndElementAccess} from '../ast_tools';
 import {isExpressionOfAllowedTrustedType} from '../is_trusted_type';
 import {TrustedTypesConfig} from '../trusted_types_configuration';
 
@@ -21,19 +22,32 @@ function isCalledWithAllowedTrustedType(
   return false;
 }
 
+function isPolyfill(n: ts.Node, matcher: AbsoluteMatcher) {
+  if (matcher.filePath === 'GLOBAL') {
+    const wholeExp = walkUpPropertyAndElementAccess(n);
+    const parent = wholeExp.parent;
+    if (parent && ts.isBinaryExpression(parent) && parent.left === wholeExp &&
+        parent.operatorToken.kind === ts.SyntaxKind.EqualsToken) {
+      return true;
+    }
+  }
+  return false;
+}
+
 function checkIdentifierNode(
     tc: ts.TypeChecker, n: ts.Identifier, matcher: AbsoluteMatcher,
     allowedTrustedType: TrustedTypesConfig|undefined): ts.Node|undefined {
+  if (isPolyfill(n, matcher)) return;
   if (!matcher.matches(n, tc)) return;
   if (isCalledWithAllowedTrustedType(tc, n, allowedTrustedType)) return;
 
-
   return n;
 }
 
 function checkElementAccessNode(
     tc: ts.TypeChecker, n: ts.ElementAccessExpression, matcher: AbsoluteMatcher,
     allowedTrustedType: TrustedTypesConfig|undefined): ts.Node|undefined {
+  if (isPolyfill(n, matcher)) return;
   if (!matcher.matches(n.argumentExpression, tc)) return;
   if (isCalledWithAllowedTrustedType(tc, n, allowedTrustedType)) return;
 
@@ -46,9 +60,9 @@ export class NameEngine extends PatternEngine {
     for (const value of this.config.values) {
       const matcher = new AbsoluteMatcher(value);
 
-      // `String.prototype.split` only returns emtpy array when both the string
-      // and the splitter are empty. Here we should be able to safely assert pop
-      // returns a non-null result.
+      // `String.prototype.split` only returns emtpy array when both the
+      // string and the splitter are empty. Here we should be able to safely
+      // assert pop returns a non-null result.
       const bannedIdName = matcher.bannedName.split('.').pop()!;
       checker.onNamedIdentifier(
           bannedIdName,