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-09-02 12:43:28 Comparing candidate commit 82db9a3 in PR branch Found 5 performance improvements and 43 performance regressions! Performance is the same for 145 metrics, 1 unstable metrics.
|
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".
|
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
34e4e8b to
f42c408
Compare
cataphract
left a comment
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
5d1199f to
d5e909b
Compare
|
@cataphract pr is ready back to you with all your comments addressed |
bdadbff to
eddfd4f
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>
…coding, regex constraints - expandBracketOptionals: use strpos > 0 (not != false) so optional sections whose text coincidentally appears in the mandatory route prefix are not falsely detected as present; add case-insensitive fallback for percent-encoded param values so %c3%a9 (lowercase hex from browsers) matches %C3%A9 (rawurlencode output) - normalizeFromLaminas: accept pre-computed $urlMatchedParams to skip inferSymfonyRouteParams when the caller has better information - LaminasIntegration cache key: use '/'+value prefix check at position > 0 so params whose default value equals the mandatory route text are not treated as present; include static-only optional sections ([/draft]) in the key so absent and present shapes get distinct entries; for Regex routes, extract the actual route regex via reflection and run it against the URL to get accurate named captures, avoiding inferSymfonyRouteParams which ignores Laminas constraints - SymfonyIntegration: use $route->compile()->getRegex() when available to determine URL-matched params instead of generic URL inference, so routes with requirements (e.g. format=html|json with a default) correctly exclude defaulted params from the normalized route Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4cbeed5 to
82db9a3
Compare
cataphract
left a comment
cataphract
left a comment
There was a problem hiding this comment.
The mostly unreviewed implementation of the suggestions is at 82db9a3...glopes/normalise-http-route
|
|
||
| for ($k = $n; $k >= 1; $k--) { | ||
| $regexBody = ''; | ||
| for ($ri = 0; $ri < $k; $ri++) { |
There was a problem hiding this comment.
Infer optional participation with Laravel’s compiled matcher
This does not follow the RFC’s request-specific optional-shape rule for mixed segments: it replaces every parameter with (.+), which ignores Laravel where() requirements. The subsequent comparison against $allParams cannot recover capture provenance because Laravel has already merged ->defaults() into that map. Example of such a failure:
route: Regex /normalized-regex-chain/%name%.%ext% + Literal /view
requirement: ext=pdf|json
default: ext=txt
request: /normalized-regex-chain/report.txt/view
captures: name=report.txt, ext absent
post-default route params: name=report.txt, ext=txt
RFC-compliant: omit the tag, or emit /normalized-regex-chain/{name}/view
actual: /normalized-regex-chain/{name+ext}/view
Suggestion: replace this parser with the framework matcher Laravel used to bind the route. It already preserves requirements and optional-group participation:
private static function laravelUrlMatchedParams($route, $request, array $allParams): array
{
$matches = [];
$path = '/' . ltrim($request->decodedPath(), '/');
if (preg_match($route->getCompiled()->getRegex(), $path, $matches) !== 1) {
return $allParams;
}
$matched = [];
foreach ($route->parameterNames() as $name) {
if (isset($matches[$name]) && $matches[$name] !== ''
&& key_exists($name, $allParams)) {
$matched[$name] = $allParams[$name];
}
}
return $matched;
}| for ($k = $n; $k >= 1; $k--) { | ||
| $regexBody = ''; | ||
| for ($i = 0; $i < $k; $i++) { | ||
| $regexBody .= preg_quote($staticParts[$i], '/') . '(.+)'; |
There was a problem hiding this comment.
Do not infer dynamic participation without the framework matcher
This helper is not an RFC-compliant fallback for dynamic optional components. The RFC requires optional parameter participation to come from framework match metadata or an exact replay of the compiled matcher; this synthesized (.+) expression ignores route requirements, optional-capture semantics, and unmatched positions. It can therefore report a parameter as present even when the framework rejected that capture and supplied a default instead.
The inference logic here also fails in specific examples, which I can provide if you want.
Please make “accurate participation unavailable” explicit (for example, return null) and have dynamic-route callers omit _dd.appsec.normalized_route in that case. URL-only best effort should be limited to purely static optional components, as allowed by the RFC.
| $_present = []; | ||
| foreach ($_pm[1] as $_p) { | ||
| if (isset($allParams[$_p]) && $urlPath !== null && | ||
| strpos($urlPath, '/' . (string)$allParams[$_p]) > 0) { |
There was a problem hiding this comment.
Match Laminas optionals with the compiled Segment matcher
This cache is bounded, but the optional parameter participation test is not equivalent to Laminas routing. $allParams comes from RouteMatch::getParams(), where captured parameters and route defaults have already been merged. Searching those values and static fragments anywhere in the URL loses both capture provenance and route position.
Two concrete false positives are:
template: /prefix/:id[/:value]
URL: /prefix/foo
matcher: id=foo; value=foo comes only from the route default
expected: /prefix/{id}
actual: /prefix/{id}/{value}
template: /prefix/:id[/foo]
URL: /prefix/foo
matcher: id=foo; optional static /foo absent
expected: /prefix/{id}
actual: /prefix/{id}/foo
Nested static groups also collide in the cache:
template: /archive[/draft[/preview]]
first URL: /archive -> key /archive# -> caches /archive
second URL: /archive/draft -> same key, because the one-pass bracket regex
sees only the inner [/preview]
expected: /archive/draft
actual: /archive
For dynamic optionals, the RFC requires participation to come from framework match metadata or an exact replay of the framework matcher. A defaulted value occurring somewhere in the URL is not evidence that its capture participated.
I tested replacing this heuristic with a replay of Laminas Segment::match() before defaults are merged. The important part is to use the Segment's compiled regex and paramMap, retain capture offsets, and walk the Segment's parsed parts at those exact offsets to recover the expanded route shape:
public static function inferLaminasSegmentMatch(
\Laminas\Router\Http\Segment $route,
string $urlPath
) {
$routeData = \Closure::bind(
static function ($segment) {
return [
$segment->regex,
$segment->paramMap,
$segment->parts,
$segment->translationKeys,
];
},
null,
\Laminas\Router\Http\Segment::class
)($route);
if (!is_array($routeData) || count($routeData) !== 4 || !empty($routeData[3])) {
return null;
}
$matches = [];
if (@preg_match(
'(^' . $routeData[0] . '$)',
$urlPath,
$matches,
PREG_OFFSET_CAPTURE
) !== 1) {
return null;
}
$captures = [];
$params = [];
foreach ($routeData[1] as $group => $name) {
if (!isset($matches[$group][0]) || $matches[$group][0] === '') {
continue;
}
$captures[$name][] = [
'raw' => $matches[$group][0],
'offset' => $matches[$group][1],
];
$params[$name] = rawurldecode($matches[$group][0]);
}
$states = [[
'offset' => 0,
'template' => '',
'capture_indexes' => [],
]];
$states = self::matchLaminasSegmentParts(
$routeData[2],
$urlPath,
$captures,
$states
);
foreach ($states as $state) {
if ($state['offset'] !== strlen($urlPath)) {
continue;
}
foreach ($captures as $name => $values) {
if (($state['capture_indexes'][$name] ?? 0) !== count($values)) {
continue 2;
}
}
return [
'template' => $state['template'],
'params' => $params,
];
}
return null;
}matchLaminasSegmentParts() advances literal parts only at the current URL offset, consumes each parameter only at its matcher-reported capture offset, and branches structurally for each optional part. That preserves nested optional groups and repeated values without putting raw request values into the cache key. The call site then uses the exact expanded template as the bounded shape key and normalizes with only the pre-default captured parameters:
$segmentMatch = self::inferLaminasSegmentMatch($leafRoute, $urlPath);
$cacheKey = $httpRoute . '#' . $segmentMatch['template'];
$normalizedRoute = RouteNormalizer::normalizeFromLaminas(
$segmentMatch['template'],
$segmentMatch['params'],
null,
null
);| } | ||
| $_braceTemp = preg_replace('/%([a-zA-Z_][a-zA-Z0-9_]*)%/', '{$1}', $httpRoute); | ||
| $_urlMatchedKeys = array_keys( | ||
| $urlMatchedFromRegex ?? \DDTrace\Util\RouteNormalizer::inferSymfonyRouteParams($_braceTemp, $urlPath) |
There was a problem hiding this comment.
Do not use constraint-free URL inference as a fallback
As mentioned in other comments, RFC-1103 says: “Tracers must use the framework's matched-parameter map,” “must not infer presence from the URL alone,” and must “fall back to omitting the tag rather than emitting an inaccurate value.” inferSymfonyRouteParams() does the prohibited URL-only inference with generic (.+) captures, so it loses matcher constraints and cannot distinguish an absent capture from a route default.
rule: /x/{name}.{ext?}; ext=pdf|json; default ext=txt
request: /x/report.txt
expected: /x/{name} (or no tag)
actual: /x/{name+ext}
rule: Regex /normalized-regex-chain/%name%.%ext% + Literal /view; ext=pdf|json; default ext=txt
request: /normalized-regex-chain/report.txt/view
expected: /normalized-regex-chain/{name}/view (or no tag)
actual: /normalized-regex-chain/{name+ext}/view
In both cases the framework captures the entire report.txt value as name; ext does not participate and is added only from the default. Please support each specific pattern by replaying its actual matcher before defaults are merged, or omit _dd.appsec.normalized_route.
| * in the match, so phantom segments are not emitted. | ||
| * @return string|null | ||
| */ | ||
| public static function normalizeFromWordPress(string $matchedRule, $urlPath = null) |
There was a problem hiding this comment.
Use PCRE's match result instead of parsing the rewrite expression
The cache now distinguishes capture participation, but the route plan feeding it is still produced by a partial PCRE parser. That parser misclassifies valid syntax:
^normalized-quoted-literal/\Qfile.json\E$
expected: /normalized-quoted-literal/file.json
actual: /normalized-quoted-literal/Qfile.jsonE
^normalized-quoted-captures/(?'first'[^/]+)-(?'second'[^/]+)$
expected: /normalized-quoted-captures/{first+second}
actual: /normalized-quoted-captures/{param1}
PHP's match result already provides the authoritative numeric captures, named aliases, participation, and byte offsets. We can build the request-specific route from that result instead of extracting literals and capture names from the regex source. Before retaining concrete gaps as constants, use a conservative rejection filter: if the rule can consume variable text outside a capture (for example an uncaptured character class, repetition, or backreference, say ^foo/[a-z]+/bar), return null and omit the tag. Fixed literal alternatives and optional literals like red|blue remain bounded.
4 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