Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FrankenPHP: Laravel api throttling not working (429 Too Many Requests) #889

Open
jhm-ciberman opened this issue May 14, 2024 · 12 comments
Open
Assignees

Comments

@jhm-ciberman
Copy link

Octane Version

v2.3.10

Laravel Version

v11.6.0

PHP Version

v8.3.6

What server type are you using?

FrankenPHP

Server Version

v1.1.4 PHP 8.3.6 Caddy v2.7.6

Database Driver & Version

Postgress

Description

Last week we updated our app previously using php-fpm running on Forge to use Laravel Octane with FrankenPHP. Our site is mostly an API that handles analytics events (Like google analytics). It uses the default Laravel api throttling.

In staging our app worked fine (30 req/sec same IP), but when deploying to production (1400 req/sec, different IPs) it started to fail, giving a lot of 429 Too Many Requests.

image

I quickly rolled back to php-fpm and after a few hours tried again with the same problem. Rolled back and the next day I switched to Swoole and it worked perfectly without changing a single line of code nor having to redeploy anything. So I can confidently say that is NOT a bug in my code, but rather a bug with FrankenPHP or the Octane integration with FrankenPHP.

My theory is that the RateLimiter is not reseting between requests so it's shared between different users. So multiple different users trigger the rate limiter:

This is my Rate limiter configuration:

// AppServiceProvider

RateLimiter::for('api', function (Request $request) {
    return Limit::perMinute(60)->by($request->user()?->id ?: $request->ip());
});

our production CACHE_STORE is redis. Throttling worked perfectly fine without octane and with octane but using Swoole. It failed with hundred of 429 Too Many Requests after installing FrankenPHP.

This is our bootstrap/app.php:

<?php

use Illuminate\Foundation\Application;
use Illuminate\Foundation\Configuration\Exceptions;
use Illuminate\Foundation\Configuration\Middleware;
use Illuminate\Support\Facades\App;

return Application::configure(basePath: dirname(__DIR__))
    ->withRouting(
        commands: __DIR__.'/../routes/console.php',
        health: '/up',
        then: function () {
            Route::middleware('api')
                ->prefix('api')
                ->as('api.')
                ->domain(config('app.domain'))
                ->group(base_path('routes/api.php'));

            Route::middleware('web')
                ->domain(config('app.domain'))
                ->group(base_path('routes/web.php'));

            Route::middleware('web')
                ->domain(config('playsaurus.ads.domain'))
                ->group(base_path('routes/ads.php'));
        }
    )
    ->withMiddleware(function (Middleware $middleware) {
        $middleware->throttleApi();

        $middleware->redirectTo(
            guests: '/login',
            users: '/',
        );

        $middleware->web(append: [
            \App\Http\Middleware\HandleInertiaRequests::class,
            \Illuminate\Http\Middleware\AddLinkHeadersForPreloadedAssets::class,
        ]);

        $middleware->api(append: [
            \App\Http\Middleware\ConfigureLocale::class,
        ]);

        $middleware->alias([
            'localize' => \App\Http\Middleware\ConfigureLocale::class,
            'embed' => \App\Http\Middleware\AllowsEmbeding::class,
        ]);
    })
    ->withExceptions(function (Exceptions $exceptions) {
        $exceptions->dontReport([
            \App\Services\Announcements\InvalidVariantKey::class,
            \App\Exceptions\CouponRedeemException::class,
        ]);
    })->create();

Steps To Reproduce

It's difficult to reproduce. Because I can't test it in production because that would mean a lot of downtime for our users.

My theory is that it would be possible to reproduce from multiple different IPs. But since I don't have the means to test it, I don't know.

@driesvints
Copy link
Member

@dunglas do you maybe know the answer to this one?

@dunglas
Copy link
Contributor

dunglas commented May 14, 2024

I suspect a trusted proxy misconfiguration. Could you check if $request->ip() isn't always returning the same IP?

If the same IP is always returned, it's likely because you are using a proxy in front of FrankenPHP. In this case, you'll have to follow these steps: dunglas/frankenphp#718 (reply in thread)

We could automatically configure Caddy's trusted headers if Laravel's "trusted proxies" are set. Would you accept a patch doing this @driesvints @nunomaduro?

@jhm-ciberman
Copy link
Author

jhm-ciberman commented May 14, 2024

That makes sense. I can't easily check that. I would need to deploy a special route in our staging environment and then ask our teammates to access from different IPs. Maybe I can do that later.

I can confirm that I am using Laravel Forge with the default configuration. I activated Laravel Octane using Forge UI and then added the extra lines to the nginx configuration that are needed to run our application.

Here is the nginx config we are using. The custom configuration is marked with a CUSTOM DIRECTIVE comment. Everything else is exactly as configured by forge when you toggle on Laravel Octane.

(I replaced our real domain for example.com)

