From 7e2f755dfd26e6cc115a9763a915e877496cdd9f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 18 Jul 2024 21:23:48 +0200 Subject: [PATCH] Validate IP address patterns when creating ip-address redirect conditions --- .../Validation/RedirectRulesInputFilter.php | 16 +++-- module/Core/src/Util/IpAddressUtils.php | 31 +++++++-- .../Model/RedirectRulesDataTest.php | 67 +++++++++++++++++++ module/Core/test/Util/IpAddressUtilsTest.php | 26 +++++++ .../test-api/Action/SetRedirectRulesTest.php | 65 ++++++++++++++++-- 5 files changed, 185 insertions(+), 20 deletions(-) create mode 100644 module/Core/test/Util/IpAddressUtilsTest.php diff --git a/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php b/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php index 5decaf4c..892b93e4 100644 --- a/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php +++ b/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php @@ -12,6 +12,7 @@ use Shlinkio\Shlink\Common\Validation\InputFactory; use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; +use Shlinkio\Shlink\Core\Util\IpAddressUtils; use function Shlinkio\Shlink\Core\ArrayUtils\contains; use function Shlinkio\Shlink\Core\enumValues; @@ -71,13 +72,14 @@ class RedirectRulesInputFilter extends InputFilter $redirectConditionInputFilter->add($type); $value = InputFactory::basic(self::CONDITION_MATCH_VALUE, required: true); - $value->getValidatorChain()->attach(new Callback(function (string $value, array $context) { - if ($context[self::CONDITION_TYPE] === RedirectConditionType::DEVICE->value) { - return contains($value, enumValues(DeviceType::class)); - } - - return true; - })); + $value->getValidatorChain()->attach(new Callback( + fn (string $value, array $context) => match ($context[self::CONDITION_TYPE]) { + RedirectConditionType::DEVICE->value => contains($value, enumValues(DeviceType::class)), + RedirectConditionType::IP_ADDRESS->value => IpAddressUtils::isStaticIpCidrOrWildcard($value), + // RedirectConditionType::LANGUAGE->value => TODO, + default => true, + }, + )); $redirectConditionInputFilter->add($value); $redirectConditionInputFilter->add( diff --git a/module/Core/src/Util/IpAddressUtils.php b/module/Core/src/Util/IpAddressUtils.php index f5e1b052..66354c37 100644 --- a/module/Core/src/Util/IpAddressUtils.php +++ b/module/Core/src/Util/IpAddressUtils.php @@ -18,6 +18,11 @@ use function str_contains; final class IpAddressUtils { + public static function isStaticIpCidrOrWildcard(string $candidate): bool + { + return self::candidateToRange($candidate, ['0', '0', '0', '0']) !== null; + } + /** * Checks if an IP address matches any of provided groups. * Every group can be a static IP address (100.200.80.40), a CIDR block (192.168.10.0/24) or a wildcard pattern @@ -40,15 +45,29 @@ final class IpAddressUtils $ipAddressParts = explode('.', $ipAddress); - return some($groups, function (string $value) use ($ip, $ipAddressParts): bool { - $range = str_contains($value, '*') - ? self::parseValueWithWildcards($value, $ipAddressParts) - : Factory::parseRangeString($value); - - return $range !== null && $ip->matches($range); + return some($groups, function (string $group) use ($ip, $ipAddressParts): bool { + $range = self::candidateToRange($group, $ipAddressParts); + return $range !== null && $range->contains($ip); }); } + /** + * Convert a static IP, CIDR block or wildcard pattern into a Range object + * + * @param string[] $ipAddressParts + */ + private static function candidateToRange(string $candidate, array $ipAddressParts): ?RangeInterface + { + return str_contains($candidate, '*') + ? self::parseValueWithWildcards($candidate, $ipAddressParts) + : Factory::parseRangeString($candidate); + } + + /** + * Try to generate an IP range from a wildcard pattern. + * Factory::parseRangeString can usually do this automatically, but only if wildcards are at the end. This also + * covers cases where wildcards are in between. + */ private static function parseValueWithWildcards(string $value, array $ipAddressParts): ?RangeInterface { $octets = explode('.', $value); diff --git a/module/Core/test/RedirectRule/Model/RedirectRulesDataTest.php b/module/Core/test/RedirectRule/Model/RedirectRulesDataTest.php index f0ded32b..e71140cb 100644 --- a/module/Core/test/RedirectRule/Model/RedirectRulesDataTest.php +++ b/module/Core/test/RedirectRule/Model/RedirectRulesDataTest.php @@ -51,9 +51,76 @@ class RedirectRulesDataTest extends TestCase ], ], ]]])] + #[TestWith([['redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'ip-address', + 'matchKey' => null, + 'matchValue' => 'not an IP address', + ], + ], + ], + ]]])] public function throwsWhenProvidedDataIsInvalid(array $invalidData): void { $this->expectException(ValidationException::class); RedirectRulesData::fromRawData($invalidData); } + + #[Test] + #[TestWith([['redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'ip-address', + 'matchKey' => null, + 'matchValue' => '1.2.3.4', + ], + ], + ], + ]]], 'static IP')] + #[TestWith([['redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'ip-address', + 'matchKey' => null, + 'matchValue' => '1.2.3.0/24', + ], + ], + ], + ]]], 'CIDR block')] + #[TestWith([['redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'ip-address', + 'matchKey' => null, + 'matchValue' => '1.2.3.*', + ], + ], + ], + ]]], 'IP wildcard pattern')] + #[TestWith([['redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'ip-address', + 'matchKey' => null, + 'matchValue' => '1.2.*.4', + ], + ], + ], + ]]], 'in-between IP wildcard pattern')] + public function allowsValidDataToBeSet(array $validData): void + { + $result = RedirectRulesData::fromRawData($validData); + self::assertEquals($result->rules, $validData['redirectRules']); + } } diff --git a/module/Core/test/Util/IpAddressUtilsTest.php b/module/Core/test/Util/IpAddressUtilsTest.php new file mode 100644 index 00000000..c1045e68 --- /dev/null +++ b/module/Core/test/Util/IpAddressUtilsTest.php @@ -0,0 +1,26 @@ +callApiWithKey(self::METHOD_POST, '/short-urls/invalid/redirect-rules'); $payload = $this->getJsonResponsePayload($response); @@ -39,16 +39,67 @@ class SetRedirectRulesTest extends ApiTestCase } #[Test] - public function errorIsReturnedWhenInvalidDataProvided(): void - { - $response = $this->callApiWithKey(self::METHOD_POST, '/short-urls/abc123/redirect-rules', [ - RequestOptions::JSON => [ - 'redirectRules' => [ + #[TestWith([[ + 'redirectRules' => [ + [ + 'longUrl' => 'invalid', + ], + ], + ]], 'invalid long URL')] + #[TestWith([[ + 'redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => 'foo', + ], + ], + ]], 'non-array conditions')] + #[TestWith([[ + 'redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ [ - 'longUrl' => 'invalid', + 'type' => 'invalid', + 'matchKey' => null, + 'matchValue' => 'foo', ], ], ], + ], + ]], 'invalid condition type')] + #[TestWith([[ + 'redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'device', + 'matchValue' => 'invalid-device', + 'matchKey' => null, + ], + ], + ], + ], + ]], 'invalid device type')] + #[TestWith([[ + 'redirectRules' => [ + [ + 'longUrl' => 'https://example.com', + 'conditions' => [ + [ + 'type' => 'ip-address', + 'matchKey' => null, + 'matchValue' => 'not an IP address', + ], + ], + ], + ], + ]], 'invalid IP address')] + public function errorIsReturnedWhenInvalidDataIsProvided(array $bodyPayload): void + { + $response = $this->callApiWithKey(self::METHOD_POST, '/short-urls/abc123/redirect-rules', [ + RequestOptions::JSON => $bodyPayload, ]); $payload = $this->getJsonResponsePayload($response);