Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow any layout for amp-social-share, with test. #3099

Merged
merged 6 commits into from
May 10, 2016
Merged

Allow any layout for amp-social-share, with test. #3099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c361ef6
Allow any layout for amp-social-share, with test.
powdercloud May 4, 2016
926a6cb
Merge branch 'master' into amp-social-share
powdercloud May 6, 2016
3f759da
Fix amp-social-share validation and tests.
powdercloud May 6, 2016
7a39b54
Add j a v a s c r i p t : example.
powdercloud May 9, 2016
736eea9
allow tel: protocol as well
powdercloud May 9, 2016
0fc0b3a
add snapchat
powdercloud May 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 0 additions & 41 deletions extensions/amp-social-share/0.1/test/validator-amp-social-share-pass.html
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion extensions/amp-social-share/0.1/test/validator-amp-social-share-pass.out
View file
Open in desktop

This file was deleted.

60 changes: 49 additions & 11 deletions extensions/amp-social-share/0.1/test/validator-amp-social-share.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,59 @@
<meta name="viewport" content="width=device-width,minimum-scale=1">
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script async custom-element="amp-social-share" src="https://cdn.ampproject.org/v0/amp-social-share-0.1.js"></script>
</head>
<body>
<!-- Error, container layout not allowed -->
<amp-social-share layout="container" height="50" width="50">
<!-- Valid: simplest possible example -->
<amp-social-share type="twitter"></amp-social-share>

<!-- Valid: a simple example with params -->
<amp-social-share type="linkedin" width="60" height="44"
data-param-text="Hello world"
data-param-url="https://example.com/">
</amp-social-share>
<amp-social-share height="50" width="50" type="email">

<!-- Valid: A share endpoint that's a URL with whitelisted app protocol -->
<amp-social-share type="whatsapp"
layout="container"
data-share-endpoint="whatsapp://send"
data-param-text="Check this out: TITLE - CANONICAL_URL">
Share on Whatsapp
</amp-social-share>
<amp-social-share type="pinterest" width="60" height="44">
<script type="application/json">
{
"text": "Hello world",
"url": "https://example.com/",
"attribution": "AMPhtml"
}
</script>

<!-- Valid: A share endpoint that's a URL with the whitelisted https protocol
-->
<amp-social-share type="vote-this-up"
layout="container"
data-share-endpoint="https://example.com/vote-this-up"
data-param-text="Check this out: TITLE - CANONICAL_URL">
Share on Whatsapp
</amp-social-share>

<!-- Valid: unknown type, no endpoint given
Note: It would be good to consider this invalid, but given the choices
around required / optional parameters for this tag, we don't have a
convenient way to do that. This will probablly yield a runtime error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I think runtime error should be good enough. @dvoytenko what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I believe this should be a runtime error, not a validator error.

-->
<amp-social-share type="unknown" width="60" height="44"
data-param-text="Hello world"
data-param-url="https://example.com/">
</amp-social-share>

<!-- Invalid: Endpoint given with a protocol that doesn't fit the whitelist -->
<amp-social-share type="vote-this-up"
layout="container"
data-share-endpoint="votethisup://vote"
data-param-text="Check this out: TITLE - CANONICAL_URL">
Share on Whatsapp
</amp-social-share>

