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

Fix #1349 relative path issue in action #1352

Merged
merged 3 commits into from Aug 24, 2018

Conversation

Projects
None yet
2 participants
@ricardobrg
Copy link
Contributor

ricardobrg commented Aug 23, 2018

Detect if action URL in the form is relative and format it properly.

Fixes #1349.

Fix #1349 relative path issue in action
Detect if action URL in the form is relative and format it properly
@westonruter

This comment has been minimized.

I suggest instead:

if ( ! preg_match( '#^(https?:)?//#', $action_url ) ) {
    $action_url = esc_url_raw( '//' . $_SERVER['HTTP_HOST'] . $action_url );
}

In actuality, we should also make sure that if it starts with http: that it should also get that stripped out, since forms must be submitted over HTTPS in AMP.

This comment has been minimized.

Copy link
Owner

ricardobrg replied Aug 22, 2018

@westonruter The http strip out is already done at L77-82.

I will update with your regex. It makes more sense to check if it doesn't start with http(s) then to check if it start with a single /

Fix #1349. Detect http(s) and format it.
Detect if action URL in the form doesn't start with http(s) and format it properly.
@westonruter
Copy link
Member

westonruter left a comment

There are some fixes needed for PHPCS:

/includes/sanitizers/class-amp-form-sanitizer.php:70:17: error - No space found before comment text; expected "// check if action_url is a relative path and add the host to it" but found "//check if action_url is a relative path and add the host to it" (Squiz.Commenting.InlineComment.NoSpaceBefore)
/includes/sanitizers/class-amp-form-sanitizer.php:70:17: error - Inline comments must end in full-stops, exclamation marks, or question marks (Squiz.Commenting.InlineComment.InvalidEndChar)
/includes/sanitizers/class-amp-form-sanitizer.php:72:1: error - Tabs must be used to indent lines; spaces are not allowed (Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed)

Also, would you add a unit test in wp-content/plugins/amp/tests/test-amp-form-sanitizer.php to verify that the fix works as expected? Adding a new entry in \AMP_Form_Sanitizer_Test::get_data() would be sufficient.

@ricardobrg

This comment has been minimized.

Copy link
Contributor

ricardobrg commented Aug 24, 2018

@westonruter thank you for the review!

@westonruter westonruter added this to the v1.0 milestone Aug 24, 2018

@westonruter westonruter merged commit 29f50d9 into ampproject:develop Aug 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ricardobrg ricardobrg deleted the ricardobrg:patch-1 branch Aug 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment