Skip to content

feat: rate-limit unauthenticated public API routes#192

Merged
dan2k3k4 merged 1 commit into
devfrom
advisor/002-rate-limit-public-api
Jul 2, 2026
Merged

feat: rate-limit unauthenticated public API routes#192
dan2k3k4 merged 1 commit into
devfrom
advisor/002-rate-limit-public-api

Conversation

@dan2k3k4

@dan2k3k4 dan2k3k4 commented Jul 1, 2026

Copy link
Copy Markdown
Member

Adds rate limiting to the unauthenticated public API routes (register, register status, regions, and the health callback).

Greptile Summary

This PR adds rate limiting to the four unauthenticated public API routes (/register POST, /register/{uuid} GET, /regions GET, and the instance health callback) using Laravel named rate limiters defined in AppServiceProvider::boot(). Trusted internal IPs (configured via POLYDOCK_TRUSTED_IPS) and callers presenting a valid health token bypass the limits entirely.

  • Named limiters (register at 10/min, public-read at 60/min, instance-health at 120/min) replace what would have been a shared anonymous throttle counter, giving each route its own per-IP bucket.
  • A new trusted_ips config key is parsed from a comma-separated env var; test suites add tearDown cache flushes to prevent counter bleed between tests.
  • One test — it_allows_trusted_ips_to_bypass_registration_status_throttle — passes REMOTE_ADDR as a header argument to getJson rather than via withServerVariables, so the trusted-IP guard never fires and the test fails at request 61 with a 429.

Confidence Score: 4/5

The production rate-limiting logic is correct; a broken test will cause CI to fail on the trusted-IP bypass path for the registration-status route.

The core implementation — named rate limiters, trusted-IP bypass, token bypass for instance-health — is well-structured and the logic is sound. One test (it_allows_trusted_ips_to_bypass_registration_status_throttle) passes REMOTE_ADDR as a header to getJson instead of using withServerVariables, so $request->ip() returns 127.0.0.1, the trusted-IP check fails, and the test fails at request 61 with 429. The equivalent test for the POST register route was fixed correctly in this PR; this getJson variant was missed.

tests/Feature/Controllers/Api/RegisterControllerTest.php — the it_allows_trusted_ips_to_bypass_registration_status_throttle test uses the wrong approach for spoofing the client IP.

Important Files Changed

Filename Overview
app/Providers/AppServiceProvider.php Adds three named rate limiters (register, public-read, instance-health) to boot(). Trusted-IP bypass and valid-token bypass for instance-health are correctly evaluated at request time via closures. Logic looks sound.
config/polydock.php Adds trusted_ips config key parsed from POLYDOCK_TRUSTED_IPS env var via explode/trim/array_filter. Empty-string and missing-env edge cases are handled correctly.
routes/api.php Applies named throttle middleware to all four public routes. Each route now uses its own isolated counter (register, public-read, instance-health).
tests/Feature/Api/InstanceHealthApiTest.php Adds three new rate-limit tests. Trusted-IP test correctly uses withServerVariables. tearDown flushes the cache to prevent cross-test leakage. No issues.
tests/Feature/Controllers/Api/RegisterControllerTest.php it_allows_trusted_ips_to_bypass_registration_status_throttle passes REMOTE_ADDR as a header argument to getJson instead of using withServerVariables, so the trusted-IP bypass never triggers and the test fails at request 61 with 429.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming Request] --> B{throttle middleware}
    B --> C{Named limiter closure}
    C --> D{IP in trusted_ips?}
    D -->|Yes| E[Limit::none — bypass]
    D -->|No| F{instance-health route?}
    F -->|Yes| G{Valid health token?}
    G -->|Yes| E
    G -->|No| H[Limit::perMinute 120 by IP]
    F -->|No| I{register route?}
    I -->|Yes| J[Limit::perMinute 10 by IP]
    I -->|No| K[Limit::perMinute 60 by IP]
    H --> L{Under limit?}
    J --> L
    K --> L
    L -->|Yes| M[Proceed to controller]
    L -->|No| N[429 Too Many Requests]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Incoming Request] --> B{throttle middleware}
    B --> C{Named limiter closure}
    C --> D{IP in trusted_ips?}
    D -->|Yes| E[Limit::none — bypass]
    D -->|No| F{instance-health route?}
    F -->|Yes| G{Valid health token?}
    G -->|Yes| E
    G -->|No| H[Limit::perMinute 120 by IP]
    F -->|No| I{register route?}
    I -->|Yes| J[Limit::perMinute 10 by IP]
    I -->|No| K[Limit::perMinute 60 by IP]
    H --> L{Under limit?}
    J --> L
    K --> L
    L -->|Yes| M[Proceed to controller]
    L -->|No| N[429 Too Many Requests]
Loading

Reviews (5): Last reviewed commit: "feat: rate-limit unauthenticated public ..." | Re-trigger Greptile

Comment thread tests/Feature/Controllers/Api/RegisterControllerTest.php
Comment thread routes/api.php Outdated
@dan2k3k4

dan2k3k4 commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

Re: the trusted-IP bypass tests failing in CI — this is a false positive. The tests pass as written, and REMOTE_ADDR doesn't need withServerVariables.

Laravel special-cases REMOTE_ADDR. In MakesHttpRequests::formatServerHeaderKey, header keys get an HTTP_ prefix except CONTENT_TYPE and REMOTE_ADDR:

protected function formatServerHeaderKey($name)
{
    if (! str_starts_with($name, 'HTTP_') && $name !== 'CONTENT_TYPE' && $name !== 'REMOTE_ADDR') {
        return 'HTTP_'.$name;
    }
    return $name;
}

So ['REMOTE_ADDR' => '10.0.0.5'] lands as a server variable — identical to withServerVariables(['REMOTE_ADDR' => '10.0.0.5']). $request->ip() sees 10.0.0.5, not loopback.

Confirmed empirically. it_allows_trusted_ips_to_bypass_registration_throttle posts 15 times asserting 202 each, against a 10/min limit. The sibling it_throttles_repeated_registration_attempts proves the limiter accumulates across requests in-test (11th → 429). If the bypass hadn't activated, requests 11–15 would 429 and the assertion would fail — but the full suite is green (298 passed). Same for the health tests (130 hits vs a 120 limit). If the claim held, these would already be red.

Leaving the tests as-is.


Separately, I pushed a fix for a real issue: the public-read and instance-health limiters were keyed ->by($uuid), which hands an enumerating attacker a fresh budget per guessed UUID — defeating the anti-enumeration purpose. Reverted both to ->by($request->ip()) and updated the two tests to assert that switching UUIDs from the same IP is also throttled.

Comment thread tests/Feature/Controllers/Api/RegisterControllerTest.php
Comment thread tests/Feature/Api/InstanceHealthApiTest.php
@dan2k3k4 dan2k3k4 force-pushed the advisor/002-rate-limit-public-api branch from 29daf5e to a2a5741 Compare July 1, 2026 18:23
@dan2k3k4 dan2k3k4 force-pushed the advisor/002-rate-limit-public-api branch from a2a5741 to b022d68 Compare July 1, 2026 18:24
@dan2k3k4 dan2k3k4 merged commit fcd323f into dev Jul 2, 2026
5 checks passed
@dan2k3k4 dan2k3k4 deleted the advisor/002-rate-limit-public-api branch July 2, 2026 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant