Forms: form validation does not work when JavaScript is disabled

[talldan]

Impacted plugin

Jetpack

Quick summary

The Form block (and possibly shortcode) usually performs JavaScript form validation. See the accessible-form.js file.

There's also a lot of PHP code for form validation, which is (I assume) for when JavaScript is disabled. See the Contact_Form_Field->validate method:

jetpack/projects/packages/forms/src/contact-form/class-contact-form-field.php

Lines 204 to 266 in 7a3559d

	/**
	* Validates the form input
	*/
	public function validate() {
	// If it's not required, there's nothing to validate
	if ( ! $this->get_attribute( 'required' ) ) {
	return;
	}
	$field_id = $this->get_attribute( 'id' );
	$field_type = $this->maybe_override_type();
	$field_label = $this->get_attribute( 'label' );
	if ( isset( $_POST[ $field_id ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no site changes.
	if ( is_array( $_POST[ $field_id ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- no site changes.
	$field_value = array_map( 'sanitize_text_field', wp_unslash( $_POST[ $field_id ] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Missing -- nonce verification should happen in caller.
	} else {
	$field_value = sanitize_text_field( wp_unslash( $_POST[ $field_id ] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Missing -- nonce verification should happen in caller.
	}
	} else {
	$field_value = '';
	}
	switch ( $field_type ) {
	case 'url':
	if ( ! is_string( $field_value ) || empty( $field_value ) || ! preg_match(
	// Changes to this regex should be synced with the regex in the render_url_field method of this class as both validate the same input. Note that this regex is in PCRE format.
	'%^(?:(?:https?|ftp)://)?(?:\S+(?::\S*)?@|\d{1,3}(?:\.\d{1,3}){3}|(?:(?:[a-z\d\x{00a1}-\x{ffff}]+-?)*[a-z\d\x{00a1}-\x{ffff}]+)(?:\.(?:[a-z\d\x{00a1}-\x{ffff}]+-?)*[a-z\d\x{00a1}-\x{ffff}]+)*(?:\.[a-z\x{00a1}-\x{ffff}]{2,6}))(?::\d+)?(?:[^\s]*)?$%iu',
	$field_value
	) ) {
	/* translators: %s is the name of a form field */
	$this->add_error( sprintf( __( '%s: Please enter a valid URL - https://www.example.com', 'jetpack-forms' ), $field_label ) );
	}
	break;
	case 'email':
	// Make sure the email address is valid
	if ( ! is_string( $field_value ) || ! is_email( $field_value ) ) {
	/* translators: %s is the name of a form field */
	$this->add_error( sprintf( __( '%s requires a valid email address', 'jetpack-forms' ), $field_label ) );
	}
	break;
	case 'checkbox-multiple':
	// Check that there is at least one option selected
	if ( empty( $field_value ) ) {
	/* translators: %s is the name of a form field */
	$this->add_error( sprintf( __( '%s requires at least one selection', 'jetpack-forms' ), $field_label ) );
	}
	break;
	case 'number':
	// Make sure the number address is valid
	if ( ! is_numeric( $field_value ) ) {
	/* translators: %s is the name of a form field */
	$this->add_error( sprintf( __( '%s requires a number', 'jetpack-forms' ), $field_label ) );
	}
	break;
	default:
	// Just check for presence of any text
	if ( ! is_string( $field_value ) || ! strlen( trim( $field_value ) ) ) {
	/* translators: %s is the name of a form field */
	$this->add_error( sprintf( __( '%s is required', 'jetpack-forms' ), $field_label ) );
	}
	}
	}

This PHP validation doesn't seem to work, at least on a block theme. Submitting a form that has validation errors sends the user to a 404 page.

If this needs to be supported, then it looks like something to be fixed. Alternatively, the code could be removed if form validation with JS disabled isn't a feature that needs to be kept.

Steps to reproduce

1. Create a form with at least one required field in a post and view the post
2. In the browser dev tools, disable JS then reload the page to make sure that take effect
3. Submit the form

Expected: The form shows validation errors
Actual: A 404 page is shown. This seems to be because ?page=1 is appended to the URL.

Site owner impact

Fewer than 20% of the total website/platform users

Severity

Minor

What other impact(s) does this issue have?

No revenue impact

If a workaround is available, please outline it here.

No response

Platform (Simple and/or Atomic)

Self-hosted

[talldan]

The first issue is very easy to fix. There's a second issue, it now takes a user two submissions for the validation messages to show, and the form will be reset the first time. I think this is due to the form's internal id that it uses for state tracking changing after the submission. Essentially there's then a second instance of the form in state even though it's the same form.

Edit: It's actually that the form rendering happens twice, so it ends up being like there are two forms with mismatched ids, the process_form_submission method seems to be the culprit, here's the stack trace:

[talldan]

I see this bug seems to have been introduced fairly recently via #40998. Basically the incrementing id means forms aren't idempotent, if the same form is run through the rendering code twice it'll receive a different id and cause a mismatch..

@simison just making you aware of this issue. It seems very low priority to me and I can push a small fix to solve the 404, but I don't plan to look much deeper. I just need to be able to test that the block based field has parity with the shortcode for #41840, and this bug was blocking me.

That said, if the form ids are brittle, they might cause other problems. I personally think a more solid solution is to use a uuid style id that's stored with the form markup, but I might not be aware of some of the constraints.

[talldan]

Another thing related to this that I've discovered on my journey through the form block. If using the form block in a post, the form submission expects the form id to be the post id. This code here is passing the $id into functions like is_post_publicly_viewable, current_user_can and post_password_required:

jetpack/projects/packages/forms/src/contact-form/class-contact-form-plugin.php

Lines 732 to 740 in 49183ea

	if ( ! is_post_publicly_viewable( $id ) && ! current_user_can( 'read_post', $id ) ) {
	// The user can't see the post.
	return false;
	}
	if ( post_password_required( $id ) ) {
	// The post is password-protected and the password is not provided.
	return false;
	}

That $id variable is retrieved here:

jetpack/projects/packages/forms/src/contact-form/class-contact-form-plugin.php

Line 621 in 49183ea

$id = isset( $_POST['contact-form-id'] ) ? sanitize_text_field( wp_unslash( $_POST['contact-form-id'] ) ) : null;

This isn't going to work so well with the way the id is now a lot more complex when initially set:

jetpack/projects/packages/forms/src/contact-form/class-contact-form.php

Line 126 in 49183ea

$attributes['id'] = $attributes['id'] . '-' . ( count( self::$forms ) + 1 ) . '-' . $page;

It's really surprising that the id is being suddenly converted to a post id this way.