Add support for comments by DavidCramer · Pull Request #871 · ampproject/amp-wp

DavidCramer · 2018-01-17T07:39:11Z

westonruter · 2018-01-17T18:55:20Z

includes/sanitizers/class-amp-form-sanitizer.php

+
+			// Set a target if needed.
+			if ( ! $node->hasAttribute( 'target' ) ) {
+				$node->setAttribute( 'target', '_blank' );


Per the spec: https://www.ampproject.org/docs/reference/components/amp-form#target

Indicates where to display the form response after submitting the form. The value must be _blank or _top.

The default I think should be _top as that is closer to the default behavior for a form. The actual default is _self but that's not valid AMP. So I think this should be doing:

if ( ! in_array( $node->hasAttribute( 'target' ), array( '_top', '_blank' ) ) ) { $node->setAttribute( 'target', '_top' );

westonruter · 2018-01-17T18:56:58Z

includes/sanitizers/class-amp-form-sanitizer.php

+			// Correct action.
+			if ( $node->hasAttribute( 'action' ) && ! $node->hasAttribute( 'action-xhr' ) ) {
+				$action_url = $node->getAttribute( 'action' );
+				$action_url = str_replace( 'http:', '', $action_url );


It's possible that http: appears somewhere else in the URL. So then this should instead use a regex like: preg_replace( '#^http:(?=//)#', '', $action_url ) to ensure it only replaces at the beginning of the string.

westonruter · 2018-01-17T18:59:11Z

includes/sanitizers/class-amp-form-sanitizer.php

+				$action_url = str_replace( 'http:', '', $action_url );
+				$node->setAttribute( 'action', $action_url );
+
+				if ( 'post' === $node->getAttribute( 'method' ) ) {


Per above, this could compare with $method.

westonruter · 2018-01-17T19:04:06Z

includes/sanitizers/class-amp-form-sanitizer.php

+				continue;
+			}
+
+			if ( ! $node->hasAttribute( 'action' ) || '' === $node->getAttribute( 'action' ) ) {


Per the spec: https://www.ampproject.org/docs/reference/components/amp-form#target

This attribute is required for method=GET. For method=POST, the action attribute is invalid, use action-xhr instead.

Is the second part of this conditional intended to check if 'get' === $node->getAttribute( 'method' )? Since the default method is get then I think there should be something above that does:

$method = 'get'; if ( $node->hasAttribute( 'method' ) ) { $method = strtolower( $node->getAttribute( 'method' ) ); }

And then use the $method variable going forward.

westonruter · 2018-01-17T19:06:29Z

includes/sanitizers/class-amp-form-sanitizer.php

+			}
+
+			if ( ! $node->hasAttribute( 'action' ) || '' === $node->getAttribute( 'action' ) ) {
+				$node->parentNode->removeChild( $node );


I don't think this is right. If method is get and there is no action, then what this really should do I think is just set the action to be esc_url_raw( 'https://' . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'] ), because the default action for a form is the URL that the form appears on.

westonruter · 2018-01-17T19:09:48Z

includes/utils/class-amp-dom-utils.php

 		 * See: http://php.net/manual/en/domdocument.loadhtml.php#78243
 		 */
-		$result = $dom->loadHTML( '<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body>' . $content . '</body></html>' );
+		$result = $dom->loadHTML( $document );


Changes here are conflicting in develop.

westonruter · 2018-01-17T19:20:19Z

For handling form method=POST submissions I think we can pretty cleanly handle them via https://www.ampproject.org/docs/reference/components/amp-form#redirecting-after-a-submission

Usually a POST request should be accompanied by a redirect. So we should be able to handle that via:

add_filter( 'wp_redirect', function( $location ) {
    $location = wp_sanitize_redirect( $location );
    header( "AMP-Redirect-To: $location" );
    header( "Access-Control-Expose-Headers: AMP-Redirect-To", false );
    return false; // Prevent header( 'Location:' ). I'm not sure this is necessary, or if we can just return $location.
}, 1000 );

Some more examples in general I've found at https://ampbyexample.com/components/amp-form/

westonruter · 2018-01-17T22:26:49Z

includes/class-amp-theme-support.php

+		$sanitizer->sanitize();
+
 		// @todo Add more validation checking and potentially the whitelist sanitizer.
+		$output = $dom->saveHTML();


@DavidCramer I chatted about this with @ThierryA, but I think we should avoid having to load the entire response into the DOM for sanitization. I think that we should limit sanitization to just the post content, 3rd-party widgets, and other “leaf nodes” which we know are liable to be invalid AMP.

Is this right? Or am I off track? My worry is that loading the HTML into a DOMDocument for every request will be too heavy. But at the same time, if we're already spinning up a DOMDocument for every instance of the_content() maybe it is actually better to just do one for the entire HTML. This would certainly simplify somethings for sanitizing widgets (cc @kienstra). It could also mean that we wouldn't have to have this ad hoc logic to search for elements requiring components: https://github.com/Automattic/amp-wp/blob/e60cb152a50e2ffbf5504d41aee859ad1d0e0baa/includes/class-amp-theme-support.php#L448-L455

Maybe we should decide to not prematurely optimize and instead keep things simple at first and just go ahead and sanitize the entire response. Thoughts?

See findings at #875 (comment)

It seems we should definitely not sanitize the entire output buffer.

@westonruter whats the consensus now on this? It looks like your findings indicated it may be a good idea to sanitize the whole buffer output. I personally think this is the most efficient. I spoke with @ThierryA about this yesterday and he was going to do some tests, like you did.

I'm leaning toward sanitizing the entire output buffer now.

@DavidCramer do you want to build off of #875 (comment) in a new PR to serve as a basis for what you're introducing here with form sanitization?

westonruter · 2018-01-19T08:41:40Z

@DavidCramer FYI: There won't need to be a get_scripts() method defined in the form sanitizer once #882 is merged.

ThierryA · 2018-01-19T14:22:36Z

@DavidCramer #882 was just merged to develop so you can apply @westonruter suggestion here.

westonruter · 2018-01-19T18:50:10Z

includes/class-amp-comment-walker.php

+			$url = substr( $url, 5 );
+		}
+		// @todo Identify arguments and make filterable/settable.
+		$template .= '<amp-list src="' . esc_attr( $url ) . '" height="400" single-item="true" layout="fixed-height">';


What about calling the actual comments walker here too and provide the content as placeholder/fallback content? Maybe that is overkill.

That's not a bad idea. However, it would increase page weight. But setting some kind of fallback is great idea.

DavidCramer · 2018-01-22T13:48:47Z

includes/sanitizers/class-amp-form-sanitizer.php

+
+			// Get the action URL.
+			if ( ! $node->hasAttribute( 'action' ) ) {
+				$action_url = esc_url_raw( '//' . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'] ); // WPCS: ignore. input var okay, sanitization ok.


@westonruter I don't normally like using $_SERVER if I can avoid it, but this is from your suggestion here: #871 (comment)
I feel this should be a little cleaner, but can't think how. any suggestions?

I think this is right what you have here, except that there should a beautiful wp_unslash() call added to it, since $_SERVER vars are auto-slashed for back-compat reasons:

esc_url_raw( '//' . wp_unslash( $_SERVER['HTTP_HOST'] ) . wp_unslash( $_SERVER['REQUEST_URI'] ) );

The one for HTTP_HOST is somewhat overkill.

agreed on the overkill. but will unslash the request uri.

DavidCramer · 2018-01-22T15:07:14Z

includes/class-amp-theme-support.php

+		}
+		return 'element';
+	}
+


@westonruter component scripts are have custom-element in the script tags, however amp-mustache used for amp-list does not, instead it has custom-template. see https://www.ampproject.org/docs/reference/components/amp-mustache
It looks like this is the only component that's different, however I separated it to its own method in case the spec changes, or additionals are added in future.

I don't think get_component_type would be needed if we sanitize the entire body, right?

@westonruter I think we still do, since this is done after the sanitisation and the scripts are placed in. All the scripts are custom-element but the template is custom-template.

DavidCramer · 2018-01-22T15:32:55Z

@westonruter Regarding moving the sanitisation from the_content to the output buffer: I needed this in order to have the comments to work, which I have done in my dev, but have not added to this PR. Is that still under discussion or could I do a PR on it based on what I've already got?

westonruter · 2018-01-23T00:47:46Z

@DavidCramer we'll go ahead and sanitize the entire body, or actually, the entire html document. I was also going to work on a PR, so if you can get one up first that is good as well.

westonruter · 2018-01-23T01:05:39Z

includes/amp-rest-functions.php

+		$GLOBALS['comment']    = $comment; // WPCS: override ok.
+		$comment->comment_date = get_comment_date();
+		$comment->comment_time = get_comment_time();
+		$comment->comment      = amp_get_comments_recursive( $id, $comment->comment_ID );


I believe this will have performance problems, as it could result in a lot of DB queries. If you look at wp_list_comments() I believe you can see it gets a flat list of all comments for the post, and then it passes it into Walker::paged_walk() which then goes about identifying which comments are children of other comments.

That is, AMP_Comment_Walker:: paged_walk() here.

@westonruter Agreed. Can revise it to be more efficient.

@westonruter how about this? b7c5963

DavidCramer · 2018-01-25T13:05:26Z

@westonruter You can start reviewing this, but there is an issue with the url template strings I need to fix. I'm needing to head off now, but will finish when I'm back. If not by the time you get on, I'll sort it in the morning.

…dd/862-support-comment-submissions

westonruter · 2018-01-25T22:34:12Z

there is an issue with the url template strings I need to fix

Shoot. This is a side effect of switching from saveXML to saveHTML in #891.

Similar to what was done in #895 with AMP_DOM_Utils::convert_amp_bind_attributes(), we need to do a temporary replacement of amp-mustache tokens during parsing and serialization, then swap them back after. I'm working on the change.

westonruter · 2018-01-26T06:10:00Z

We talked it over and we decided to try the route of amp-live-list instead and see how it goes. 🎉

DavidCramer added 3 commits January 17, 2018 09:31

Make add get_dom method to AMP_DOM_Utils

7ec60e4

add amp_form_sanitizer

eed214f

add form sanitisation on output buuffer

4067b59

DavidCramer requested review from ThierryA and westonruter January 17, 2018 07:39

westonruter reviewed Jan 17, 2018

View reviewed changes

merge develop in

a8d2ad3

DavidCramer added 5 commits January 19, 2018 11:29

Merge develop

98874ba

add comments REST endpoints

4f6dcd6

Add AMP Comments Walker

317f90f

Add comments template overrides and methods

451db91

add comment sanitizer

0541686

DavidCramer changed the title ~~[WIP] - Add form sanitization for comment submissions~~ [WIP] - Add support for comments Jan 19, 2018

Make walker add the HTML to reduce load in the sanitiser.

2c8575e

its a filter, not an action.

ac0cc53

westonruter reviewed Jan 19, 2018

View reviewed changes

DavidCramer added 3 commits January 22, 2018 12:25

Merge develop

2741fdf

add form sanitisation changes

ebffaae

make action setting more sane

9790be3

DavidCramer commented Jan 22, 2018

View reviewed changes

Add sanitisation and component_type method

b425b19

DavidCramer commented Jan 22, 2018

View reviewed changes

westonruter mentioned this pull request Jan 22, 2018

Adds dynamic header and footer in WordPress theme. xwp/ampnews#36

Merged

westonruter reviewed Jan 23, 2018

View reviewed changes

DavidCramer added 14 commits January 23, 2018 14:13

wp_unslash request_uri for action attribute.

94aee02

cleaner comments template generation

d0fc6c5

merge develop

6b660b3

cleanup formatting

69d1342

output comment template on same comments pass

4200d9e

cleanup comments sanitiser and add url template string filtering

366bef3

add comment template url string filters

7b63836

remove unnecessary duplicated wp_list_comments pass

8b289c1

Hook into AMP xhr-submissions to handle headers and JSON responses.

15d327d

add amp-list fallback for comments.

561189b

add comment form success and error handlers and wrapper code.

a1bf8d3

lowercase post k, thanks.

b40e932

Merge develop

dc1f9c3

fix formatting fail.

187e136

DavidCramer changed the title ~~[WIP] - Add support for comments~~ Add support for comments Jan 24, 2018

DavidCramer added 7 commits January 25, 2018 08:03

if no template, move on.

82e8021

No need to check for the amp_comments tag anymore.

d4fd8bc

add tests for form and comment sanitisation.

bbabbd7

add docs to tests

0fd1ab5

No need to get images again.

9f99abe

check has images before converting urls

9216e4e

fix phpdoc

fafa679

Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…

9536794

…dd/862-support-comment-submissions

Prevent saveHTML from munging amp-mustache curly braces via URL-encoding

74ae3b5

westonruter closed this Jan 26, 2018

westonruter mentioned this pull request Jan 26, 2018

Prevent saveHTML from munging amp-mustache curly braces via URL-encoding #906

Merged

westonruter deleted the add/862-support-comment-submissions branch July 5, 2018 15:36

Conversation

DavidCramer commented Jan 17, 2018 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jan 19, 2018

Uh oh!

ThierryA commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidCramer Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidCramer commented Jan 22, 2018

Uh oh!

westonruter commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidCramer commented Jan 25, 2018

Uh oh!

westonruter commented Jan 25, 2018

Uh oh!

westonruter commented Jan 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DavidCramer commented Jan 17, 2018 •

edited by westonruter

Loading

westonruter commented Jan 17, 2018 •

edited

Loading

DavidCramer Jan 19, 2018 •

edited

Loading