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.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-1860594-36.txt | 553 bytes | damiankloip |
#36 | 1860594-36.patch | 2.91 KB | damiankloip |
#20 | simpletest-1860594-20.patch | 2.62 KB | xjm |
#19 | interdiff-19.txt | 2.44 KB | xjm |
#19 | simpletest-1860594-19.patch | 2.62 KB | xjm |
Comments
Comment #1
sunI 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.Comment #2
penyaskitoI 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.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedLooks 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).
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commenteddrupal8.test-randomstring.0.patch queued for re-testing.
Comment #5
webchickHm, nice!
However, we should update the PHPDoc of this function accordingly to make it clear what's happening and when to use it.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?
Comment #7
penyaskitoShould we change that to
?
Comment #8
penyaskitoRerolled.
Comment #10
penyaskitoOops. Previous approach was totally wrong.
Comment #11
penyaskitoComment #12
penyaskitoRerolled. Only conflicted with #1676910: Rename randomName() to randomMachineName().
Comment #13
penyaskitoHere it goes.
Comment #14
pcambraExceeds 80 chars
Isn't enough with implode('&', $parts)?
Do we need a test for this?
Comment #15
penyaskitoRight, this is far more readable IMHO.
Created a very simple unit test.
Comment #16
pcambraLooks fine to me
Comment #17
alexpottLets join the paragraphs together and the final sentence should read something like
At least one special character is ensured to avoid random test failures.
.This can randomly fail since a random string could start with an ampersand.
Comment #18
xjmFixing #17.
Comment #19
xjmComment #20
xjmCaught another typo.
Comment #22
alexpottRealised 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.
Comment #23
xjmGood catch.
Comment #24
xjmMarked #1659682: Consider renaming randomString() and/or adding additional, context-appropriate helpers for random string generation as a duplicate of this issue.
Comment #25
damiankloip CreditAttribution: damiankloip commentedGlad 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?Comment #26
alexpott@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 becomeWE&GE
and if the generator returnedWEAGE
it would becomeWE&GE
. This type of thing can also lead to random fails.Comment #27
alexpottPerhaps 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?Comment #28
damiankloip CreditAttribution: damiankloip commentedHmm, 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
andWEA&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.
Comment #29
alexpott@damiankloip ah yes actually the approach in #25 can't get duplicates so everything is looking good - ignore #26 and #27.
Comment #30
damiankloip CreditAttribution: damiankloip commentedSuper, let's proceed. Anyone want to RTBC that?
Comment #31
dawehnerDid we considered to also add some html tag, like ?
Comment #32
damiankloip CreditAttribution: damiankloip commentedYes. I thibk so. Read the first few comments.
Comment #33
dawehnerDid we considered to make the position also random?
Let's ensure that '&' is not in that string in the case of a short string.
Comment #34
damiankloip CreditAttribution: damiankloip commentedGood 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.
Comment #35
alexpottHmmm... this will produce random fails since we're not stopping randomString from generating an ampersand.
Comment #36
damiankloip CreditAttribution: damiankloip commentedHmm, good point. Blame Daniel :p
Comment #37
dawehnerMeh.
Comment #38
neclimdulThe 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?
Comment #39
mpdonadioSummarized the background, problem, and proposed solution.
Comment #40
mpdonadioGrrr; reset status.
Comment #41
neclimdul<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.
Comment #42
catchI think this is useful for flushing out the bad tests. Committed/pushed to 8.0.x, thanks!