Add opt-in to prevent `POST` forms from being converted to `amp-form` (with `action-xhr`) #6527

westonruter · 2021-08-11T00:19:35Z

Summary

One of the very frequent issues encountered by users of the AMP plugin is POST form submissions not behaving properly. In fact, we have a support page all about handling form submissions. The issue is that AMP requires POST forms to be submitted over XHR rather than a traditional page navigation, but this breaks if the endpoint sends back a Location header to redirect or if the endpoint is on another host that doesn't send CORS headers. There is a feature request to allow non-XHR POST forms in AMP (ampproject/amphtml#27638), but for now we should allow an opt-in to prevent conversion of such POST forms and to omit the amp-form component. This amp-form component must be excluded or else it will intercept any submissions on POST forms to block them and show an error message.

To opt-in to traditional non-XHR POST forms, do the following:

add_filter( 'amp_native_post_form_allowed', '__return_true' );

When this is done, page that have such a form will now emit a new validation error: FORM_HAS_POST_METHOD. When the user marks this validation error as kept, then:

Conversion of the form is skipped.
The data-ampdevmode attribute is added to the form element and the html root element (to prevent removal).
The amp-form component script is omitted from the page.

A user may decide to opt-in to preventing any such POST form from being converted by default via the following code:

add_filter(
	'amp_validation_error_default_sanitized',
	static function ( $sanitized, $error ) {
		if ( isset( $error['code'] ) && 'FORM_HAS_POST_METHOD_WITHOUT_ACTION_XHR_ATTR' === $error['code'] ) {
			$sanitized = false;
		}
		return $sanitized;
	},
	10,
	2
);

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

…n converted to amp-form

…t amp-form script

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

github-actions · 2021-08-11T02:42:45Z

Plugin builds for ef4653e are ready 🛎️!

Download development build
Download production build

pierlon · 2021-08-11T04:39:25Z

Noticed a bug while testing this; given the HTML:

<form method="get" action-xhr="https://example.com"></form>

It will be sanitized to become:

<form method="get" action-xhr="https://example.com" action="//amp-dev.test/test-6527/?amp=1" target="_top" rel="amphtml" novalidate="" class="i-amphtml-form"><input name="amp" value="1" type="hidden"></form>

The action attribute should not be in the output.

pierlon · 2021-08-11T04:51:57Z

When the amp_allowing_native_post_forms filter returns true and given the markup:

<form method="post" action-xhr="https://example.com"></form>

Marking the validation error for that markup to "Kept" produces:

<form method="post" action-xhr="https://example.com" data-ampdevmode="" novalidate="" class="i-amphtml-form"></form>

Should the action-xhr attribute instead be attribute, or is it on the user for using action-xhr?

pierlon · 2021-08-11T04:20:01Z

includes/amp-helper-functions.php

+	 *
+	 * @param bool $use_native Whether to allow native `POST` forms.
+	 */
+	return (bool) apply_filters( 'amp_allowing_native_post_forms', false );


Any reason for using allowing instead of allow (or allows)? Just wondering since we don't have any hooks that use present participle verbs, AFAIK.

There's amp_using_native_img 😄

If you think the filters make more sense as amp_use_native_img and amp_allow_native_form then that I'm happy with that.

I had gone with singular for img since imgs looks bad.

But then what about the function names?

If you think the filters make more sense as amp_use_native_img and amp_allow_native_form then that I'm happy with that.

I was only concerned with it as it doesn't follow the usual naming conventions we use for our hooks, but either naming works for me 😄.

But then what about the function names?

They can stay if we're keeping the hook names.

What about:

Function Filter

amp_is_native_img_used() amp_native_img_used

amp_is_native_post_form_allowed() amp_native_post_form_allowed

I'm on board with that.

pierlon · 2021-08-11T04:24:56Z

includes/class-amp-theme-support.php

@@ -1514,6 +1513,11 @@ public static function ensure_required_markup( Document $dom, $script_handles =
 			$superfluous_script_handles = array_diff( $superfluous_script_handles, [ 'amp-carousel' ] );
 		}

