Security hardening: enforce secure URLs and transport safeguards
This commit is contained in:
@@ -127,6 +127,8 @@ This wrapper now delegates to `BlackWall\Auth\AuthClient`.
|
|||||||
- Store refresh tokens securely.
|
- Store refresh tokens securely.
|
||||||
- Keep access tokens out of logs and browser-visible output.
|
- Keep access tokens out of logs and browser-visible output.
|
||||||
- Use short session lifetimes where possible.
|
- Use short session lifetimes where possible.
|
||||||
|
- `Config::fromArray()` enforces HTTPS URLs by default.
|
||||||
|
- For localhost-only development over HTTP, set `allowInsecureHttp => true`.
|
||||||
|
|
||||||
## Repository layout
|
## Repository layout
|
||||||
|
|
||||||
|
|||||||
@@ -124,3 +124,5 @@ try {
|
|||||||
- Rotate client secrets for confidential clients.
|
- Rotate client secrets for confidential clients.
|
||||||
- Validate `state` on every callback request.
|
- Validate `state` on every callback request.
|
||||||
- Use secure session cookies (`Secure`, `HttpOnly`, `SameSite=Lax` or stricter).
|
- 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`.
|
||||||
|
|||||||
+50
-6
@@ -13,7 +13,8 @@ final class Config
|
|||||||
public readonly string $redirectUri,
|
public readonly string $redirectUri,
|
||||||
public readonly ?string $userInfoUrl = null,
|
public readonly ?string $userInfoUrl = null,
|
||||||
public readonly ?string $clientSecret = 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(
|
return new self(
|
||||||
clientId: $config['clientId'],
|
clientId: $config['clientId'],
|
||||||
authorizeUrl: rtrim($config['authorizeUrl'], '/'),
|
authorizeUrl: $authorizeUrl,
|
||||||
tokenUrl: rtrim($config['tokenUrl'], '/'),
|
tokenUrl: $tokenUrl,
|
||||||
redirectUri: $config['redirectUri'],
|
redirectUri: $redirectUri,
|
||||||
userInfoUrl: isset($config['userInfoUrl']) ? rtrim((string) $config['userInfoUrl'], '/') : null,
|
userInfoUrl: $userInfoUrl,
|
||||||
clientSecret: isset($config['clientSecret']) ? (string) $config['clientSecret'] : null,
|
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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -8,15 +8,19 @@ use BlackWall\Auth\Exception\TransportException;
|
|||||||
|
|
||||||
final class CurlHttpClient implements HttpClientInterface
|
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
|
public function postForm(string $url, array $fields, array $headers = []): array
|
||||||
{
|
{
|
||||||
|
$this->assertValidUrl($url);
|
||||||
$ch = curl_init($url);
|
$ch = curl_init($url);
|
||||||
if ($ch === false) {
|
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([
|
$headerLines = $this->normaliseHeaders(array_merge([
|
||||||
@@ -29,16 +33,31 @@ final class CurlHttpClient implements HttpClientInterface
|
|||||||
CURLOPT_POSTFIELDS => http_build_query($fields, '', '&', PHP_QUERY_RFC3986),
|
CURLOPT_POSTFIELDS => http_build_query($fields, '', '&', PHP_QUERY_RFC3986),
|
||||||
CURLOPT_HTTPHEADER => $headerLines,
|
CURLOPT_HTTPHEADER => $headerLines,
|
||||||
CURLOPT_RETURNTRANSFER => true,
|
CURLOPT_RETURNTRANSFER => true,
|
||||||
|
CURLOPT_CONNECTTIMEOUT => $this->connectTimeoutSeconds,
|
||||||
CURLOPT_TIMEOUT => $this->timeoutSeconds,
|
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);
|
$body = curl_exec($ch);
|
||||||
$status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
|
$status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
|
||||||
|
$errno = curl_errno($ch);
|
||||||
$error = curl_error($ch);
|
$error = curl_error($ch);
|
||||||
curl_close($ch);
|
curl_close($ch);
|
||||||
|
|
||||||
if ($body === false) {
|
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];
|
return ['status' => $status, 'body' => $body];
|
||||||
@@ -46,9 +65,10 @@ final class CurlHttpClient implements HttpClientInterface
|
|||||||
|
|
||||||
public function get(string $url, array $headers = []): array
|
public function get(string $url, array $headers = []): array
|
||||||
{
|
{
|
||||||
|
$this->assertValidUrl($url);
|
||||||
$ch = curl_init($url);
|
$ch = curl_init($url);
|
||||||
if ($ch === false) {
|
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([
|
$headerLines = $this->normaliseHeaders(array_merge([
|
||||||
@@ -58,16 +78,31 @@ final class CurlHttpClient implements HttpClientInterface
|
|||||||
curl_setopt_array($ch, [
|
curl_setopt_array($ch, [
|
||||||
CURLOPT_HTTPHEADER => $headerLines,
|
CURLOPT_HTTPHEADER => $headerLines,
|
||||||
CURLOPT_RETURNTRANSFER => true,
|
CURLOPT_RETURNTRANSFER => true,
|
||||||
|
CURLOPT_CONNECTTIMEOUT => $this->connectTimeoutSeconds,
|
||||||
CURLOPT_TIMEOUT => $this->timeoutSeconds,
|
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);
|
$body = curl_exec($ch);
|
||||||
$status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
|
$status = (int) curl_getinfo($ch, CURLINFO_RESPONSE_CODE);
|
||||||
|
$errno = curl_errno($ch);
|
||||||
$error = curl_error($ch);
|
$error = curl_error($ch);
|
||||||
curl_close($ch);
|
curl_close($ch);
|
||||||
|
|
||||||
if ($body === false) {
|
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];
|
return ['status' => $status, 'body' => $body];
|
||||||
@@ -81,9 +116,30 @@ final class CurlHttpClient implements HttpClientInterface
|
|||||||
{
|
{
|
||||||
$result = [];
|
$result = [];
|
||||||
foreach ($headers as $name => $value) {
|
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);
|
$result[] = sprintf('%s: %s', $name, $value);
|
||||||
}
|
}
|
||||||
|
|
||||||
return $result;
|
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');
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,39 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace BlackWall\Auth\Tests;
|
||||||
|
|
||||||
|
use BlackWall\Auth\Config;
|
||||||
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
|
final class ConfigSecurityTest extends TestCase
|
||||||
|
{
|
||||||
|
public function testRejectsInsecureHttpForNonLocalhostByDefault(): void
|
||||||
|
{
|
||||||
|
$this->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);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@@ -0,0 +1,31 @@
|
|||||||
|
<?php
|
||||||
|
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
namespace BlackWall\Auth\Tests;
|
||||||
|
|
||||||
|
use BlackWall\Auth\Exception\TransportException;
|
||||||
|
use BlackWall\Auth\Http\CurlHttpClient;
|
||||||
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
|
final class CurlHttpClientSecurityTest extends TestCase
|
||||||
|
{
|
||||||
|
public function testRejectsUnsupportedUrlScheme(): void
|
||||||
|
{
|
||||||
|
$client = new CurlHttpClient();
|
||||||
|
|
||||||
|
$this->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",
|
||||||
|
]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
Reference in New Issue
Block a user