Skip to content

Allow variable substitution for whitelisted origins#7344

Merged
cramforce merged 2 commits intoampproject:masterfrom
cpapazian:allow-url-substition-for-whitelisted-origins
Feb 10, 2017
Merged

Allow variable substitution for whitelisted origins#7344
cramforce merged 2 commits intoampproject:masterfrom
cpapazian:allow-url-substition-for-whitelisted-origins

Conversation

@cpapazian
Copy link
Copy Markdown
Contributor

Allow for an additional domain (or list of domains) for which variable substitution is allowed in links.

  • list of domains is specified via meta tag with name=amp-link-variable-allowed-origin
  • multiple whitelisted domains can be specified by using whitespace separated list
  • allowed variables uses existing whitelist on the element (data-amp-replace)

Implements #6141

Fixes ampproject#6141

Allow for an additional domain (or list of domains) for which variable substitution is allowed in links.
@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

return true;
}

const meta = this.ampdoc.getRootNode().querySelector(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @honeybadgerdontcare on meta tag.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, is whitelisted now.

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks perfect! One comment on the test. Otherwise this can go right in.

querySelector: selector => {
if (selector.startsWith('meta')) {
return {
getAttribute: () => {return 'https://whitelisted.com';},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a space separated second origin.

@cpapazian
Copy link
Copy Markdown
Contributor Author

I signed it!

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@cpapazian
Copy link
Copy Markdown
Contributor Author

@cramforce i've updated the test. are we waiting on anything else?

@cramforce
Copy link
Copy Markdown
Member

@rudygalfi We are good with this, right?

@rudygalfi
Copy link
Copy Markdown
Contributor

LGTM

@cramforce cramforce merged commit 7ee4bdc into ampproject:master Feb 10, 2017
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017
Fixes ampproject#6141

Allow for an additional domain (or list of domains) for which variable substitution is allowed in links.
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
Fixes ampproject#6141

Allow for an additional domain (or list of domains) for which variable substitution is allowed in links.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants