Empty state is wrong for input when filled with whitespaces

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Think we will need a test case to show what this solving?

Tagging for IS also for what the problem is.

Correct me if i'm wrong, but it's s security task. Wondering why it's still not merged?
Just realized with `webform` contrib module state `Empty` doesn't work still (really!) if text fields for example are filled with whitespaces only. Which means users can attack web-sites by filling only whitespaces and submit.

Here is reroll against 11.x (but i almost sure it should be merged directly in 10.x because of security risks). This patch is working also against 10.x

Not sure if it is a security issue, but if it is don't think it's severe enough to skip straight to merge. So will still need an issue summary update and test case showing this problem.

If you're on slack would post in #security-discussion to see.

updated summary with steps and title

it needs some javascript test which I not ready to write atm

You don't have to write new test for this, extend the existing #states test in core:

core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php

Meanwhile, this sounds like a bug, not a feature request. However, definitely *not* a "security" bug. No one should be relying on #states to enforce anything for their site, since it can be trivially subverted by a client that disables JS. Even the "required" toggle via #states is kinda bogus -- it literally just toggles the little red star for display, it doesn't actually make the field required. No, if you need validation on a form, you must use server-side validation and/or Entity constraints, not trying to let #states impose anything on the front end.

Akhil Babu → made their first commit to this issue’s fork.

Tested patch #19, and it works as expected. Raising it as a merge request along with tests to validate the states.

Ran tests-only feature https://git.drupalcode.org/issue/drupal-3010895/-/jobs/678951 and coverage appears to be there

Hiding patches for clarity.

Issue summary of using trim() matches solution.

Believe tests cover all input tags.

Since this is a minor think it's fine to mark now vs waiting a few days.

I was on the fence, but our php validation does consider blank spaces as empty

I think there is an outside chance this will be disruptive. Because of that I think we need a change record here.

I've confirmed we indeed trim strings in our PHP form validation.

In \Drupal\Core\Form\FormValidator::doValidateForm

$is_empty_string = (is_string($elements['#value']) && mb_strlen(trim($elements['#value'])) == 0);

Can we please have a change record - fine to self RTBC afterwards.

I have added a change record for this.
https://www.drupal.org/node/3420453 →
Moving this ticket back to RTBC as per #30.

According to the docs, there's some slight nuances between what trim() in PHP trims and String.prototype.trim() does in Javascript - with the Javascript version removing more characters.

See https://www.php.net/trim vs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

https://3v4l.org/HHjqS vs this in your console console.log("\u00A0".trim().length)

I think we probably want them to be consistent?

Can we use regex and string function like replace to get it more consitent with the PHP's trim function.

console.log("\u00A0".replace('/^[\t\n\v\f\r ]+|[\t\n\v\f\r ]+$/g', '').length)

give the results comparable with https://3v4l.org/HHjqS.

Can we use regex and string function like replace to get it more consitent with the PHP's trim function.

That should work. Since FunctionalJavascript Tests have access to both PHP and JS (via evaluateScript) you could quickly iterate through all sorts of potential strings and confirm the results are identical, and if there are use cases that won't pass the test provides a clean way for a l33t regexer to hop in and potentially solve it fully.

Removing CR tag as it appears to have been added, with example also

Ran test-only feature

1) Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest::testJavascriptStates
Failed asserting that true is false.
/builds/issue/drupal-3010895/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:355
/builds/issue/drupal-3010895/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:69
FAILURES!
Tests: 1, Assertions: 159, Failures: 1.
Exiting with EXIT_CODE=1

So test coverage appears to be there.

Use of regex makes sense so believe this one to be good.

small detail

amanmansuri72 → made their first commit to this issue’s fork.

Removing duplication of regex with a variable

Feedback appears to be addressed.

Made some comments in the MR that require a bit more work.