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.
ampproject · May 10, 2022 · 43b4e8f · 43b4e8f
1 parent 1a650fa
commit 43b4e8f
Show file tree

Hide file tree

Showing 4 changed files with 8 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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.', () => {