Background

`DrupalTestCase::randomString` generates a random string, which can be used in various places for unit testing. For example, it can be used to generate a random user name, which then gets created to support testing.

Currently, the code is

public static function randomString($length = 8) {
  $str = '';
  for ($i = 0; $i < $length; $i++) {
    $str .= chr(mt_rand(32, 126));
  }
  return $str;
}

This will generate a truly random string, with a uniform distribution of characters.

Problem

When `DrupalTestCase::randomString` is used in places where it is output to HTML, the string may not be sufficient to fully satisfy a test. This happens in places where the random string is used in place of user input and needs to be escaped for output. When the random string doesn't contain an escapable character, a test may falsely come back as PASSED, and this has been demonstrated to happen with some tests in HEAD. The current algorithm will not reliably detect unescaped string, nor doubly escaped strings.

Proposed Solution

`DrupalTestCase::randomString` should be refactored to always include an escapable character to ensure that tests results are accurate.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

I considered to inject an opening angle bracket (<) instead, but since we're generating random characters, it wouldn't have the expected effect anyway, since the valid character range for an HTML element tag is limited (and I think the tag/element would also have to be closed, in order to have a visible/significant effect). So there's not really a difference between & and <, I think.

penyaskito’s picture

I was pretty embarrassed when I broke the build in my first core patch, so +1 to this change.

Also helping pushing the inclusion of #1659682: Consider renaming randomString() and/or adding additional, context-appropriate helpers for random string generation and/or #1676910: Rename randomName() to randomMachineName() would help people to call the most appropriate method(s), reducing this "random" failures.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, makes sense, and works as intended (a little weird in the case where $length is 1, but I guess that doesn't really matter in practice).

David_Rothstein’s picture

drupal8.test-randomstring.0.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm, nice!

However, we should update the PHPDoc of this function accordingly to make it clear what's happening and when to use it.

David_Rothstein’s picture

Status: Needs work » Needs review

I read through the documentation at http://api.drupal.org/api/drupal/core!modules!simpletest!lib!Drupal!simpletest!TestBase.php/function/TestBase%3A%3ArandomString/8 and don't immediately see anything that needs to be changed as a result of this.

What do you think should be changed?

penyaskito’s picture

Issue summary: View changes

Generates a unique random string of ASCII characters of codes 32 to 126.

Should we change that to

Generates a unique pseudo-random string of ASCII characters of codes 32 to 126.

?

penyaskito’s picture

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 8: drupal8.test-randomstring.8.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
675 bytes

Oops. Previous approach was totally wrong.

penyaskito’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
penyaskito’s picture

Here it goes.

