Improve handling of form submissions that redirect and implement form submission proxy#4212
Improve handling of form submissions that redirect and implement form submission proxy#4212westonruter wants to merge 5 commits intodevelopfrom
Conversation
e5c71dd to
b246797
Compare
|
Build for testing: amp.zip (v1.5.0-alpha-20200205T225235Z-b246797c6) |
|
In testing making Genesis eNews Extended AMP-compatible in kraftbj/genesis-enews-extended#146, I was trying to supply an diff --git a/class-bjgk-genesis-enews-extended.php b/class-bjgk-genesis-enews-extended.php
index 0dbce39..e3bf7d1 100644
--- a/class-bjgk-genesis-enews-extended.php
+++ b/class-bjgk-genesis-enews-extended.php
@@ -130,14 +130,15 @@ class BJGK_Genesis_ENews_Extended extends WP_Widget {
if ( ! empty( $instance['id'] ) ) : ?>
<form
id="subscribe-<?php echo esc_attr( $this->id ); ?>"
- action="https://feedburner.google.com/fb/a/mailverify"
method="post"
name="<?php echo esc_attr( $this->id ); ?>"
<?php if ( ! $this->is_amp() ) : ?>
target="popupwindow"
onsubmit="window.open( 'https://feedburner.google.com/fb/a/mailverify?uri=<?php echo esc_js( $instance['id'] ); ?>', 'popupwindow', 'scrollbars=yes,width=550,height=520');return true"
+ action="https://feedburner.google.com/fb/a/mailverify"
<?php else: ?>
on="<?php echo esc_attr( sprintf( "submit-success:AMP.navigateTo( url=%s, target=_blank )", wp_json_encode( 'https://feedburner.google.com/fb/a/mailverify?uri=' . $instance['id'], JSON_UNESCAPED_SLASHES ) ) ); ?>"
+ action-xhr="https://feedburner.google.com/fb/a/mailverify"
<?php endif; ?>
>
<label for="subbox" class="screenread"><?php echo esc_attr( $instance['input_text'] ); ?></label><input type="<?php echo current_theme_supports( 'html5' ) ? 'email' : 'text'; ?>" value="" id="subbox" placeholder="<?php echo esc_attr( $instance['input_text'] ); ?>" name="email"When I did this, however, I got an error in the console:
I did not get this error when I used the proxy. This would seem to indicate a problem with access-control. That brings something to mind. Should the proxy be prevented from relaying JSON if the proxied endpoint does not return I believe to harden this the proxy should be looking at any This PR warrants a good security review. |
|
I just tested this version of the AMP plugin, and it worked great! I also tested the Genesis eNews Extended plugin and the Thanks! |
…submission-proxy * 'develop' of github.com:ampproject/amp-wp: (4571 commits) Fix logic error Skip `AMP_Post_Meta_Box::get_amp_blocks_in_use()` test if on WP < 5.0 Add tests Remove deprecation notice Use alternative method of getting blocks used in post Fix typo Show deprecation warning notice if an AMP-specific block is in the editor Hide AMP-specific blocks if they are not in the post Extend @wordpress/stylelint-config stylelint config Remove puppeteer dependency (now provided by @wordpress/scripts) Update package-lock.json Bump @wordpress/scripts from 12.6.1 to 13.0.1 Bump @wordpress/blocks from 6.25.1 to 6.25.2 Bump @wordpress/plugins from 2.24.1 to 2.24.2 Bump @wordpress/data from 4.26.1 to 4.26.2 Bump @wordpress/url from 2.21.0 to 2.21.1 Bump @wordpress/babel-preset-default from 4.20.0 to 5.0.0 Bump @wordpress/edit-post from 3.26.1 to 3.26.2 Bump eslint-plugin-jsdoc from 31.2.1 to 31.4.0 Update package-lock.json ...
Codecov Report
@@ Coverage Diff @@
## develop #4212 +/- ##
=============================================
- Coverage 75.13% 74.83% -0.31%
- Complexity 5647 5665 +18
=============================================
Files 210 210
Lines 16961 17029 +68
=============================================
Hits 12743 12743
- Misses 4218 4286 +68
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Plugin builds for f2484db are ready 🛎️!
|
|
Hi @westonruter, Will it be released with AMP-wp v2.2 ? I've also been testing Jetpack Revue block along with AMP-wp v2.1.0-alpha-20210201T193527Z-cdfd29c. Thanks :) |
|
@natoinet Yes, I want to finally get this into 2.2. This has been a long-standing pain point that needs to finally be addressed. I'll look at the Revue Block to see how it can be made compatible in the mean time. |
|
I've implemented an external form submission proxy for the AMP plugin in this mini plugin: https://gist.github.com/westonruter/c08134bf8b33a49ef780a70b337d456f I tried testing it with Revue, however, and I wasn't having much luck. More testing is needed. |
|
The better bet here is going to be just allowing non-XHR POST forms on AMP pages. Without this, we have to resort to hacks. See ampproject/amphtml#27638 (comment). In the mean time, I hope my mini plugin is useful as a workaround. |
Summary
header('Location: …')in the same way as beforewp_redirect(…)was able to automatically supply theAMP-Redirect-Toresponse header. This should eliminate the need for this shim code in Gravity Forms. The corresponding playbook information will also be obsolete with this PR.Fixes #4191.
Iterates on #2425. Revisits #911. See also #923.
Testing
[test_post_form]and[test_post_form action="https://brook-paranthodon-k6ttvyw9a.glitch.me/"]. For example:Any submission request type that has a JSON response (including the use of
wp_die()) should have a response looking like:or
If the form handler does not respond with something that can be represented as JSON (e.g. returning with an HTML page) then the expected output should look like:
or
Note that the underlying HTTP status code and status text should be displayed in this message.
Additionally, if a form submission results in a redirect, either via
wp_redirect()or using an underlyingheader('Location: …')call, then the user should be redirected after seeing a “Redirecting…” message momentarily:Similar behavior should work both for when the form submission is handled internally by the present WordPress or if the form submission is pointing to another domain.
Checklist