From 43b4e8f019d6dc0f9cb086d360be82a0214cb866 Mon Sep 17 00:00:00 2001
From: Justin Ridgewell <justin@ridgewell.name>
Date: Tue, 10 May 2022 15:04:00 -0400
Subject: [PATCH] Prevent expandTemplate from ReDOSing (#38178)

If the input string contains many `${` which do not have a closing `}`, the `expandTemplate`'s replace will iterate the entire string at every beginning mark.

Fixes #36108.
---
 extensions/amp-analytics/0.1/test/test-variables.js | 8 ++++----
 extensions/amp-analytics/0.1/variables.js           | 2 +-
 src/core/types/string/index.js                      | 2 +-
 test/unit/core/types/string/test-string.js          | 3 ++-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
index 467d1798a9a7..bebd262d25a1 100644
--- a/extensions/amp-analytics/0.1/test/test-variables.js
+++ b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -99,10 +99,10 @@ describes.fakeWin('amp-analytics.VariableService', {amp: true}, (env) => {
     });
 
     it('does not handle nested macros using ${} syntax', () => {
-      // VariableService.expandTemplate's regex cannot parse nested ${}.
-      return check('${a${b}}', '}', {
-        'a': 'TIMESTAMP',
-        'b': 'TIMESTAMP',
+      // VariableService.expandTemplate's regex cannot parse outer ${}.
+      return check('${a${b}}', '${atwo}', {
+        'a': 'one',
+        'b': 'two',
       });
     });
 
diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
index 907cea00cc35..91a329e90472 100644
--- a/extensions/amp-analytics/0.1/variables.js
+++ b/extensions/amp-analytics/0.1/variables.js
@@ -395,7 +395,7 @@ export class VariableService {
    * @return {!Promise<string>} The expanded string.
    */
   expandTemplate(template, options, element, opt_bindings, opt_allowlist) {
-    return asyncStringReplace(template, /\${([^}]*)}/g, (match, key) => {
+    return asyncStringReplace(template, /\${([^{}]*)}/g, (match, key) => {
       if (options.iterations < 0) {
         user().error(
           TAG,
diff --git a/src/core/types/string/index.js b/src/core/types/string/index.js
index 75793259b4b9..e4ce03e36167 100644
--- a/src/core/types/string/index.js
+++ b/src/core/types/string/index.js
@@ -89,7 +89,7 @@ export function expandTemplate(template, getter, opt_maxIterations) {
   const maxIterations = opt_maxIterations || 1;
   for (let i = 0; i < maxIterations; i++) {
     let matches = 0;
-    template = template.replace(/\${([^}]*)}/g, (_a, b) => {
+    template = template.replace(/\${([^{}]*)}/g, (_a, b) => {
       matches++;
       return getter(b);
     });
diff --git a/test/unit/core/types/string/test-string.js b/test/unit/core/types/string/test-string.js
index 6387c4ffc72b..77cd526923f1 100644
--- a/test/unit/core/types/string/test-string.js
+++ b/test/unit/core/types/string/test-string.js
@@ -89,7 +89,8 @@ describes.sandboxed('type helpers - strings', {}, () => {
       expect(expandTemplate('$x}', testGetter)).to.equal('$x}');
       expect(expandTemplate('$x', testGetter)).to.equal('$x');
       expect(expandTemplate('{x}', testGetter)).to.equal('{x}');
-      expect(expandTemplate('${{x}', testGetter)).to.equal('not found');
+      expect(expandTemplate('${{x}', testGetter)).to.equal('${{x}');
+      expect(expandTemplate('${${x}', testGetter)).to.equal('${Test 1');
     });
 
     it('should default to one iteration.', () => {