Make check for meta tag to verify site more permissive by Tug · Pull Request #10040 · Automattic/jetpack

Tug · 2018-08-21T18:26:25Z

Changes proposed in this Pull Request:

The validation for meta tags entered in the Jetpack Settings page for site verification is overly strict. In fact, even the same meta tags we offer in our alternative UI (under "tools" in the wp-admin menu) don't work! (because they aren't self-closing tags, lack a space before the >, and use single instead of double quotes).

This PR updates the input validation we have for meta tags used for website verification services. We just want a meta tag that is valid enough so we can extract the value of the content property from it. HTML parser on browsers are quite flexible so we need to allow all kinds of valid meta tags.
For instance this change makes it so those are valid:

<meta name="google-site-verification" content="1234"/> (no space before />)
<meta name='google-site-verification' content='1234' /> (use of ' instead of ")
<meta name='google-site-verification' content=1234 /> (does not use any quotes)
<meta content="1234" name="google-site-verification" /> (switches the order)
<meta name="google-site-verification" content="1234" some-prop /> (has extra properties)
<meta name="google-site-verification" content="1234"> (does not have a closing character)
...

Testing instructions:

Enter any of the formats above, both in the Jetpack Settings UI, and the same UI under "tools".

The content should be saved successfully without validation errors.

Enter a "bad" string in the JS UI, e.g. <moota name="google-site-verification" content="1234"/>

The content should fail to save with a validation error.

Note that the legacy UI under "tools" will actually accept <moota name="google-site-verification" content="1234"/> - that UI is going to be removed eventually so it's less of a concern.

Proposed changelog entry for your changes:

Update input validation for meta tags given in site verification

…elds

…sive

jetpackbot · 2018-08-21T18:29:06Z

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

gravityrail · 2018-08-21T22:21:12Z

I found a bug: pasting in a meta tag with single quotes and the meta name "blah" results in the following output in the HTML:

<meta name="google-site-verification" content="&lt;meta name=&#039;blah&#039; content=&#039;dBw5CvburAxi537Rp9qi5uG2174Vb6JwHwIRwPSLIK8&#039;&gt;" />

Trying to fix now.

Tug · 2018-08-21T22:27:40Z

@gravityrail it's not a regex that's able to parse any html property value, just a verification code which according to the regex for the code only (on the same line) is simply [a-z0-9_-]+

gravityrail · 2018-08-21T22:30:47Z

I get the same issue when I enter this value:

<meta name='google-site-verification' content='dBw5CvburAxi537Rp9qi5uG2174Vb6JwHwIRwPSLIK8'>

The meta tag shows:

<meta name="google-site-verification" content="&lt;meta name=&#039;google-site-verification&#039; content=&#039;dBw5CvburAxi537Rp9qi5uG2174Vb6JwHwIRwPSLIK8&#039;&gt;" />

gravityrail · 2018-08-21T22:32:03Z

But it works correctly when I enter:

<meta name="google-site-verification" content="dBw5CvburAxi537Rp9qi5uG2174Vb6JwHwIRwPSLIK8" />

(same values as above, but tag is self-closing and uses double-quotes)

This outputs correctly:

<meta name="google-site-verification" content="dBw5CvburAxi537Rp9qi5uG2174Vb6JwHwIRwPSLIK8" />

gravityrail

Works now - hooray!

cfinke · 2018-08-22T02:55:50Z

This line:

foreach( [ 'google', 'bing', 'yandex' ] as $key ) {

isn't valid syntax in PHP 5.2 and is preventing new builds from being created by TravisCI, so I can't test the current code.

mdawaffe · 2018-08-22T03:12:37Z

_inc/lib/class.core-rest-api-endpoints.php

 	 */
 	public static function validate_verification_service( $value = '', $request, $param ) {
-		if ( ! empty( $value ) && ! ( is_string( $value ) && ( preg_match( '/^[a-z0-9_-]+$/i', $value ) || preg_match( '#^<meta name="([a-z0-9_\-.:]+)?" content="([a-z0-9_-]+)?" />$#i', $value ) ) ) ) {
+		if ( ! empty( $value ) && ! ( is_string( $value ) && ( preg_match( '/^[a-z0-9_-]+$/i', $value ) || preg_match( '#^<meta[^<>]+content=["\']?([^"\' <>]*)[^<>]*/?>$#i', $value ) ) ) ) {


If the content attribute has an apostrophe or quotation mark in it, this regex will fail.

<meta content="Jetpack's the best" />

It could also perceive this HTML as passing the check:

<meta name="content=" />

Or even <meta-content=>. Instead of duplicating work, this function should be calling jetpack_verification_get_code() and checking if it returns something, since that's the code that will actually be extracting the verification code. (jetpack_verification_get_code() should also be given a second look, as its regex is pretty liberal as well.)

As I said here this regex will only work for valid content values. I agree it's cleaner to use jetpack_verification_get_code() here instead though 👍I'll update!

Tug · 2018-08-22T13:36:57Z

I updated the form on the client to replace the code value by the full meta tag. On the server nothing is changed and the code value is persisted by default (and the code stays backward-compatible with full meta tag saved in the db).

…ent="value" part

…the client

mdawaffe · 2018-08-22T18:41:03Z

I don't think it matters too much that the regex in jetpack_verification_get_code() is a bit forgiving. It seems to me the failure case is: someone didn't paste quite the right thing, so they try again.

The latest changes have a some failing tests, though.

#9958 #9983 #9992 #10002 #10004 #10010 #10011 #10017 #10031 #10037 #10039 #10040 #10051

#9958 #9983 #9992 #10002 #10004 #10010 #10017 #10031 #10037 #10039 #10040

Tug added 2 commits August 21, 2018 17:20

Use new syntax of register_setting for verification_services_codes_fi…

3e1b1f4

…elds

Update regex to check the meta tag input to verify a site more permis…

e7d3dbe

…sive

Tug added the [Status] In Progress label Aug 21, 2018

Tug self-assigned this Aug 21, 2018

Tug requested a review from a team as a code owner August 21, 2018 18:26

gravityrail self-requested a review August 21, 2018 22:12

PHP 5.2 array

c77b98c

gravityrail added 2 commits August 21, 2018 15:47

Validate meta tag or value on rest_api_init

af068cf

Fix broken test

0cc07fe

gravityrail reviewed Aug 21, 2018

View reviewed changes

gravityrail approved these changes Aug 21, 2018

View reviewed changes

gravityrail added [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended and removed [Status] In Progress labels Aug 21, 2018

gravityrail added this to the 6.5 milestone Aug 21, 2018

Function-style array literal to appease PHP 5.2

9884674

mdawaffe requested changes Aug 22, 2018

View reviewed changes

Tug added 2 commits August 22, 2018 12:32

call jetpack_verification_get_code from validate_verification_service

b704c32

Always show the full tag on the UI form

f04926d

Tug added 2 commits August 22, 2018 15:51

Allow invalid HTML to be given as input as long as it contains a cont…

6d55c74

…ent="value" part

Refresh settings after save as the updated array is not forwarded to …

15f7a48

…the client

mdawaffe approved these changes Aug 22, 2018

View reviewed changes

Replace verification_tools tests

86e98f3

cbauerman added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 22, 2018

abidhahmed merged commit 51564c2 into master Aug 28, 2018

matticbot added the [Status] Needs Changelog label Aug 28, 2018

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Aug 28, 2018

abidhahmed deleted the fix/meta-tag-verify-site branch August 28, 2018 07:39

abidhahmed added the General label Aug 28, 2018

abidhahmed added [Status] Has Changelog and removed [Status] Needs Changelog labels Aug 28, 2018

oskosk pushed a commit that referenced this pull request Aug 28, 2018

[skip-ci] Added changelogs

7c86fd2

#9958 #9983 #9992 #10002 #10004 #10010 #10011 #10017 #10031 #10037 #10039 #10040 #10051

oskosk pushed a commit that referenced this pull request Aug 28, 2018

[skip-ci] Add test instructions

1942746

#9958 #9983 #9992 #10002 #10004 #10010 #10017 #10031 #10037 #10039 #10040

jeherve added [Feature] Verification tools Admin Page React-powered dashboard under the Jetpack menu and removed General labels Sep 5, 2018

This was referenced Sep 5, 2018

Verification Tools: Update example text #8485

Closed

Verification Tools: strip meta tag and only keep verification code on save. #5782

Closed

Conversation

Tug commented Aug 21, 2018 • edited by gravityrail Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

jetpackbot commented Aug 21, 2018

Uh oh!

gravityrail commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tug commented Aug 21, 2018

Uh oh!

gravityrail commented Aug 21, 2018

Uh oh!

gravityrail commented Aug 21, 2018

Uh oh!

gravityrail left a comment

Choose a reason for hiding this comment

Uh oh!

cfinke commented Aug 22, 2018

Uh oh!

mdawaffe Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

cfinke Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

cfinke Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

Tug Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tug commented Aug 22, 2018

Uh oh!

mdawaffe commented Aug 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Tug commented Aug 21, 2018 •

edited by gravityrail

Loading

gravityrail commented Aug 21, 2018 •

edited

Loading

Tug Aug 22, 2018 •

edited

Loading