From 3ff5b7aa26f22f3b50bb53d2144a284e5e6d747b Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Thu, 26 Feb 2026 09:10:09 +0000 Subject: [PATCH] Security hardening: enforce secure URLs and transport safeguards --- README.md | 2 + docs/INTEGRATION_GUIDE.md | 2 + src/Auth/Config.php | 56 ++++++++++++++++++++--- src/Auth/Http/CurlHttpClient.php | 66 +++++++++++++++++++++++++--- tests/ConfigSecurityTest.php | 39 ++++++++++++++++ tests/CurlHttpClientSecurityTest.php | 31 +++++++++++++ 6 files changed, 185 insertions(+), 11 deletions(-) create mode 100644 tests/ConfigSecurityTest.php create mode 100644 tests/CurlHttpClientSecurityTest.php diff --git a/README.md b/README.md index 5d3cdd1..6a32b6e 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,8 @@ This wrapper now delegates to `BlackWall\Auth\AuthClient`. - Store refresh tokens securely. - Keep access tokens out of logs and browser-visible output. - Use short session lifetimes where possible. +- `Config::fromArray()` enforces HTTPS URLs by default. +- For localhost-only development over HTTP, set `allowInsecureHttp => true`. ## Repository layout diff --git a/docs/INTEGRATION_GUIDE.md b/docs/INTEGRATION_GUIDE.md index 524689b..1c45994 100644 --- a/docs/INTEGRATION_GUIDE.md +++ b/docs/INTEGRATION_GUIDE.md @@ -124,3 +124,5 @@ try { - Rotate client secrets for confidential clients. - Validate `state` on every callback request. - Use secure session cookies (`Secure`, `HttpOnly`, `SameSite=Lax` or stricter). +- HTTPS URLs are enforced by default in `Config::fromArray()`. +- For localhost-only HTTP testing, explicitly set `allowInsecureHttp => true`. diff --git a/src/Auth/Config.php b/src/Auth/Config.php index 1043488..d22e501 100644 --- a/src/Auth/Config.php +++ b/src/Auth/Config.php @@ -13,7 +13,8 @@ final class Config public readonly string $redirectUri, public readonly ?string $userInfoUrl = null, public readonly ?string $clientSecret = null, - public readonly string $defaultScope = 'openid profile email' + public readonly string $defaultScope = 'openid profile email', + public readonly bool $allowInsecureHttp = false ) { } @@ -28,14 +29,57 @@ final class Config } } + $allowInsecureHttp = isset($config['allowInsecureHttp']) && (bool) $config['allowInsecureHttp']; + $authorizeUrl = rtrim(trim((string) $config['authorizeUrl']), '/'); + $tokenUrl = rtrim(trim((string) $config['tokenUrl']), '/'); + $redirectUri = trim((string) $config['redirectUri']); + $userInfoUrl = isset($config['userInfoUrl']) ? rtrim(trim((string) $config['userInfoUrl']), '/') : null; + + self::assertSecureUrl($authorizeUrl, 'authorizeUrl', $allowInsecureHttp); + self::assertSecureUrl($tokenUrl, 'tokenUrl', $allowInsecureHttp); + self::assertSecureUrl($redirectUri, 'redirectUri', $allowInsecureHttp); + if ($userInfoUrl !== null && $userInfoUrl !== '') { + self::assertSecureUrl($userInfoUrl, 'userInfoUrl', $allowInsecureHttp); + } + return new self( clientId: $config['clientId'], - authorizeUrl: rtrim($config['authorizeUrl'], '/'), - tokenUrl: rtrim($config['tokenUrl'], '/'), - redirectUri: $config['redirectUri'], - userInfoUrl: isset($config['userInfoUrl']) ? rtrim((string) $config['userInfoUrl'], '/') : null, + authorizeUrl: $authorizeUrl, + tokenUrl: $tokenUrl, + redirectUri: $redirectUri, + userInfoUrl: $userInfoUrl, clientSecret: isset($config['clientSecret']) ? (string) $config['clientSecret'] : null, - defaultScope: isset($config['scope']) ? (string) $config['scope'] : 'openid profile email' + defaultScope: isset($config['scope']) ? (string) $config['scope'] : 'openid profile email', + allowInsecureHttp: $allowInsecureHttp ); } + + private static function assertSecureUrl(string $url, string $field, bool $allowInsecureHttp): void + { + $parts = parse_url($url); + if ($parts === false || !isset($parts['scheme'])) { + throw new \InvalidArgumentException("{$field} must be a valid absolute URL"); + } + + $scheme = strtolower((string) $parts['scheme']); + if (!in_array($scheme, ['https', 'http'], true)) { + throw new \InvalidArgumentException("{$field} must use http or https"); + } + + if ($scheme === 'https') { + return; + } + + if ($allowInsecureHttp && self::isLocalHost((string) ($parts['host'] ?? ''))) { + return; + } + + throw new \InvalidArgumentException("{$field} must use https (set allowInsecureHttp=true for localhost development)"); + } + + private static function isLocalHost(string $host): bool + { + $normalized = strtolower(trim($host)); + return in_array($normalized, ['localhost', '127.0.0.1', '::1'], true); + } } diff --git a/src/Auth/Http/CurlHttpClient.php b/src/Auth/Http/CurlHttpClient.php index 4772fd9..40916c0 100644 --- a/src/Auth/Http/CurlHttpClient.php +++ b/src/Auth/Http/CurlHttpClient.php @@ -8,15 +8,19 @@ use BlackWall\Auth\Exception\TransportException; final class CurlHttpClient implements HttpClientInterface { - public function __construct(private readonly int $timeoutSeconds = 20) + public function __construct( + private readonly int $timeoutSeconds = 20, + private readonly int $connectTimeoutSeconds = 10 + ) { } public function postForm(string $url, array $fields, array $headers = []): array { + $this->assertValidUrl($url); $ch = curl_init($url); if ($ch === false) { - throw new TransportException('Unable to initialise cURL for POST request'); + throw new TransportException('Unable to initialise cURL for POST request', 'curl_init_failed'); } $headerLines = $this->normaliseHeaders(array_merge([ @@ -29,16 +33,31 @@ final class CurlHttpClient implements HttpClientInterface CURLOPT_POSTFIELDS => http_build_query($fields, '', '&', PHP_QUERY_RFC3986), CURLOPT_HTTPHEADER => $headerLines, CURLOPT_RETURNTRANSFER => true, + CURLOPT_CONNECTTIMEOUT => $this->connectTimeoutSeconds, CURLOPT_TIMEOUT => $this->timeoutSeconds, + CURLOPT_SSL_VERIFYPEER => true, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_FOLLOWLOCATION => false, + CURLOPT_MAXREDIRS => 0, ]); + if (defined('CURLOPT_PROTOCOLS')) { + curl_setopt($ch, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS); + } + if (defined('CURLOPT_REDIR_PROTOCOLS')) { + curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS); + } $body = curl_exec($ch); $status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE); + $errno = curl_errno($ch); $error = curl_error($ch); curl_close($ch); if ($body === false) { - throw new TransportException($error !== '' ? $error : 'Unknown cURL POST error'); + throw new TransportException( + $error !== '' ? $error : 'Unknown cURL POST error', + $errno > 0 ? ('curl_errno_' . $errno) : 'curl_post_failed' + ); } return ['status' => $status, 'body' => $body]; @@ -46,9 +65,10 @@ final class CurlHttpClient implements HttpClientInterface public function get(string $url, array $headers = []): array { + $this->assertValidUrl($url); $ch = curl_init($url); if ($ch === false) { - throw new TransportException('Unable to initialise cURL for GET request'); + throw new TransportException('Unable to initialise cURL for GET request', 'curl_init_failed'); } $headerLines = $this->normaliseHeaders(array_merge([ @@ -58,16 +78,31 @@ final class CurlHttpClient implements HttpClientInterface curl_setopt_array($ch, [ CURLOPT_HTTPHEADER => $headerLines, CURLOPT_RETURNTRANSFER => true, + CURLOPT_CONNECTTIMEOUT => $this->connectTimeoutSeconds, CURLOPT_TIMEOUT => $this->timeoutSeconds, + CURLOPT_SSL_VERIFYPEER => true, + CURLOPT_SSL_VERIFYHOST => 2, + CURLOPT_FOLLOWLOCATION => false, + CURLOPT_MAXREDIRS => 0, ]); + if (defined('CURLOPT_PROTOCOLS')) { + curl_setopt($ch, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS); + } + if (defined('CURLOPT_REDIR_PROTOCOLS')) { + curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS); + } $body = curl_exec($ch); $status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE); + $errno = curl_errno($ch); $error = curl_error($ch); curl_close($ch); if ($body === false) { - throw new TransportException($error !== '' ? $error : 'Unknown cURL GET error'); + throw new TransportException( + $error !== '' ? $error : 'Unknown cURL GET error', + $errno > 0 ? ('curl_errno_' . $errno) : 'curl_get_failed' + ); } return ['status' => $status, 'body' => $body]; @@ -81,9 +116,30 @@ final class CurlHttpClient implements HttpClientInterface { $result = []; foreach ($headers as $name => $value) { + if ( + str_contains((string) $name, "\r") + || str_contains((string) $name, "\n") + || str_contains((string) $value, "\r") + || str_contains((string) $value, "\n") + ) { + throw new TransportException('Header names/values must not contain CR/LF characters', 'invalid_header'); + } $result[] = sprintf('%s: %s', $name, $value); } return $result; } + + private function assertValidUrl(string $url): void + { + $parts = parse_url($url); + if ($parts === false || !isset($parts['scheme'])) { + throw new TransportException('Request URL must be an absolute URL', 'invalid_url'); + } + + $scheme = strtolower((string) $parts['scheme']); + if (!in_array($scheme, ['http', 'https'], true)) { + throw new TransportException('Only http/https URLs are supported', 'invalid_url_scheme'); + } + } } diff --git a/tests/ConfigSecurityTest.php b/tests/ConfigSecurityTest.php new file mode 100644 index 0000000..f3dc3ea --- /dev/null +++ b/tests/ConfigSecurityTest.php @@ -0,0 +1,39 @@ +expectException(\InvalidArgumentException::class); + Config::fromArray([ + 'clientId' => 'client-123', + 'authorizeUrl' => 'http://provider.example/oauth/authorize', + 'tokenUrl' => 'https://provider.example/oauth/token', + 'userInfoUrl' => 'https://provider.example/oauth/userinfo', + 'redirectUri' => 'https://app.example/callback.php', + ]); + } + + public function testAllowsInsecureHttpForLocalhostWhenExplicitlyEnabled(): void + { + $config = Config::fromArray([ + 'clientId' => 'client-123', + 'authorizeUrl' => 'http://localhost/oauth/authorize', + 'tokenUrl' => 'http://127.0.0.1/oauth/token', + 'userInfoUrl' => 'http://localhost/oauth/userinfo', + 'redirectUri' => 'http://localhost/callback.php', + 'allowInsecureHttp' => true, + ]); + + self::assertTrue($config->allowInsecureHttp); + self::assertSame('http://localhost/oauth/authorize', $config->authorizeUrl); + } +} + diff --git a/tests/CurlHttpClientSecurityTest.php b/tests/CurlHttpClientSecurityTest.php new file mode 100644 index 0000000..84912e9 --- /dev/null +++ b/tests/CurlHttpClientSecurityTest.php @@ -0,0 +1,31 @@ +expectException(TransportException::class); + $client->get('file:///etc/passwd'); + } + + public function testRejectsHeaderInjectionAttempt(): void + { + $client = new CurlHttpClient(); + + $this->expectException(TransportException::class); + $client->get('https://example.com', [ + 'X-Test' => "ok\r\nInjected: yes", + ]); + } +} +