- ๐บ๐ธUnited States smustgrave
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.
- Status changed to Needs review
10 months ago 2:23pm 4 November 2023 - ๐ท๐บRussia kostyashupenko Omsk
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
- Status changed to Needs work
10 months ago 4:08pm 4 November 2023 - ๐บ๐ธUnited States smustgrave
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.
- ๐ซ๐ทFrance andypost
updated summary with steps and title
it needs some javascript test which I not ready to write atm
- ๐บ๐ธUnited States dww
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. - last update
10 months ago 30,486 pass - ๐ฎ๐ณIndia Akhil Babu Chengannur
Akhil Babu โ made their first commit to this issueโs fork.
- Merge request !6286Issues/3010895: Empty state is wrong for input when filled with whitespaces. โ (Open) created by Akhil Babu
- Status changed to Needs review
8 months ago 12:17pm 23 January 2024 - ๐ฎ๐ณIndia Akhil Babu Chengannur
Tested patch #19, and it works as expected. Raising it as a merge request along with tests to validate the states.
- Status changed to RTBC
8 months ago 3:33pm 25 January 2024 - ๐บ๐ธUnited States smustgrave
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.
- ๐ซ๐ทFrance nod_ Lille
I was on the fence, but our php validation does consider blank spaces as empty
- Status changed to Needs work
7 months ago 6:01am 7 February 2024 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
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.
- Status changed to RTBC
7 months ago 3:21pm 9 February 2024 - ๐ฎ๐ณIndia Akhil Babu Chengannur
I have added a change record for this.
https://www.drupal.org/node/3420453 โ
Moving this ticket back to RTBC as per #30. - Status changed to Needs work
7 months ago 6:18am 12 February 2024 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
According to the docs, there's some slight nuances between what
trim()
in PHP trims andString.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.
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
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. - Status changed to Needs review
about 2 months ago 6:59am 15 July 2024 - Status changed to RTBC
about 2 months ago 2:39pm 22 July 2024 - ๐บ๐ธUnited States smustgrave
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.
- Status changed to Needs work
about 2 months ago 6:58pm 22 July 2024 - First commit to issue fork.
- Status changed to Needs review
about 2 months ago 11:50am 23 July 2024 - ๐ฎ๐ณIndia amanmansuri72
Removing duplication of regex with a variable
- Status changed to RTBC
about 1 month ago 6:48pm 29 July 2024 - Status changed to Needs work
30 days ago 6:19am 9 August 2024 - ๐ณ๐ฟNew Zealand quietone New Zealand
Made some comments in the MR that require a bit more work.