<!-- Invalid: Endpoint given with a protocol that doesn't fit the whitelist -->
<amp-social-share type="uhm-no-hahaha"
layout="container"
data-share-endpoint="j a v a s c r i p t : alert('oh hi')"
data-param-text="Check this out: TITLE - CANONICAL_URL">
Share on Whatsapp
</amp-social-share>
</body>
</html>
4 changes: 2 additions & 2 deletions extensions/amp-social-share/0.1/test/validator-amp-social-share.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
FAIL
amp-social-share/0.1/test/validator-amp-social-share.html:31:2 The specified layout 'CONTAINER' is not supported by tag 'amp-social-share'. (see https://www.ampproject.org/docs/reference/extended/amp-social-share.html) [AMP_LAYOUT_PROBLEM]
amp-social-share/0.1/test/validator-amp-social-share.html:45:7 The tag 'amp-social-share extension .js script' is missing or incorrect, but required by 'amp-social-share'. (see https://www.ampproject.org/docs/reference/extended/amp-social-share.html) [AMP_TAG_PROBLEM]
amp-social-share/0.1/test/validator-amp-social-share.html:68:2 Invalid URL protocol 'votethisup:' for attribute 'data-share-endpoint' in tag 'amp-social-share'. (see https://www.ampproject.org/docs/reference/extended/amp-social-share.html) [AMP_TAG_PROBLEM]
amp-social-share/0.1/test/validator-amp-social-share.html:76:2 Invalid URL protocol 'j a v a s c r i p t :' for attribute 'data-share-endpoint' in tag 'amp-social-share'. (see https://www.ampproject.org/docs/reference/extended/amp-social-share.html) [AMP_TAG_PROBLEM]
42 changes: 23 additions & 19 deletions extensions/amp-social-share/0.1/validator-amp-social-share.protoascii
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,24 +46,6 @@ tags: { # amp-social-share
}
spec_url: "https://www.ampproject.org/docs/reference/extended/amp-social-share.html"
}
tags: { # amp-social-share (json)
tag_name: "script"
spec_name: "amp-social-share extension .json script"
mandatory_parent: "amp-social-share"
attrs: {
name: "type"
mandatory: true
value: "application/json"
dispatch_key: true
}
cdata: {
blacklisted_cdata_regex: {
regex: "<!--"
error_message: "html comments"
}
}
spec_url: "https://www.ampproject.org/docs/reference/extended/amp-social-share.html"
}
tags: { # <amp-social-share>
tag_name: "amp-social-share"
disallowed_ancestor: "head"
Expand All @@ -72,11 +54,33 @@ tags: { # <amp-social-share>
attrs: {
name: "type"
mandatory: true
value_regex: "(twitter|facebook|pinterest|linkedin|gplus|email)"
}
attrs: {
name: "data-share-endpoint"
value_url: {
allowed_protocol: "ftp"
allowed_protocol: "http"
allowed_protocol: "https"
allowed_protocol: "mailto"
# Whitelisting additional commonly observed third party
# protocols which should be safe
allowed_protocol: "snapchat"
allowed_protocol: "sms"
allowed_protocol: "tel"
allowed_protocol: "viber"
allowed_protocol: "whatsapp"
Copy link
Contributor

Choose a reason for hiding this comment

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

@dvoytenko I want to get your opinion on this as well. @powdercloud and I chatted about this, this is mainly to blacklist bad protocols (I know javascript:, not sure what else). But blacklisting is not possible currently in the validator.

The problem with this is that it really limits what users can use until they add them to the validator, which I am not really a big fan of. Should we just drop the whitelisting in the validator and enforce it on runtime?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fairly important topic. I have two questions here:

  1. /to @powdercloud / Do we currently have any validation rules <a href="">? Technically our case is an extension if a simple <a> tag.
  2. /to @mkhatib / Can you test what would happen for a endpoint URL that looks like this:
j
a
v
a
s
r
i
p
t:

Ideally we'd have a blacklist here, but let's first see what are the answers to these questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes.
  2. Added some example with 'j a v a s c r i p t : '. Since this exercises the same code as the anchor checks, the validation is already quite covered with the tests in here:
    https://github.com/ampproject/amphtml/blob/master/validator/testdata/feature_tests/urls.html

The gist is that the validator will detect the protocol and compare it to a whitelist.

allow_relative: false
}
}
attr_lists: "extended-amp-global"
spec_url: "https://www.ampproject.org/docs/reference/extended/amp-social-share.html"
amp_layout: {
supported_layouts: CONTAINER
supported_layouts: FILL
supported_layouts: FIXED
supported_layouts: FIXED_HEIGHT
supported_layouts: FLEX_ITEM
supported_layouts: NODISPLAY
supported_layouts: RESPONSIVE
}
}