Use whitelist to restrict urlReplacement for scoped analytics element#8360
Use whitelist to restrict urlReplacement for scoped analytics element#8360zhouyx merged 4 commits intoampproject:masterfrom
Conversation
| */ | ||
| export const ScopedElementWhiteList = { | ||
| // TODO: Please Review the list! | ||
| 'RANDOM': true, |
There was a problem hiding this comment.
I suggest we turn off everything for now so we can get the PR in independently of a privacy review.
There was a problem hiding this comment.
even for variables like RAMDOM ?
| */ | ||
| export const ScopedElementWhiteList = { | ||
| // TODO: Please Review the list! | ||
| 'RANDOM': true, |
There was a problem hiding this comment.
i suggest we only keep the trues to shorten the list.
There was a problem hiding this comment.
That's what I plan to do eventually. I put the complete list for the convenience of privacy review. But If we turn off everything for now, I can definitely do this now.
| this.type_ = null; | ||
|
|
||
| /** @private {!boolean} */ | ||
| this.isScoped_ = !!element.getAttribute('scope'); |
| // TODO: Please Review the list! | ||
| 'RANDOM': true, | ||
| 'COUNTER': true, | ||
| 'CANONICAL_URL': false, |
There was a problem hiding this comment.
CANONICAL should be allowed.
There was a problem hiding this comment.
I assume all CANONICAL_URL, CANONICAL_HOST, CANONICAL_HOSTNAME, CANONICAL_PATH are good. I set all of them to true.
| 'CANONICAL_PATH': false, | ||
| 'DOCUMENT_REFERRER': false, | ||
| 'TITLE': true, | ||
| 'AMPDOC_URL': false, |
There was a problem hiding this comment.
AMPDOC_ URL stuff is good too.
| 'AMPDOC_URL': false, | ||
| 'AMPDOC_HOST': false, | ||
| 'AMPDOC_HOSTNAME': false, | ||
| 'SOURCE_URL': false, |
|
@lannka convinced me that we should first turn off everything in the whitelist. And only turn them on if anyone request. |
|
@zhouyx I think it's safe to turn on AMPDOC_* vars and if we do that, it's the same as enabling CANONICAL_ and SOURCE_. 3p ampcontext is a good source for this data and canonical, et all are allowed there. So, let's enable these right away. |
|
Otherwise, LGTM |
| this.type_ = null; | ||
|
|
||
| /** @private {!boolean} */ | ||
| this.isScoped_ = element.hasAttribute('scope'); |
There was a problem hiding this comment.
shall we give it a different name? scope is already used in selectionMethod which has a quite different meaning.
how about <amp-analytics sandbox>
| * Used for inserted scoped analytics element. | ||
| * @const {!Object<string, boolean>} | ||
| */ | ||
| export const ScopedElementWhiteList = { |
There was a problem hiding this comment.
SANDBOX_AVAILABLE_VARS
maybe also change the file name.
* master: (34 commits) Prevent amp-carousel next/previous icons fade away on desktop (ampproject#8428) Turn on flag slidescroll-disable-css-snap” (ampproject#8436) Revert "temporarily turn off yarn (ampproject#8356)" (ampproject#8384) initial commit (ampproject#8404) Upgrades for Index Exchange amp-ad tags to report load statistics (ampproject#8054) amp-bind validation tweak (ampproject#8414) Fix an amp-instagram race condition (ampproject#8192) Use whitelist to restrict urlReplacement for scoped analytics element (ampproject#8360) Report active experiments in error logs (ampproject#8108) amp-bind: Catch exceptions in mutatedAttributesCallback (ampproject#8383) Fixing custom scroll-snap on IOS (ampproject#8391) Add experiment for using AmpContext class in integration.js (ampproject#8348) add (ampproject#8349) swipe api (ampproject#8357) skip 3 flaky tests (ampproject#8388) amp-bind: Expression complexity limit (ampproject#8321) add margin-bottom (ampproject#8350) Flying carpet: make container full viewport and center content (ampproject#8292) Service Registration: Document Click (ampproject#7882) Add a8ad (ampproject#8036) ...
…ampproject#8360) * use whitelist when replace scoped url * address comment * shorten-whitelist * rename
@dvoytenko As we agreed, used whitelist to apply the restriction as the first step
I created #8396 to for future review of adding extra variables to the whitelist.