Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
🟢 Tier S · Ready to mergeThe PR validates Appwrite migration source endpoints as public HTTP(S) URLs, with an optional hostname/CIDR allowlist, worker revalidation, and connection pinning. It adds reusable allowlist/subnet validators, tightens reserved-address and ambiguous-URL checks, preserves URL metadata in SDK specifications, and adds unit and e2e coverage. It also updates Composer orchestration wiring and the ClickHouse configuration mount. Latest changes: The latest commits remove appwrite.test from the development hostname allowlist and use the shared web endpoint in subnet migration tests, exercising DNS resolution and subnet authorization for that hostname.
📂 Walkthrough · 19
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
abnegate
force-pushed
the
fix/migration-endpoint-ssrf
branch
from
October 1, 2026 04:33
5b38258 to
fb1f75a
Compare
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Some callers need to reach destinations that are deliberately private, such as another service on the same network, while still refusing every other private or reserved address. PublicHostname and PublicURL now take an optional Allowlist of exact hostnames and IPv4/IPv6 subnets. A listed hostname is accepted without a lookup, an IP in a listed subnet is accepted, and a hostname is accepted when every address it resolves to is public or listed. Hostnames never match by suffix, and numeric spellings such as 2130706433 or 127.1 are never treated as listed hostnames. The allowlist defaults to empty, so existing callers keep their current behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Appwrite migration report and create endpoints accepted any URL for the source endpoint, so the server could be pointed at loopback, private, link-local or other reserved addresses, and the migrations worker used whatever endpoint had been stored. Both params now use an Endpoint validator built on PublicURL: http or https only, a known public domain or public IP, and every resolved address public. The worker validates a stored endpoint again before it builds the source, so migrations queued earlier or retried are held to the same rule; the internal endpoint the worker writes itself is exempt. PublicHostname now also treats IPv4-compatible IPv6 addresses (::/96, e.g. ::7f00:1) and the deprecated site-local range (fec0::/10) as reserved, and PublicURL refuses URLs with userinfo or backslashes, where URL parsers and curl can disagree on the host. The Endpoint validator reports one fixed description, so a rejection does not reveal whether a host exists or what it resolves to. _APP_MIGRATIONS_ALLOWED_HOSTS takes comma-separated hostnames and IPv4 or IPv6 CIDR ranges for sources that are deliberately private, such as an instance on the same network. Hostnames match exactly. It is empty by default, which allows public sources only; the development .env lists appwrite and appwrite.test so the local stack can migrate from itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
abnegate
force-pushed
the
fix/migration-endpoint-ssrf
branch
from
October 1, 2026 04:39
fb1f75a to
576790f
Compare
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…hp/validators Subnet and Allowlist are generic network primitives, so they belong in the validators package rather than src/Appwrite. Both are now Utopia validators: Subnet matches an address against one CIDR range or single address, and Allowlist matches exact hostnames or addresses inside its subnets. Reading _APP_MIGRATIONS_ALLOWED_HOSTS stays in the migrations Endpoint, since the comma-separated format and the rule that unparsable entries are ignored are Appwrite configuration policy. PublicURL now only asks whether a resolved address is listed; the per-address public-or-listed rule is enforced once, by the PublicHostname check that already follows it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Worker behaviour is covered by e2e tests, not worker unit tests. The create route now refuses private endpoints, so the e2e test stores an Appwrite migration directly, the way a job created before validation would look, retries it through the API and checks that the real worker records the endpoint failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ated addresses The endpoint was validated once, and then the Appwrite source resolved the host again for every request it sent and followed redirects, so the checked addresses were not necessarily the ones it connected to. Apply the 2.0.x and 1.9.x change here. Endpoint::resolve() validates a request URL and returns the CURLOPT_RESOLVE entries for the addresses it checked. The report route and the worker pass it to the Appwrite source through setResolver(), so every source request is validated again and connects only to those addresses. The worker skips its own internal _APP_MIGRATION_HOST endpoint, which now comes from one helper shared with processMigration() and authenticateSource(). setResolver() comes from utopia-php/migration#230, the backport cut from 2.0.8, the version main locks today. It overrides the SDK 27 client's call(). utopia-php/migration#229, the main-line change, builds on Appwrite SDK 30, which imports Utopia\Client; this repository's packages/client only provides Utopia\Client\Client, so that change cannot load here yet. The requirement points at the backport branch until 2.0.9 is tagged, and the lock change covers only that package. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PublicHostname::resolve() uses Swoole\Coroutine\System::getaddrinfo() inside any coroutine, to avoid the ~140 KiB that Swoole 6.2 keeps for each hooked dns_get_record() call. getaddrinfo() always yields, even when the runtime hooks nothing, and a caller that waits for it in an unhooked usleep() loop never lets the reactor resume it. The 1.9.x and 2.0.x backports of this PR hit exactly that: their HTTP servers set no hook_flags, GraphQL's Promise::then() waits in a usleep() loop, and every GraphQL avatar request that fetches a URL hung. The retained memory only comes from the SWOOLE_HOOK_NET_FUNCTION hook, so resolve natively only when that hook is active, and otherwise make the blocking dns_get_record() call, which neither yields nor retains memory without the hook. Main's HTTP server and the workers enable SWOOLE_HOOK_ALL, so they keep the native lookup; this keeps the three lines identical. The regression test resolves inside a child of an unhooked coroutine and fails if control returns to the parent before the lookup finishes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… to end Every Appwrite migration test used appwrite.test, which the allowlist admits by name without resolving it, so no E2E request went through the resolved and pinned path. List the compose network (172.16.238.0/24) in the development allowlist and report and migrate from http://traefik/v1. traefik is not a listed hostname, so the validator resolves it, admits the address because it is inside the listed range, and the source connects to that address through the CURLOPT_RESOLVE entry. Each test uses its own source project so the user counts stay exact while other migration tests run in parallel. Also report from the allowlisted appwrite.test, as 1.9.x already does, and reject http://localhost/v1, a hostname that resolves outside every listed range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
composer.lock: kept main's lock and re-applied only the utopia-php/migration backport branch with composer update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AGENTS.md allows no new regular expressions. Split the CURLOPT_RESOLVE entry on its separators, compare the host and port exactly, and check every address with the Subnet validator for 127.0.0.0/8 or ::1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI enables _APP_OPTIONS_ROUTER_PROTECTION, so requests to http://traefik/v1 reached the API through the pinned address and were refused with 401 because traefik is not a platform hostname. Stop listing appwrite.test by name in the development allowlist instead. It is _APP_DOMAIN, so the router accepts it, and the validator now resolves it to traefik's address inside the listed 172.16.238.0/24 range. The subnet tests use it, and every other Appwrite migration E2E test now takes the resolved and pinned path too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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
1 participant
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.
What
GET /v1/migrations/appwrite/reportandPOST /v1/migrations/appwritechecked the sourceendpointonly withUtopia\Validator\URL. They now use a migrationsEndpointvalidator built onPublicURL: http or https only, a known public domain or public IP, and every resolved address public.The migrations worker validates a stored Appwrite-source endpoint again before it builds the source, so migrations queued earlier or retried follow the same rule. The internal endpoint the worker writes itself (
http://{_APP_MIGRATION_HOST}/v1) is exempt.Connection pinning
The report route and the worker pass
(new Endpoint())->resolve(...)to the Appwrite source throughsetResolver(); the worker skips its own internalhttp://{_APP_MIGRATION_HOST}/v1endpoint, which onegetInternalEndpoint()helper now builds forprocessMigration(),processSource()andauthenticateSource().Endpoint::resolve(string $url)validates each request URL and returns theCURLOPT_RESOLVEentries for the addresses it checked, or throwsgeneral_argument_invalid. Every source request is therefore validated again and connects only to the checked addresses, and the source no longer follows redirects.Dependency
setResolver()comes from utopia-php/migration#230, the backport cut from 2.0.8, which is the version main locks today.composer.jsonrequiresdev-feat/source-request-controls-2.0.x as 2.0.9until 2.0.9 is tagged, and the lock change covers only that package. Switch it back to^2.0.9before merge. utopia-php/migration#229, the main-line change, builds on Appwrite PHP SDK 30, which importsUtopia\Client; this repository'spackages/clientonly providesUtopia\Client\Client, so that version cannot load here until the SDK or the package catches up.SubnetandAllowlistlive inpackages/validators, which the root autoload loads directly, so no utopia-php/validators release is needed for this PR; the packageCHANGELOG.mdlists them under Unreleased for the next mirror release.Validator corrections this relies on
PublicHostnametreats IPv4-compatible IPv6 (::/96, e.g.::7f00:1) and site-local (fec0::/10) addresses as reserved.PublicURLrefuses URLs with userinfo or backslashes, where URL parsers and curl can disagree on the host.2130706433,0x7f.1,127.1) are never treated as allowed hostnames.Endpointreports one fixed description, so a rejection does not reveal whether a host exists or what it resolves to.Opt-in allowlist
_APP_MIGRATIONS_ALLOWED_HOSTStakes comma-separated hostnames and IPv4/IPv6 CIDR ranges for sources that are deliberately private, such as another instance on the same network. A listed hostname, an IP in a listed range, or a hostname whose resolved addresses are all public or listed is accepted. Hostnames match exactly:example.comdoes not allowevil-example.comordb.example.com. The default is empty, which allows public sources only. The development.envlistsappwriteand the compose network172.16.238.0/24so the local stack can migrate from itself.Effect on other callers
PublicHostnameandPublicURLtake an optionalAllowlistthat defaults to empty. The other callers keep their behaviour except for the two corrections above:::/96andfec0::/10addresses are now rejected (avatars favicon/image/screenshot, webhooks worker), and URLs with userinfo or backslashes are now rejected (avatars favicon/image/screenshoturl).Lookups in unhooked coroutines
PublicHostname::resolve()usesSwoole\Coroutine\System::getaddrinfo()only when the coroutine hooks network functions (SWOOLE_HOOK_NET_FUNCTION), the only case where Swoole 6.2'sdns_get_record()retains ~140 KiB per lookup. Elsewhere it keeps the blockingdns_get_record().getaddrinfo()always yields, and the 1.9.x and 2.0.x HTTP servers set nohook_flags, so GraphQL'sPromise::then(), which waits in an unhookedusleep()loop, never let a yielded avatars lookup resume: every GraphQL avatar request that fetches a URL hung (seven 120 s timeouts in the GraphQL E2E job). Main's HTTP server and the workers enableSWOOLE_HOOK_ALLand keep the native lookup.Tests
AllowlistTest,SubnetTest,PublicHostnameTest,PublicURLTest(exact host, IPv4 CIDR, IPv6 CIDR, non-match, lookalike hosts, numeric spellings, IPv6 forms, userinfo, backslashes),EndpointTest.EndpointTestcoversresolve()returning the checked addresses inside a hooked coroutine, returning none for IP literals, and throwing for every rejected endpoint. All the new cases fail without the change.testGetAppwriteReportWithSubnetEndpointandtestCreateAppwriteMigrationWithSubnetEndpointreport and migrate fromhttp://appwrite.test/v1. The development.envno longer listsappwrite.testby name, so the validator resolves it to traefik's address, admits it because.envlists the compose network172.16.238.0/24, and the source connects to that address through theCURLOPT_RESOLVEentry.appwrite.testis_APP_DOMAIN, so CI's router protection accepts it, and every other Appwrite migration E2E test takes the same pinned path. Each uses its own source project, so the user counts are exact.testGetAppwriteReportreports from the shared source project, and the rejection test now also refuseshttp://localhost/v1.PublicHostnameTest::testResolvesWithoutYieldingInsideUnhookedCoroutinefails if a lookup in an unhooked coroutine yields.testAppwriteMigrationRejectsPrivateEndpointschecks that report and create returngeneral_argument_invalidfor link-local, loopback, IPv6 loopback and non-http endpoints.Locally: lint, PHPStan and the network, migrations-validator and migrations-worker unit tests pass.
🤖 Generated with Claude Code