fix(laravel): don't let Kernel::handle hook throw on hostile method override#650
Conversation
…verride Request::method()/getMethod() throws SuspiciousOperationException for a malformed method override (e.g. a `_method` or `X-HTTP-METHOD-OVERRIDE` value that isn't plain letters, such as `_method=__construct` from a scanner probe). The Kernel::handle pre-hook called this unguarded while building the span, but this hook runs before the intercepted method body executes and therefore before Laravel's own exception handling is engaged. An uncaught throw here is reported by the extension as a raw PHP warning printed outside of the normal response lifecycle, and the span is silently dropped. getHost() has the same failure mode for a Host header that fails validation, reached via httpHostName(). Wrap both in try/catch and fall back to a safe placeholder so hostile input can't break the instrumentation itself; the framework's own request handling still processes (and can reject) the request normally afterwards.
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #650 +/- ##
============================================
- Coverage 80.72% 80.07% -0.65%
- Complexity 1446 1600 +154
============================================
Files 95 118 +23
Lines 5343 6184 +841
============================================
+ Hits 4313 4952 +639
- Misses 1030 1232 +202 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 22 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
fullUrl() (used for the URL_FULL attribute) internally calls getHost(), which has the same pre-boot SuspiciousOperationException risk as getMethod() for a malformed Host header, and it runs before httpHostName() in the attribute chain — so a hostile Host header still crashed the hook even with the previous fix. Guard it the same way as httpMethod(). Add a regression test for the malformed-Host-header case (bypassing call()'s URI-driven HTTP_HOST override to actually exercise it), plus focused unit tests invoking the three guard methods directly so their catch branches are exercised regardless of call order. Also scope the httpHostName() try/catch to only the reachable branch: Illuminate\Http\Request always defines host(), so the getHost() fallback is dead code that was already uncovered on main; wrapping it too just re-flagged pre-existing untested lines as part of this diff.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Laravel Kernel::handle() instrumentation pre-hook against SuspiciousOperationException (and other throwables) triggered by malformed HTTP method override and Host headers, preventing uncaught pre-hook exceptions from breaking the response lifecycle and dropping spans.
Changes:
- Add guarded helpers (
httpMethod(),httpFullUrl()) to safely read request method and full URL in the pre-hook. - Wrap the
host()call insidehttpHostName()to avoid hook crashes on invalidHostheaders. - Add unit and integration regression tests covering malformed method override and
Hostheader cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Instrumentation/Laravel/src/Hooks/Illuminate/Contracts/Http/Kernel.php |
Introduces guarded helpers and uses them in Kernel::handle pre/post hooks to prevent pre-hook throws from dropping spans. |
src/Instrumentation/Laravel/tests/Integration/LaravelInstrumentationTest.php |
Adds integration coverage ensuring instrumentation remains functional with malformed method override / host headers. |
src/Instrumentation/Laravel/tests/Unit/Hooks/Illuminate/Contracts/Http/KernelTest.php |
Adds focused unit tests for the new guard helpers’ exception-swallowing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $this->assertSame(200, $response->status()); | ||
| $this->assertCount(1, $this->storage); | ||
| $span = $this->storage[0]; | ||
| $this->assertSame('POST /', $span->getName()); | ||
| } |
PuvaanRaaj
left a comment
PuvaanRaaj
left a comment
There was a problem hiding this comment.
Reviewed the current Laravel hardening diff and the relevant CI failures. The guarded request accessors and regression coverage are directionally useful, but the branch still has a directly introduced quality-check failure.
| $kernel = (new ReflectionClass(Kernel::class))->newInstanceWithoutConstructor(); | ||
|
|
||
| $reflectionMethod = new ReflectionMethod(Kernel::class, $method); | ||
| $reflectionMethod->setAccessible(true); |
There was a problem hiding this comment.
This line currently fails the Laravel quality matrix with Psalm UnusedMethodCall. On the supported PHP 8.1+ runtimes, reflection members can be invoked without calling setAccessible(), so please remove this call (or otherwise adjust the helper) to restore the required quality checks.
Which problem is this PR solving?
Illuminate\Http\Request::method()/getMethod()throws aSuspiciousOperationExceptionwhen the request carries a malformed HTTP method override — e.g._method=__constructor aX-HTTP-METHOD-OVERRIDE: __constructheader, which is a fairly common vulnerability-scanner probe.getHost()has the same failure mode for aHostheader that fails validation, and it's reached both directly (host()/getHost()) and indirectly viafullUrl()(used for theURL_FULLattribute).The
Kernel::handleprehook calls all of these unguarded while building the span. Because this is aprehook, it runs before the interceptedKernel::handle()method body executes — i.e. before Laravel's owntry { ... } catch (Throwable $e)around request handling is even reached. An uncaught throw here isn't caught by the application at all; the extension reports it as a raw PHP warning outside the normal response lifecycle (OpenTelemetry: pre hook threw exception, ... message=Invalid HTTP method override.), and the span for the request is silently dropped.In a real deployment this surfaced as a
FatalError: Cannot modify header information - headers already sent— the warning text got written to the response body before any headers were sent, so a later attempt to set a header (e.g. by the framework's own exception rendering) fatals.Short description of the changes
$request->method()access into a newhttpMethod()helper that catchesThrowableand falls back to'unknown', used in both thepreandposthooks (also removes a redundant second call tomethod()that existed in theprehook).httpFullUrl()helper guarding$request->fullUrl()the same way, since it independently callsgetHost()and runs beforehttpHostName()in the attribute chain — so a hostileHostheader could still crash the hook even withhttpHostName()guarded.getHost()call site insidehttpHostName(). ThegetHost()fallback branch there is dead code (Illuminate\Http\Requestalways defineshost()) and was already uncovered onmain, so it's intentionally left unwrapped/untouched rather than dragging pre-existing untested lines into this diff.X-HTTP-METHOD-OVERRIDEheader case and a malformedHostheader case (the latter constructs the request manually, sinceMakesHttpRequests::call()'sRequest::create()always overwritesHTTP_HOSTfrom the parsed URI). Plus focused unit tests invokinghttpMethod()/httpFullUrl()/httpHostName()directly via reflection so each guard's catch branch is exercised independent of call order.Reverting the
Kernel.phpguards locally reproduces the exact warning and the dropped span for both the method-override and Host-header cases, confirming this is the same failure mode in both spots.