fix: trusted request coroutine safety#384
Conversation
… test Sets RequestContext::set() before calling Request::setTrustedProxies() and passes the same Request instance into the UrlGenerator. Works with both the existing static-storage implementation and the upcoming per-instance trust state, so the tree stays green across the migration.
… prefix test Reorders setup to install the Request in RequestContext before calling Request::setTrustedProxies(), so the test exercises the trust state through the per-coroutine context the new implementation requires.
Symfony stores trusted proxy/host configuration on Request as class-level statics. Under Swoole's coroutine concurrency that becomes shared mutable state across all in-flight requests in a worker — a request that yields during async I/O can resume after another coroutine has overwritten the trust config, then read the wrong client IP / host / scheme back from the static. This moves the four trust statics ($trustedProxies, $trustedHostPatterns, $trustedHosts, $trustedHeaderSet) plus the three per-request privates ($trustedValuesCache, $isHostValid, $isForwardedValid) onto Hypervel's Request subclass as protected instance properties. The Symfony-compatible static setters (setTrustedProxies / setTrustedHosts) keep their signatures and route writes to the current Request through RequestContext::getOrNull(). Read sites stay on plain property access — no Context lookups on the hot path. Reimplements Symfony's private getTrustedValues, normalizeAndFilterClientIps, getBaseUrlReal, and isHostValid as protected so the overridden public methods reach them. Redeclares the private FORWARDED_PARAMS and TRUSTED_HEADERS constants as protected for the same reason. Wires the request lifecycle so trust state flows correctly: initialize() resets to defaults, createFrom() / createFromBase() copy from the source request (so FormRequest::newInstance() preserves trust through the container's SelfBuilding path), __clone() clears the parsed-values cache and one-shot exception flags while preserving config (covers both direct clone and parent::duplicate()'s internal clone). setTrustedProxies and setTrustedHosts clear the trusted-values cache + one-shot flags so a mid-request config change doesn't return stale parsed values.
Covers the proxy/header/host read paths (getClientIps, getHost, isSecure, getPort, getBaseUrl), the REMOTE_ADDR and PRIVATE_SUBNETS sentinel resolution in setTrustedProxies, the regex-compilation in setTrustedHosts, and the lifecycle paths (createFrom, createFromBase, initialize, __clone, duplicate). The duplicate test reflection-asserts the trustedValuesCache clears on clone so the assertion can't pass via header-key auto-invalidation. A final parity test runs the same headers through Symfony's own Request and Hypervel's Request and asserts identical outputs across getClientIps, getHost, getPort, isSecure — guarding against subtle copy-paste drift from the Symfony private helpers we reimplemented as protected.
Runs two coroutines via parallel() with usleep() between mutation and read to force interleaving. Each coroutine configures its own trust state and verifies its reads (getTrustedProxies / isFromTrustedProxy / ip / getHost) return its own values, not the other coroutine's. The host-validity test also confirms the SuspiciousOperationException one-shot flag stays per-coroutine — one coroutine's invalid host doesn't suppress the other's exception. These would fail on the previous static-storage implementation, where the second coroutine's setTrustedProxies() would overwrite the first coroutine's reads on resume.
Exercises the middleware end-to-end through the Testbench HTTP dispatcher: explicit IP list, untrusted-proxy fallthrough to REMOTE_ADDR, the '*' wildcard mode, and a FormRequest path that asserts \$this->ip() inside authorize() reflects the forwarded IP — the regression that motivated moving trust state off Symfony statics, since FormRequest goes through createFrom() in the container's SelfBuilding path. The concurrent-through-middleware test fires two overlapping requests via parallel() against a slow route that usleep()s in the handler. Under the old static-storage implementation the second request's middleware would overwrite the first's trust config while the first was yielded; both responses now correctly report their own forwarded IPs.
Verifies trusted-host pattern matching end-to-end: a matching host returns 200, an untrusted host surfaces SuspiciousOperationException through the exception handler, and the closure form of trustHosts(at: fn () => [...]) runs per request and can read request-specific state (HOST header) when deciding what to trust. Subclasses TrustHosts to bypass the local/testing-environment opt-out so the middleware actually runs under Testbench.
| * Determine whether the request is secure. | ||
| */ | ||
| #[Override] | ||
| public function isSecure(): bool |
There was a problem hiding this comment.
@binaryfire is this function exactly the same as its parent function?
There was a problem hiding this comment.
Hi @albertcht. Yes this is funtionally identical, and we have tests to enforce that as well. Please see: #384 (comment)
| * Return the port on which the request is made. | ||
| */ | ||
| #[Override] | ||
| public function getPort(): int|string|null |
There was a problem hiding this comment.
@binaryfire is this function exactly the same as its parent function?
There was a problem hiding this comment.
Hi @albertcht. Yes this is funtionally identical, and we have tests to enforce that as well. Please see: #384 (comment)
| * Return the root URL from which this request is executed. | ||
| */ | ||
| #[Override] | ||
| public function getBaseUrl(): string |
There was a problem hiding this comment.
@binaryfire is this function exactly the same as its parent function?
There was a problem hiding this comment.
Hi @albertcht. Yes this is funtionally identical, and we have tests to enforce that as well. Please see: #384 (comment)
| * Get the client IP addresses. | ||
| */ | ||
| #[Override] | ||
| public function getClientIps(): array |
There was a problem hiding this comment.
@binaryfire is this function exactly the same as its parent function?
There was a problem hiding this comment.
Hi @albertcht. Yes this is funtionally identical, and we have tests to enforce that as well. Please see: #384 (comment)
|
Hi @albertcht Yes, functionally these are all identical and there are parity tests for all of them to enforce that. The internal differences are intentional and necessary. The reason we can’t just call These override methods mirror Symfony’s implementation functionally, but they intentionally substitute Symfony’s static trusted request state with Hypervel’s context-backed request-scoped state. Here is the test that ensure parity. The test creates the same server/header input as both a Symfony Request and a Hypervel Request, configures the same trusted proxy/header bitmask, then asserts the returned values are identical: tests/Http/HttpRequestTrustedStateTest.php::testSingleRequestMatchesSymfonyTrustedProxyBehavior It covers all the overrides: |
oh, I didn't notice that |
2 participants
This PR makes trusted proxy and trusted host request state coroutine-safe while preserving the same public API and end-user behavior as Laravel / Symfony.
Symfony stores trusted proxy and host configuration in static properties on
Request. That fine for the FPM request lifecycle, but is unsafe in Hypervel's long-running Swoole workers where multiple requests run concurrently in the same process. One request can update trusted proxy state while another request is still resolving$request->ip(),$request->host(),$request->isSecure(), URL generation, or forwarded-prefix handling.Hypervel now keeps this state on the current
Requestinstance instead. The existingRequest::setTrustedProxies(),Request::setTrustedHosts(),TrustProxies, andTrustHostsAPIs continue to work as before, but their state is scoped to the request being handled.What Changed
Hypervel\Http\Request.getClientIps()getBaseUrl()getPort()isSecure()getHost()isFromTrustedProxy()createFrom()andcreateFromBase()when converting Hypervel requests.__clone(), which also covers Symfony'sduplicate()flow.duplicate()focused on Hypervel's existing file filtering adaptation.API Compatibility
This doesn't change the public API or the expected application behavior. Applications can continue configuring trusted proxies and trusted hosts the same way:
trustProxies(...)trustHosts(...)TrustProxies::at(...)TrustHosts::at(...)Request::setTrustedProxies(...)Request::setTrustedHosts(...)The difference is purely internal.
Tests
Added coverage for:
X-Forwarded-*handling for IP, host, proto, port, and prefix.ForwardedvsX-Forwarded-*conflict behavior.REMOTE_ADDRandPRIVATE_SUBNETSsentinel handling.createFrom()preserving trusted request state.createFromBase()preserving trusted request state for Hypervel requests.initialize()resetting trusted request state.cloneandduplicate()preserving configuration while resetting caches / one-shot flags.setTrustedProxies()andsetTrustedHosts()clearing stale caches.TrustProxiesandTrustHostsmiddleware behavior.TrustProxies.TrustProxies.