-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: prevent Maximum call stack size exceeded on client-managed requests
#9852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.7
Are you sure you want to change the base?
Changes from all commits
6c41a51
18d0d49
4473aff
b5093c4
52919c5
1482cdd
cce2477
17cf621
d744b4d
792e921
1cc588e
7f65297
d3249e7
e2ef9a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| use Rector\EarlyReturn\Rector\If_\RemoveAlwaysElseRector; | ||
| use Rector\EarlyReturn\Rector\Return_\PreparedValueToEarlyReturnRector; | ||
| use Rector\Php70\Rector\FuncCall\RandomFunctionRector; | ||
| use Rector\Php71\Rector\FuncCall\RemoveExtraParametersRector; | ||
| use Rector\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector; | ||
| use Rector\Php81\Rector\FuncCall\NullToStrictStringFuncCallArgRector; | ||
| use Rector\PHPUnit\CodeQuality\Rector\Class_\YieldDataProviderRector; | ||
|
|
@@ -107,6 +108,10 @@ | |
| __DIR__ . '/system/HTTP/Response.php', | ||
| ], | ||
|
|
||
| RemoveExtraParametersRector::class => [ | ||
| __DIR__ . '/tests/system/Debug/ToolbarTest.php', | ||
| ], | ||
|
Comment on lines
+111
to
+113
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add above that this is required because of is_cli() mocking? |
||
|
|
||
| // check on constant compare | ||
| UnwrapFutureCompatibleIfPhpVersionRector::class => [ | ||
| __DIR__ . '/system/Autoloader/Autoloader.php', | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -42,6 +42,12 @@ class Toolbar | |||||
| */ | ||||||
| protected $config; | ||||||
|
|
||||||
| /** | ||||||
| * Indicates if the current request is a custom AJAX-like request | ||||||
| * (HTMX, Unpoly, Turbo, etc.) that expects clean HTML fragments. | ||||||
| */ | ||||||
| protected bool $isCustomAjax = false; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||||||
|
|
||||||
| /** | ||||||
| * Collectors to be used and displayed. | ||||||
| * | ||||||
|
|
@@ -365,10 +371,8 @@ protected function roundTo(float $number, int $increments = 5): float | |||||
|
|
||||||
| /** | ||||||
| * Prepare for debugging. | ||||||
| * | ||||||
| * @return void | ||||||
| */ | ||||||
| public function prepare(?RequestInterface $request = null, ?ResponseInterface $response = null) | ||||||
| public function prepare(?RequestInterface $request = null, ?ResponseInterface $response = null): void | ||||||
| { | ||||||
| /** | ||||||
| * @var IncomingRequest|null $request | ||||||
|
|
@@ -385,7 +389,9 @@ public function prepare(?RequestInterface $request = null, ?ResponseInterface $r | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| $toolbar = service('toolbar', config(ToolbarConfig::class)); | ||||||
| $config = config(ToolbarConfig::class); | ||||||
|
|
||||||
| $toolbar = service('toolbar', $config); | ||||||
| $stats = $app->getPerformanceStats(); | ||||||
| $data = $toolbar->run( | ||||||
| $stats['startTime'], | ||||||
|
|
@@ -407,10 +413,17 @@ public function prepare(?RequestInterface $request = null, ?ResponseInterface $r | |||||
|
|
||||||
| $format = $response->getHeaderLine('content-type'); | ||||||
|
|
||||||
| foreach ($config->disableOnHeaders as $header) { | ||||||
| if ($request->hasHeader($header)) { | ||||||
| $this->isCustomAjax = true; | ||||||
| break; | ||||||
| } | ||||||
|
Comment on lines
+417
to
+420
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we will need more complex checks, this can be delegated to a separate method. |
||||||
| } | ||||||
|
|
||||||
| // Non-HTML formats should not include the debugbar | ||||||
| // then we send headers saying where to find the debug data | ||||||
| // for this response | ||||||
| if ($request->isAJAX() || ! str_contains($format, 'html')) { | ||||||
| if ($request->isAJAX() || ! str_contains($format, 'html') || $this->isCustomAjax) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This way, we can make this check a bit simpler. |
||||||
| $response->setHeader('Debugbar-Time', "{$time}") | ||||||
| ->setHeader('Debugbar-Link', site_url("?debugbar_time={$time}")); | ||||||
|
|
||||||
|
|
@@ -454,10 +467,8 @@ public function prepare(?RequestInterface $request = null, ?ResponseInterface $r | |||||
| * Inject debug toolbar into the response. | ||||||
| * | ||||||
| * @codeCoverageIgnore | ||||||
| * | ||||||
| * @return void | ||||||
| */ | ||||||
| public function respond() | ||||||
| public function respond(): void | ||||||
| { | ||||||
| if (ENVIRONMENT === 'testing') { | ||||||
| return; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * This file is part of CodeIgniter 4 framework. | ||
| * | ||
| * (c) CodeIgniter Foundation <[email protected]> | ||
| * | ||
| * For the full copyright and license information, please view | ||
| * the LICENSE file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace CodeIgniter\Debug; | ||
|
|
||
| use CodeIgniter\CodeIgniter; | ||
| use CodeIgniter\Config\Factories; | ||
| use CodeIgniter\Config\Services; | ||
| use CodeIgniter\HTTP\IncomingRequest; | ||
| use CodeIgniter\HTTP\ResponseInterface; | ||
| use CodeIgniter\Test\CIUnitTestCase; | ||
| use Config\Toolbar as ToolbarConfig; | ||
| use PHPUnit\Framework\Attributes\BackupGlobals; | ||
| use PHPUnit\Framework\Attributes\Group; | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| #[BackupGlobals(true)] | ||
| #[Group('Others')] | ||
| final class ToolbarTest extends CIUnitTestCase | ||
| { | ||
| private ToolbarConfig $config; | ||
| private ?IncomingRequest $request = null; | ||
| private ?ResponseInterface $response = null; | ||
|
|
||
| protected function setUp(): void | ||
| { | ||
| parent::setUp(); | ||
| Services::reset(); | ||
|
|
||
| is_cli(false); | ||
|
|
||
| $this->config = new ToolbarConfig(); | ||
|
|
||
| // Mock CodeIgniter core service to provide performance stats | ||
| $app = $this->createMock(CodeIgniter::class); | ||
| $app->method('getPerformanceStats')->willReturn([ | ||
| 'startTime' => microtime(true), | ||
| 'totalTime' => 0.05, | ||
| ]); | ||
| Services::injectMock('codeigniter', $app); | ||
| } | ||
|
|
||
| protected function tearDown(): void | ||
| { | ||
| // Restore is_cli state | ||
| is_cli(true); | ||
|
|
||
| parent::tearDown(); | ||
| } | ||
|
|
||
| public function testPrepareRespectsDisableOnHeaders(): void | ||
| { | ||
| // Set up the new configuration property | ||
| $this->config->disableOnHeaders = ['HX-Request']; | ||
| Factories::injectMock('config', 'Toolbar', $this->config); | ||
|
|
||
| // Initialize Request with the custom header | ||
| $this->request = service('incomingrequest', null, false); | ||
| $this->request->setHeader('HX-Request', 'true'); | ||
|
|
||
| // Initialize Response | ||
| $this->response = service('response', null, false); | ||
| $this->response->setBody('<html><body>Content</body></html>'); | ||
| $this->response->setHeader('Content-Type', 'text/html'); | ||
|
|
||
| $toolbar = new Toolbar($this->config); | ||
| $toolbar->prepare($this->request, $this->response); | ||
|
|
||
| // Assertions | ||
| $this->assertTrue($this->response->hasHeader('Debugbar-Time')); | ||
| $this->assertStringNotContainsString('id="debugbar_loader"', (string) $this->response->getBody()); | ||
| } | ||
|
|
||
| public function testPrepareInjectsNormallyWithoutIgnoredHeader(): void | ||
| { | ||
| $this->config->disableOnHeaders = ['HX-Request']; | ||
| Factories::injectMock('config', 'Toolbar', $this->config); | ||
|
|
||
| $this->request = service('incomingrequest', null, false); | ||
| $this->response = service('response', null, false); | ||
| $this->response->setBody('<html><body>Content</body></html>'); | ||
| $this->response->setHeader('Content-Type', 'text/html'); | ||
|
|
||
| $toolbar = new Toolbar($this->config); | ||
| $toolbar->prepare($this->request, $this->response); | ||
|
|
||
| // Assertions | ||
| $this->assertStringContainsString('id="debugbar_loader"', (string) $this->response->getBody()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need to loop over these values, we can also add AJAX requests here. Additionally, we can check not only for the presence of the header but also for the value, which is important in some cases. The
nullvalue indicates that we are only interested in the header presence.