Add URL rewriting and srcset support to amp-bind (#7206)

* add srcset support * fix types * rewrite urls for cache * ignore compiler warnings in third_party/caja * whitelist amp-bind to depend on sanitizer.js * ali's comments
ampproject · Jan 27, 2017 · 4ecfb21 · 4ecfb21
1 parent f33b581
commit 4ecfb21
Show file tree

Hide file tree

Showing 6 changed files with 109 additions and 28 deletions.
diff --git a/build-system/dep-check-config.js b/build-system/dep-check-config.js
@@ -37,8 +37,10 @@ exports.rules = [
   {
     filesMatching: '**/*.js',
     mustNotDependOn: 'src/sanitizer.js',
-    whitelist: 'extensions/amp-mustache/0.1/amp-mustache.js->' +
-        'src/sanitizer.js',
+    whitelist: [
+      'extensions/amp-mustache/0.1/amp-mustache.js->src/sanitizer.js',
+      'extensions/amp-bind/0.1/bind-evaluator.js->src/sanitizer.js',
+    ],
   },
   {
     filesMatching: '**/*.js',

diff --git a/build-system/tasks/compile.js b/build-system/tasks/compile.js
@@ -261,6 +261,7 @@ function compile(entryModuleFilenames, outputDir,
         jscomp_off: ['unknownDefines'],
         define: [],
         hide_warnings_for: [
+          'third_party/caja/',
           'third_party/closure-library/sha384-generated.js',
           'third_party/d3/',
           'third_party/vega/',

diff --git a/extensions/amp-bind/0.1/bind-evaluator.js b/extensions/amp-bind/0.1/bind-evaluator.js
@@ -16,6 +16,7 @@
 
 import {BindExpression} from './bind-expression';
 import {BindValidator} from './bind-validator';
+import {rewriteAttributeValue} from '../../../src/sanitizer';
 import {user} from '../../../src/log';
 
 const TAG = 'AMP-BIND';
@@ -107,7 +108,10 @@ export class BindEvaluator {
 
         const resultString = this.stringValueOf_(property, result);
         if (this.validator_.isResultValid(tagName, property, resultString)) {
-          cache[expr] = result;
+          // Rewrite URL attributes for CDN if necessary.
+          cache[expr] = typeof result === 'string'
+              ? rewriteAttributeValue(tagName, property, result)
+              : result;
         } else {
           invalid[expr] = true;
         }

diff --git a/extensions/amp-bind/0.1/bind-validator.js b/extensions/amp-bind/0.1/bind-validator.js
@@ -14,9 +14,15 @@
  * limitations under the License.
  */
 
+import {parseSrcset} from '../../../src/srcset';
+import {user} from '../../../src/log';
+
+const TAG = 'amp-bind';
+
 /**
  * @typedef {{
  *   allowedProtocols: (!Object<string,boolean>|undefined),
+ *   alternativeName: (string|undefined),
  *   blockedURLs: (Array<string>|undefined),
  * }}
  */
@@ -44,6 +50,16 @@ const GLOBAL_PROPERTY_RULES = {
  */
 const ELEMENT_RULES = createElementRules_();
 
+/**
+ * Map whose keys comprise all properties that contain URLs.
+ * @private {Object<string, boolean>}
+ */
+const URL_PROPERTIES = {
+  'src': true,
+  'srcset': true,
+  'href': true,
+};
+
 /**
  * BindValidator performs runtime validation of Bind expression results.
  *
@@ -72,23 +88,72 @@ export class BindValidator {
    * @return {boolean}
    */
   isResultValid(tag, property, value) {
-    const attrRules = this.rulesForTagAndProperty_(tag, property);
+    let rules = this.rulesForTagAndProperty_(tag, property);
+
+    // `alternativeName` is a reference to another property's rules.
+    if (rules && rules.alternativeName) {
+      rules = this.rulesForTagAndProperty_(tag, rules.alternativeName);
+    }
 
     // If binding to (tag, property) is not allowed, return false.
-    if (attrRules === undefined) {
+    if (rules === undefined) {
       return false;
     }
 
     // If binding is allowed but have no specific rules, return true.
-    if (attrRules === null) {
+    if (rules === null) {
       return true;
     }
 
+    // Validate URL(s) if applicable.
+    if (value && URL_PROPERTIES.hasOwnProperty(property)) {
+      let urls;
+      if (property === 'srcset') {
+        let srcset;
+        try {
+          srcset = parseSrcset(value);
+        } catch (e) {
+          user().error(TAG, 'Failed to parse srcset: ', e);
+          return false;
+        }
+        const sources = srcset.getSources();
+        urls = sources.map(source => source.url);
+      } else {
+        urls = [value];
+      }
+
+      for (let i = 0; i < urls.length; i++) {
+        if (!this.isUrlValid_(urls[i], rules)) {
+          return false;
+        }
+      }
+    }
+
+    // @see validator/engine/validator.ParsedTagSpec.validateAttributes()
+    const blacklistedValueRegex = rules.blacklistedValueRegex;
+    if (value && blacklistedValueRegex) {
+      const re = new RegExp(blacklistedValueRegex, 'i');
+      if (re.test(value)) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+  /**
+   * Returns true if a url's value is valid within a property rules spec.
+   * @param {string} url
+   * @param {!PropertyRulesDef} rules
+   * @return {boolean}
+   * @private
+   */
+  isUrlValid_(url, rules) {
     // @see validator/engine/validator.ParsedUrlSpec.validateUrlAndProtocol()
-    const allowedProtocols = attrRules.allowedProtocols;
-    if (allowedProtocols && value) {
+    const allowedProtocols = rules.allowedProtocols;
+    if (allowedProtocols && url) {
       const re = /^([^:\/?#.]+):[\s\S]*$/;
-      const match = re.exec(value);
+      const match = re.exec(url);
 
       if (match !== null) {
         const protocol = match[1].toLowerCase().trimLeft();
@@ -99,30 +164,21 @@ export class BindValidator {
     }
 
     // @see validator/engine/validator.ParsedTagSpec.validateAttributes()
-    const blockedURLs = attrRules.blockedURLs;
-    if (blockedURLs && value) {
+    const blockedURLs = rules.blockedURLs;
+    if (blockedURLs && url) {
       for (let i = 0; i < blockedURLs.length; i++) {
         let decodedURL;
         try {
-          decodedURL = decodeURIComponent(value);
+          decodedURL = decodeURIComponent(url);
         } catch (e) {
-          decodedURL = unescape(value);
+          decodedURL = unescape(url);
         }
         if (decodedURL.trim() === blockedURLs[i]) {
           return false;
         }
       }
     }
 
-    // @see validator/engine/validator.ParsedTagSpec.validateAttributes()
-    const blacklistedValueRegex = attrRules.blacklistedValueRegex;
-    if (blacklistedValueRegex && value) {
-      const re = new RegExp(blacklistedValueRegex, 'i');
-      if (re.test(value)) {
-        return false;
-      }
-    }
-
     return true;
   }
 
@@ -171,7 +227,9 @@ function createElementRules_() {
         },
         blockedURLs: ['__amp_source_origin'],
       },
-      // TODO: Support `srcset`.
+      srcset: {
+        alternativeName: 'src',
+      },
     },
     'AMP-SELECTOR': {
       selected: null,
@@ -192,7 +250,6 @@ function createElementRules_() {
         },
         blockedURLs: ['__amp_source_origin'],
       },
-      // TODO: Support `srcset`.
     },
     A: {
       href: {

diff --git a/extensions/amp-bind/0.1/test/test-bind-validator.js b/extensions/amp-bind/0.1/test/test-bind-validator.js
@@ -186,12 +186,27 @@ describe('BindValidator', () => {
       expect(val.canBind('AMP-IMG', 'width')).to.be.true;
       expect(val.canBind('AMP-IMG', 'height')).to.be.true;
 
+      // src
       expect(val.isResultValid(
           'AMP-IMG', 'src', 'http://foo.com/bar.jpg')).to.be.true;
       expect(val.isResultValid('AMP-IMG', 'src',
           /* eslint no-script-url: 0 */ 'javascript:alert(1)\n;')).to.be.false;
       expect(val.isResultValid(
           'AMP-IMG', 'src', '__amp_source_origin')).to.be.false;
+
+      // srcset
+      expect(val.isResultValid(
+          'AMP-IMG',
+          'srcset',
+          'http://a.com/b.jpg 1x, http://c.com/d.jpg 2x')).to.be.true;
+      expect(val.isResultValid(
+          'AMP-IMG',
+          'srcset',
+          'http://a.com/b.jpg 1x, __amp_source_origin 2x')).to.be.false;
+      expect(val.isResultValid(
+          'AMP-IMG',
+          'srcset',
+          /* eslint no-script-url: 0 */ 'javascript:alert(1)\n;')).to.be.false;
     });
 
     it('should support <amp-selector>', () => {
@@ -203,6 +218,7 @@ describe('BindValidator', () => {
       expect(val.canBind('AMP-VIDEO', 'poster')).to.be.true;
       expect(val.canBind('AMP-VIDEO', 'src')).to.be.true;
 
+      // src
       expect(val.isResultValid(
           'AMP-VIDEO', 'src', 'https://foo.com/bar.mp4')).to.be.true;
       expect(val.isResultValid(

diff --git a/src/sanitizer.js b/src/sanitizer.js
@@ -254,7 +254,7 @@ export function sanitizeHtml(html) {
         emit(attrName);
         emit('="');
         if (attrValue) {
-          emit(htmlSanitizer.escapeAttrib(resolveAttrValue(
+          emit(htmlSanitizer.escapeAttrib(rewriteAttributeValue(
               tagName, attrName, attrValue)));
         }
         emit('"');
@@ -350,14 +350,15 @@ export function isValidAttr(tagName, attrName, attrValue) {
 }
 
 /**
- * Resolves the attribute value. The main purpose is to rewrite URLs as
- * described in `resolveUrlAttr`.
+ * If (tagName, attrName) is a CDN-rewritable URL attribute, returns the
+ * rewritten URL value. Otherwise, returns the unchanged `attrValue`.
+ * @see resolveUrlAttr for rewriting rules.
  * @param {string} tagName
  * @param {string} attrName
  * @param {string} attrValue
  * @return {string}
  */
-function resolveAttrValue(tagName, attrName, attrValue) {
+export function rewriteAttributeValue(tagName, attrName, attrValue) {
   if (attrName == 'src' || attrName == 'href' || attrName == 'srcset') {
     return resolveUrlAttr(tagName, attrName, attrValue, self.location);
   }