From a33e5c164624f9028af7418df8cfa48e661ac00e Mon Sep 17 00:00:00 2001 From: Sourov Biswas Date: Sun, 20 Sep 2026 16:49:00 +0600 Subject: [PATCH] Check and pin a shop's address on every request, not just on the form Everything under a shop's URL is fetched by this server, with that shop's credentials attached, and the outcome is reported back in the interface. That makes the URL field a request forgery primitive, and until now the only thing standing in front of it was a validation rule that ran once, when the form was saved. Three holes, in order of how easy they are to walk through: - DNS is checked at save time and never again. Whoever controls the name can answer publicly while the form is validated and privately once the hourly sync runs. Nothing looked again. - HostResolver asked for A and AAAA in one combined query. Some resolvers fail that outright when the host has records of only one type, and an empty answer fell straight through the rule's foreach without failing anything. An unresolvable host was accepted. - The blocklist was PHP's NO_PRIV_RANGE|NO_RES_RANGE, which allows NAT64 (64:ff9b::/96, which wraps an IPv4 address and so reaches the private network under another name), multicast, carrier-grade NAT and the IETF protocol assignments. ProductSyncManager solved this a while ago and the code is worth having twice rather than solving it differently a second time: HostResolver, PublicHostGuard and UnsafeDestinationException are copied from it unchanged, under the same App\Services\Network namespace, and TestCase gains the same resolver stub. PublicShopUrl now sits on the guard, and keeps its own wording and its own rule that a shop lives at a domain name rather than at a bare address. The new part is WooCommerceClient::pinnedDestination(). Every call resolves the host again, refuses any non-public answer, refuses a host that will not resolve at all, and hands curl CURLOPT_RESOLVE so the third lookup cannot come back different from the one just approved. The proxy is cleared for the same reason: a proxy would resolve the shop on the far side, where the pin does not reach. Both attempts of the 401 key-and-secret fallback share one resolution. A blocked destination is an outcome, not a crash: the connection test records it as Unreachable, and the sync already caught RuntimeException, which UnsafeDestinationException extends. WOOCOMMERCE_ALLOW_PRIVATE_HOSTS turns all of this off for a fake shop on a .test domain during local development, defaulting on in local and nowhere else. One existing test moved off 203.0.113.10: TEST-NET-3 is documentation space, which the wider blocklist correctly refuses. Co-Authored-By: Claude Opus 5 --- app/Actions/Shops/TestShopConnection.php | 11 +- app/Rules/PublicShopUrl.php | 37 ++-- app/Services/Net/HostResolver.php | 31 ---- app/Services/Network/HostResolver.php | 36 ++++ app/Services/Network/PublicHostGuard.php | 66 +++++++ .../Network/UnsafeDestinationException.php | 10 ++ .../WooCommerce/WooCommerceClient.php | 67 ++++++- config/services.php | 5 + tests/Feature/Shops/ShopDestinationTest.php | 167 ++++++++++++++++++ tests/Feature/Shops/ShopTest.php | 18 +- tests/Pest.php | 19 +- tests/TestCase.php | 52 +++--- 12 files changed, 421 insertions(+), 98 deletions(-) delete mode 100644 app/Services/Net/HostResolver.php create mode 100644 app/Services/Network/HostResolver.php create mode 100644 app/Services/Network/PublicHostGuard.php create mode 100644 app/Services/Network/UnsafeDestinationException.php create mode 100644 tests/Feature/Shops/ShopDestinationTest.php 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."); - } - } }