fix(security): contain worker death on control-byte headers (closes #577) - #598
Merged
Conversation
added 3 commits
July 27, 2026 19:08
…577) Wrap the entire request lifecycle in HttpRequestHandler::__invoke() in a try/catch so that any throwable — from request conversion (control byte in a header value), a middleware, response conversion, or response preparation — is turned into a 400 (client error) or 500 (server fault) response instead of escaping into Workerman's TcpConnection error handler, which terminates the worker process. Add a defence-in-depth backstop: ServerWorker::onConnect() now installs $connection->errorHandler so that any throwable the handler misses closes the connection cleanly instead of calling Worker::stopAll(250). doTerminate() and the reboot check still run on the failure path (see #572). Client errors (\InvalidArgumentException, FileUploadValidationException) are logged at debug level to prevent log flooding by an unauthenticated attacker; server faults are logged at error level with the full exception.
Blocker: wrap error-response send in nested try/catch so a throw from sendResponse() during error handling does not escape __invoke() (skipping doTerminate/reboot and reaching Workerman's error handler). Major: tighten isClientError classification — introduce ClientInputExceptionInterface marker implemented by MalformedRequestException (new) and FileUploadValidationException. RequestConverter now throws MalformedRequestException instead of bare \InvalidArgumentException, so a middleware throwing \InvalidArgumentException is correctly classified as a 500 server fault, not a 400 client error. Major: backstop errorHandler now logs to error_log and cleans up per-connection timers (keepaliveTimerId, connectionTimerId) before closing, instead of silently swallowing the throwable. Major: replace tautology in soak test with real count + per-response 400 assertion. Minor: strengthen doTerminate/reboot tests using spy reboot strategy mock with expects(once()). Rename test methods dropping misleading 'KeepsWorkerAlive' suffix (unit tests cannot verify liveness).
added 2 commits
July 27, 2026 20:12
CI rector (PHP 8.2) flags RemoveDeadInstanceOfAssertRector on the
DateInterval assert after createFromDateString('+1 hour').
- Assert FileUploadValidationException/MalformedRequestException from RequestConverter without middleware injection - Route incomplete multipart files through HttpRequestHandler to 400 - Spawn a real single-worker process and prove PID stability under control-byte requests, 10k soak, and errorHandler-only backstop
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.
Description
Closes #577
A single control byte in any request header value killed the Workerman worker process (remote unauthenticated DoS). The request lifecycle in
HttpRequestHandler::__invoke()had no try/catch, andServerWorkernever assigned$connection->errorHandler, so Workerman calledWorker::stopAll(250).Changes
HttpRequestHandler::__invoke()with try/catch; client input errors → 400, server faults → 500sendResponse()so send failure cannot escape (doTerminate + reboot still run)ClientInputExceptionInterfacemarker;MalformedRequestException(new) andFileUploadValidationExceptionimplement itRequestConverterthrowsMalformedRequestExceptioninstead of bare\InvalidArgumentExceptionServerWorker::onConnectinstalls$connection->errorHandlerbackstop (log + timer cleanup + close)error_logfallbackChangelog
RequestConverterthrowsMalformedRequestExceptioninstead of bare\InvalidArgumentExceptionClientInputExceptionInterface,MalformedRequestExceptionCode Review