Skip to content

Commit

Permalink
fix: stop using is_string() for validation (#233)
Browse files Browse the repository at this point in the history
Stop using is_string() for validation as it fails for strings that have
automatically been converted into integers when used as keys in
associative arrays.

Update isNullOrEmpty to check if the given value is a scalar (a string,
int, float, or bool) and then check if its string value is empty instead
of explicitly taking a string.

Remove a dictionary set fields test case that checks for an error when
given this array: [""]. PHP will implicitly treat it as this associative
array: [0 => ""], which is valid with our new validation. We need to
choose whether to support integer string keys or whether to be able to
reject this specific type of invalid input.
  • Loading branch information
nand4011 authored Oct 18, 2024
1 parent 91a18d8 commit 711683c
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 15 deletions.
16 changes: 7 additions & 9 deletions src/Utilities/_DataValidation.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@ function validateTtl($ttlSeconds): void
}

if (!function_exists('isNullOrEmpty')) {
function isNullOrEmpty(?string $str = null): bool
function isNullOrEmpty($value): bool
{
return (is_null($str) || $str === "");
// if the value is not null, check if the string value is empty. This covers strings that have been
// automatically converted into integers. Scalars cover int, float, string, and bool.
return is_null($value) || (is_scalar($value) && strval($value) === "");
}
}

Expand Down Expand Up @@ -68,7 +70,7 @@ function validateKeys(array $keys): void
foreach ($keys as $key) {
// Explicitly test type of key. If someone passes us a ["a", "b", "c"] style list upstream
// instead of a ["key"=>"val"] dict, the "keys" will be integers and we want to reject the payload.
if (!is_string($key) || isNullOrEmpty($key)) {
if (isNullOrEmpty($key)) {
throw new InvalidArgumentError("Keys must all be non-empty strings");
}
}
Expand Down Expand Up @@ -196,12 +198,8 @@ function validateSortedSetRanks(?int $startRank, ?int $endRank): void
function validateSortedSetElements(array $elements): void
{
foreach ($elements as $value => $score) {
if (!is_string($value)) {
throw new InvalidArgumentError("Sorted set value must be a string");
}

if (!is_float($score)) {
throw new InvalidArgumentException("Sorted set score must be a float");
throw new InvalidArgumentError("Sorted set score must be a float");
}

validateValueName($value);
Expand All @@ -216,7 +214,7 @@ function validateSortedSetValues(array $values): void
throw new InvalidArgumentError("sorted set values must be a non-empty array");
}
foreach ($values as $value) {
if (!is_string($value) || isNullOrEmpty($value)) {
if (isNullOrEmpty($value)) {
throw new InvalidArgumentError("sorted set values must all be non-empty strings");
}
}
Expand Down
8 changes: 2 additions & 6 deletions tests/Cache/CacheClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2119,10 +2119,6 @@ public function testDictionarySetFieldsWithNullItems_ThrowsException()
public function testDictionarySetFieldsWithEmptyItems_IsError()
{
$dictionaryName = uniqid();
$items = [""];
$response = $this->client->dictionarySetFields($this->TEST_CACHE_NAME, $dictionaryName, $items, CollectionTtl::fromCacheTtl()->withNoRefreshTtlOnUpdates());
$this->assertNotNull($response->asError(), "Expected error but got: $response");
$this->assertEquals(MomentoErrorCode::INVALID_ARGUMENT_ERROR, $response->asError()->errorCode());

$items = [];
$response = $this->client->dictionarySetFields($this->TEST_CACHE_NAME, $dictionaryName, $items, CollectionTtl::fromCacheTtl()->withNoRefreshTtlOnUpdates());
Expand Down Expand Up @@ -2240,7 +2236,7 @@ public function testDictionaryGetFieldsWithEmptyFields_IsError()
public function testDictionaryGetFields_HappyPath()
{
$dictionaryName = uniqid();
$field1 = uniqid();
$field1 = "1234"; // explicit integer-like string to make sure php casting doesn't break validation
$field2 = uniqid();
$field3 = uniqid();
$value1 = uniqid();
Expand All @@ -2267,7 +2263,7 @@ public function testDictionaryGetFields_HappyPath()
public function testDictionaryGetBatchFieldsValuesArray_HappyPath()
{
$dictionaryName = uniqid();
$field1 = uniqid();
$field1 = "1234"; // explicit integer-like string to make sure php casting doesn't break validation
$field2 = uniqid();
$field3 = uniqid();
$value1 = uniqid();
Expand Down

0 comments on commit 711683c

Please sign in to comment.