Implement normalised http routes - #4101
Conversation
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 |
|
0cc69dd to
1fa5485
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-08-27 10:44:57 Comparing candidate commit e02670a in PR branch Found 4 performance improvements and 38 performance regressions! Performance is the same for 151 metrics, 1 unstable metrics.
|
a2be41b to
7b7f971
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b7f9710a5
ℹ️ 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".
|
Do you have a rough estimate on the overhead of the route normalization? It seems expensive. Laminas is obviously just slow because it does runtime route matching, but other stacks like Slim or Symfony rely on precompiled routes. |
ff7f8fe to
8632abe
Compare
|
Approving from the stance of IDM, due to the new field being introduced |
b57a9e9 to
2ab71bd
Compare
Add missing Tag::APPSEC_NORMALIZED_ROUTE assertions to: - Symfony TraceSearchConfigTest (V4_4, V5_0, V5_1, V5_2, V6_2) - Laravel TraceSearchConfigTest (V4, V5_7, V5_8, V8_x) - Laravel V8_x RouteCachingTest and InternalExceptionsTest - Laravel Octane CommonScenariosTest - Yii ParameterizedRouteTest, ModuleTest, LazyLoadingIntegrationsFromYiiTest - CodeIgniter ExitTest and NoCI_ControllertTest - Fix UserAvailableConstantsTest tag ordering (APPSEC_NORMALIZED_ROUTE must appear after HTTP_ROUTE to match Tag.php declaration order) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
34e4e8b to
f42c408
Compare
cataphract
left a comment
There was a problem hiding this comment.
It seems there are many gaps in framework support. I asked AI for failing counterexamples against the spec and got many:
I already removed a few that were obviously invalid (like, changing the routing type at runtime and complained about cache staleness), but the rest seem mostly valid
| $locale = $request->attributes->get('_locale'); | ||
| if ($locale !== null) { | ||
| $path = EndpointCatalog::pathForRoute($route_name . '.' . $locale, $container); | ||
| $cacheKey = $route_name; |
There was a problem hiding this comment.
Also in other frameworks: the cache key needs to be richer otherwise, for optional parameters, it caches only the first materialization of route that was found.
Description
Add
_dd.appsec.normalized_routetag to HTTP framework integrations (Laravel, Slim, Symfony, Laminas, CakePHP, Yii, CodeIgniter, WordPress) per RFC-1103. The tag exposes a normalized form of the matched route, stripping concrete parameter values and framework-specific syntax (regexconstraints, optional markers) into a canonical{param}notation suitable for security analysis.Reviewer checklist