Repository navigation
Add runtime anomaly detection for PHP errors and upload progress - #525
tudor-timcu wants to merge 4 commits into
Conversation
tudor-timcu
commented
Oct 22, 2026
tudor-timcu
commented
- Introduced new fields in RequestShutdownParams to capture PHP fatal error kind and upload progress status.
- Enhanced attack wave detection logic to include runtime anomalies based on PHP errors and upload progress.
- Updated sliding window logic to store reasons for suspicious requests.
- Added tests for runtime anomaly detection and ensured proper handling of various error signals.
- Updated Go wrappers and server methods to support new context fields for PHP error handling.
- Introduced new fields in RequestShutdownParams to capture PHP fatal error kind and upload progress status. - Enhanced attack wave detection logic to include runtime anomalies based on PHP errors and upload progress. - Updated sliding window logic to store reasons for suspicious requests. - Added tests for runtime anomaly detection and ensured proper handling of various error signals. - Updated Go wrappers and server methods to support new context fields for PHP error handling.
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| QueryParsed map[string]interface{} | ||
| IsWebScanner bool | ||
| ShouldDiscoverRoute bool | ||
| ThreadID uint64 |
There was a problem hiding this comment.
The existing RequestShutdownParams fields are re-aligned even though only two fields are added. Keeping their existing formatting would avoid unrelated churn.
Details
✨ AI Reasoning
The behavioral change is to add PHP fatal-error and upload-progress status to the shutdown parameters. The existing fields have no behavioral change, and their alignment changes are unnecessary to carry those new values.
🔧 How do I fix it?
Limit the change to what's needed to achieve its intent. Avoid bundling unrelated reformatting or refactoring, and don't rewrite code that didn't need to change.
Reply on this comment to give feedback or ignore the issue.
More info
| RateLimitGroup: context.GetRateLimitGroup(instance), | ||
| RateLimited: context.IsEndpointRateLimited(instance), | ||
| QueryParsed: context.GetQueryParsed(instance), | ||
| APISpec: api_discovery.GetApiInfo(instance, instance.GetCurrentServer()), | ||
| PhpFatalErrorKind: context.GetPhpFatalErrorKind(instance), | ||
| HasUploadProgressField: context.HasUploadProgressField(instance), |
There was a problem hiding this comment.
🟡 Medium - Runtime anomaly signals are collected after allocation-heavy API schema parsing
A request that triggers a PHP fatal or out-of-memory condition reaches OnPostRequest with runtime anomaly collection enabled. The shutdown handler evaluates api_discovery.GetApiInfo before calling the new fatal-error and upload-progress callbacks, so body deserialization/schema construction can allocate or bail out first and the anomaly is never forwarded to the agent for wave detection.
Show fix
| RateLimitGroup: context.GetRateLimitGroup(instance), | |
| RateLimited: context.IsEndpointRateLimited(instance), | |
| QueryParsed: context.GetQueryParsed(instance), | |
| APISpec: api_discovery.GetApiInfo(instance, instance.GetCurrentServer()), | |
| PhpFatalErrorKind: context.GetPhpFatalErrorKind(instance), | |
| HasUploadProgressField: context.HasUploadProgressField(instance), | |
| RateLimitGroup: context.GetRateLimitGroup(instance), | |
| RateLimited: context.IsEndpointRateLimited(instance), | |
| PhpFatalErrorKind: context.GetPhpFatalErrorKind(instance), | |
| HasUploadProgressField: context.HasUploadProgressField(instance), | |
| QueryParsed: context.GetQueryParsed(instance), | |
| APISpec: api_discovery.GetApiInfo(instance, instance.GetCurrentServer()), |
More info - Reply on this comment to give feedback or ignore the issue.
| if (!fieldName || !*fieldName) { | ||
| fieldName = "PHP_SESSION_UPLOAD_PROGRESS"; | ||
| } |
There was a problem hiding this comment.
🔵 Low - An explicitly empty upload-progress name is treated as the default field
A deployment that disables session upload-progress recognition with session.upload_progress.name="" receives a multipart request containing a normal field named PHP_SESSION_UPLOAD_PROGRESS. The new detector converts the explicit empty setting back to the default name and flags that field as upload progress, so ten such requests from an IP can generate a false php_runtime_anomalies attack-wave event and pollute telemetry.
Show fix
| if (!fieldName || !*fieldName) { | |
| fieldName = "PHP_SESSION_UPLOAD_PROGRESS"; | |
| } | |
| if (!fieldName) { | |
| fieldName = "PHP_SESSION_UPLOAD_PROGRESS"; | |
| } else if (!*fieldName) { | |
| return false; | |
| } |
More info - Reply on this comment to give feedback or ignore the issue.
…andling logic - Added a new boolean field `threwBlockingException` to the `RequestCache` class to track if a blocking exception was thrown. - Updated the `Action::executeThrow` method to set `threwBlockingException` to true when an exception is thrown. - Modified the `GoContextCallback` function to prevent reporting a fatal error as an attack if it was caused by a blocking exception. - Adjusted the test for PHP runtime anomalies to account for gateway errors when the PHP worker is terminated.
| // This works for both standard and frankenphp. | ||
| CallPhpFunctionWithOneParam("http_response_code", _code); | ||
| zend_throw_exception(GetFirewallDefaultExceptionCe(), _message.c_str(), _code); | ||
| AIKIDO_GLOBAL(requestCache).threwBlockingException = true; |
There was a problem hiding this comment.
🟡 Medium - Caught blocking exceptions suppress later fatal-anomaly signals
A PHP application catches the generic exception raised for a blocked sink or blocking-status decision and continues the same request, then encounters an unrelated fatal error. The new request-wide flag records only that a blocking exception was attempted, so the shutdown callback clears every later fatal_error result and the runtime-anomaly wave misses the fatal signal.
Show fix
Track whether the blocking exception actually remains the uncaught exception terminating the request, or clear/consume the marker when the exception is handled, instead of suppressing all later fatal_error values for the request.
More info - Reply on this comment to give feedback or ignore the issue.
- Updated installation commands in README.md, aws-elastic-beanstalk.md, fly-io.md, and laravel-forge.md to reference version 1.5.30. - Changed version constants in constants.go, globals.go, php_aikido.h, APKBUILD, and aikido.spec files to reflect the new version. - Ensured consistency across all relevant files for the release of version 1.5.30.
- Introduced a new static function `IsExceptionUnwindingThroughFinally` to determine if an exception is being unwound through a finally block. - Updated the `GetPhpFatalErrorKind` method to return "fatal_error" in FrankenPHP worker mode when an exception is unwinding through a finally block. - Enhanced error handling to improve the detection of fatal errors during request shutdown.
| continue; | ||
| } | ||
| zval* fastCall = ZEND_CALL_VAR(ex, opArray->opcodes[tryCatch->finally_end].op1.var); | ||
| if (Z_OBJ_P(fastCall)) { |
There was a problem hiding this comment.
🟡 Medium - Normal worker finally blocks are reported as fatal errors
A normal FrankenPHP worker request reaches the documented finally block and calls worker_rshutdown(). The new helper reads the Zend finally fast-call slot with Z_OBJ_P without verifying that the slot is an object, so the normal nonzero IS_LONG return target is treated as an exception and every ordinary request is labeled fatal_error, causing false php_runtime_anomalies waves after the per-IP threshold is reached.
| if (Z_OBJ_P(fastCall)) { | |
| if (Z_TYPE_P(fastCall) == IS_OBJECT && Z_OBJ_P(fastCall)) { |
More info - Reply on this comment to give feedback or ignore the issue.
