From 4ecfb215d78c0589ba1b4c7e8bc799df7d22e901 Mon Sep 17 00:00:00 2001 From: William Chou Date: Fri, 27 Jan 2017 15:05:35 -0500 Subject: [PATCH] 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 --- build-system/dep-check-config.js | 6 +- build-system/tasks/compile.js | 1 + extensions/amp-bind/0.1/bind-evaluator.js | 6 +- extensions/amp-bind/0.1/bind-validator.js | 99 +++++++++++++++---- .../amp-bind/0.1/test/test-bind-validator.js | 16 +++ src/sanitizer.js | 9 +- 6 files changed, 109 insertions(+), 28 deletions(-) diff --git a/build-system/dep-check-config.js b/build-system/dep-check-config.js index e0cb25b310d7..b55ffd15f1a7 100644 --- 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 index 076138db4e0d..0accd44abfd6 100644 --- 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 index 6636c18d6319..8b6db76f6f4f 100644 --- 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 index 8763c9442f1e..43920787691c 100644 --- 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|undefined), + * alternativeName: (string|undefined), * blockedURLs: (Array|undefined), * }} */ @@ -44,6 +50,16 @@ const GLOBAL_PROPERTY_RULES = { */ const ELEMENT_RULES = createElementRules_(); +/** + * Map whose keys comprise all properties that contain URLs. + * @private {Object} + */ +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,14 +164,14 @@ 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; @@ -114,15 +179,6 @@ export class BindValidator { } } - // @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 index 36f34b3d8a66..0c75bd0154aa 100644 --- 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 ', () => { @@ -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 index 0fabee559b83..b1f4d0e1b9dd 100644 --- 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); }