+		// When opting-in to POST forms, omit the amp-form component entirely since it blocks submission.
+		if ( amp_is_allowing_native_post_forms() && $dom->xpath->query( '//form[ @method = "post" and @action ]' )->length > 0 ) {


Would the value of @method be case sensitive here?

Good catch. Fixed in dfb5223.

westonruter · 2021-08-11T05:15:44Z

Noticed a bug while testing this; given the HTML:
<form method="get" action-xhr="https://example.com"></form>
It will be sanitized to become:
<form method="get" action-xhr="https://example.com" action="//amp-dev.test/test-6527/?amp=1" target="_top" rel="amphtml" novalidate="" class="i-amphtml-form"><input name="amp" value="1" type="hidden"></form>
The action attribute should not be in the output.

That's tricky. It turns out in the case of a get form, it's the action attribute that's required and not the action-xhr. You can provide both, but the action-xhr is optional.

See the above is valid (playground):

Attempting to omit the action attribute results in an error:

pierlon · 2021-08-11T05:18:11Z

I see, that makes sense. The docs also confirm using both action and action-xhr for GET requests:

This attribute is required for method=POST, and is optional for method=GET.

The value for action-xhr can be the same or a different endpoint than action and has the same action requirements above.

westonruter · 2021-08-11T05:29:14Z

When the amp_allowing_native_post_forms filter returns true and given the markup:
<form method="post" action-xhr="https://example.com"></form>
Marking the validation error for that markup to "Kept" produces:
<form method="post" action-xhr="https://example.com" data-ampdevmode="" novalidate="" class="i-amphtml-form"></form>
Should the action-xhr attribute instead be attribute, or is it on the user for using action-xhr?

Oops. Yeah, POST forms with action-xhr should be skipped. I believe this is fixed as of a017973.

pierlon

LGTM

pierlon · 2021-08-11T05:58:20Z

Oh wait there seems to be some failing PHP tests.

pierlon · 2021-08-11T06:14:20Z

Oh wait there seems to be some failing PHP tests.

The failing test highlighted a bug in Core. The issue resides in this block of code, where there is no check to verify $res is not a WP_Error before operating on it, which is why the tests throw the error:

Undefined property: WP_Error::$themes

westonruter · 2021-08-11T06:23:59Z

Right, so it's a network issue.

pierlon · 2021-08-11T13:17:24Z

The failing test highlighted a bug in Core...

Created WordPress/wordpress-develop#1571 to fix the issue and it was just recently merged.

milindmore22 · 2021-12-10T13:03:45Z

QA Passed

Input for Post method

<form method="post" action-xhr="https://example.com"></form>

Output for post method

<form method="post" action-xhr="https://example.com" target="_top" novalidate="" class="i-amphtml-form"></form>

Input for Get Method

<form method="get" action-xhr="https://example.com"></form>

Output for Post method

<form method="get" action-xhr="https://example.com" action="//amp-local.test/form-test/amp/" target="_top" novalidate="" class="i-amphtml-form"></form>

westonruter added this to the v2.2 milestone Aug 11, 2021

westonruter added the Bento label Aug 11, 2021

Base automatically changed from add/native-img-opt-in to develop August 11, 2021 02:33

westonruter added 4 commits August 10, 2021 19:34

Update comments sanitizer to skip processing forms which have not bee…

0cf45e2

…n converted to amp-form

Move form target attribute normalization to helper method

d82c64f

Add allow_post_forms opt-in arg for form sanitizer

be2270f

Eliminate examination of collected validation errors to decide to omi…

6550aa1

…t amp-form script

westonruter force-pushed the add/post-form-allowance branch from 531de25 to 6550aa1 Compare August 11, 2021 02:34

westonruter marked this pull request as ready for review August 11, 2021 02:34

westonruter requested a review from pierlon August 11, 2021 02:34

westonruter commented Aug 11, 2021

View reviewed changes

includes/sanitizers/class-amp-form-sanitizer.php Outdated Show resolved Hide resolved

Better explain POST forms as native

cf8c16b

pierlon suggested changes Aug 11, 2021

View reviewed changes

westonruter added 2 commits August 10, 2021 22:22

Ignore considering POST forms that already have action-xhr attributes

a017973

Fix case sensitivity of method attribute in XPath query

dfb5223

pierlon approved these changes Aug 11, 2021

View reviewed changes

Improve function, filter, and arg names

ef4653e

westonruter merged commit 8898527 into develop Aug 11, 2021

westonruter deleted the add/post-form-allowance branch August 11, 2021 06:24

This was referenced Aug 17, 2021

Add support for custom non-AMP scripts #6528

Merged

Experimental: Add sandboxing levels #6546

Merged

Add AutoExtensions transformer ampproject/amp-toolbox-php#210

Merged

bartoszgadomski mentioned this pull request Dec 8, 2021

Facilitate documents in AMP Dev Mode to be unmarked as AMP pages when site is in Standard mode #5549

Closed

maitreyie-chavan assigned delawski Dec 10, 2021

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021

westonruter added the Sandboxing Experiment label Feb 22, 2022

miguelpeixe mentioned this pull request Aug 4, 2022

Reader Activation: Login form throws JS errors in AMP Plus Automattic/newspack-plugin#1848

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opt-in to prevent `POST` forms from being converted to `amp-form` (with `action-xhr`) #6527

Add opt-in to prevent `POST` forms from being converted to `amp-form` (with `action-xhr`) #6527

westonruter commented Aug 11, 2021 •

edited

Loading

github-actions bot commented Aug 11, 2021 •

edited

Loading

pierlon commented Aug 11, 2021

pierlon commented Aug 11, 2021 •

edited

Loading

pierlon Aug 11, 2021

westonruter Aug 11, 2021

westonruter Aug 11, 2021

westonruter Aug 11, 2021

pierlon Aug 11, 2021

westonruter Aug 11, 2021

pierlon Aug 11, 2021

pierlon Aug 11, 2021

westonruter Aug 11, 2021

westonruter commented Aug 11, 2021

pierlon commented Aug 11, 2021 •

edited

Loading

westonruter commented Aug 11, 2021

pierlon left a comment

pierlon commented Aug 11, 2021 •

edited

Loading

pierlon commented Aug 11, 2021 •

edited

Loading

westonruter commented Aug 11, 2021

pierlon commented Aug 11, 2021

milindmore22 commented Dec 10, 2021

Function	Filter
`amp_is_native_img_used()`	`amp_native_img_used`
`amp_is_native_post_form_allowed()`	`amp_native_post_form_allowed`

Add opt-in to prevent POST forms from being converted to amp-form (with action-xhr) #6527

Add opt-in to prevent POST forms from being converted to amp-form (with action-xhr) #6527

Conversation

westonruter commented Aug 11, 2021 • edited Loading

Summary

Checklist

github-actions bot commented Aug 11, 2021 • edited Loading

pierlon commented Aug 11, 2021

pierlon commented Aug 11, 2021 • edited Loading

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

Choose a reason for hiding this comment

westonruter commented Aug 11, 2021

pierlon commented Aug 11, 2021 • edited Loading

westonruter commented Aug 11, 2021

pierlon left a comment

Choose a reason for hiding this comment

pierlon commented Aug 11, 2021 • edited Loading

pierlon commented Aug 11, 2021 • edited Loading

westonruter commented Aug 11, 2021

pierlon commented Aug 11, 2021

milindmore22 commented Dec 10, 2021

Input for Post method

Output for post method

Input for Get Method

Output for Post method

Add opt-in to prevent `POST` forms from being converted to `amp-form` (with `action-xhr`) #6527

Add opt-in to prevent `POST` forms from being converted to `amp-form` (with `action-xhr`) #6527

westonruter commented Aug 11, 2021 •

edited

Loading

github-actions bot commented Aug 11, 2021 •

edited

Loading

pierlon commented Aug 11, 2021 •

edited

Loading

pierlon commented Aug 11, 2021 •

edited

Loading

pierlon commented Aug 11, 2021 •

edited

Loading

pierlon commented Aug 11, 2021 •

edited

Loading