Expand appsec integrations to Laminas Framework#3716
Expand appsec integrations to Laminas Framework#3716estringana wants to merge 17 commits intomasterfrom
Conversation
estringana
force-pushed
the
estringana/add-laminas-appsec-integration
branch
from
820057d to
eae5325
Compare
|
✨ Fix all issues with BitsAI or with Cursor
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3716 +/- ##
==========================================
- Coverage 68.81% 68.74% -0.08%
==========================================
Files 166 166
Lines 19030 19030
Branches 1797 1797
==========================================
- Hits 13096 13082 -14
- Misses 5121 5135 +14
Partials 813 813
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2026-04-14 15:46:16 Comparing candidate commit a35cd52 in PR branch Found 0 performance improvements and 6 performance regressions! Performance is the same for 188 metrics, 0 unstable metrics. scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:PDOBench/benchPDOOverhead-opcache
scenario:PDOBench/benchPDOOverheadWithDBM-opcache
scenario:PHPRedisBench/benchRedisOverhead
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching3
|
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. If you need to update snapshots, please refer to CONTRIBUTING.md |
estringana
force-pushed
the
estringana/add-laminas-appsec-integration
branch
from
d2866e8 to
651e235
Compare
estringana
force-pushed
the
estringana/add-laminas-appsec-integration
branch
from
c199b10 to
9b662b3
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 140b8c4617
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $adapter = $hook->args[0] ?? null; | ||
| $userLogin = null; | ||
|
|
||
| if ($adapter && method_exists($adapter, 'getIdentity')) { | ||
| $userLogin = $adapter->getIdentity(); |
There was a problem hiding this comment.
The failure path only inspects $hook->args[0] for the adapter, so applications that use setAdapter($adapter) followed by authenticate() (no argument) emit login-failure events without a user identifier. That drops automated identification data for a common Laminas authentication flow and makes failure telemetry much less useful.
Useful? React with 👍 / 👎.
| if ($event->getRouteMatch() === null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Endpoint collection is gated on getRouteMatch() !== null, so requests that route to 404/no-match paths skip collection entirely. If early traffic is mostly unmatched (or an app primarily serves unmatched probes), are_endpoints_collected() can remain false and endpoints may never be registered despite the router being available.
Useful? React with 👍 / 👎.
| static boolean expectedVersion = | ||
| ['7.3', '7.4', '8.0', '8.1'].contains(getPhpVersion()) && !getVariant().contains('zts') |
There was a problem hiding this comment.
Shouldn't we test with a more recent version? This seems very old. I fyou want you can test the oldest supported and the newest supported (make a base class / trait with the tests and then extend in two separate test classes)
| if ($route instanceof \Laminas\Router\Http\Part) { | ||
| $base = self::partRouteBaseTemplate($route); | ||
| $base = $base ?? ''; | ||
| if (!$hasChild) { | ||
| return $base !== '' ? $base : null; | ||
| } | ||
| $child = self::httpRouteTemplateFromNamedRouteStack($route, $segments[1]); | ||
| if ($child === null) { | ||
| return $base !== '' ? $base : null; | ||
| } | ||
|
|
||
| return $base . $child; | ||
| } | ||
|
|
||
| if ($route instanceof \Laminas\Router\Http\TreeRouteStack && $hasChild) { | ||
| return self::httpRouteTemplateFromNamedRouteStack($route, $segments[1]); | ||
| } | ||
|
|
||
| if ($hasChild) { | ||
| return self::httpRouteTemplateFromMatchedRoute($route, null); | ||
| } |
There was a problem hiding this comment.
Wouldn't this cause the route name to be set multiple times? For the individual parts and for the high level router? I think you should not call this method unless it's the top level.
| @Testcontainers | ||
| @EnabledIf('isExpectedVersion') | ||
| @TestMethodOrder(MethodOrderer.OrderAnnotation) | ||
| class Laminas33Tests { |
There was a problem hiding this comment.
You don't have tests for interesting routers. Try to test the full hierarchy:
RouteInterface (single method: match())
└── RouteStackInterface (adds: addRoute, addRoutes, removeRoute, setRoutes)
└── SimpleRouteStack (concrete; adds getRoutes() + PriorityList storage)
└── TreeRouteStack (adds path-offset matching, defaultParams injection)
├── Part (a "node" in the tree: base route + lazy children)
└── Chain (concatenates multiple route stages sequentially)
HttpRouteInterface extends RouteInterface
├── Literal (exact string match)
├── Segment (`:param` placeholders, optional parts)
├── Regex (regex match with named captures)
├── Hostname (matches the Host header)
├── Method (matches HTTP verb)
├── Scheme (matches http/https)
├── Wildcard (key/value pairs after a prefix)
└── Placeholder (always matches, no constraints)
For instance, I can tell that chain doesn't work. This test fails the final assertion
@Test
@Order(8)
void 'nested Part and Chain routes produce correct http route'() {
HttpRequest nestedReq = container.buildReq('/resource/42/99').GET().build()
Trace nestedTrace = container.traceFromRequest(nestedReq, ofString()) { HttpResponse<String> resp ->
assert resp.statusCode() == 200
}
assert nestedTrace.first().meta.'http.route' == '/resource/:resourceId/:subId'
HttpRequest chainReq = container.buildReq('/chain/abc').GET().build()
Trace chainTrace = container.traceFromRequest(chainReq, ofString()) { HttpResponse<String> resp ->
assert resp.statusCode() == 200
}
assert chainTrace.first().meta.'http.route' == '/chain/:chainId'
}with
diff --git a/appsec/tests/integration/src/test/www/laminas33/module/Application/config/module.config.php b/appsec/tests/integration/src/test/www/laminas33/module/Application/config/module.config.php
index ee2635397..c9c91f285 100644
--- a/appsec/tests/integration/src/test/www/laminas33/module/Application/config/module.config.php
+++ b/appsec/tests/integration/src/test/www/laminas33/module/Application/config/module.config.php
@@ -56,6 +56,60 @@ return [
],
],
],
+ 'nested_resource' => [
+ 'type' => Literal::class,
+ 'options' => [
+ 'route' => '/resource',
+ 'defaults' => [
+ 'controller' => DynamicPathController::class,
+ 'action' => 'index',
+ ],
+ ],
+ 'may_terminate' => true,
+ 'child_routes' => [
+ 'item' => [
+ 'type' => Segment::class,
+ 'options' => [
+ 'route' => '/:resourceId',
+ 'defaults' => [
+ 'controller' => DynamicPathController::class,
+ 'action' => 'index',
+ ],
+ ],
+ 'may_terminate' => true,
+ 'child_routes' => [
+ 'sub' => [
+ 'type' => Segment::class,
+ 'options' => [
+ 'route' => '/:subId',
+ 'defaults' => [
+ 'controller' => DynamicPathController::class,
+ 'action' => 'index',
+ ],
+ ],
+ ],
+ ],
+ ],
+ ],
+ ],
+ 'chained_resource' => [
+ 'type' => Literal::class,
+ 'options' => [
+ 'route' => '/chain',
+ ],
+ 'chain_routes' => [
+ [
+ 'type' => Segment::class,
+ 'options' => [
+ 'route' => '/:chainId',
+ 'defaults' => [
+ 'controller' => DynamicPathController::class,
+ 'action' => 'index',
+ ],
+ ],
+ ],
+ ],
+ ],
],
],
'controllers' => [
| assert span.metrics.'_dd.appsec.waf.duration' > 0.0d | ||
| assert span.meta.'_dd.appsec.event_rules.version' != '' | ||
| assert span.meta.'appsec.blocked' == 'true' | ||
| } |
There was a problem hiding this comment.
no tests for http.route
| // Push path params to appsec | ||
| if (function_exists('\datadog\appsec\push_addresses')) { | ||
| $params = $routeMatch->getParams(); | ||
| // Filter out the framework-specific params (controller, action) | ||
| $pathParams = array_diff_key($params, array_flip(['controller', 'action'])); | ||
| if (count($pathParams) > 0) { | ||
| \datadog\appsec\push_addresses(["server.request.path_params" => $pathParams]); | ||
| } | ||
| } |
There was a problem hiding this comment.
This should fire only for the top level router, otherwise you're pushing to the WAF multiple times.
|
|
||
| hook_method( | ||
| 'Laminas\Authentication\AuthenticationService', | ||
| 'getIdentity', |
There was a problem hiding this comment.
You prob should dedup these calls to avoid calling track_authenticated_user_event_automated multiple times per request. For instance, you call this method yourself from another hook.
| return $out; | ||
| } | ||
|
|
||
| private static function walkRouteStackCollectEndpointRows( |
There was a problem hiding this comment.
Part children are only populated on first match(). The onRoute hook fires after one path through the route tree is matched, so only the subtree along the matched path is materialized. You need to force this materialization.
See the in the other comment when I added extra routes with child routes. Those are not found by the Endpoints are sent test.
| $currentStack, | ||
| string $namePrefix, | ||
| array &$rows | ||
| ): void { |
There was a problem hiding this comment.
This does not seem to catch the Chain router I added. The code only recurses into Part. Note that Chain is fundamentally different from Part in this context - its children are sequential stages that together form a single endpoint URL, not separate sibling endpoints.
| $qualifiedName = $namePrefix === '' ? (string) $name : $namePrefix . '/' . $name; | ||
| $path = self::httpRouteTemplateFromNamedRouteStack($rootRouter, $qualifiedName); | ||
| if ($path !== null && $path !== '') { | ||
| $method = 'GET'; |
There was a problem hiding this comment.
This hardcoded GET is not correct. Make sure the integration tests include non-GET endpoints.
| string $namePrefix, | ||
| array &$rows | ||
| ): void { | ||
| foreach ($currentStack->getRoutes() as $name => $route) { |
There was a problem hiding this comment.
The code guards on instanceof RouteStackInterface but then calls->getRoutes(). getRoutes() is defined only on the concrete SimpleRouteStack
3 participants
Add AAP support to Laminas framework
This PR introduces AAP support to the Laminas framework, enabling improved observability and authentication-related event tracking.
Features added
Provide
http.routeCaptures and exposes the resolved route for incoming requests.
Collect endpoints at startup
Gathers and registers available endpoints during application bootstrap.
Login success event
Emits an event when a user successfully authenticates.
Login failure event
Emits an event when authentication fails.
Authenticated request events
Tracks requests performed by authenticated users.
Collect path parameters
Extracts and includes route path parameters in the collected data.
Limitations / Missing features
Not implemented because Laminas does not provide a built-in signup mechanism. This functionality is application-specific and must be implemented by developers if needed.
Reviewer checklist