Improve Test Coverage Even More

[defuse]

We need this to avoid problems like #384. Maybe there's some static analysis tool we can add to travis that makes sure everything is fine for a given version of PHP?

[defuse]

Okay, so #384 turns out to not be an issue after all. But I'm still worried something like this could happen in the future.

[paragonie-scott]

Travis CI is supposed to catch these failures.

[defuse]

Travis would miss something like...

<?php
if ($argv[1] == "crash") {
    throw new \TypeError("blah");
}
?>

...unless that branch actually gets taken on Travis. So Travis can miss non-supported language features if they're in very rarely taken branches that aren't fully covered by test cases. So to really resolve this we either need near-100% code coverage in our tests or some sort of static analysis tool that identifies unsupported language features (but such a tool probably can't exist, or will false positive like crazy, since it can't determine if the thing that's missing will be polyfilled at runtime).

[defuse]

Googling lead me to a php compatibility checker. Let's see how many false positives this gives.

[defuse]

That thing doesn't do what I want either. If I replace all occurrences of TypeError except for the one on line 59 with ThisDoesNotExistAtAllin Crypto.php, neither our unit tests or that tool catches it. So... I guess the only way to resolve this is to have excellent code coverage.

[defuse]

We're now measuring code coverage! https://codecov.io/gh/defuse/php-encryption

[defuse]

Looking through these coverage results, branches that are feasible to test (e.g. excluding things like code paths that require the CSPRNG to be unavailable, since those are hard to test) but aren't being tested are...

Core.php

• integer overflow detection in counter (dead code, a check earlier on catches it)
• Bad HKDF output length.
• ensureConstant exists's exception throw case (pass a constant name that doesn't exist)
• ensureFunctionExists's exception throw case (pass a function name that doesn't exist)
• ourSubstr with negative $start
• ourSubstr should throw an exception when $length is negative
• passing invalid types for the $algorithm, $password, $salt parameters (non-strings) of pbkdf2
• passing an invalid hash algorithm to pbkdf2
• passing an algorithm that's valid but not whitelisted (e.g. CRC32) to pbkdf2
• test hex output of PBKDF2 (ensure it generates the correct number of hex characters)

Crypto.php

• TypeError exceptions for encrypt's $plaintext (string), $raw_binary (bool).
• TypeErrors for encryptWithPassword
• TypeErrors for decrypt
• TypeErrors for decryptWithPassword
• TypeErrors for legacydecrypt
• legacyDecrypt with a ciphertext that is too short (shorter than the MAC size)
• lecacyDecrypt with a ciphertext that is too short (longer than the MAC size but doesn't have the IV) this can't be tested because it's after the MAC check
• A lot of the if ... throw EnvironmentIsBrokenException could be replaced with a call to a function called assertOrEnvironmentIsBroken($bool, $message), so those lines would be green, and the code would look cleaner. This applies to all other files too.

Encoding.php

• The point above would give this file 100% coverage.

File.php

• There is actually a dead code path in encryptFileInternal (it tries to catch a CryptoException from encryptResourceInternal but encryptResourceInternal can't throw a CryptoException, at least not according to its docblock).
• There are some branches where we'd need to have a stream's fclose fail to test those paths, which might be possible somehow with mocking.
• A bunch where we'd need to simulate ftell failing.
• A bunch where we'd need to simulate fseek failing.
• The check for $macs being empty might be dead, the loop should always add at least one MAC to the array.
• Test the case where the file is modified during decryption, after the MAC verification.
• Test readBytes encountering a read error (might be hard).
• Test readBytes reading past the end of the file (should be easy to test).
• We never call writeBytes with a non-null $num_bytes so either test it or remove $num_bytes altogether and write out all of $buf.
• If we keep writeByte's $num_bytes, test the exceptions get thrown when it's larger than the length of $buf or negative.
• Test writeBytes encountering a write error (might be hard).

Key.php

• Will have 100% coverage if we make the EnvironmentIsBrokenException a one-line assert function call.

KeyOrPassword.php

• Make the constructor check that $secret's type is consistent with $secret_type and remove the line that checks in deriveKeys. And add a test for erroneous calls with (Key, string), (password, Key).

KeyProtectedByPassword.php

• Test trying to unlock a bad-format encrypted key (e.g. a string that's shorter than the version header), should throw the exception.

RuntimeTests.php

• We can remove the catch and $test_state 3 by just erroring immediately if $test_state === 2. If $test_state === 2 the only possible explanation assuming a single-threaded program was that an exception was thrown during the test and ignored by the past caller.
• Most of the uncovered lines in here can't be covered without mocking/breaking stuff.

[defuse]

Working on this in #398.

[defuse]

Now that #398 landed, I'm satisfied we have enough test coverage for v2.2. There's still room for improvement, but it can wait until later.

[defuse]

We have pretty good coverage, and we get diminishing returns for the effort required to bring it higher. Let's just be careful about code review and make sure new features/code come with good tests.