pcambra’s picture

  1. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -1298,22 +1298,32 @@ protected function settingsSet($name, $value) {
    +   * Generates a unique pseudo-random string of ASCII characters of codes 32 to 126.
    

    Exceeds 80 chars

  2. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -1298,22 +1298,32 @@ protected function settingsSet($name, $value) {
    +    return implode('', $parts);
    

    Isn't enough with implode('&', $parts)?

Do we need a test for this?

penyaskito’s picture

Right, this is far more readable IMHO.
Created a very simple unit test.

pcambra’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine to me

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -1298,22 +1298,32 @@ protected function settingsSet($name, $value) {
        * Do not use this method when special characters are not possible (e.g., in
        * machine or file names that have already been validated); instead, use
        * \Drupal\simpletest\TestBase::randomMachineName().
    +   * For avoiding random test failures, special characters will be ensured.
    

    Lets join the paragraphs together and the final sentence should read something like At least one special character is ensured to avoid random test failures..

  2. +++ b/core/modules/simpletest/tests/src/TestBaseTest.php
    @@ -63,4 +63,17 @@ public function testRandomStringValidate($string, $expected) {
    +    $this->assertTrue(strpos($string, '&') > 0);
    

    This can randomly fail since a random string could start with an ampersand.

xjm’s picture

Assigned: Unassigned » xjm

Fixing #17.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
2.62 KB
2.44 KB
xjm’s picture

Caught another typo.

The last submitted patch, 19: simpletest-1860594-19.patch, failed testing.

alexpott’s picture

FileSize
2.62 KB
2.93 KB

Realised we also have a problem with very short random strings. The patch in #20 (and all the ones before) will thrown an exception if you pass a value of 1 as $length. I think if the length is less than 3 we should not bother with adding the ampersand.

xjm’s picture

Good catch.

xjm’s picture

damiankloip’s picture

FileSize
2.92 KB
1.26 KB

Glad I found this issue, this could remove a lot of silly random failures.

This needed a reroll. So did that, then thought it might be better to just use substr_replace() instead of the whole $parts/implode() dance. This seems much easier to me, and avoids another call to $this->getRandomGenerator()->string(). Thoughts?

alexpott’s picture

Status: Needs review » Needs work

@damiankloip the problem with that approach is that it would make it technically possible to generate the same random string twice ... for example if the generator returns WETGE this would become WE&GE and if the generator returned WEAGE it would become WE&GE. This type of thing can also lead to random fails.

alexpott’s picture

Perhaps as @chx suggests return $this->getRandomGenerator()->string($length - 1, TRUE, array($this, 'randomStringValidate')) . '&'; might be the best solution. It should not matter where the & appears right?

damiankloip’s picture

Status: Needs work » Needs review

Hmm, I see how the chances could maybe be increased. Could still happen both ways I guess.

I don't see how the suggestion in #27 makes this any better though? Just moves the & to the end.

Also, Your example is not correct, in my patch from #25 you would get WET&GE and WEA&GE. It is added, not replaced (with those params). So I think that approach is good tbh.

EDIT: See the test coverage with the assertion on the string length. It's correct, with the -1 taken from the length param.

alexpott’s picture

@damiankloip ah yes actually the approach in #25 can't get duplicates so everything is looking good - ignore #26 and #27.

damiankloip’s picture

Super, let's proceed. Anyone want to RTBC that?

dawehner’s picture

Did we considered to also add some html tag, like ?

damiankloip’s picture

Yes. I thibk so. Read the first few comments.

dawehner’s picture

  1. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -1283,22 +1283,35 @@ protected function settingsSet($name, $value) {
    +    $replacement_pos = floor($length / 2);
    ...
    +    return substr_replace($string, '&', $replacement_pos, 0);
    

    Did we considered to make the position also random?

  2. +++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
    @@ -63,4 +63,21 @@ public function testRandomStringValidate($string, $expected) {
    +    $this->assertEquals(8, strlen($string));
    +    $this->assertTrue(strpos($string, '&') !== FALSE);
    ...
    +    $string = $this->stub->randomString(1);
    +    $this->assertEquals(1, strlen($string));
    

    Let's ensure that '&' is not in that string in the case of a short string.

damiankloip’s picture

FileSize
2.95 KB
812 bytes

Good spot with the test assertion. Changed the other one to use assertContains() too. I think the random & position is a step too far here? I don't feel strongly though.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/tests/src/Unit/TestBaseTest.php
@@ -73,11 +73,12 @@ public function testRandomStringValidate($string, $expected) {
+    $this->assertNotContains('&', $string);

Hmmm... this will produce random fails since we're not stopping randomString from generating an ampersand.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
553 bytes

Hmm, good point. Blame Daniel :p

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Meh.

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work

The code does what's in the title and there seems to be a lot of buy in here but I don't understand what we're buying in to. Can someone update the summary before we commit it?

mpdonadio’s picture

Issue summary: View changes

Summarized the background, problem, and proposed solution.

mpdonadio’s picture

Status: Needs work » Needs review

Grrr; reset status.

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

<rant>I still maintain that better tests would handle this problem better. I don't believe this will actually magically solve anything because we're not implementing any assertions in methods calling this. The only failure we could hope to trigger with this by itself is a failure of the database to escape something and & doesn't accomplish this. Furthermore, the fact that this method has caught bugs in the past or could surface bugs in the future is not proof that is works but symptomatic of bad tests. If a method expects escaping, it should be testing that and any bug that we might find from this should trigger an explicit test with the fix.</rant>

Even so, for what it is, the patch is not about removing randomString and does what it claims and it doesn't hurt anything so I'm putting it back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I think this is useful for flushing out the bad tests. Committed/pushed to 8.0.x, thanks!

  • catch committed 2f9c1bc on 8.0.x
    Issue #1860594 by penyaskito, damiankloip, xjm, alexpott, sun: Ensure...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.