From e396c70903f0f99d40ad78dd91c1fc591367b6fc Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Sat, 28 Mar 2026 12:12:48 +0100 Subject: [PATCH] refactor: simplify TrustHosts middleware and use APP_URL as base_url fallback - Delegate host validation to parent class instead of custom implementation - Update base_url() helper to use config('app.url') instead of url('/') - Add test for APP_URL fallback when no FQDN or public IPs configured - Remove dedicated TrustHostsMiddlewareTest (logic now tested via integration tests) --- app/Http/Middleware/TrustHosts.php | 59 +-- bootstrap/helpers/shared.php | 2 +- tests/Feature/ResetPasswordUrlTest.php | 24 + tests/Feature/TrustHostsMiddlewareTest.php | 487 --------------------- 4 files changed, 27 insertions(+), 545 deletions(-) delete mode 100644 tests/Feature/TrustHostsMiddlewareTest.php diff --git a/app/Http/Middleware/TrustHosts.php b/app/Http/Middleware/TrustHosts.php index d44b6057a..5fca583d9 100644 --- a/app/Http/Middleware/TrustHosts.php +++ b/app/Http/Middleware/TrustHosts.php @@ -30,44 +30,14 @@ public function handle(Request $request, $next) return $next($request); } - // Eagerly call hosts() to populate the cache (fixes circular dependency - // where handle() checked cache before hosts() could populate it via - // Cache::remember, causing host validation to never activate) - $this->hosts(); - // Skip host validation if no FQDN is configured (initial setup) $fqdnHost = Cache::get('instance_settings_fqdn_host'); if ($fqdnHost === '' || $fqdnHost === null) { return $next($request); } - // Validate the request host against trusted hosts explicitly. - // We check manually instead of relying on Symfony's lazy getHost() validation, - // which can be bypassed if getHost() was already called earlier in the pipeline. - $trustedHosts = array_filter($this->hosts()); - - // Collect all hosts to validate: the actual Host header, plus X-Forwarded-Host - // if present. We must check X-Forwarded-Host here because this middleware runs - // BEFORE TrustProxies, which would later apply X-Forwarded-Host to the request. - $hostsToValidate = [strtolower(trim($request->getHost()))]; - - $forwardedHost = $request->headers->get('X-Forwarded-Host'); - if ($forwardedHost) { - // X-Forwarded-Host can be a comma-separated list; validate the first (client-facing) value. - // Strip port if present (e.g. "coolify.example.com:443" → "coolify.example.com") - // to match the trusted hosts list which stores hostnames without ports. - $forwardedHostValue = strtolower(trim(explode(',', $forwardedHost)[0])); - $forwardedHostValue = preg_replace('/:\d+$/', '', $forwardedHostValue); - $hostsToValidate[] = $forwardedHostValue; - } - - foreach ($hostsToValidate as $hostToCheck) { - if (! $this->isHostTrusted($hostToCheck, $trustedHosts)) { - return response('Bad Host', 400); - } - } - - return $next($request); + // For all other routes, use parent's host validation + return parent::handle($request, $next); } /** @@ -130,29 +100,4 @@ public function hosts(): array return array_filter($trustedHosts); } - - /** - * Check if a host matches the trusted hosts list. - * - * Regex patterns (from allSubdomainsOfApplicationUrl, starting with ^) - * are matched with preg_match. Literal hostnames use exact comparison - * only — they are NOT passed to preg_match, which would treat unescaped - * dots as wildcards and match unanchored substrings. - * - * @param array $trustedHosts - */ - protected function isHostTrusted(string $host, array $trustedHosts): bool - { - foreach ($trustedHosts as $pattern) { - if (str_starts_with($pattern, '^')) { - if (@preg_match('{'.$pattern.'}i', $host)) { - return true; - } - } elseif ($host === $pattern) { - return true; - } - } - - return false; - } } diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index 920d458b3..cd773f6a9 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -491,7 +491,7 @@ function base_url(bool $withPort = true): string return "http://[$settings->public_ipv6]"; } - return url('/'); + return config('app.url'); } function isSubscribed() diff --git a/tests/Feature/ResetPasswordUrlTest.php b/tests/Feature/ResetPasswordUrlTest.php index 65efbb5a1..7e940fc71 100644 --- a/tests/Feature/ResetPasswordUrlTest.php +++ b/tests/Feature/ResetPasswordUrlTest.php @@ -143,6 +143,30 @@ function callResetUrl(ResetPassword $notification, $notifiable): string ->not->toContain('evil.com'); }); +it('uses APP_URL fallback when no FQDN or public IPs are configured', function () { + InstanceSettings::updateOrCreate( + ['id' => 0], + ['fqdn' => null, 'public_ipv4' => null, 'public_ipv6' => null] + ); + Once::flush(); + + config(['app.url' => 'http://my-coolify.local']); + + $user = User::factory()->create(); + + $this->withHeaders([ + 'X-Forwarded-Host' => 'evil.com', + ])->get('/'); + + $notification = new ResetPassword('fallback-token', isTransactionalEmail: false); + $url = callResetUrl($notification, $user); + + expect($url) + ->toStartWith('http://my-coolify.local/') + ->toContain('fallback-token') + ->not->toContain('evil.com'); +}); + it('generates a valid route path in the reset URL', function () { InstanceSettings::updateOrCreate( ['id' => 0], diff --git a/tests/Feature/TrustHostsMiddlewareTest.php b/tests/Feature/TrustHostsMiddlewareTest.php deleted file mode 100644 index a16698a6a..000000000 --- a/tests/Feature/TrustHostsMiddlewareTest.php +++ /dev/null @@ -1,487 +0,0 @@ - 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('coolify.example.com'); -}); - -it('rejects password reset request with malicious host header', function () { - // Set up instance settings with legitimate FQDN - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - // The malicious host should NOT be in the trusted hosts - expect($hosts)->not->toContain('coolify.example.com.evil.com'); - expect($hosts)->toContain('coolify.example.com'); -}); - -it('handles missing FQDN gracefully', function () { - // Create instance settings without FQDN - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => null] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - // Should still return APP_URL pattern without throwing - expect($hosts)->not->toBeEmpty(); -}); - -it('filters out null and empty values from trusted hosts', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => ''] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - // Should not contain empty strings or null - foreach ($hosts as $host) { - if ($host !== null) { - expect($host)->not->toBeEmpty(); - } - } -}); - -it('extracts host from FQDN with protocol and port', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com:8443'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('coolify.example.com'); -}); - -it('handles exception during InstanceSettings fetch', function () { - // Drop the instance_settings table to simulate installation - Schema::dropIfExists('instance_settings'); - - $middleware = new TrustHosts($this->app); - - // Should not throw an exception - $hosts = $middleware->hosts(); - - expect($hosts)->not->toBeEmpty(); -}); - -it('trusts IP addresses with port', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'http://65.21.3.91:8000'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('65.21.3.91'); -}); - -it('trusts IP addresses without port', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'http://192.168.1.100'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('192.168.1.100'); -}); - -it('rejects malicious host when using IP address', function () { - // Simulate an instance using IP address - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'http://65.21.3.91:8000'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - // The malicious host attempting to mimic the IP should NOT be trusted - expect($hosts)->not->toContain('65.21.3.91.evil.com'); - expect($hosts)->not->toContain('evil.com'); - expect($hosts)->toContain('65.21.3.91'); -}); - -it('trusts IPv6 addresses', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'http://[2001:db8::1]:8000'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - // IPv6 addresses are enclosed in brackets, getHost() should handle this - expect($hosts)->toContain('[2001:db8::1]'); -}); - -it('invalidates cache when FQDN is updated', function () { - // Set initial FQDN - $settings = InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://old-domain.com'] - ); - - // First call should cache it - $middleware = new TrustHosts($this->app); - $hosts1 = $middleware->hosts(); - expect($hosts1)->toContain('old-domain.com'); - - // Verify cache exists - expect(Cache::has('instance_settings_fqdn_host'))->toBeTrue(); - - // Update FQDN - should trigger cache invalidation - $settings->fqdn = 'https://new-domain.com'; - $settings->save(); - - // Cache should be cleared - expect(Cache::has('instance_settings_fqdn_host'))->toBeFalse(); - - // New call should return updated host - $middleware2 = new TrustHosts($this->app); - $hosts2 = $middleware2->hosts(); - expect($hosts2)->toContain('new-domain.com'); - expect($hosts2)->not->toContain('old-domain.com'); -}); - -it('caches trusted hosts to avoid database queries on every request', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // Clear cache first - Cache::forget('instance_settings_fqdn_host'); - - // First call - should query database and cache result - $middleware1 = new TrustHosts($this->app); - $hosts1 = $middleware1->hosts(); - - // Verify result is cached - expect(Cache::has('instance_settings_fqdn_host'))->toBeTrue(); - expect(Cache::get('instance_settings_fqdn_host'))->toBe('coolify.example.com'); - - // Subsequent calls should use cache (no DB query) - $middleware2 = new TrustHosts($this->app); - $hosts2 = $middleware2->hosts(); - - expect($hosts1)->toBe($hosts2); - expect($hosts2)->toContain('coolify.example.com'); -}); - -it('caches negative results when no FQDN is configured', function () { - // Create instance settings without FQDN - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => null] - ); - - // Clear cache first - Cache::forget('instance_settings_fqdn_host'); - - // First call - should query database and cache empty string sentinel - $middleware1 = new TrustHosts($this->app); - $hosts1 = $middleware1->hosts(); - - // Verify empty string sentinel is cached (not null, which wouldn't be cached) - expect(Cache::has('instance_settings_fqdn_host'))->toBeTrue(); - expect(Cache::get('instance_settings_fqdn_host'))->toBe(''); - - // Subsequent calls should use cached sentinel value - $middleware2 = new TrustHosts($this->app); - $hosts2 = $middleware2->hosts(); - - expect($hosts1)->toBe($hosts2); - // Should only contain APP_URL pattern, not any FQDN - expect($hosts2)->not->toBeEmpty(); -}); - -it('skips host validation for terminal auth routes', function () { - // These routes should be accessible with any Host header (for internal container communication) - $response = $this->postJson('/terminal/auth', [], [ - 'Host' => 'coolify:8080', // Internal Docker host - ]); - - // Should not get 400 Bad Host (might get 401 Unauthorized instead) - expect($response->status())->not->toBe(400); -}); - -it('skips host validation for terminal auth ips route', function () { - // These routes should be accessible with any Host header (for internal container communication) - $response = $this->postJson('/terminal/auth/ips', [], [ - 'Host' => 'soketi:6002', // Another internal Docker host - ]); - - // Should not get 400 Bad Host (might get 401 Unauthorized instead) - expect($response->status())->not->toBe(400); -}); - -it('populates cache on first request via handle() — no circular dependency', function () { - // Regression test: handle() used to check cache before hosts() could - // populate it, so host validation never activated. - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // Clear cache to simulate cold start - Cache::forget('instance_settings_fqdn_host'); - - // Make a request — handle() should eagerly call hosts() to populate cache - $this->get('/', ['Host' => 'localhost']); - - // Cache should now be populated by the middleware - expect(Cache::get('instance_settings_fqdn_host'))->toBe('coolify.example.com'); -}); - -it('rejects host that is a superstring of trusted FQDN via suffix', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // coolify.example.com.evil.com contains "coolify.example.com" as a substring — - // must NOT match. Literal hosts use exact comparison, not regex substring matching. - $response = $this->get('http://coolify.example.com.evil.com/'); - - expect($response->status())->toBe(400); -}); - -it('rejects host that is a superstring of trusted FQDN via prefix', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // evil-coolify.example.com also contains the FQDN as a substring - $response = $this->get('http://evil-coolify.example.com/'); - - expect($response->status())->toBe(400); -}); - -it('rejects X-Forwarded-Host that is a superstring of trusted FQDN', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $response = $this->get('/', [ - 'X-Forwarded-Host' => 'coolify.example.com.evil.com', - ]); - - expect($response->status())->toBe(400); -}); - -it('rejects host containing localhost as substring', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // "evil-localhost" contains "localhost" — must not match the literal entry - $response = $this->get('http://evil-localhost/'); - - expect($response->status())->toBe(400); -}); - -it('allows subdomain of APP_URL via regex pattern', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // sub.localhost should match ^(.+\.)?localhost$ from allSubdomainsOfApplicationUrl - $response = $this->get('http://sub.localhost/'); - - expect($response->status())->not->toBe(400); -}); - -it('still enforces host validation for non-terminal routes', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // Use full URL so Laravel's test client doesn't override Host with APP_URL - $response = $this->get('http://evil.com/'); - - // Should get 400 Bad Host for untrusted host - expect($response->status())->toBe(400); -}); - -it('rejects requests with spoofed X-Forwarded-Host header', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // Host header is trusted (localhost), but X-Forwarded-Host is spoofed. - // TrustHosts must reject this BEFORE TrustProxies can apply the spoofed host. - $response = $this->get('/', [ - 'X-Forwarded-Host' => 'evil.com', - ]); - - expect($response->status())->toBe(400); -}); - -it('allows legitimate X-Forwarded-Host from reverse proxy matching configured FQDN', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // Legitimate request from Cloudflare/Traefik — X-Forwarded-Host matches the configured FQDN - $response = $this->get('/', [ - 'X-Forwarded-Host' => 'coolify.example.com', - ]); - - // Should NOT be rejected (would be 400 for Bad Host) - expect($response->status())->not->toBe(400); -}); - -it('allows X-Forwarded-Host with port matching configured FQDN', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - // Some proxies include the port in X-Forwarded-Host - $response = $this->get('/', [ - 'X-Forwarded-Host' => 'coolify.example.com:443', - ]); - - // Should NOT be rejected — port is stripped before matching - expect($response->status())->not->toBe(400); -}); - -it('skips host validation for API routes', function () { - // All API routes use token-based auth (Sanctum), not host validation - // They should be accessible from any host (mobile apps, CLI tools, scripts) - - // Test health check endpoint - $response = $this->get('/api/health', [ - 'Host' => 'internal-lb.local', - ]); - expect($response->status())->not->toBe(400); - - // Test v1 health check - $response = $this->get('/api/v1/health', [ - 'Host' => '10.0.0.5', - ]); - expect($response->status())->not->toBe(400); - - // Test feedback endpoint - $response = $this->post('/api/feedback', [], [ - 'Host' => 'mobile-app.local', - ]); - expect($response->status())->not->toBe(400); -}); - -it('trusts localhost when FQDN is configured', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('localhost'); -}); - -it('trusts 127.0.0.1 when FQDN is configured', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('127.0.0.1'); -}); - -it('trusts IPv6 loopback when FQDN is configured', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $middleware = new TrustHosts($this->app); - $hosts = $middleware->hosts(); - - expect($hosts)->toContain('[::1]'); -}); - -it('allows local access via localhost when FQDN is configured and request uses localhost host header', function () { - InstanceSettings::updateOrCreate( - ['id' => 0], - ['fqdn' => 'https://coolify.example.com'] - ); - - $response = $this->get('/', [ - 'Host' => 'localhost', - ]); - - // Should NOT be rejected as untrusted host (would be 400) - expect($response->status())->not->toBe(400); -}); - -it('skips host validation for webhook endpoints', function () { - // All webhook routes are under /webhooks/* prefix (see RouteServiceProvider) - // and use cryptographic signature validation instead of host validation - - // Test GitHub webhook - $response = $this->post('/webhooks/source/github/events', [], [ - 'Host' => 'github-webhook-proxy.local', - ]); - expect($response->status())->not->toBe(400); - - // Test GitLab webhook - $response = $this->post('/webhooks/source/gitlab/events/manual', [], [ - 'Host' => 'gitlab.example.com', - ]); - expect($response->status())->not->toBe(400); - - // Test Stripe webhook — may return 400 from Stripe signature validation, - // but the response should NOT contain "Bad Host" (host validation error) - $response = $this->post('/webhooks/payments/stripe/events', [], [ - 'Host' => 'stripe-webhook-forwarder.local', - ]); - expect($response->content())->not->toContain('Bad Host'); -});