AppSec support for Frankenphp #3165
Conversation
cataphract
force-pushed
the
glopes/appsec-frankenphp
branch
from
259c3e8 to
e07bb91
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3165 +/- ##
============================================
- Coverage 79.09% 75.86% -3.23%
- Complexity 2890 2910 +20
============================================
Files 114 141 +27
Lines 11443 15967 +4524
Branches 0 1105 +1105
============================================
+ Hits 9051 12114 +3063
- Misses 2392 3278 +886
- Partials 0 575 +575
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 Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2025-04-02 17:00:31 Comparing candidate commit 8b71b6b in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 176 metrics, 0 unstable metrics. scenario:PDOBench/benchPDOBaseline-opcache
scenario:TraceFlushBench/benchFlushTrace
|
Benchmarks [ appsec ]Benchmark execution time: 2025-04-02 17:08:28 Comparing candidate commit 8b71b6b in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. |
| @@ -14,7 +13,7 @@ static struct { | |||
|
|
|||
| DDTRACE_PUBLIC bool ddtrace_user_req_add_listeners(ddtrace_user_req_listeners *listeners) | |||
| { | |||
| if (strcmp(sapi_module.name, "cli") != 0) { | |||
| if (strcmp(sapi_module.name, "cli") != 0 && strcmp(sapi_module.name, "frankenphp") != 0) { | |||
There was a problem hiding this comment.
You know that the frankenphp SAPI has two different ways of working, one worker mode and one which essentially works like fpm ("classic mode")? In the latter case you likely want this function to not be skipped.
There was a problem hiding this comment.
The condition does the opposite: it skips adding the listeners if not on cli (or frankenphp).
If we're running in classic mode, this has no ill effects: frankenphp_handle_request will never be called; the frankenphp integration won't be installed and \DDTrace\UserRequest\notify_start will never be called; the installed listeners will never be called.
I've added support and tests for classic mode; however, because I'm unable to detect it, it requires option DD_APPSEC_CLI_START_ON_RINIT, which disables the "user request" lifecycle and delineates the request with RINIT/RSHUTDOWN
There was a problem hiding this comment.
Ah right, this code runs in MINIT, and then you don't know yet. You can only detect that in RINIT via contents of $_SERVER.
cataphract
force-pushed
the
glopes/appsec-frankenphp
branch
from
e07bb91 to
3efd5ea
Compare
cataphract
force-pushed
the
glopes/appsec-frankenphp
branch
from
3efd5ea to
8b71b6b
Compare
| #ifdef FRANKENPHP_SUPPORT | ||
| if (strcmp(sapi_module.name, "frankenphp") == 0) { | ||
| // prepare a noop zend_compile_file for req init blocking in frankenphp | ||
| _orig_zend_compile_file = zend_compile_file; | ||
| zend_compile_file = _req_init_block_zend_compile_file; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This needs to happen in post_startup (after executing other modules post_startup) otherwise it won't be executed with opcache active.
Description
Reviewer checklist