# FORGE CONFIG (DO NOT REMOVE!)
include forge-conf/app.example.com/before/*;

map $http_upgrade $connection_upgrade {
    default upgrade;
    ''      close;
}

map $upstream_http_x_frame_options $x_frame_options { ## CUSTOM DIRECTIVE
    '' SAMEORIGIN;
}

server {
    listen 443 ssl http2;
    listen [::]:443 ssl http2;
    server_name .app.example.com;
    server_tokens off;
    root /home/forge/app.example.com/public;

    # FORGE SSL (DO NOT REMOVE!)
    ssl_certificate /etc/nginx/ssl/app.example.com/2169528/server.crt;
    ssl_certificate_key /etc/nginx/ssl/app.example.com/2169528/server.key;

    ssl_protocols TLSv1.2 TLSv1.3;
    ssl_ciphers ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384;
    ssl_prefer_server_ciphers off;
    ssl_dhparam /etc/nginx/dhparams.pem;

    add_header X-Frame-Options $x_frame_options; ## CUSTOM DIRECTIVE
    add_header X-XSS-Protection "1; mode=block";
    add_header X-Content-Type-Options "nosniff";
    proxy_hide_header X-Powered-By; ## CUSTOM DIRECTIVE

    index index.html index.htm index.php;

    charset utf-8;

    # CORS for public storage folder ## CUSTOM DIRECTIVE
    location /storage/ {
        add_header Access-Control-Allow-Origin "*";
        add_header Access-Control-Allow-Methods "GET, OPTIONS";
        add_header Access-Control-Allow-Headers "Authorization, Origin, X-Requested-With, Content-Type, Accept";
        add_header Access-Control-Allow-Credentials "true";
    }

    # FORGE CONFIG (DO NOT REMOVE!)
    include forge-conf/app.example.com/server/*;

    location /index.php {
        try_files /not_exists @octane;
    }

    location / {
        try_files $uri $uri/ @octane;
    }

    location = /favicon.ico { access_log off; log_not_found off; }
    location = /robots.txt  { access_log off; log_not_found off; }

    access_log off;
    error_log  /var/log/nginx/app.example.com-error.log error;

    error_page 404 /index.php;

    location @octane {
        set $suffix "";

        if ($uri = /index.php) {
            set $suffix ?$query_string;
        }

        proxy_http_version 1.1;
        proxy_set_header Host $http_host;
        proxy_set_header Scheme $scheme;
        proxy_set_header SERVER_PORT $server_port;
        proxy_set_header REMOTE_ADDR $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header Upgrade $http_upgrade;
        proxy_set_header Connection $connection_upgrade;

        proxy_pass http://127.0.0.1:8000$suffix;
    }

    location ~ /\.(?!well-known).* {
        deny all;
    }
}

# FORGE CONFIG (DO NOT REMOVE!)
include forge-conf/app.example.com/after/*;

@dunglas
Copy link
Contributor

dunglas commented May 14, 2024

Ok. Setting the environment variable CADDY_GLOBAL_OPTIONS to:

servers { trusted_proxies static private_range }

should fix the issue.

I'll try to write a patch to make Octane automate this when needed.

@nunomaduro
Copy link
Member

@dunglas ping me when that PR is ready.

@colorando-de
Copy link

@jhm-ciberman I just needed to add trusted proxy 127.0.0.1 in the app/Http/Middleware/TrustProxies.php:

<?php

namespace App\Http\Middleware;

use Illuminate\Http\Middleware\TrustProxies as Middleware;
use Illuminate\Http\Request;

class TrustProxies extends Middleware
{
    /**
     * The trusted proxies for this application.
     *
     * @var array|string|null
     */
    protected $proxies = [
        '127.0.0.1'
    ];

    /*
     * The headers that should be used to detect proxies.
     *
     * @var int
     */
    protected $headers =
        Request::HEADER_X_FORWARDED_FOR |
        Request::HEADER_X_FORWARDED_HOST |
        Request::HEADER_X_FORWARDED_PORT |
        Request::HEADER_X_FORWARDED_PROTO |
        Request::HEADER_X_FORWARDED_AWS_ELB;
}

You find the documentation here for Laravel 10 and Laravel 11

@driesvints
Copy link
Member

@dunglas heya, just wanted to check if the patch for this was still on your radar?

@dunglas
Copy link
Contributor

dunglas commented Jun 18, 2024

@driesvints yes, it's still on my todo list, but I have to admit there are a lot of items before.... 😅

If it's critical I can work on this next week.

@driesvints
Copy link
Member

No worries, was just checking! Don't think this is super urgent.

@laravel laravel deleted a comment from github-actions bot Jul 4, 2024
@dunglas
Copy link
Contributor

dunglas commented Dec 4, 2024

I finally digged into this, and I was wrong. There is nothing to do in Octane nor in FrankenPHP.

The only thing to do is to configure the trusted proxies middleware, which is intended because there is NGINX in front of Caddy.

Maybe should you configure this middleware by default in Forge?

Anyway, this issue can be closed.

@driesvints driesvints assigned jbrooksuk and unassigned dunglas Dec 4, 2024
@jbrooksuk
Copy link
Member

@dunglas thanks for looking into this. To sense check this, this is something we can't do in Nginx and needs to be configured in the TrustedProxies middleware within the application itself?

@jhm-ciberman
Copy link
Author

Maybe should you configure this middleware by default in Forge?

Is this documented anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants