[Security] Improve profiler’s panel #1

MatTheCat · 2024-06-16T09:05:52Z

Q	A
Branch?	7.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	N/A
License	MIT

Let’s display the profiler for a request matching a lone lazy firewall:

firewalls:
    main:
        lazy: true

Since no channel is forced, we know the ChannelListener did not run. To make it more obvious, this PR displays (none) instead of 0.00 ms when the duration is null.

Before

After

But what about the ContextListener? Since the firewall is stateful we know it ran, yet its displayed duration also is 0.00 ms.
Turns out that because the firewall is lazy, the ContextListener ran way past the moment the TraceableFirewallListener stored its data. In fact, it may be the SecurityDataCollector itself which trigger it by accessing the security token. This PR makes the TraceableFirewallListener fetch data only when needed, so that they’re up-to-date when the SecurityDataCollector asks for them.

Before

After

Now, let’s add a global access control so that the AccessListener can do its job:

access_control:
    - { roles: ROLE_USER }

The profiler then says no security listeners have been recorded 🤔

This is because the AccessListener let the ExceptionListener work out a response by throwing AccessDeniedExceptions. When this happens, the TraceableFirewallListener is cut short before it can store the data it needs (note that it also impacts non-lazy firewalls, but past listeners would then be recorded).

This PR stores these data before listeners are called, so that they are available even if one of them throws (this includes authenticators’ data which suffer from the same issue).

Before

After

(Other listeners are hidden on this screenshot but they would be displayed in the profiler.)

MatTheCat · 2024-06-17T09:13:07Z

Opened symfony#57425

…rsimpsons) This PR was merged into the 5.4 branch. Discussion ---------- [Yaml] 🐛 throw ParseException on invalid date | Q | A | ------------- | --- | Branch? | 5.4  | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Issues | None  | License | MIT (found in symfony-tools/docs-builder#179) When parsing the following yaml: ``` date: 6418-75-51 ``` `symfony/yaml` will throw an exception: ``` $ php main.php PHP Fatal error: Uncaught Exception: Failed to parse time string (6418-75-51) at position 6 (5): Unexpected character in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php:714 Stack trace: #0 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(714): DateTimeImmutable->__construct() #1 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(312): Symfony\Component\Yaml\Inline::evaluateScalar() #2 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(80): Symfony\Component\Yaml\Inline::parseScalar() symfony#3 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(790): Symfony\Component\Yaml\Inline::parse() symfony#4 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(341): Symfony\Component\Yaml\Parser->parseValue() symfony#5 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(86): Symfony\Component\Yaml\Parser->doParse() symfony#6 /tmp/symfony-yaml/vendor/symfony/yaml/Yaml.php(77): Symfony\Component\Yaml\Parser->parse() symfony#7 /tmp/symfony-yaml/main.php(8): Symfony\Component\Yaml\Yaml::parse() symfony#8 {main} thrown in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php on line 714 ``` This is because the "month" is invalid. Fixing the "month" will trigger about the same issue because the "day" would be invalid. With the current change it will throw a `ParseException`. Commits ------- 6d71a7e 🐛 throw ParseException on invalid date

…nse from transport (ZhukV) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Notifier][TurboSMS] Process partial accepted response from transport | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | None | License | MIT TurboSMS can return `null` as message id, if sms not sent to recipient. Example: ```json { "response_code": 802, "response_status": "SUCCESS_MESSAGE_PARTIAL_ACCEPTED", "response_result": [ { "phone": "recipient_1", "response_code": 406, "message_id": null, "response_status": "NOT_ALLOWED_RECIPIENT_COUNTRY" }, { "phone": "recipient_2", "response_code": 0, "message_id": "f83f8868-5e46-c6cf-e4fb-615e5a293754", "response_status": "OK" } ] } ``` And we receive error: ``` Symfony\Component\Notifier\Message\SentMessage::setMessageId(): Argument #1 ($id) must be of type string, null given, called in /code/vendor/symfony/turbo-sms-notifier/TurboSmsTransport.php on line 93 ``` Symfony use only one phone number for sent, as result we check only first `response_result`. Commits ------- 932dbe3 [Notifier][TurboSMS] Process partial accepted response from transport

Without the fix running `SYMFONY_PHPUNIT_SKIPPED_TESTS='phpunit.skipped' php ./phpunit src/Symfony/Component/Lock/Tests/Store/DoctrineDbalPostgreSqlStoreTest.php` without the pdo_pgsql extension enabled the generated skip file looked like this: ``` <?php return array ( 'PHPUnit\\Framework\\DataProviderTestSuite' => array ( 'Symfony\\Component\\Lock\\Tests\\Store\\DoctrineDbalPostgreSqlStoreTest::testInvalidDriver' => 1, ), 'Symfony\\Component\\Lock\\Tests\\Store\\DoctrineDbalPostgreSqlStoreTest' => array ( 'testSaveAfterConflict' => 1, 'testWaitAndSaveAfterConflictReleasesLockFromInternalStore' => 1, 'testWaitAndSaveReadAfterConflictReleasesLockFromInternalStore' => 1, 'testSave' => 1, 'testSaveWithDifferentResources' => 1, 'testSaveWithDifferentKeysOnSameResources' => 1, 'testSaveTwice' => 1, 'testDeleteIsolated' => 1, 'testBlockingLocks' => 1, 'testSharedLockReadFirst' => 1, 'testSharedLockWriteFirst' => 1, 'testSharedLockPromote' => 1, 'testSharedLockPromoteAllowed' => 1, 'testSharedLockDemote' => 1, ), ); ``` Thus, running the tests again with the extension enabled would only run 14 tests instead of the expected total number of 16 tests. With the patch applied the generated skip file looks like this: ``` <?php return array ( 'Symfony\\Component\\Lock\\Tests\\Store\\DoctrineDbalPostgreSqlStoreTest' => array ( 'testInvalidDriver with data set #0' => 1, 'testInvalidDriver with data set #1' => 1, 'testSaveAfterConflict' => 1, 'testWaitAndSaveAfterConflictReleasesLockFromInternalStore' => 1, 'testWaitAndSaveReadAfterConflictReleasesLockFromInternalStore' => 1, 'testSave' => 1, 'testSaveWithDifferentResources' => 1, 'testSaveWithDifferentKeysOnSameResources' => 1, 'testSaveTwice' => 1, 'testDeleteIsolated' => 1, 'testBlockingLocks' => 1, 'testSharedLockReadFirst' => 1, 'testSharedLockWriteFirst' => 1, 'testSharedLockPromote' => 1, 'testSharedLockPromoteAllowed' => 1, 'testSharedLockDemote' => 1, ), ); ```

… providers (xabbuh) This PR was merged into the 5.4 branch. Discussion ---------- [PhpUnitBridge] fix dumping tests to skip with data providers | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | | License | MIT Without the fix running `SYMFONY_PHPUNIT_SKIPPED_TESTS='phpunit.skipped' php ./phpunit src/Symfony/Component/Lock/Tests/Store/DoctrineDbalPostgreSqlStoreTest.php` without the `pdo_pgsql` extension enabled the generated skip file looked like this: ``` <?php return array ( 'PHPUnit\\Framework\\DataProviderTestSuite' => array ( 'Symfony\\Component\\Lock\\Tests\\Store\\DoctrineDbalPostgreSqlStoreTest::testInvalidDriver' => 1, ), 'Symfony\\Component\\Lock\\Tests\\Store\\DoctrineDbalPostgreSqlStoreTest' => array ( 'testSaveAfterConflict' => 1, 'testWaitAndSaveAfterConflictReleasesLockFromInternalStore' => 1, 'testWaitAndSaveReadAfterConflictReleasesLockFromInternalStore' => 1, 'testSave' => 1, 'testSaveWithDifferentResources' => 1, 'testSaveWithDifferentKeysOnSameResources' => 1, 'testSaveTwice' => 1, 'testDeleteIsolated' => 1, 'testBlockingLocks' => 1, 'testSharedLockReadFirst' => 1, 'testSharedLockWriteFirst' => 1, 'testSharedLockPromote' => 1, 'testSharedLockPromoteAllowed' => 1, 'testSharedLockDemote' => 1, ), ); ``` Thus, running the tests again with the extension enabled would only run 14 tests instead of the expected total number of 16 tests. With the patch applied the generated skip file looks like this: ``` <?php return array ( 'Symfony\\Component\\Lock\\Tests\\Store\\DoctrineDbalPostgreSqlStoreTest' => array ( 'testInvalidDriver with data set #0' => 1, 'testInvalidDriver with data set #1' => 1, 'testSaveAfterConflict' => 1, 'testWaitAndSaveAfterConflictReleasesLockFromInternalStore' => 1, 'testWaitAndSaveReadAfterConflictReleasesLockFromInternalStore' => 1, 'testSave' => 1, 'testSaveWithDifferentResources' => 1, 'testSaveWithDifferentKeysOnSameResources' => 1, 'testSaveTwice' => 1, 'testDeleteIsolated' => 1, 'testBlockingLocks' => 1, 'testSharedLockReadFirst' => 1, 'testSharedLockWriteFirst' => 1, 'testSharedLockPromote' => 1, 'testSharedLockPromoteAllowed' => 1, 'testSharedLockDemote' => 1, ), ); ``` Commits ------- 95f41cc fix dumping tests to skip with data providers

