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-14 14:11:05 Comparing candidate commit 7b7f971 in PR branch Found 0 performance improvements and 4 performance regressions! Performance is the same for 190 metrics, 0 unstable metrics.
|
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>
a2be41b to
7b7f971
Compare
![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: 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".
| } | ||
|
|
||
| $raw = ltrim($route, '/'); | ||
| $parts = explode('/', $raw); |
There was a problem hiding this comment.
When a brace-style constraint contains /, such as the common Slim pattern /files/{name:[^/]+}, splitting the raw template first breaks the placeholder into multiple segments. The normalizer consequently returns /files/%7Bname%3A%5E/%2B%7D instead of /files/{name}, so constrained routes receive a corrupt AppSec route tag.
Useful? React with 👍 / 👎.
| $rootSpan->meta[Tag::HTTP_ROUTE] = $return->getPattern(); | ||
| $pattern = $return->getPattern(); | ||
| $rootSpan->meta[Tag::HTTP_ROUTE] = $pattern; | ||
| $normalizedRoute = \DDTrace\Util\RouteNormalizer::normalizeFromSlim($pattern); |
There was a problem hiding this comment.
For Slim 3 routes containing optional sections, this call always supplies an empty matched-parameter array, and only the Slim 4 controller path later refines the result. Thus a request matching /users/{id}[/{format}] with format present is still tagged as /users/{id} rather than /users/{id}/{format}.
Useful? React with 👍 / 👎.
| $rootSpan->meta[Tag::HTTP_ROUTE] = $This->matched_rule; | ||
| $matchedRule = $This->matched_rule; | ||
| $rootSpan->meta[Tag::HTTP_ROUTE] = $matchedRule; | ||
| $normalizedRoute = \DDTrace\Util\RouteNormalizer::normalizeFromWordPress($matchedRule); |
There was a problem hiding this comment.
When a WordPress rule contains an optional path capture, only the regex template is passed to the normalizer, so it cannot tell whether that capture participated in the match. For example, the ordinary rule ([^/]+)(?:/([0-9]+))?/?$ tags a /simple request as /{param1}/{param2}, incorrectly inventing a second path segment and grouping requests with different actual route shapes.
Useful? React with 👍 / 👎.
| $normalizedRoute = \DDTrace\Util\RouteNormalizer::normalizeFromCodeIgniter($uri); | ||
| if ($normalizedRoute !== null) { | ||
| $rootSpan->meta[Tag::APPSEC_NORMALIZED_ROUTE] = $normalizedRoute; |
There was a problem hiding this comment.
When no configured route matches, $uri is the concrete request path rather than a route template. Normalizing it as static text means arbitrary requests such as /users/12345 or 404 paths are emitted verbatim in _dd.appsec.normalized_route, retaining parameter values and creating attacker-controlled high-cardinality route tags; this fallback should derive a framework route shape or omit the normalized tag.
Useful? React with 👍 / 👎.
| if (preg_match('/^\([^)]+\)$/', $segment)) { | ||
| $normalizedSegments[] = '{param' . $paramIndex++ . '}'; | ||
| } elseif (preg_match('/[()[\].*+?|^${}\\\\]/', $segment)) { | ||
| $normalizedSegments[] = '{param' . $paramIndex++ . '}'; |
There was a problem hiding this comment.
The WordPress normalizer assigns one placeholder per regex-containing URL segment rather than per capturing group. A valid rule such as ^post-([^/]+)-([0-9]+)/?$ therefore becomes /{param1} even though the segment contains two dynamic values and should be represented as /{param1+param2}, causing distinct route structures to collapse to the same normalized tag.
Useful? React with 👍 / 👎.
| if ($isOptional && !array_key_exists($name, $matchedParams)) { | ||
| return null; |
There was a problem hiding this comment.
When an absent optional placeholder shares a segment with required placeholders, returning null discards the entire segment. For example, Laravel normalization of /files/{name}.{ext?} with only name matched currently produces /files rather than /files/{name}, removing a required part of the matched route.
Useful? React with 👍 / 👎.
| $innerWithValue = preg_replace( | ||
| '/' . preg_quote($paramPrefix . $param, '/') . '/', | ||
| $value, | ||
| $inner | ||
| ); |
There was a problem hiding this comment.
For a Laminas optional section containing multiple parameters, this loop substitutes only one parameter at a time before comparing the section with the URL. Consequently /archive[/:year/:month] with both values present cannot match /archive/2024/08; nested patterns such as /foo[/:bar[/:baz]] collapse into the same multi-parameter case and are normalized as /foo even when both optional segments were requested.
Useful? React with 👍 / 👎.
|
|
||
| if ($path !== null) { | ||
| $rootSpan->meta[Tag::HTTP_ROUTE] = $path; | ||
| $normalizedRoute = \DDTrace\Util\RouteNormalizer::normalizeFromSymfony($path); |
There was a problem hiding this comment.
With DD_TRACE_SYMFONY_HTTP_ROUTE enabled, the normalizer receives only the catalog template and no request path or route-default information. Symfony generator templates retain trailing defaulted variables, so a route such as /blog/{page} requested as /blog is tagged /blog/{page} even though page was absent; the request must be used to remove optional variables before setting the normalized tag.
Useful? React with 👍 / 👎.
| if (empty($innerParams)) { | ||
| return $inner; | ||
| } |
There was a problem hiding this comment.
Slim/FastRoute permits optional portions that contain no placeholder, such as /feed[.json], but this branch unconditionally includes such a portion. Because matchedParams cannot indicate whether a static optional was matched, both /feed and /feed.json are normalized as /feed.json; the actual request path is needed to decide whether to retain the section.
Useful? React with 👍 / 👎.
| /** | ||
| * Normalize a Laminas route template. | ||
| * | ||
| * Laminas uses :param for dynamic parameters and [...] for optional sections. |
There was a problem hiding this comment.
According to https://docs.laminas.dev/laminas-router/routing/ the :colon syntax is not the only one and they also have e.g. Laminas\Router\Http\Regex.
|
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. |
2 participants
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