amp-bind: Throw user error when replacing non-whitelisted URL vars #13081

dreamofabear · 2018-01-27T05:03:17Z

Step 1 for #12498.

Throw user error when performing non-whitelisted URL var replacement via amp-bind
Default to no replacement with new optional param opt_urlReplacement
Add test-batched-json.js file
Rename fetchBatchedJsonFor to batchFetchJsonFor

Aside: I think we should merge batched-json.js into batched-xhr.js. It'll also make stubbing the function possible.

dreamofabear · 2018-01-29T17:43:22Z

alabiaga · 2018-01-29T17:57:27Z

src/service/url-replacements-impl.js

+   * @return {!Promise<!Array<string>>}
+   */
+  collectUnwhitelistedVars(element) {
+    const url = element.getAttribute('src');


const url only used once, just inline?

I'd prefer this for readability and debuggability.

alabiaga · 2018-01-29T17:58:16Z

src/service/url-replacements-impl.js

+    const url = element.getAttribute('src');
+    return this.collectVars(url).then(vars => {
+      const whitelist = this.getWhitelistForElement_(element);
+      return Object.keys(vars).filter(v => !whitelist[v]);


just inline?

Had to change this to account for whitelist == undefined case.

aghassemi · 2018-01-30T20:14:42Z

extensions/amp-list/0.1/amp-list.js

-    return fetchBatchedJsonFor(this.getAmpDoc(), this.element, itemsExpr);
+    // Require opt-in for URL variable replacements on fetches triggered
+    // by [src] mutation. @see spec/amp-var-substitutions.md
+    const policy = (this.element.getAttribute('src') == this.initialSrc_)


Can we just compare the origin's and allow path and hash to differ? That would make it less of a breaking change and I don't see particular security concerns if origins match, but I might be wrong.

You're right that this only matters for requests to non-pub-origins but I thought that would too specific to message gracefully. I'll try it out though.

This is done.

aghassemi · 2018-01-30T20:19:25Z

src/batched-json.js

+/**
+ * @enum {number}
+ */
+export const UrlReplacementPolicy = {


nit: Maybe FetchUrlReplacementPolicy since the policy does not apply to other cases?

I think we don't need to disambiguate here since none/opt-in/all does fit the system in general. E.g. <input> and <a> tags currently require opt-in, and some URL attributes don't support substitutions at all.

aghassemi · 2018-01-30T20:21:35Z

src/batched-json.js

+
+  // Replace vars in URL if desired.
+  const urlReplacements = Services.urlReplacementsForDoc(ampdoc);
+  const srcPromise = (opt_urlReplacement >= UrlReplacementPolicy.OPT_IN)


In the none case, should they be replaced with ``? leaving them as is would generate strange and wrong Urls.

Leaving it unmodified is the default behavior for other opt-ins, e.g. <a>.

cramforce · 2018-02-01T17:15:59Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Syntax error

Comment posted by lgtm.com

cramforce · 2018-02-01T19:02:58Z

This pull request introduces 1 alert - view on lgtm.com

new alerts:

1 for Syntax error

Comment posted by lgtm.com

dreamofabear

Thanks for the comments everyone.

dreamofabear · 2018-02-01T18:52:33Z

src/service/url-replacements-impl.js

+   * @return {!Promise<!Array<string>>}
+   */
+  collectUnwhitelistedVars(element) {
+    const url = element.getAttribute('src');


I'd prefer this for readability and debuggability.

dreamofabear · 2018-02-01T18:52:51Z

src/service/url-replacements-impl.js

+    const url = element.getAttribute('src');
+    return this.collectVars(url).then(vars => {
+      const whitelist = this.getWhitelistForElement_(element);
+      return Object.keys(vars).filter(v => !whitelist[v]);


Had to change this to account for whitelist == undefined case.

dreamofabear · 2018-02-01T18:53:10Z

extensions/amp-list/0.1/amp-list.js

-    return fetchBatchedJsonFor(this.getAmpDoc(), this.element, itemsExpr);
+    // Require opt-in for URL variable replacements on fetches triggered
+    // by [src] mutation. @see spec/amp-var-substitutions.md
+    const policy = (this.element.getAttribute('src') == this.initialSrc_)


This is done.

dreamofabear · 2018-02-01T19:17:50Z

src/batched-json.js

+
+  // Replace vars in URL if desired.
+  const urlReplacements = Services.urlReplacementsForDoc(ampdoc);
+  const srcPromise = (opt_urlReplacement >= UrlReplacementPolicy.OPT_IN)


Leaving it unmodified is the default behavior for other opt-ins, e.g. <a>.

dreamofabear · 2018-02-01T19:19:29Z

src/batched-json.js

+/**
+ * @enum {number}
+ */
+export const UrlReplacementPolicy = {


I think we don't need to disambiguate here since none/opt-in/all does fit the system in general. E.g. <input> and <a> tags currently require opt-in, and some URL attributes don't support substitutions at all.

dreamofabear · 2018-02-01T19:40:07Z

spec/amp-var-substitutions.md

@@ -48,7 +48,7 @@ The following table lists the features that enable variable substitutions, as we
  <tr>
    <td width="25%"><code>amp-list</code><br><a href="https://github.com/ampproject/amphtml/blob/master/extensions/amp-list/amp-list.md#substitutions">Detailed documentation</a></td>
    <td width="25%">Requests must be HTTPS URLs (not a requirement specific to variable substitutions)</td>
-    <td width="25%">No</td>
+    <td width="25%">Yes, if fetching cross-origin resources via <code>[src]</code> <a href="https://www.ampproject.org/docs/reference/components/amp-bind#element-specific-attributes">attribute binding</a>. Otherwise, no. Read more about <a href="#per-use-opt-in">per-use opt-in</a></td>


@bpaduch Would like your input on whether this is confusing or not.

dreamofabear · 2018-02-01T19:41:18Z

src/batched-json.js

+          user().error(TAG, 'URL variable substitutions in CORS fetches ' +
+              'triggered by amp-bind will soon require opt-in. Please add ' +
+              `data-amp-replace="${unwhitelisted.join(' ')}" to the ` +
+              `<${TAG}> element. See "bit.ly/amp-var-subs" for details.`);


@bpaduch Ditto.

dreamofabear · 2018-02-01T19:42:07Z

@ericlindley-g FYI adding a user error for new opt-in requirement in preparation for two-way amp-frame messaging.

ghost

Docs are good.

ericlindley-g · 2018-02-01T19:48:40Z

Thanks for the heads up! Could you refresh my memory on the end-to-end path for this to give developers notice before an error is triggered that invalidates the page or breaks functionality? (e.g. Validation warning -> error)

dreamofabear · 2018-02-01T20:31:13Z

#12498 lists all the steps you suggested. This PR will only show a user error in dev console but retain current functionality. After deployment, we'll see how common this is in the wild via our error logs and can decide next steps from there.

ericlindley-g · 2018-02-01T20:33:51Z

Perfect — thanks! (I knew I had seen those somewhere :)

…mpproject#13081) * first pass at user().error for future opt-in requirement * url replacements unit test * rename to batchFetchJsonFor and add test-batched-json * lint fixes, use enum for replacement policy * refine docs * fix lint * use opt-in policy for amp-bind mutations only, fix types * more lint fixes * only require opt-in for CORS * fix collectUnwhitelistedVars for elements with no whitelist * refine error message * fix string error, thanks lgtm

William Chou added 5 commits January 26, 2018 16:21

first pass at user().error for future opt-in requirement

899bf4d

url replacements unit test

a4cd1f9

rename to batchFetchJsonFor and add test-batched-json

c8a159e

lint fixes, use enum for replacement policy

6e48710

refine docs

b081195

googlebot added the cla: yes label Jan 27, 2018

William Chou added 2 commits January 27, 2018 00:19

fix lint

27e4fb0

use opt-in policy for amp-bind mutations only, fix types

01cce0d

dreamofabear force-pushed the bind-opt-in-error branch from a8d94b4 to 5632ec8 Compare January 27, 2018 18:22

more lint fixes

959f430

dreamofabear force-pushed the bind-opt-in-error branch from 5632ec8 to 959f430 Compare January 27, 2018 18:40

dreamofabear requested review from alabiaga and aghassemi January 29, 2018 17:43

alabiaga reviewed Jan 29, 2018

View reviewed changes

aghassemi reviewed Jan 30, 2018

View reviewed changes

aghassemi approved these changes Jan 30, 2018

View reviewed changes

only require opt-in for CORS

59f75a4

dreamofabear force-pushed the bind-opt-in-error branch from df5deb9 to 59f75a4 Compare February 1, 2018 16:30

William Chou added 2 commits February 1, 2018 11:55

fix collectUnwhitelistedVars for elements with no whitelist

d547c5f

refine error message

f92544a

fix string error, thanks lgtm

c04f81a

dreamofabear force-pushed the bind-opt-in-error branch from ee0e0fb to c04f81a Compare February 1, 2018 19:06

dreamofabear commented Feb 1, 2018

View reviewed changes

dreamofabear requested a review from a user February 1, 2018 19:39

dreamofabear commented Feb 1, 2018

View reviewed changes

ghost approved these changes Feb 1, 2018

View reviewed changes

dreamofabear merged commit 2bfb03b into ampproject:master Feb 1, 2018

dreamofabear deleted the bind-opt-in-error branch February 1, 2018 20:42

ampprojectbot added the PR use: In Stable label Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

amp-bind: Throw user error when replacing non-whitelisted URL vars #13081

amp-bind: Throw user error when replacing non-whitelisted URL vars #13081

dreamofabear commented Jan 27, 2018 •

edited

dreamofabear commented Jan 29, 2018

alabiaga Jan 29, 2018

dreamofabear Feb 1, 2018

alabiaga Jan 29, 2018

dreamofabear Feb 1, 2018

aghassemi Jan 30, 2018 •

edited

dreamofabear Jan 30, 2018

dreamofabear Feb 1, 2018

aghassemi Jan 30, 2018

dreamofabear Feb 1, 2018

aghassemi Jan 30, 2018

dreamofabear Feb 1, 2018

cramforce commented Feb 1, 2018

cramforce commented Feb 1, 2018

dreamofabear left a comment

dreamofabear Feb 1, 2018

dreamofabear Feb 1, 2018

dreamofabear Feb 1, 2018

dreamofabear Feb 1, 2018

dreamofabear Feb 1, 2018

dreamofabear Feb 1, 2018

dreamofabear Feb 1, 2018

dreamofabear commented Feb 1, 2018

ghost left a comment

ericlindley-g commented Feb 1, 2018

dreamofabear commented Feb 1, 2018

ericlindley-g commented Feb 1, 2018

amp-bind: Throw user error when replacing non-whitelisted URL vars #13081

amp-bind: Throw user error when replacing non-whitelisted URL vars #13081

Conversation

dreamofabear commented Jan 27, 2018 • edited

dreamofabear commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi Jan 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Feb 1, 2018

cramforce commented Feb 1, 2018

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamofabear commented Feb 1, 2018

ghost left a comment

Choose a reason for hiding this comment

ericlindley-g commented Feb 1, 2018

dreamofabear commented Feb 1, 2018

ericlindley-g commented Feb 1, 2018

dreamofabear commented Jan 27, 2018 •

edited

aghassemi Jan 30, 2018 •

edited