… not throw exception (lyrixx) This PR was merged into the 5.4 branch. Discussion ---------- [HttpKernel] Ensure `HttpCache::getTraceKey()` does not throw exception | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT We have such logs in our logs. It's in our raw PHP logs. They are not caught by monolog, it's too early ``` [11-Oct-2024 01:23:33 UTC] PHP Fatal error: Uncaught Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException: Invalid method override "__CONSTRUCT". in /var/www/redirection.io/backend/blue/vendor/symfony/http-foundation/Request.php:1234 Stack trace: #0 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/HttpCache/HttpCache.php(728): Symfony\Component\HttpFoundation\Request->getMethod() #1 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/HttpCache/HttpCache.php(207): Symfony\Component\HttpKernel\HttpCache\HttpCache->getTraceKey() #2 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/Kernel.php(188): Symfony\Component\HttpKernel\HttpCache\HttpCache->handle() symfony#3 /var/www/redirection.io/backend/blue/web/app.php(9): Symfony\Component\HttpKernel\Kernel->handle() symfony#4 {main} thrown in /var/www/redirection.io/backend/blue/vendor/symfony/http-foundation/Request.php on line 1234 ``` I managed to reproduced locally. * Before the patch, without the http_cache, symfony returns a 405 * After the patch, without the http_cache, symfony returns a 405 * Before the patch, with the http_cache, symfony returns a 500, without any information (too early) * After the patch, with the http_cache, symfony ret 8000 urns a 405 Commits ------- a2ebbe0 [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception

…ctor (MaximePinot) This PR was merged into the 6.4 branch. Discussion ---------- [Mime] Fix wrong PHPDoc in `FormDataPart` constructor | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no  | Deprecations? | no  | Issues | -  | License | MIT I believe the PHPDoc is wrong. As far as I understand, the `FormDataPart` expects instances of `TextPart`: ```php if (!\is_string($item) && !$item instanceof TextPart) { throw new InvalidArgumentException(sprintf('The value of the form field "%s" can only be a string, an array, or an instance of TextPart, "%s" given.', $fieldName, get_debug_type($item))); } ``` https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Mime/Part/Multipart/FormDataPart.php#L74 The following code is rejected by PHPStan while it works: ```php final readonly class Foo { public function bar(): void { new FormDataPart([ new TextPart('baz'), ]); } } ``` ```shell ------ ------------------------------------------------------------------------------------------------------------------- Line src/Foo.php ------ ------------------------------------------------------------------------------------------------------------------- 14 Parameter #1 $fields of class Symfony\Component\Mime\Part\Multipart\FormDataPart constructor expects array<array|string|Symfony\Component\Mime\Part\DataPart>, array<int, Symfony\Component\Mime\Part\TextPart> given. ------ ------------------------------------------------------------------------------------------------------------------- ``` (cc `@B`-Galati) Commits ------- 886d4ed [Mime] Fix wrong PHPDoc in `FormDataPart` constructor

MatTheCat changed the title ~~Security profiler lazy~~ [Security] Improve profiler’s panel Jun 16, 2024

MatTheCat force-pushed the security_profiler_lazy branch 7 times, most recently from a36c79f to b5f34b8 Compare June 17, 2024 08:57

MatTheCat added 2 commits June 17, 2024 10:59

Get up-to-date informations from lazy listeners

d8f4d5d

Prevent data to be missing if a listener throws

fc9f753

MatTheCat force-pushed the security_profiler_lazy branch from b5f34b8 to 573e8cb Compare June 17, 2024 08:59

Do not display duration if there is none

0c51184

MatTheCat force-pushed the security_profiler_lazy branch from 573e8cb to 0c51184 Compare June 17, 2024 09:05

MatTheCat closed this Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Improve profiler’s panel #1

[Security] Improve profiler’s panel #1

[Security] Improve profiler’s panel #1

[Security] Improve profiler’s panel #1

Conversation