String::_source open_basedir pre-check for urandom

[ghost]

Requires open_basedir to be unset in order to use /dev/urandom. Prevents "is_readable(): open_basedir restriction in effect. File(/dev/urandom) is not within the allowed path(s)" error. This is the proper way to handle this case instead of silencing is_readable.

[mariuswilms]

As we generally need tests for changes: do you think it's possible to create one for this case? I currently cannot imagine how, as we'd need to mock open base dir. Might as well create a test that skips if open base dir is not in effect?

[ghost]

@DavidPersson What I know is that before I made the change, I got a nasty PHP warning and the random string generator was broken, and now it's working. As you said, not possible to do a test case.

You'll have to use your discretion here, this is a special case.

[mariuswilms]

I absolutely believe this is an issue. As a compromise would you add a test that skips if open base dir is not in effect and sets expectations on generated errors?

lithium/tests/cases/storage/session/strategy/HmacTest.php

Line 24 in 51aaf1d

$this->assertException('/HMAC strategy requires a secret key./', function() {

lithium/tests/cases/storage/SessionTest.php

Line 264 in 51aaf1d

$this->skipIf(!MockEncrypt::enabled(), 'The Mcrypt extension is not installed or enabled.');

Thanks!

[nateabele]

open_basedir is not ini_set()-able, so this would require a fully separate Travis build... 😐

[ghost]

Have you guys ever heard that story about a hammer factory...

[mariuswilms]

Then I'd vote for creating the conditionally run test (I've mentioned earlier), but don't create a new travis build. The decision if we may always want to run under the open_basedir restriction (kind of leads up to there), is one I don't want to make now.

[ghost]

To be clear, I just edit the String test file and add a function to test this case, and then make a pull request?

[mariuswilms]

Yes and run lithium/console/li3 test lithium/tests/cases/util/StringTest.php to check if the test skips if open_basedir isn't set, and does not skip and is green when it is set.

You can amend this PR with the test. Just push the changes into the same PR branch.

[ghost]

Done.

[mariuswilms]

I gave this issue a little bit more thought. Sorry for doing this a bit late - but better late than never ;)

ini_get('open_basedir') != true is not enough, as it would fail even if I put /dev/urandom into the open_basedir.

This would also be the strategy to resolve the problem, if you control the setting. For other environments I suggest we add other sources of randomness before urandom is selected.

This would target lithium\security\Random in 1.1 as per our release policy:
https://github.com/UnionOfRAD/lithium/blob/1.1/security/Random.php
https://github.com/ircmaxell/random_compat/blob/master/lib/random.php#L189-L194

[mariuswilms]

Checking for open_basedir isn't an option as path checking would get too complex.

For the time being I've added a note, that shows how to workaround the issue.
https://github.com/UnionOfRAD/lithium/blob/1.0/util/String.php#L118-L120

Mid-term, I'm thinking about adding more sources for randomness, or make Random adaptable.

[mariuswilms]

Refs #1216