diff --git a/src/RecoveryCodes.php b/src/RecoveryCodes.php index 26cff66..bc64ba4 100644 --- a/src/RecoveryCodes.php +++ b/src/RecoveryCodes.php @@ -45,10 +45,21 @@ public function generate( throw new InvalidArgumentException('Invalid recovery code configuration.'); } + $characters = $this->characterSet($characterSet); + if (!$this->canGenerateUniqueCodes($count, $length, count($characters))) { + throw new InvalidArgumentException('Recovery code configuration cannot produce the requested number of unique codes.'); + } + $plainCodes = []; $hashedCodes = []; - for ($i = 0; $i < $count; $i++) { - $code = $this->randomCode($length, $characterSet); + $generatedCodes = []; + while (count($plainCodes) < $count) { + $code = $this->randomCode($length, $characters); + if (isset($generatedCodes[$code])) { + continue; + } + + $generatedCodes[$code] = true; $plainCodes[] = $groupSize > 0 ? trim(chunk_split($code, $groupSize, '-'), '-') : $code; $hashedCodes[] = $this->hash($code); } @@ -60,21 +71,54 @@ public function generate( return new RecoveryCodeGenerationResult($plainCodes, $metadata['total'], $metadata['remaining'], $metadata['lastUsedAt']); } - private function hash(string $code): string + private function canGenerateUniqueCodes(int $count, int $length, int $characterCount): bool { - return hash_hmac($this->hashAlgorithm, $code, $this->hashKey ?? 'otp-recovery-codes'); + $capacity = 1; + for ($i = 0; $i < $length; $i++) { + if ($capacity >= $count || $capacity > intdiv($count - 1, $characterCount)) { + return true; + } + + $capacity *= $characterCount; + } + + return $capacity >= $count; } - private function randomCode(int $length, string $characterSet): string + /** + * @param $characterSet Candidate recovery-code characters. + * @return array Unique recovery-code characters. + * @phpstan-return non-empty-list + */ + private function characterSet(string $characterSet): array { - $characters = array_values(array_unique(str_split($characterSet))); + $characters = []; + foreach (str_split($characterSet) as $character) { + $characters[$character] = true; + } if ($characters === []) { throw new InvalidArgumentException('Recovery code character set cannot be empty.'); } + return array_keys($characters); + } + + private function hash(string $code): string + { + return hash_hmac($this->hashAlgorithm, $code, $this->hashKey ?? 'otp-recovery-codes'); + } + + /** + * @param $length Recovery-code length. + * @param $characterSet Unique recovery-code characters. + * @phpstan-param non-empty-list $characterSet + */ + private function randomCode(int $length, array $characterSet): string + { $code = ''; + $characterCount = count($characterSet); for ($i = 0; $i < $length; $i++) { - $code .= $characters[random_int(0, count($characters) - 1)]; + $code .= $characterSet[random_int(0, $characterCount - 1)]; } return $code; diff --git a/src/Stores/InMemoryRecoveryCodeStore.php b/src/Stores/InMemoryRecoveryCodeStore.php index 8c709a8..24aae10 100644 --- a/src/Stores/InMemoryRecoveryCodeStore.php +++ b/src/Stores/InMemoryRecoveryCodeStore.php @@ -49,7 +49,7 @@ public function replace(string $binding, array $hashedCodes, DateTimeImmutable $ $this->storage[$binding] = [ 'codes' => $codes, - 'total' => count($hashedCodes), + 'total' => count($codes), 'lastUsedAt' => null, ]; } diff --git a/src/Support/AlgorithmValidator.php b/src/Support/AlgorithmValidator.php index 6ba891e..a6e830f 100644 --- a/src/Support/AlgorithmValidator.php +++ b/src/Support/AlgorithmValidator.php @@ -11,11 +11,11 @@ final class AlgorithmValidator public static function normalize(string $algorithm): string { $algorithm = strtolower(trim($algorithm)); - if (!in_array($algorithm, self::supported(), true)) { - throw new InvalidArgumentException('Unsupported OTP algorithm.'); - } - return $algorithm; + return match ($algorithm) { + 'sha1', 'sha256', 'sha512' => $algorithm, + default => throw new InvalidArgumentException('Unsupported OTP algorithm.'), + }; } /** diff --git a/src/Support/ProvisioningUriParser.php b/src/Support/ProvisioningUriParser.php index 6297359..bf334ae 100644 --- a/src/Support/ProvisioningUriParser.php +++ b/src/Support/ProvisioningUriParser.php @@ -28,7 +28,13 @@ public static function parse(string $uri): ParsedOtpAuthUri $labelParts = LabelHelper::parseLabel(ltrim((string) ($parts['path'] ?? ''), '/'), $issuer); $algorithmValue = self::optionalStringQueryValue($query, 'algorithm'); $algorithm = $algorithmValue !== null ? AlgorithmValidator::normalize($algorithmValue) : 'sha1'; - $digits = isset($query['digits']) ? (int) $query['digits'] : 6; + $digits = self::optionalNonNegativeIntQueryValue($query, 'digits') ?? 6; + if (($type === 'hotp' || $type === 'totp') && ($digits < 4 || $digits > 10)) { + throw new InvalidArgumentException('HOTP and TOTP digit counts must be between 4 and 10.'); + } + + $period = self::optionalPositiveIntQueryValue($query, 'period'); + $counter = self::optionalNonNegativeIntQueryValue($query, 'counter'); return new ParsedOtpAuthUri( $type, @@ -37,12 +43,45 @@ public static function parse(string $uri): ParsedOtpAuthUri $labelParts['issuer'], $algorithm, $digits, - isset($query['period']) ? (int) $query['period'] : null, - isset($query['counter']) ? (int) $query['counter'] : null, + $period, + $counter, self::optionalStringQueryValue($query, 'ocraSuite'), ); } + /** + * @param $query Parsed URI query values. + * @param $key Query parameter name. + * @phpstan-param array $query + */ + private static function optionalNonNegativeIntQueryValue(array $query, string $key): ?int + { + $value = self::optionalStringQueryValue($query, $key); + if ($value === null) { + return null; + } + if (!ctype_digit($value)) { + throw new InvalidArgumentException(sprintf('Invalid non-negative integer otpauth query parameter "%s".', $key)); + } + + return (int) $value; + } + + /** + * @param $query Parsed URI query values. + * @param $key Query parameter name. + * @phpstan-param array $query + */ + private static function optionalPositiveIntQueryValue(array $query, string $key): ?int + { + $value = self::optionalNonNegativeIntQueryValue($query, $key); + if ($value !== null && $value < 1) { + throw new InvalidArgumentException(sprintf('Otpauth query parameter "%s" must be greater than zero.', $key)); + } + + return $value; + } + /** * @param $query Parsed URI query values. * @param $key Query parameter name. diff --git a/src/Support/StepUp.php b/src/Support/StepUp.php index e4b8bd8..7ec3872 100644 --- a/src/Support/StepUp.php +++ b/src/Support/StepUp.php @@ -19,11 +19,12 @@ public static function assess(?DateTimeImmutable $verifiedAt, int $seconds, ?Dat { self::assertWindow($seconds); $now ??= new DateTimeImmutable(); + $age = $verifiedAt !== null ? self::ageInSeconds($verifiedAt, $now) : null; return new StepUpResult( - self::requiresFreshOtp($verifiedAt, $seconds, $now), + $verifiedAt === null || $age > $seconds, $verifiedAt, - $verifiedAt !== null ? self::ageInSeconds($verifiedAt, $now) : null, + $age, $seconds, $verifiedAt?->modify(sprintf('+%d seconds', $seconds)), ); diff --git a/src/TOTP.php b/src/TOTP.php index 1bd8c9c..b106b86 100644 --- a/src/TOTP.php +++ b/src/TOTP.php @@ -187,10 +187,6 @@ public function planSecretRotation( bool $withQrSvg = false, int $imageSize = 200, ): SecretRotation { - if ($gracePeriodInSeconds !== null && $gracePeriodInSeconds < 0) { - throw new \InvalidArgumentException('Grace period must be non-negative.'); - } - $rotation = $this->rotateSecret($newSecret, $gracePeriodInSeconds, $now); $next = new self($rotation['next'], $this->digitCount, $this->period); $next->setAlgorithm($this->algorithm); @@ -215,6 +211,10 @@ public function rotateSecret( ?int $gracePeriodInSeconds = null, ?int $now = null, ): array { + if ($gracePeriodInSeconds !== null && $gracePeriodInSeconds < 0) { + throw new \InvalidArgumentException('Grace period must be non-negative.'); + } + $now ??= time(); return [ @@ -254,9 +254,6 @@ public function verifyWithWindow( ): VerificationResult { $this->assertOtp($otp, $this->digitCount); $window ??= new VerificationWindow(); - if ($window->past < 0 || $window->future < 0) { - throw new \InvalidArgumentException('Verification windows must be non-negative.'); - } $baseTimestamp = $timestamp ?? time(); $currentStep = $this->getTimeStepFromTimestamp($baseTimestamp); diff --git a/src/ValueObjects/DeviceEnrollment.php b/src/ValueObjects/DeviceEnrollment.php index 36f8e53..8774449 100644 --- a/src/ValueObjects/DeviceEnrollment.php +++ b/src/ValueObjects/DeviceEnrollment.php @@ -28,10 +28,6 @@ public static function create( string $secretReference, ?DateTimeImmutable $createdAt = null, ): self { - self::assertNonEmpty('deviceId', $deviceId); - self::assertNonEmpty('label', $label); - self::assertNonEmpty('secretReference', $secretReference); - return new self($deviceId, $label, $secretReference, $createdAt ?? new DateTimeImmutable()); } diff --git a/src/ValueObjects/VerificationWindow.php b/src/ValueObjects/VerificationWindow.php index 9bfd02f..064ebb2 100644 --- a/src/ValueObjects/VerificationWindow.php +++ b/src/ValueObjects/VerificationWindow.php @@ -4,12 +4,18 @@ namespace Infocyph\OTP\ValueObjects; +use InvalidArgumentException; + final readonly class VerificationWindow { public function __construct( public int $past = 0, public int $future = 0, - ) {} + ) { + if ($past < 0 || $future < 0) { + throw new InvalidArgumentException('Verification windows must be non-negative.'); + } + } public static function symmetric(int $window): self { diff --git a/tests/AdvancedOTPTest.php b/tests/AdvancedOTPTest.php index 7f25cf0..f876cf7 100644 --- a/tests/AdvancedOTPTest.php +++ b/tests/AdvancedOTPTest.php @@ -73,6 +73,13 @@ ->and($regenerated->remainingCount)->toBe(10); }); +test('Recovery code generation rejects insufficient unique code space', function () { + $codes = new RecoveryCodes(new InMemoryRecoveryCodeStore()); + + expect(fn () => $codes->generate('user-1', count: 2, length: 6, characterSet: 'A')) + ->toThrow(InvalidArgumentException::class); +}); + test('otpauth URIs round-trip through parser with issuer-safe labels', function () { $secret = TOTP::generateSecret(); $totp = (new TOTP($secret))->setAlgorithm('sha256'); @@ -85,3 +92,17 @@ ->and($parsed->algorithm)->toBe('sha256') ->and($parsed->period)->toBe(30); }); + +test('otpauth URI parser rejects malformed numeric parameters', function () { + $baseUri = 'otpauth://totp/Example:user?secret=JBSWY3DPEHPK3PXP'; + + foreach (['&digits=invalid', '&digits=3', '&period=0', '&counter=-1'] as $parameter) { + expect(fn () => ProvisioningUriParser::parse($baseUri . $parameter)) + ->toThrow(InvalidArgumentException::class); + } +}); + +test('verification windows reject negative bounds when constructed', function () { + expect(fn () => new VerificationWindow(-1, 0)) + ->toThrow(InvalidArgumentException::class); +}); diff --git a/tests/WorkflowHelpersTest.php b/tests/WorkflowHelpersTest.php index 7278dfb..da51aaa 100644 --- a/tests/WorkflowHelpersTest.php +++ b/tests/WorkflowHelpersTest.php @@ -54,6 +54,15 @@ ->and($rotation->nextEnrollment?->qrSvg)->toContain(' $totp->rotateSecret(TOTP::generateSecret(), -1)) + ->toThrow(InvalidArgumentException::class) + ->and(fn () => $totp->planSecretRotation(TOTP::generateSecret(), 'alice@example.com', 'Example App', -1)) + ->toThrow(InvalidArgumentException::class); +}); + test('hotp and ocra secret rotation can prepare replacement enrollment payloads', function () { $hotp = (new HOTP(HOTP::generateSecret()))->setAlgorithm('sha512')->setCounter(5); $hotpRotation = $hotp->planSecretRotation(