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

Jump to bottom

Fix conversion of forms with relative action URLs #4003

Merged
merged 2 commits into from
Dec 22, 2019
Merged

Fix conversion of forms with relative action URLs #4003

merged 2 commits into from
Dec 22, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Dec 20, 2019 •
edited
Loading

Summary

The subscription form in Jetpack uses an action of #:

<form action="#" method="post" accept-charset="utf-8" id="subscribe-blog-blog_subscription-2">

When the form is being displayed on a URL like https://example.org/about/ it is getting converted by the form sanitizer as:

<form action-xhr="//example.org?_wp_amp_action_xhr_converted=1#" method="post" accept-charset="utf-8" id="subscribe-blog-blog_subscription-2" target="_top">

This is wrong. It should instead be:

<form action-xhr="//example.org/about/?_wp_amp_action_xhr_converted=1#" method="post" accept-charset="utf-8" id="subscribe-blog-blog_subscription-2" target="_top">

This PR fixes the sanitization of action attribute containing values such as:

  • ../
  • ?foo=bar
  • #foo

Issue discovered in a WordPress.org support topic.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • 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).

@westonruter westonruter added Bug Something isn't working Sanitizers labels Dec 20, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Dec 20, 2019
@westonruter westonruter added this to the v1.4.3 milestone Dec 20, 2019
pierlon
pierlon suggested changes Dec 21, 2019
View reviewed changes
includes/sanitizers/class-amp-form-sanitizer.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-form-sanitizer.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-form-sanitizer.php Outdated
$action_url = '//' . $_SERVER['HTTP_HOST'] . wp_unslash( $_SERVER['REQUEST_URI'] ) . $action_url;
} elseif ( '.' === $action_url[0] ) {
// For actions consisting of relative paths (e.g. '../'), prepend the schemeless-host and a trailing-slashed REQUEST URI.
$action_url = '//' . $_SERVER['HTTP_HOST'] . trailingslashit( wp_unslash( $_SERVER['REQUEST_URI'] ) ) . $action_url;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the trailing slash should always be added. Taking the action .foo for example, depending on the server configuration .foo could be a file while .foo/ could be a directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, but the trailing slash is being added after the path in order to append the relative segment. For example, if the user is currently on /about/team and there is a form that has an action of ../ the resulting action URL path needs to be /about/team/../ not /about/team../. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks for the clarification.

@westonruter @pierlon
Re-use schemeless-host variable
1378cd0 
Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@westonruter westonruter merged commit c26b635 into develop Dec 22, 2019
@westonruter westonruter deleted the fix/relative-form-actions branch December 22, 2019 02:10
westonruter added a commit that referenced this pull request Dec 22, 2019
@westonruter @pierlon
Fix conversion of forms with relative action URLs (#4003)
865bc05 
* Fix conversion of forms with relative action URLs

* Re-use schemeless-host variable

Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@pierlon pierlon mentioned this pull request Jan 31, 2020
Closed
@pierlon
Copy link
Contributor

pierlon commented Jan 31, 2020

Testing instructions:

  1. Create a post with a Custom HTML block filled with the following HTML:

    <form method="post" action="#"></form>

  2. On the AMP page, verify that the action attribute has been converted to action-xhr, and the form element now looks like:

    <form method="post" action-xhr="//example.com/about/?amp&_wp_amp_action_xhr_converted=1#" ...>...</form>

    (where example.com/about/?amp is the post URL)

@csossi
Copy link

csossi commented Feb 1, 2020

Verified in QA

image

@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
westonruter
westonruter commented Feb 9, 2020
View reviewed changes
includes/sanitizers/class-amp-form-sanitizer.php
if ( '?' === $action_url[0] || '#' === $action_url[0] ) {
// For actions consisting of only a query or URL fragment, include the schemeless-host and the REQUEST URI of the current page.
$action_url = $schemeless_host . wp_unslash( $_SERVER['REQUEST_URI'] ) . $action_url;
} elseif ( '.' === $action_url[0] ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: the '.' check was not correct here. See #4250 (review)

westonruter added a commit that referenced this pull request Feb 13, 2020
@westonruter
Merge tag '1.4.3'
06162df 
* tag '1.4.3': (22 commits)
  Update readme and screenshots for Stories removal (#4259)
  Open story export instructions in a new window (#4258)
  Bump version to 1.4.3-RC1
  Hide Stories options and add deprecation notice (#4219)
  Fix malformed conversion of relative action URLs for forms (#4250)
  Limit Stories experience to WP 5.3 & Gutenberg 7.1.0 (#4217)
  Prevent errors in admin bar filters from non-array arguments (#4207)
  Update @wordpress/e2e-test-utils dependency
  Revert update of mustache/mustache dependency
  Update composer.lock
  Update WP CLI to 2.4.0
  For WordPress.tv embed, Use an oembed filter instead of block filter (#4164)
  Update readme to add FAQs section (#4159)
  Apply workaround to fix test__multiple_valid_image_files (#4034)
  Ignore Story editor tests (#4043)
  Update amp-video embed regex pattern to include other Vimeo URL formats (#4051)
  Update amp-instagram embed regex (#4053)
  Update wp-dev-lib package (#4029)
  Fix conversion of forms with relative action URLs (#4003)
  Improve release instructions (#3995)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierlon pierlon pierlon approved these changes

Assignees
No one assigned
Labels
Bug Something isn't working cla: yes Signed the Google CLA QA passed Has passed QA and is done Sanitizers
Projects
None yet
Milestone
v1.4.3
Development

Successfully merging this pull request may close these issues.
4 participants
@westonruter @pierlon @csossi @googlebot