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.

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

Support external POST forms via form submission proxy #4191

Closed
chvillanuevap opened this issue Jan 29, 2020 · 32 comments
Closed

Support external POST forms via form submission proxy #4191

chvillanuevap opened this issue Jan 29, 2020 · 32 comments
Labels
P0 High priority Punted WS:Core Work stream for Plugin core

Comments

@chvillanuevap
Copy link

Hi there!

I'm using the Genesis eNews Extended plugin to create a form and have my website visitors subscribe to my newsletter. I recently noticed that when I use AMP, the form does not work at all. I get an error message that says: "Your submission failed. The server responded with (code ). Please contact the developer of this form processor to improve this message. Learn More"

Do I need to add some headers or something to a filter in functions.php? Or is this a server issue?

Thanks!

@westonruter
Copy link
Member

Please share the URL for the AMP page that has the form on it.

@chvillanuevap
Copy link
Author

@chvillanuevap
Copy link
Author

Also, the console error is:

Access to fetch at 'https://thepostmansknock.boldermail.com/subscribe?_wp_amp_action_xhr_converted=1&__amp_source_origin=http%3A%2F%2Fthepostmanskck.staging.wpengine.com' from origin 'http://thepostmanskck.staging.wpengine.com' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.
log.js:252 [amp-form] Form submission failed: Error: XHR Failed fetching (https://thepostmansknock.boldermail.com/...): Failed to fetch​​​
    at ab (https://cdn.ampproject.org/v0.js:28:169)
    at Xa.f.createExpectedError (https://cdn.ampproject.org/v0.js:23:343)
    at https://cdn.ampproject.org/v0.js:142:154

@westonruter
Copy link
Member

Thanks. I'm also seeing this:

image

I can see that the HTTP response headers for a request are:

HTTP/1.1 302 Found
Date: Thu, 30 Jan 2020 06:46:30 GMT
Server: Apache
X-Frame-Options: SAMEORIGIN
X-Powered-By: PHP/7.1.28
Location: https://thepostmansknock.com/welcome-to-the-blog-list/
Content-Length: 0
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Content-Type: text/html; charset=UTF-8

This is not expected. I expected instead of a Location header to see an AMP-Redirect-To header. I believe this is an issue with the Genesis eNews Extended plugin. Is that the one you are using?

@chvillanuevap
Copy link
Author

Yes. That's the plugin I'm using. Once the form is submitted with the plugin and the user is subscribed, my mailing system redirects the user to https://thepostmansknock.com/welcome-to-the-blog-list/.

Is that redirection not compatible with AMP?

@westonruter
Copy link
Member

Oh, also I'm seeing the form having this action URL:

https://thepostmansknock.boldermail.com/subscribe?_wp_amp_action_xhr_converted=1

That's not expected. Should it not rather be:

https://thepostmanskck.staging.wpengine.com/subscribe?_wp_amp_action_xhr_converted=1

Have the URLs perhaps not been replaced properly on your staging site?

The redirection should be compatible with AMP. In particular if the redirection is being done via wp_redirect() (as it should).

You can see that the plugin intercepts calls to wp_redirect to then send back the AMP-Redirect-To header here:

public static function intercept_post_request_redirect( $location ) {
// Make sure relative redirects get made absolute.
$parsed_location = array_merge(
[
'scheme' => 'https',
'host' => wp_parse_url( home_url(), PHP_URL_HOST ),
'path' => isset( $_SERVER['REQUEST_URI'] ) ? strtok( wp_unslash( $_SERVER['REQUEST_URI'] ), '?' ) : '/',
],
wp_parse_url( $location )
);
$absolute_location = '';
if ( 'https' === $parsed_location['scheme'] ) {
$absolute_location .= $parsed_location['scheme'] . ':';
}
$absolute_location .= '//' . $parsed_location['host'];
if ( isset( $parsed_location['port'] ) ) {
$absolute_location .= ':' . $parsed_location['port'];
}
$absolute_location .= $parsed_location['path'];
if ( isset( $parsed_location['query'] ) ) {
$absolute_location .= '?' . $parsed_location['query'];
}
if ( isset( $parsed_location['fragment'] ) ) {
$absolute_location .= '#' . $parsed_location['fragment'];
}
self::send_header( 'AMP-Redirect-To', $absolute_location );

@chvillanuevap
Copy link
Author

https://thepostmansknock.boldermail.com is the correct URL.

The form sends the data there, the user is subscribed and then boldermail.com redirects the user to https://thepostmansknock.com/welcome-to-the-blog-list/. The process works fine on non-AMP pages -- see the form on the sidebar on https://thepostmansknock.com/blog/

So there would not be any call to wp_redirect.

@westonruter
Copy link
Member

OK, the response from https://thepostmansknock.boldermail.com will need to send a AMP-Redirect-To response header to redirect the AMP page. More information at https://amp.dev/documentation/components/amp-form/?format=websites#redirecting-after-a-submission

@chvillanuevap
Copy link
Author

Ok, that makes sense!

I just took a quick look at what the AMP for WordPress plugin does with forms. In one of their extensions, AMP Opt-in, they provide a form builder for 25 different companies (Mailchimp, Mailerlite, etc.) that redirects all actions to an AJAX request within the plugin. Then, the AJAX function actually submits the information to these companies. I think I'll have to implement something like this.

Thanks for your help!

@westonruter
Copy link
Member

Yes, implementing a proxy endpoint would work if you can't add the AMP-Redirect-To response header to the existing endpoint. Such a proxy endpoint could be built into the Genesis eNews Extended plugin.

@westonruter
Copy link
Member

Actually I wonder if this proxy would make sense to be built directly into the AMP plugin. This would make the form submissions much less likely to fail, such as when sites don't use wp_redirect() or send requests off to other domains. My only concern here is guarding against abuse. We'd need to make sure that the proxy is not used as a mechanism to do attacks.

@chvillanuevap
Copy link
Author

Submitting a form to an external URL to subscribe a user and then redirecting them back is something a lot of companies do (like Mailchimp). I have used the Genesis eNews Extended with Mailchimp before, and that was the process. However, I couldn't find any references to using AMP with a Mailchimp form besides the AMP for WordPress plugin and this gist: https://gist.github.com/pshapiro/3a1f5286ff98abd3bd3d7dbe3b5ff70d

The AMP for WordPress plugin has this proxy as a paid extension.

@westonruter
Copy link
Member

Okay, let me draft a PR to implement this. Would you test?

@chvillanuevap
Copy link
Author

Sure!

@westonruter
Copy link
Member

@chvillanuevap I have a PR for you to test: #4212. See build of ZIP if you don't want to do a build yourself: #4212 (comment)

@westonruter
Copy link
Member

I have also opened a PR to make Genesis eNews Extended AMP-compatible: kraftbj/genesis-enews-extended#146

@chvillanuevap
Copy link
Author

Sure! I'll test today.

@kraftbj
Copy link
Contributor

kraftbj commented Feb 14, 2020

Thanks y'all for your work here. I'll review the PR on the eNews Extended side and get it out.

@DarrenHuangTW
Copy link

Hi All - I have a similar commenting issue on my site, which uses the native Wordpress Twenty Fifteen theme and the AMP Plugin.

Website: https://www.darrenhuang.com/2020/03/seo-newsletter-issue-4.html
AMP Comment Error

I was about to submit a new issue but noticed this thread and decided to address it here. Is it the same issue and will be fixed in the same pull request? Thank you!

@westonruter
Copy link
Member

westonruter commented Mar 23, 2020

@DarrenHuangTW Hey. The issue you have may be resolved by this, but there is another existing solution. Namely, the problem is that your server has in place strict Mod Security rules which reject requests that contain the query string parameter __amp_source_origin. The HTTP response when submitting the form on your site has:

image

This has been reported before:

This was resolved by contacting the host to add the __amp_source_origin query param to the allowlist.

@DarrenHuangTW
Copy link

@westonruter Thank you for answering so quickly! I'll reach out to Bluehost to resolve it. :-)

@kmyram kmyram added this to the Sprint 27 milestone Apr 14, 2020
@westonruter
Copy link
Member

@kmyram kmyram added the WS:Core Work stream for Plugin core label May 15, 2020
@kmyram kmyram modified the milestones: Sprint 27, v1.6 May 27, 2020
@westonruter westonruter removed this from the v1.6 milestone Jun 16, 2020
@westonruter westonruter added this to the v1.7 milestone Jul 7, 2020
@chvillanuevap
Copy link
Author

What are the plans to merge this into the plugin? I just updated to version 1.5.5 because the version I downloaded initially (1.5.0-alpha-20200205T225235Z-b246797c6) was breaking the display of posts for logged in users.

@westonruter
Copy link
Member

It's currently milestoned for v2.1. So hopefully in a couple months.

@westonruter
Copy link
Member

This is also needed by the Revue widget in Jetpack: Automattic/jetpack#16922.

@westonruter
Copy link
Member

Also let's make sure it accounts for the PayPal payment snippet code: https://wordpress.stackexchange.com/questions/367201/amp-and-paypal-form-cors-issue

@chvillanuevap
Copy link
Author

Hey guys it's going to be almost a year since I opened this ticket. Any chance of the solution seeing the light of day anytime soon?

@westonruter
Copy link
Member

Yeah, it's still on my radar. I'm planning to pick it up again after I finish #5558. I do have a work-in-progress PR (#4212) which I will be bringing up to date. It's still milestoned for 2.1 which we're planning for the first part of 2021.

@westonruter
Copy link
Member

Not enough time to get this into 2.1, but I'm planning to create a mini plugin that implements the proxy in a standalone way to prototype it for 2.2.

@westonruter westonruter modified the milestones: v2.1, v2.2 Feb 11, 2021
@westonruter westonruter changed the title Issue with Genesis eNews Extended form Support external POST forms via form submission proxy Feb 12, 2021
@westonruter
Copy link
Member

Sorry for the delay. I have put together a mini plugin that implements an external form submission proxy: https://gist.github.com/westonruter/c08134bf8b33a49ef780a70b337d456f

Hopefully this is helpful while waiting to get this into the plugin. Please let me know if there are any issues.

@westonruter
Copy link
Member

As opposed to creating a proxy, I've also written ampproject/amphtml#27638 (comment) to advocate non-XHR POST forms. This will save many headaches.

@westonruter westonruter removed this from the v2.2 milestone Jun 4, 2021
@westonruter westonruter removed their assignment Jun 4, 2021
@westonruter
Copy link
Member

Here is another alternative which utilizes an iframe to embed the external POST form: https://amp-external-subscription-form-in-iframe.glitch.me/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Punted WS:Core Work stream for Plugin core
Projects
None yet
5 participants