Skip to content

fix: fail closed on instance health endpoint when token unset #194

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.

Sign up for GitHub

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

Open
dan2k3k4 wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix: fail closed on instance health endpoint when token unset #194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions app/Http/Controllers/Api/PolydockInstanceHealthController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,25 @@ public function __invoke(Request $request, string $uuid, string $status)
$expectedToken = config('polydock.health_token');
$suppliedToken = $request->query('token');

if ($expectedToken) {
// Timing-safe comparison to prevent timing-based enumeration
if (! is_string($suppliedToken) || ! hash_equals((string) $expectedToken, $suppliedToken)) {
return response()->json([
'error' => 'Unauthorized: Invalid or missing health token',
'status_code' => 401,
], 401);
}
} else {
Log::warning('POLYDOCK_HEALTH_TOKEN is not configured. Health endpoint is currently running without authentication.');
// Note: not empty() — a token literally set to "0" is valid and must not
// be treated as "unconfigured". We use is_string() so that falsy
// non-string values (e.g. false from POLYDOCK_HEALTH_TOKEN=false) are
// also treated as unconfigured rather than as an effective empty token.
if (! is_string($expectedToken) || $expectedToken === '') {
Log::error('POLYDOCK_HEALTH_TOKEN is not configured; rejecting health request (fail-closed).');

return response()->json([
'error' => 'Health endpoint is not configured',
'status_code' => 503,
], 503);
}

// Timing-safe comparison to prevent timing-based enumeration
if (! is_string($suppliedToken) || ! hash_equals((string) $expectedToken, $suppliedToken)) {
return response()->json([
'error' => 'Unauthorized: Invalid or missing health token',
'status_code' => 401,
], 401);
}

$query = $request->query();
Expand Down
32 changes: 21 additions & 11 deletions tests/Feature/Api/InstanceHealthApiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,25 @@ protected function setUp(): void
$this->uuid = $this->instance->uuid;
}

public function test_health_check_without_configured_token_allows_request(): void
public function test_health_check_without_configured_token_returns_503(): void
{
// GIVEN no token is configured
// GIVEN no token is configured, and the instance is in a state distinct
// from the one the request would set (so "unchanged" is meaningful).
Config::set('polydock.health_token', null);
$this->instance->status = PolydockAppInstanceStatus::RUNNING_UNHEALTHY;
$this->instance->save();

// WHEN we hit the endpoint without a token
// WHEN we hit the endpoint (without a token) asking for a *different* status
$response = $this->getJson("/api/instance/{$this->uuid}/health/running-healthy-claimed");

// THEN it should be successful and update the status
$response->assertStatus(200);
// THEN it should fail closed with 503 and NOT update the status
$response->assertStatus(503);
$response->assertJson([
'error' => 'Health endpoint is not configured',
'status_code' => 503,
]);
$this->instance->refresh();
$this->assertEquals(PolydockAppInstanceStatus::RUNNING_HEALTHY_CLAIMED, $this->instance->status);
$this->assertEquals(PolydockAppInstanceStatus::RUNNING_UNHEALTHY, $this->instance->status);
}

public function test_health_check_with_configured_token_and_correct_token_allows_request(): void
Expand Down Expand Up @@ -129,12 +136,12 @@ public function test_health_check_from_trusted_ip_bypasses_rate_limit(): void
{
// GIVEN a trusted IP is configured
Config::set('polydock.trusted_ips', ['10.0.0.5']);
Config::set('polydock.health_token', null);
Config::set('polydock.health_token', 'secure-test-token');

// WHEN we hit the endpoint 130 times from trusted IP
for ($i = 0; $i < 130; $i++) {
$this->withServerVariables(['REMOTE_ADDR' => '10.0.0.5'])
->getJson("/api/instance/{$this->uuid}/health/running-healthy-claimed")
->getJson("/api/instance/{$this->uuid}/health/running-healthy-claimed?token=secure-test-token")
->assertStatus(200);
}
}
Expand All @@ -153,7 +160,7 @@ public function test_health_check_with_valid_token_bypasses_rate_limit(): void

public function test_health_check_throttles_per_ip_across_uuids(): void
{
Config::set('polydock.health_token', null); // No token gating
Config::set('polydock.health_token', 'secure-test-token');

// GIVEN another instance exists
$storeApp = PolydockStoreApp::first();
Expand All @@ -168,10 +175,13 @@ public function test_health_check_throttles_per_ip_across_uuids(): void
$anotherInstance->status = PolydockAppInstanceStatus::RUNNING_HEALTHY_CLAIMED;
$anotherInstance->saveQuietly();

// WHEN we exhaust the per-minute limit (120) against the first UUID
// WHEN we exhaust the per-minute limit (120) against the first UUID.
// A valid token bypasses the limiter, so these are unauthenticated
// (401) requests — the realistic UUID-enumeration scenario the limiter
// exists to stop. They still count against the per-IP budget.
for ($i = 0; $i < 120; $i++) {
$this->getJson("/api/instance/{$this->uuid}/health/running-healthy-claimed")
->assertStatus(200);
->assertStatus(401);
}

// THEN a further request for the first UUID is throttled
Expand Down