Add healthcheck endpoint and harden geo service#2
Merged
lohanidamodar merged 18 commits intomainfrom Mar 23, 2026
Merged
Conversation
- Add /v1/health endpoint that bypasses auth for container health checks - Add HEALTHCHECK instruction to Dockerfile - Validate IP format with filter_var before geodb lookup (400 on invalid) - Harden bearer token parsing to require "Bearer" prefix - Fix unsafe array access in geodb record with null coalescing - Fix log namespace typo: "executor" -> "geo" in Error.php - Make geodb path configurable via GEO_DBIP_PATH env var - Update base image from 0.9.3 to 1.1.1 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFinal Docker stage base image updated to appwrite/base:1.1.1 and a HEALTHCHECK added that probes GET /v1/health. docker-compose: adds GEO_DBIP_PATH to the geo service environment and a new geo-tests service to run PHPUnit e2e tests. Added PHPUnit config and E2E test suite with helper base class and tests for /v1/ips/{ip} and /v1/health. Added Health HTTP action (GET /v1/health) and registered it. Get handler now validates IPs and uses safe fallbacks for geo fields. Init tightens Authorization parsing and error messages. Error and server logging switched to use error_log and log namespace "geo". Server now reads GEO_DBIP_PATH and validates the MaxMind DB path. Composer dependency constraints, dev tools, scripts, and autoload-dev adjusted. CI workflows updated to run E2E tests and lint/check steps. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR hardens the geo service with several well-scoped improvements: an unauthenticated Key changes:
Confidence Score: 4/5
Important Files Changed
Reviews (1): Last reviewed commit: "Add healthcheck endpoint and harden geo ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Geo/Modules/Core/Http/Error.php (1)
87-87:⚠️ Potential issue | 🟠 MajorUpdate
logErrornullable parameter types for PHP 8.4 compatibility.Line 87 uses implicitly nullable typed parameters (
Logger $logger = null,Route $route = null), which is deprecated in PHP 8.4 and fails static analysis. Add explicit nullable type syntax using the?prefix.Suggested fix
- protected function logError(Log $log, Throwable $error, string $action, Logger $logger = null, Route $route = null): void + protected function logError(Log $log, Throwable $error, string $action, ?Logger $logger = null, ?Route $route = null): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Geo/Modules/Core/Http/Error.php` at line 87, The method signature for logError uses implicitly nullable parameters which is deprecated; update the signature of protected function logError(Log $log, Throwable $error, string $action, Logger $logger = null, Route $route = null): void to use explicit nullable types: change Logger $logger = null to ?Logger $logger and Route $route = null to ?Route $route (and update any related type hints or docblocks referencing these params if present to reflect the nullable types).
🧹 Nitpick comments (1)
src/Geo/Server/Server.php (1)
58-61: Harden GeoDB path resolution for empty env values.If
GEO_DBIP_PATHis set but empty, Line 59 won’t fall back to default andReaderreceives an invalid path.🔧 Suggested hardening
$geodb->setCallback(function () { $defaultPath = __DIR__ . '/../../../app/assets/dbip/dbip-country-lite-2024-09.mmdb'; - $path = System::getEnv('GEO_DBIP_PATH', $defaultPath); + $path = System::getEnv('GEO_DBIP_PATH', '') ?: $defaultPath; + if (!\is_readable($path)) { + throw new Exception('GeoDB file is not readable: ' . $path); + } /** `@phpstan-ignore` class.notFound */ return new Reader($path); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Geo/Server/Server.php` around lines 58 - 61, The current Server::... code uses System::getEnv('GEO_DBIP_PATH', $defaultPath) which returns an empty string when the env var is set but empty, causing Reader to receive an invalid path; change it to read the env into a variable (e.g. $envPath = System::getEnv('GEO_DBIP_PATH', '')) and if trim($envPath) is empty use $defaultPath, otherwise use $envPath, then pass that $path into new Reader($path) (keep the existing `@phpstan-ignore` if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Geo/Modules/Core/Http/Error.php`:
- Line 87: The method signature for logError uses implicitly nullable parameters
which is deprecated; update the signature of protected function logError(Log
$log, Throwable $error, string $action, Logger $logger = null, Route $route =
null): void to use explicit nullable types: change Logger $logger = null to
?Logger $logger and Route $route = null to ?Route $route (and update any related
type hints or docblocks referencing these params if present to reflect the
nullable types).
---
Nitpick comments:
In `@src/Geo/Server/Server.php`:
- Around line 58-61: The current Server::... code uses
System::getEnv('GEO_DBIP_PATH', $defaultPath) which returns an empty string when
the env var is set but empty, causing Reader to receive an invalid path; change
it to read the env into a variable (e.g. $envPath =
System::getEnv('GEO_DBIP_PATH', '')) and if trim($envPath) is empty use
$defaultPath, otherwise use $envPath, then pass that $path into new
Reader($path) (keep the existing `@phpstan-ignore` if needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c13470be-e03e-4c53-ad9c-3621d24718a2
📒 Files selected for processing (8)
Dockerfiledocker-compose.ymlsrc/Geo/Modules/Core/Http/Error.phpsrc/Geo/Modules/Core/Http/Get.phpsrc/Geo/Modules/Core/Http/Health.phpsrc/Geo/Modules/Core/Http/Init.phpsrc/Geo/Modules/Core/Services/Http.phpsrc/Geo/Server/Server.php
- Add PHPUnit e2e test suite with tests for: - Health endpoint returns 200 without auth - IP lookup requires valid Bearer auth - Invalid auth scheme returns 401 - Invalid IP returns 400 - Valid IP returns geo data with all fields - IPv6 support - Unknown IP returns defaults - Add docker-compose test service for running e2e tests - Update utopia-php/system to ^0.10.0, logger to ^0.7.0 - Use canonical dev-feat-upgrade-http format for platform Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/E2E/HealthTest.php (1)
15-21: Reduce duplication and strengthen auth-independence assertion.This test currently overlaps Line 7–13 behavior. Consider sending an invalid/malformed
Authorizationheader here and still asserting200+status=okto better prove the endpoint is truly unauthenticated.Suggested test adjustment
public function testHealthEndpointDoesNotRequireAuth(): void { - // No authorization header - should still return 200 - $response = $this->getJson('/v1/health'); + $response = $this->getJson('/v1/health', [ + 'Authorization' => 'Bearer invalid-secret', + ]); $this->assertEquals(200, $response['status']); + $this->assertEquals('ok', $response['json']['status']); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/E2E/HealthTest.php` around lines 15 - 21, Update the testHealthEndpointDoesNotRequireAuth method to send a malformed/invalid Authorization header when calling getJson('/v1/health') so the endpoint is proven auth-independent; keep asserting a 200 HTTP result (using assertEquals or assertSame on the response status) and also assert the response body contains status equal to 'ok' (e.g., assertEquals('ok', $response['status']) or equivalent) to strengthen the check that the endpoint returns healthy regardless of Authorization header presence or validity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 10-11: composer.lock is out of sync with composer.json: update the
lockfile to match the bumped constraints for the packages utopia-php/system and
utopia-php/logger by running composer update utopia-php/system utopia-php/logger
(or composer update) locally, verify composer.lock now pins versions compatible
with ^0.10.0 and ^0.7.0, and commit the regenerated composer.lock so Docker/CI
installs succeed.
In `@tests/E2E/GeoTest.php`:
- Around line 56-67: Replace the weak key-existence check in
testGetIpHandlesUnknownIp with explicit assertions that verify the fallback
values: assert that $response['json']['countryCode'] equals '--' and that other
mapped fields (e.g., countryName, region, city or whichever keys your Geo
mapping returns) are empty strings as per the fallback contract; update the
assertions in the testGetIpHandlesUnknownIp method to check these exact values
on the $response variable instead of only using assertArrayHasKey.
---
Nitpick comments:
In `@tests/E2E/HealthTest.php`:
- Around line 15-21: Update the testHealthEndpointDoesNotRequireAuth method to
send a malformed/invalid Authorization header when calling getJson('/v1/health')
so the endpoint is proven auth-independent; keep asserting a 200 HTTP result
(using assertEquals or assertSame on the response status) and also assert the
response body contains status equal to 'ok' (e.g., assertEquals('ok',
$response['status']) or equivalent) to strengthen the check that the endpoint
returns healthy regardless of Authorization header presence or validity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77a2e6d8-d783-4f42-95b1-553f018887c3
📒 Files selected for processing (6)
composer.jsondocker-compose.ymlphpunit.xmltests/E2E/Base.phptests/E2E/GeoTest.phptests/E2E/HealthTest.php
✅ Files skipped from review due to trivial changes (1)
- phpunit.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
- utopia-php/fetch: ^0.2.1 -> ^1.0.0 - utopia-php/system: ^0.10.0 -> ^0.10.1 - utopia-php/logger: ^0.7.0 -> ^0.7.1 - utopia-php/di: ^0.1.0 -> 0.1.* (pinned by cli dev branch) - phpunit/phpunit: ^11.3 -> ^13.0 - laravel/pint: ^1.18 -> ^1.29 - phpstan/phpstan: ^1.12 -> ^2.1 Note: http (1.0.*), platform, and cli remain on dev-feat-upgrade-http branches as no stable release supports the http 1.x API yet. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composer.json (1)
8-8: Avoid pinning to a development branch for production dependencies.Using
dev-feat-upgrade-httpintroduces instability since dev branches are mutable and can receive breaking changes at any time. This could cause unpredictable build failures.Once the
utopia-php/platformchanges are merged and released, update this constraint to a stable version tag (e.g.,^1.0.0or similar).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` at line 8, The composer.json currently pins the dependency "utopia-php/platform" to the mutable dev branch "dev-feat-upgrade-http"; replace this dev branch constraint in composer.json with a stable version constraint (for example "^1.0.0" or the appropriate released tag) as soon as the upstream changes are merged and released to avoid instability—update the "utopia-php/platform" value accordingly and run composer update to regenerate the lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Around line 32-34: Update the phpstan/phpstan version constraint in
composer.json to a valid release range; replace the invalid "^2.1" with "^2.0"
(or the intended exact version) so Composer can resolve phpstan/phpstan (refer
to the dependency entry "phpstan/phpstan" in composer.json). Run composer update
afterwards to verify the new constraint is accepted and tests/CI pass.
---
Nitpick comments:
In `@composer.json`:
- Line 8: The composer.json currently pins the dependency "utopia-php/platform"
to the mutable dev branch "dev-feat-upgrade-http"; replace this dev branch
constraint in composer.json with a stable version constraint (for example
"^1.0.0" or the appropriate released tag) as soon as the upstream changes are
merged and released to avoid instability—update the "utopia-php/platform" value
accordingly and run composer update to regenerate the lockfile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d6b9c9d-d8fb-4858-a07b-1582a45932a7
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
composer.json
- Update Utopia\Http\Validator\Text -> Utopia\Validator\Text (validators moved to separate package in http 1.2.x) - Add GitHub Actions workflow to run e2e tests on PRs and pushes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Geo/Modules/Core/Http/Get.php`:
- Around line 39-40: Remove the now-stale "@phpstan-ignore-next-line" directive
immediately above the $geodb->get($ip) call (the line that assigns $record =
$geodb->get($ip)); simply delete that comment so PHPStan ignores are accurate,
then run PHPStan to confirm no remaining unmatched ignore directives and ensure
no other "@phpstan-ignore-next-line" exists around the same method/function
where $record and $geodb->get are used.
- Around line 43-46: The country and continent "names" fields (seen in Get.php
around the $output assignments) are locale-keyed arrays from MaxMind, so change
the assignments for $output['country'] and $output['continent'] to extract a
specific locale string (preferably 'en') from $record['country']['names'] and
$record['continent']['names']; if 'en' is not present, fall back to the first
available value or an empty string to keep types consistent with the else
branch, leaving $output['countryCode'] and $output['continentCode'] assignments
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1ec3682-b351-4d9a-a576-d5055b11a0df
📒 Files selected for processing (2)
.github/workflows/tests.ymlsrc/Geo/Modules/Core/Http/Get.php
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/tests.yml
- Fix Hook::setCallback() -> Hook::action() for http 1.2.x compat - Add file-existence check for GEO_DBIP_PATH before Reader init - Use PHP for Docker HEALTHCHECK instead of curl (not guaranteed in base image) - Downgrade 4xx errors to Console::warning, keep Console::error for 5xx only - Strengthen unknown IP test to assert exact fallback values - Update CodeQL workflow: use composer:2 image and checkout@v4 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace Utopia\CLI\Console with error_log() (Console removed in cli 0.22.x) - Replace Utopia\Validator\Text with callable validator (platform expects Utopia\Http\Validator which no longer exists in http 1.2.x) - Fix implicitly nullable params in Error.php for PHP 8.4 - Remove redundant is_array() check and null coalescing on non-nullable values - Remove stale phpstan-ignore comments - All phpstan level 8 and pint checks pass Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add utopia-php/console ^0.1.1 dependency (Console extracted from cli) - Replace error_log() with Utopia\Console methods (error, log, info) - Use Utopia\Console namespace instead of old Utopia\CLI\Console Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codeql.yml:
- Around line 15-16: The workflow is misnamed "CodeQL" but only runs composer
checks; either add the CodeQL action steps or rename the workflow. If you want
CodeQL scanning, insert the github/codeql-action/init step (setup codeql with
languages: ['php']), run the existing docker composer commands as a job step,
then add github/codeql-action/analyze to upload results; if you intend only
linters/static analysis, rename the workflow title from "CodeQL" to something
like "PHP Quality" and keep the composer run step. Reference the workflow file
name (.github/workflows/codeql.yml) and the actions github/codeql-action/init
and github/codeql-action/analyze when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7397306-42df-4679-a448-1ad93e5c5d76
📒 Files selected for processing (6)
.github/workflows/codeql.ymlDockerfilesrc/Geo/Modules/Core/Http/Error.phpsrc/Geo/Modules/Core/Http/Get.phpsrc/Geo/Server/Server.phptests/E2E/GeoTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- Dockerfile
- src/Geo/Modules/Core/Http/Get.php
- tests/E2E/GeoTest.php
- src/Geo/Server/Server.php
- src/Geo/Modules/Core/Http/Error.php
- Replace Dependency class with Container::set(string, callable) API - Restore Utopia\Validator\Text (now available via updated platform) - Remove unused Dependency import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…P requirement to >=8.1
The old composer:2.0 image ships an outdated PHP version, causing dependency resolution mismatches with the PHP 8.5 runtime in the base image. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Using a separate composer image caused PHP version mismatch — deps resolved for a different PHP than the runtime, breaking PHPUnit. Now composer runs on the same base image (PHP 8.5) ensuring compatible dependency resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Runs composer install on the same PHP version as runtime by copying the composer binary from composer:2 into the appwrite/base stage. This ensures dependency resolution matches the target PHP 8.5. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hp/platform to 0.10.*
- Http constructor no longer takes Container: Http(Adapter, string) - Use Http::setResource() instead of Container::set() for DI - Http::setMode() is now static - Remove unused Container and DI imports Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChiragAgg5k
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/v1/healthendpoint (unauthenticated) for container health checks with DockerHEALTHCHECKinstructionfilter_var()before geodb lookup, returning 400 on invalid IPsBearerprefixexecutor->geoin Error.phpGEO_DBIP_PATHenv var0.9.3to1.1.1Test plan
/v1/healthreturns{"status":"ok"}without auth header/v1/ips/:ipstill requires valid Bearer tokenGEO_DBIP_PATHenv var overrides default db path🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests / CI
Chores