diff --git a/app/Actions/Shops/TestShopConnection.php b/app/Actions/Shops/TestShopConnection.php index 4ce5fbc..0887bd7 100644 --- a/app/Actions/Shops/TestShopConnection.php +++ b/app/Actions/Shops/TestShopConnection.php @@ -5,6 +5,7 @@ use App\Data\ShopConnectionResult; use App\Enums\ShopConnectionStatus; use App\Models\Shop; +use App\Services\Network\UnsafeDestinationException; use App\Services\WooCommerce\WooCommerceClient; use Illuminate\Http\Client\ConnectionException; use Illuminate\Http\Client\Response; @@ -45,6 +46,14 @@ private function probe(Shop $shop): ShopConnectionResult try { $response = $this->client->get($shop, 'orders', $query); + } catch (UnsafeDestinationException) { + // Deliberately not the resolved address: whoever typed the URL + // does not need this server's reading of their DNS handed back. + return ShopConnectionResult::for( + ShopConnectionStatus::Unreachable, + 'This address points to a private or reserved network, so it is not contacted.', + $this->elapsedMs($startedAt), + ); } catch (ConnectionException $exception) { return ShopConnectionResult::for( ShopConnectionStatus::Unreachable, @@ -80,7 +89,7 @@ private function currencyOf(Shop $shop): ?string // Without _fields the store returns its whole currency list, // some twenty kilobytes of it, on every hourly check. $response = $this->client->get($shop, 'settings/general/woocommerce_currency', ['_fields' => 'id,value']); - } catch (ConnectionException) { + } catch (ConnectionException|UnsafeDestinationException) { return null; } diff --git a/app/Rules/PublicShopUrl.php b/app/Rules/PublicShopUrl.php index 54827b6..bf9c64a 100644 --- a/app/Rules/PublicShopUrl.php +++ b/app/Rules/PublicShopUrl.php @@ -2,7 +2,8 @@ namespace App\Rules; -use App\Services\Net\HostResolver; +use App\Services\Network\PublicHostGuard; +use App\Services\Network\UnsafeDestinationException; use Closure; use Illuminate\Contracts\Validation\ValidationRule; use Illuminate\Translation\PotentiallyTranslatedString; @@ -14,10 +15,13 @@ * attached, and the outcome is reported back in the interface. Without this a * member could aim a "shop" at a cloud metadata endpoint or an internal * service and read the difference between a refused port and an open one. + * + * This is the early feedback, not the protection. DNS can answer differently + * after the form is saved, so the client checks and pins every request too. */ class PublicShopUrl implements ValidationRule { - public function __construct(private HostResolver $resolver) {} + public function __construct(private PublicHostGuard $guard) {} /** * Run the validation rule. @@ -34,6 +38,10 @@ public function validate(string $attribute, mixed $value, Closure $fail): void return; } + if (config('services.woocommerce.allow_private_hosts')) { + return; + } + // An IPv6 literal arrives wrapped in brackets. $host = trim($host, '[]'); @@ -46,26 +54,13 @@ public function validate(string $attribute, mixed $value, Closure $fail): void return; } - foreach ($this->resolver->addressesFor($host) as $address) { - if ($this->isPublic($address)) { - continue; - } - + try { + // A host that does not resolve passes here. It may simply be down + // while someone corrects a typo elsewhere on the form, and the + // client refuses to connect to it either way. + $this->guard->publicAddresses($host); + } catch (UnsafeDestinationException) { $fail(__('That address resolves to a private network and cannot be reached as a shop.')); - - return; } } - - /** - * Determine whether an address is out on the public internet. - */ - private function isPublic(string $address): bool - { - return filter_var( - $address, - FILTER_VALIDATE_IP, - FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE, - ) !== false; - } } diff --git a/app/Services/Net/HostResolver.php b/app/Services/Net/HostResolver.php deleted file mode 100644 index 82a6eac..0000000 --- a/app/Services/Net/HostResolver.php +++ /dev/null @@ -1,31 +0,0 @@ - - */ - public function addressesFor(string $host): array - { - $records = @dns_get_record($host, DNS_A | DNS_AAAA); - - if (! is_array($records)) { - return []; - } - - return array_values(array_filter(array_map( - fn (array $record): ?string => $record['ip'] ?? $record['ipv6'] ?? null, - $records, - ))); - } -} diff --git a/app/Services/Network/HostResolver.php b/app/Services/Network/HostResolver.php new file mode 100644 index 0000000..2a3b291 --- /dev/null +++ b/app/Services/Network/HostResolver.php @@ -0,0 +1,36 @@ + + */ + public function resolve(string $host): array + { + $addresses = []; + + // Asked separately: some resolvers fail a combined A and AAAA query + // outright when the host has no records of one type. + foreach ([DNS_A => 'ip', DNS_AAAA => 'ipv6'] as $type => $field) { + $records = @dns_get_record($host, $type); + + foreach ($records === false ? [] : $records as $record) { + $address = $record[$field] ?? null; + + if (is_string($address) && filter_var($address, FILTER_VALIDATE_IP) !== false) { + $addresses[] = $address; + } + } + } + + return array_values(array_unique($addresses)); + } +} diff --git a/app/Services/Network/PublicHostGuard.php b/app/Services/Network/PublicHostGuard.php new file mode 100644 index 0000000..36a3817 --- /dev/null +++ b/app/Services/Network/PublicHostGuard.php @@ -0,0 +1,66 @@ + an empty list when the host did not resolve + * + * @throws UnsafeDestinationException + */ + public function publicAddresses(string $host): array + { + // An IP literal is its own answer and is never looked up. + $literal = trim($host, '[]'); + + $addresses = filter_var($literal, FILTER_VALIDATE_IP) !== false + ? [$literal] + : $this->resolver->resolve($host); + + foreach ($addresses as $address) { + if (! $this->isPublic($address)) { + throw new UnsafeDestinationException("The host {$host} resolves to the non-public address {$address}."); + } + } + + return $addresses; + } + + /** + * Determine if the address is on the public internet. + */ + public function isPublic(string $address): bool + { + return filter_var($address, FILTER_VALIDATE_IP) !== false + && ! IpUtils::checkIp($address, [...IpUtils::PRIVATE_SUBNETS, ...self::EXTRA_BLOCKED_SUBNETS]); + } +} diff --git a/app/Services/Network/UnsafeDestinationException.php b/app/Services/Network/UnsafeDestinationException.php new file mode 100644 index 0000000..b48cd1d --- /dev/null +++ b/app/Services/Network/UnsafeDestinationException.php @@ -0,0 +1,10 @@ +request() + // Resolved once for the pair: the fallback must go to the same + // addresses the first attempt was checked against, and asking twice + // would give a rebinding host a second chance to answer differently. + $destination = $this->pinnedDestination($shop); + + $response = $this->request($destination) ->withBasicAuth($shop->consumer_key, $shop->consumer_secret) ->get($this->endpoint($shop, $path), $query); @@ -58,7 +66,7 @@ public function get(Shop $shop, string $path, array $query = []): Response return $response; } - return $this->request()->get($this->endpoint($shop, $path), [ + return $this->request($destination)->get($this->endpoint($shop, $path), [ ...$query, 'consumer_key' => $shop->consumer_key, 'consumer_secret' => $shop->consumer_secret, @@ -82,12 +90,65 @@ public function endpoint(Shop $shop, string $path): string * * The default timeout is deliberately short: a manual connection test runs * inside a web request, so a hanging shop must never hang the page. + * + * @param array $destination */ - private function request(): PendingRequest + private function request(array $destination = []): PendingRequest { return Http::acceptJson() ->withUserAgent(config('app.name').'/1.0') + ->withOptions($destination) ->connectTimeout((int) config('services.woocommerce.connect_timeout')) ->timeout($this->timeout ?? (int) config('services.woocommerce.timeout')); } + + /** + * Check where the shop's host points now, and nail the request to it. + * + * The rule on the form is early feedback, nothing more: the name is + * looked up again there and then, and whoever controls it can answer + * publicly while the form is saved and privately once the sync runs. + * Checking here closes that gap, and pinning the answer closes the one + * that is left, where curl looks the name up a third time and gets a + * different reply than the one just approved. + * + * @return array + * + * @throws UnsafeDestinationException + * @throws ConnectionException + */ + private function pinnedDestination(Shop $shop): array + { + if (config('services.woocommerce.allow_private_hosts')) { + return []; + } + + $host = trim((string) parse_url($shop->url, PHP_URL_HOST), '[]'); + + $addresses = app(PublicHostGuard::class)->publicAddresses($host); + + if ($addresses === []) { + throw new ConnectionException("The shop host {$host} could not be resolved."); + } + + // A proxy from HTTP_PROXY or HTTPS_PROXY would make the connection to + // the proxy instead and resolve the shop on the far side, where the + // pin does not reach. An empty proxy is Guzzle's final "no proxy". + $direct = ['proxy' => '']; + + // An IP literal is never looked up, so there is nothing to pin. + if (filter_var($host, FILTER_VALIDATE_IP) !== false) { + return $direct; + } + + $port = parse_url($shop->url, PHP_URL_PORT) + ?? (strtolower((string) parse_url($shop->url, PHP_URL_SCHEME)) === 'http' ? 80 : 443); + + $pinned = implode(',', array_map( + fn (string $address) => str_contains($address, ':') ? "[{$address}]" : $address, + $addresses, + )); + + return [...$direct, 'curl' => [CURLOPT_RESOLVE => ["{$host}:{$port}:{$pinned}"]]]; + } } diff --git a/config/services.php b/config/services.php index a7930be..87bd899 100644 --- a/config/services.php +++ b/config/services.php @@ -36,6 +36,11 @@ ], 'woocommerce' => [ + // Lets a shop live on a private or loopback address, for a fake shop + // during local development. Never enable it where the app runs next + // to anything a user should not be able to reach through it. + 'allow_private_hosts' => (bool) env('WOOCOMMERCE_ALLOW_PRIVATE_HOSTS', env('APP_ENV') === 'local'), + 'connect_timeout' => (int) env('WOOCOMMERCE_CONNECT_TIMEOUT', 5), 'timeout' => (int) env('WOOCOMMERCE_TIMEOUT', 10), 'recheck_after_minutes' => (int) env('WOOCOMMERCE_RECHECK_AFTER_MINUTES', 60), diff --git a/tests/Feature/Shops/ShopDestinationTest.php b/tests/Feature/Shops/ShopDestinationTest.php new file mode 100644 index 0000000..ba0c209 --- /dev/null +++ b/tests/Feature/Shops/ShopDestinationTest.php @@ -0,0 +1,167 @@ +description() + .' (This address points to a private or reserved network, so it is not contacted.)'; +} + +/** + * Fake the shop, keeping the request options each call was sent with. + * + * @param array> $sent + */ +function fakeShopRecordingOptions(array &$sent): void +{ + Http::fake(function ($request, array $options) use (&$sent) { + $sent[] = $options; + + return Http::response([['id' => 1]]); + }); +} + +test('a shop address resolving to a non-public address is refused on the form', function (array $addresses) { + $this->resolveHostsTo($addresses); + $user = User::factory()->create(); + + $this + ->actingAs($user) + ->post(route('shops.store', $user->currentOrganization), validShopData(['url' => 'https://toysonline.test'])) + ->assertSessionHasErrors(['url' => 'That address resolves to a private network and cannot be reached as a shop.']); + + expect($user->currentOrganization->shops()->count())->toBe(0); +})->with([ + 'loopback' => [['127.0.0.1']], + 'private network' => [['10.0.0.5']], + 'cloud metadata' => [['169.254.169.254']], + 'carrier-grade nat' => [['100.64.0.1']], + 'multicast' => [['224.0.0.1']], + 'ipv6 loopback' => [['::1']], + 'ipv6 unique local' => [['fd00::1']], + 'ipv4-mapped ipv6' => [['::ffff:127.0.0.1']], + // NAT64 embeds an IPv4 address, so it reaches the private network by + // another name. PHP's own reserved-range filter does not know it. + 'nat64' => [['64:ff9b::a00:1']], + 'one private answer among public ones' => [['93.184.215.14', '10.0.0.1']], +]); + +test('every request is pinned to the addresses that were just checked', function (string $url, string $pin) { + $this->resolveHostsTo(['93.184.215.14', '2606:2800:21f:cb07:6820:80da:af6b:8b2c']); + $sent = []; + fakeShopRecordingOptions($sent); + $shop = Shop::factory()->create(['url' => $url]); + + app(WooCommerceClient::class)->get($shop, 'orders'); + + expect($sent)->not->toBeEmpty(); + + foreach ($sent as $options) { + expect($options['curl'][CURLOPT_RESOLVE])->toBe([$pin]) + ->and($options['proxy'])->toBe(''); + } +})->with([ + 'default port' => ['https://toysonline.test', 'toysonline.test:443:93.184.215.14,[2606:2800:21f:cb07:6820:80da:af6b:8b2c]'], + 'custom port' => ['https://toysonline.test:8443', 'toysonline.test:8443:93.184.215.14,[2606:2800:21f:cb07:6820:80da:af6b:8b2c]'], +]); + +test('a saved shop whose dns now answers privately is never contacted', function () { + $shop = Shop::factory()->create(['url' => 'https://toysonline.test']); + $this->resolveHostsTo(['10.0.0.1']); + Http::fake(); + + expect(fn () => app(WooCommerceClient::class)->get($shop, 'orders')) + ->toThrow(UnsafeDestinationException::class); + + Http::assertNothingSent(); +}); + +test('a shop host that does not resolve is never contacted', function () { + $shop = Shop::factory()->create(['url' => 'https://toysonline.test']); + $this->resolveHostsTo([]); + Http::fake(); + + expect(fn () => app(WooCommerceClient::class)->get($shop, 'orders')) + ->toThrow(ConnectionException::class, 'could not be resolved'); + + Http::assertNothingSent(); +}); + +test('a host that rebinds to a private address after validation is not contacted', function () { + // Public while the form is validated, private by the time the client + // connects. This is the gap the form rule on its own cannot close. + $lookups = 0; + $this->resolveHostsTo(function () use (&$lookups) { + return ++$lookups === 1 ? ['93.184.215.14'] : ['127.0.0.1']; + }); + Http::fake(); + + $user = User::factory()->create(); + + // Saving queues a connection check, which runs inline here, so the + // second lookup is the one the client makes before connecting. + $this + ->actingAs($user) + ->post(route('shops.store', $user->currentOrganization), validShopData(['url' => 'https://toysonline.test'])) + ->assertSessionHasNoErrors(); + + $shop = $user->currentOrganization->shops()->sole(); + + expect($lookups)->toBe(2) + ->and($shop->connection_status)->toBe(ShopConnectionStatus::Unreachable) + ->and($shop->connection_message)->toContain('private or reserved network'); + + Http::assertNothingSent(); +}); + +test('the toast names the blocked destination when a check is run by hand', function () { + $user = User::factory()->create(); + $shop = Shop::factory()->for($user->currentOrganization)->create(['url' => 'https://toysonline.test']); + $this->resolveHostsTo(['169.254.169.254']); + Http::fake(); + + $this + ->actingAs($user) + ->post(route('shops.connection.test', [$user->currentOrganization, $shop])) + ->assertInertiaFlash('toast', ['type' => 'error', 'message' => blockedMessage()]); + + Http::assertNothingSent(); +}); + +test('a sync to a blocked address records a failure instead of crashing the job', function () { + $user = User::factory()->create(); + $shop = Shop::factory()->for($user->currentOrganization)->create(['url' => 'https://toysonline.test']); + $this->resolveHostsTo(['169.254.169.254']); + Http::fake(); + + $this + ->actingAs($user) + ->post(route('shops.sync.store', [$user->currentOrganization, $shop])) + ->assertRedirect(); + + Http::assertNothingSent(); +}); + +test('private hosts are reachable when explicitly allowed', function () { + config(['services.woocommerce.allow_private_hosts' => true]); + $this->resolveHostsTo(['10.0.0.1']); + $sent = []; + fakeShopRecordingOptions($sent); + $shop = Shop::factory()->create(['url' => 'https://toysonline.test']); + + app(WooCommerceClient::class)->get($shop, 'orders'); + + expect($sent)->not->toBeEmpty() + ->and($sent[0])->not->toHaveKey('curl'); +}); diff --git a/tests/Feature/Shops/ShopTest.php b/tests/Feature/Shops/ShopTest.php index ecaf866..c380e20 100644 --- a/tests/Feature/Shops/ShopTest.php +++ b/tests/Feature/Shops/ShopTest.php @@ -15,22 +15,6 @@ Queue::fake(); }); -/** - * @param array $overrides - * @return array - */ -function validShopData(array $overrides = []): array -{ - return [ - 'name' => 'Toys Online', - 'url' => 'https://toysonline.test', - 'platform' => ShopPlatform::WooCommerce->value, - 'consumer_key' => 'ck_'.str_repeat('a', 40), - 'consumer_secret' => 'cs_'.str_repeat('b', 40), - ...$overrides, - ]; -} - test('organization members can see the shops page', function () { $user = User::factory()->create(); $organization = $user->currentOrganization; @@ -396,7 +380,7 @@ function validShopData(array $overrides = []): array $user = User::factory()->create(); $organization = $user->currentOrganization; - $this->resolveHostsTo(['203.0.113.10']); + $this->resolveHostsTo(['93.184.215.14']); $this ->actingAs($user) diff --git a/tests/Pest.php b/tests/Pest.php index 97e9b1a..34de444 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -1,5 +1,6 @@ $overrides + * @return array + */ +function validShopData(array $overrides = []): array { - // .. + return [ + 'name' => 'Toys Online', + 'url' => 'https://toysonline.test', + 'platform' => ShopPlatform::WooCommerce->value, + 'consumer_key' => 'ck_'.str_repeat('a', 40), + 'consumer_secret' => 'cs_'.str_repeat('b', 40), + ...$overrides, + ]; } diff --git a/tests/TestCase.php b/tests/TestCase.php index a379706..9a6b16a 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -2,50 +2,56 @@ namespace Tests; -use App\Services\Net\HostResolver; +use App\Services\Network\HostResolver; +use Closure; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; use Laravel\Fortify\Features; abstract class TestCase extends BaseTestCase { + /** + * A public address every host resolves to unless a test says otherwise. + */ + protected const string PUBLIC_ADDRESS = '93.184.215.14'; + protected function setUp(): void { parent::setUp(); - // No test should depend on what the machine running it can resolve, - // nor wait on a name server for a .test domain that never answers. - // A test that cares about resolution swaps this for its own answer. - $this->resolveHostsTo([]); + // Tests never touch real DNS. + $this->resolveHostsTo([self::PUBLIC_ADDRESS]); + } + + protected function skipUnlessFortifyHas(string $feature, ?string $message = null): void + { + if (! Features::enabled($feature)) { + $this->markTestSkipped($message ?? "Fortify feature [{$feature}] is not enabled."); + } } /** - * Answer every hostname lookup with the given addresses. + * Answer every host lookup with the given addresses. * - * @param array $addresses + * @param array|Closure(string): array $addresses */ - protected function resolveHostsTo(array $addresses): void + protected function resolveHostsTo(array|Closure $addresses): void { - $this->swap(HostResolver::class, new class($addresses) extends HostResolver + $answer = $addresses instanceof Closure ? $addresses : fn () => $addresses; + + $this->app->instance(HostResolver::class, new class($answer) extends HostResolver { /** - * @param array $addresses + * @param Closure(string): array $answer */ - public function __construct(private array $addresses) {} + public function __construct(private readonly Closure $answer) + { + // + } - /** - * @return array - */ - public function addressesFor(string $host): array + public function resolve(string $host): array { - return $this->addresses; + return ($this->answer)($host); } }); } - - protected function skipUnlessFortifyHas(string $feature, ?string $message = null): void - { - if (! Features::enabled($feature)) { - $this->markTestSkipped($message ?? "Fortify feature [{$feature}] is not enabled."); - } - } }