Skip to content

Commit b9b4e0b

Browse files
feat(appsec): RFC-1103 normalized HTTP route tag for Express (#8857)
* feat(appsec): implement RFC-1103 normalized HTTP route tag for Express Adds `_dd.appsec.normalized_route` span tag on every Express request when API Security is enabled, converting framework-specific route syntax to the RFC-1103 normalized form (e.g. `/api/:version/users/:id` → `/api/{version}/users/{id}`). Supports Express 4 and 5, named/optional/catch-all params, multi-param segments (`:a.:b` → `{a+b}`), and correctly resolves optional params for sub-routers with `mergeParams=false` by matching against the request URL. Performance: routes are compiled once per unique route string and cached; non-optional routes hit a Map lookup on every request (~25 ns). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * fix(appsec): use optional call for context().getTag in normalized route check context().getTag is absent on mock spans used in unit tests; use ?.getTag?.() to avoid a TypeError when the context object does not implement getTag. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * test(appsec): add coverage tests for normalized-route-express edge cases Cover previously-uncovered code paths to satisfy the codecov/patch 95% threshold: - trailing static text in getSegmentRegex and buildGenericSegmentRegex - buildGenericSegmentRegex fallback (invalid constraint regex) - named wildcard capture in matchSegs via URL extraction Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * refactor(appsec): rename normalized-route-express to normalized-route, add component dispatch Renames api_security/normalized-route-express.js → normalized-route.js to prepare for multi-framework support. Adds a normalizeRoute(component, ...) dispatcher with a switch on the component tag (express now; other frameworks to follow). The call site in appsec/index.js passes the component from the span instead of gating on it. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * refactor(appsec): address PR review — pass req to normalizeRoute, move rootSpan inside guard - normalizeRoute now takes req and extracts component/route/params/urlPath internally (web module imported into normalized-route.js) - rootSpan is now computed inside the if (route) guard in incomingHttpEndTranslator, avoiding wasted work when route is empty - Remove the '// Public API' section separator (flagged as not needed) - Export normalizeRouteExpress for unit testing Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * refactor(appsec): lazy evaluation in normalizeRoute, simplify call site - index.js: remove route pre-check, call normalizeRoute(req) directly; web.root(req) only fetched when result is non-null (tag is to be set) - normalizeRoute: check component first and return null early for unsupported frameworks; route/urlPath extracted only for matched case Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * feat(appsec): support Express 5 {/:param} optional-group syntax in normalized route Adds expandV5OptionalGroups() which converts Express 5 {/:id} optional-group syntax to equivalent :id? form before processing, enabling full normalization support for the standard Express 5 optional-segment pattern. Supported conversions: /items{/:id} → /items/:id? → /items/{id} or /items /api{/:version}/users → /api/:version?/users /photos/:id{.:format} → /photos/:id.:format? /posts{/:id.:format} → /posts/:id?.:format? Groups with only static content ({/draft}) are still rejected. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> * feat(appsec): rewrite normalized-route around a route tokenizer Replaces the expand+split core with a parse-once tokenizer that compiles a route into segment templates, then renders + URL-matches from that model. This adds full Express 5 support and resolves the open review threads: - Optional static groups: /posts{/draft} → /posts/draft | /posts - Optional catch-all groups: /files{/*path} → /files/{path} | /files - Quoted param names: /users{/:"user-id"} → /users/{user-id} - Nested optional groups: /a{/:b{/:c}} → /a/{b}/{c} | /a/{b} | /a - Express 4 inline constraints incl. slash: /:id([^/]+) → /{id} - Duplicate names: the last occurrence keeps the name, earlier (shadowed) ones become paramN — /:id/:id → /{param1}/{id} (RFC rule 4 uniqueness) Caching keys on the raw route string (parse/compile once); optional routes cache rendered output per presence bitmask. Internals (parseRoute/compileRoute/renderRoute) are exported for unit testing; the spec de-aliases the import and adds per-function and dispatcher tests plus the full case matrix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): address review findings in normalized-route tokenizer - Capture groups inside an inline constraint no longer corrupt optional-group detection: presence markers are named captures (?<_ddgN>) read via m.groups, immune to capture-index shifts (e.g. /:id(fo(o)).:format? → /{id}). - Param + catch-all in one segment now combines names: /x/:a-* → /x/{a+param1}. - Name uniqueness is enforced on ENCODED names so two raw names that encode identically can't collide: /:"a/b"/:"a%2Fb" → /{param1}/{a%2Fb}. - Backslash-escaped reserved chars are treated as static (Express 5): /file/\{id\} → /file/%7Bid%7D. - A non-terminal param whose constraint can consume '/' → null (rule 5); terminal one is kept as the tail element. - Guard pathological routes: cap optional groups at 24 (bitmask stays in 32 bits) and bound the backtracking matcher with a step budget (24 optionals on a non-matching URL: ~1500ms → ~1ms, falls back to req.params). - Only treat a token as intra-segment-optional when its group is a strict descendant of the segment group; harden m.groups read and the regex fallback. - groupActive guards an undefined parent; add `variants` to the typedef. Adds regression tests for each finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): round-3 review fixes for normalized-route - ReDoS: never embed a developer inline constraint in the URL matcher when it is catastrophic (nested-quantifier heuristic), invalid, or contains a named group; use a generic [^/]+? matcher instead. Constraint values are discarded from the output anyway. (/:id((a+)+$)?/x on a long URL: ~1200ms → <1ms.) This also fixes the named-marker collision when a constraint contains (?<...>). - Step-budget abort now omits the tag (null) instead of guessing from req.params; a clean URL/route mismatch still falls back to params. - The `?` modifier only absorbs a true delimiter ('.') as its optional prefix, not arbitrary preceding static: /x:id? on /x → /x (was /), /foo/v:id? on /foo/v → /foo/v. - Non-terminal slash-consuming constraints: also reject a literal '/' in the source and test more samples (/:id(foo/bar)/tail → null). - Static optionals are matched in encoded form so non-ASCII matches the encoded URL (/posts{/café} on /posts/caf%C3%A9 → /posts/caf%C3%A9). - Param names: accept Unicode letters and $ ; handle escaped quotes in quoted names. - Lower MAX_OPTIONAL_GROUPS to 12 (bounds the per-route variant cache to 4096). Adds regression tests for each finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): round-4 review fixes for normalized-route Point 1 — eliminate the ReDoS class definitively: developer inline constraints are NEVER embedded in the URL matcher (always a generic [^/]+? matcher), so a crafted URL can never trigger catastrophic backtracking in a developer regex (e.g. a*a*a*…$ or (a+)+). req.params is now the AUTHORITY for which optional params are present — it is what Express populates — and the URL matcher is used only to resolve optionals absent from req.params (mergeParams) or static/wildcard-only optional groups. This preserves adjacent-optional disambiguation (/:a(\d+)?/:b? → /{b} when Express set b) without any constraint execution on URL input. Point 2 — static segments match case-insensitively (Express default routing); the normalized output still preserves the route's declared case. Point 3 — resolvePresenceFromUrl returns an explicit { present, aborted } instead of relying on a module-global flag read after the fact. Point 4 — thread the Express major version (instrumentation → apm:express:request:handle → tracing plugin → web.setFramework → web context → normalizeRoute). Express 4 routes now parse with the v4 dialect: `{}` are literal characters (/file/{id} → /file/%7Bid%7D), `*` is an unnamed wildcard, and bare `?`/`+`/`(` string-patterns return null. Express 5 (default when version unknown) keeps {…} groups, :"quoted" names, and *name wildcards. The route cache key includes the dialect. Point 5 — matcher edges: a catch-all segment now matches its non-wildcard prefix before consuming the rest (/:id?/:a-* on /y-z/w → /{a+param1}); a terminal param whose constraint can consume '/' is treated as a catch-all so mergeParams parent params are recovered (/api/:version?/files/:rest(.+) → /api/{version}/files/{rest}). Adds regression tests (incl. v4-dialect cases via the isV5 arg) for every finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(appsec): address review comments — drop section-divider rules, add /a//b test - Remove the // ---- divider rule lines (keep one-line section labels), per review. - Add a regression test for empty-segment collapse (/a//b → /a/b, rule 2), completing the requested v4/v5 case matrix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): round-5 review fixes for normalized-route HIGH — the round-4 req.params-authoritative fast path was unsound: when req.params held any optional param it skipped URL matching entirely, which (a) defeated the mergeParams=false recovery this PR is built for (a dropped parent param was marked absent → wrong lower-cardinality tag) and (b) lit up the wrong group when a param name is shared across groups. Fix: the URL is authoritative again; req.params is used only to BIAS the backtracking order (try the params-named branch first), so URL structure decides what matches while ambiguous adjacent optionals still resolve to the param Express set. When req.params names none of the route's optionals, the matcher keeps Express's greedy present-first order. Removes the dead optionalParamNames/hasGroupNeedingUrl machinery. MED — Express 4 dialect fidelity: parseName accepts only [A-Za-z0-9_] (no quoted/$/Unicode names) and a `[` char-class string-pattern is rejected under isV5=false. LOW — consumeParens skips escaped chars, so a constraint like :id(foo\)) is accepted. LOW — the wildcard-prefix regex is cached per segment instead of rebuilt per request. Verified: mergeParams recovery, shared-name, greedy-empty-params, and the constraint disambiguation cases all correct; ReDoS still ~0.1ms. Adds a regression test per finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): round-6 review fixes for normalized-route - #3 (regression from round 5): constraintMatchesSlash no longer treats a '/' that appears only inside a character class as slash-spanning. `[^/]+` denies slashes, so /:id([^/]+)/users (Express 4) normalizes to /{id}/users again instead of null. - #2: a non-terminal catch-all (a wildcard with a non-empty segment after it, incl. an optional {/*rest}/tail) is rejected at compile (→ null) rather than silently dropped. - #1: a segment containing two independent optional groups (e.g. :a{.:b}{-:c}) is rejected (→ null); our single per-segment regex can't replicate path-to-regexp's ordered-alternative assignment, so the combined name could be wrong. Single intra-segment optionals (:id{.:format}) are unaffected. - #4: structural-only optional groups — a {...} wrapping only nested group(s) with no segment/token of its own (e.g. the outer braces in /a{{/b}}) — are collapsed by reparenting represented groups to their nearest represented ancestor, so /a{{/b}} resolves to /a/b | /a instead of always /a. Adds a regression test per finding (verified against live Express 5). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(appsec): normalize Express routes via path-to-regexp parse() Rebuild the RFC-1103 normalized-route computation on top of path-to-regexp v8's own parse() token tree instead of a hand-written route tokenizer. Reusing the framework parser removes ~250 lines of grammar code and the whole class of corner-case parsing bugs, and keeps us from drifting away from path-to-regexp's semantics. This is Express 5 only: path-to-regexp v8 is the parser Express 5 ships and the one that exposes parse(). Express 4 ships path-to-regexp 0.x, which has no parse(); getParse() returns undefined there and we omit the tag. Express 4 route syntax (:id?, :id(regexp), unnamed *, :name+/:name*) is rejected by the v8 parser anyway, so a real Express 5 app cannot register it. - path-to-regexp instrumentation: expose getParse() (captures the v8 token tree adapter), mirroring the existing getCompileToRegexp(). - normalized-route: add tokensToSegments() adapter over parse().tokens; keep the proven render / URL-presence / backtracking-matcher logic and the terminal-catch-all and structural-only-group guards. Drop the custom parser, inline-constraint handling and v4/v5 dialect threading. - Newly supported: multiple independent optional groups in one URL segment (e.g. /:a{.:b}{-:c}) now combine correctly into one atomic element. - appsec/index: drop misleading inner optional chaining on the guaranteed apiSecurity.enabled boolean. - Tests: unit spec exercises the normalizer against the real v8 parser; integration spec asserts the tag is present on Express 5 and absent on Express 4. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(appsec): drop dead code left by the parse()-based rewrite The parse()-based normalizer decides the Express dialect from getParse() availability, so the framework-version plumbing added in earlier rounds is no longer read by anything. Revert it to keep production changes minimal: - express.js, express plugin tracing.js, web.js: restore to master (the expressMajor capture, the handle-channel payload field, and the setFramework frameworkVersion parameter had no remaining consumer). Also trim dead code inside normalized-route.js: - Stop exporting renderRoute / resolvePresenceFromUrl (no test imports them; keep the public surface minimal). - Remove the wildcard `zeroOrMore` field and its unreachable guard — v8's parse() never yields a required (`+`) catch-all, so it was always true. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(appsec): drop redundant comments in normalized-route Remove section-divider comments that only restated the JSDoc of the function directly below them, a cache comment that restated its variable name, and an inline comment duplicating its function's JSDoc. Tighten the path-to-regexp getParse capture comment. Keeps only comments that carry non-obvious intent the code can't. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(appsec): drop req.params biasing from the route matcher The present/absent ordering bias existed to disambiguate adjacent optional params via their inline regex constraints — an Express 4 feature the parse()-based normalizer no longer supports (path-to-regexp v8 has no inline constraints). v8 matches greedily left-to-right, so plain greedy-present-first backtracking already resolves presence exactly as Express did. Removes optionalParamInParams, segParamInParams, the matchParamsInformative module state, and the params argument threaded through matchSegments / matchSegmentHere / resolvePresenceFromUrl. No behavior change (95 unit tests unchanged); ~70 fewer lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): guard apiSecurity access in normalized-route gate config.appsec.apiSecurity can be undefined for partially-built configs, so `config?.appsec.apiSecurity.enabled` threw "Cannot read properties of undefined (reading 'enabled')" in the HTTP-end translator — an uncaught throw on every request path when appsec is enabled, breaking web-framework and instrumentation suites broadly. Restore full optional chaining. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): gate normalized route on the real API Security config flag The gate read `config.appsec.apiSecurity.enabled`, but the config exposes the flag as `config.appsec.DD_API_SECURITY_ENABLED` (the property the API Security sampler itself reads). `apiSecurity` is never a nested object on the config, so the old path was always undefined: the non-optional form threw on every request (broad CI breakage) and the optional form silently never set the tag. Use the canonical DD_API_SECURITY_ENABLED flag so the tag is emitted when API Security is on and omitted when it is off. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): cover wildcard-prefix path and drop two unreachable branches codecov/patch was 94.32% (target 95%). The gap was the static-prefixed wildcard path, which no unit or integration test exercised. Add unit cases for `/files{/:opt}/v*rest` that drive getWildcardPrefixRegex and both outcomes of the prefix check (including a backtrack past a prefix mismatch). While confirming coverage, two branches turned out to be unreachable given the code's own contracts, so remove them rather than test dead code: - compileRoute's try/catch around parse(): the getParse adapter already swallows parser throws and returns undefined, so parse() never throws here. - getSegmentMatcher's wildcard branch: matchSegmentHere routes wildcard segments to the catch-all branch before ever calling getSegmentMatcher, so it only sees static/param tokens. The remaining uncovered lines are two deliberate hot-path crash guards (surrogate-encode fallback, RegExp-construction fallback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): drop six behaviorally-duplicate normalized-route cases A per-test coverage attribution showed these six each share an identical statement+branch footprint with a sibling that already exercises the same behavior and route shape, so removing them leaves line/branch/statement coverage unchanged (307/326 stmts, 178/205 branches, 254/265 lines): - "works when params is undefined" (== the params-is-null case) - "combines two params separated by a dash" (== the ':id.:format' combine) - "normalizes /app/*splat with mount prefix" (== '/files/*rest') - "still normalizes a terminal named wildcard" (== '/files/*rest') - "handles deeply nested mount paths" (== 'includes mount prefix') - "req.params only biases ordering..." (exact dup of the greedy first-wins case; its premise is stale since the params biasing was removed) Behaviorally-distinct permutations (delimiters, char classes, present/absent branches, independent optional groups, rejected v4 syntaxes) are kept — those guard regressions coverage numbers can't see. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): reject un-representable route segments; harden + tune normalizer Addresses the multi-agent review of the normalized-route feature. Correctness: - Reject single-segment shapes the delimiter-agnostic per-segment matcher would mis-assign, rather than emit a wrong tag: a non-terminal wildcard within a segment (`/files/*path.:ext`, `/*a-*b`) and more than one intra-segment optional group (`/:a{.:b}{-:c}`, nested `/:a{.:b{.:c}-:d}`). These previously produced incorrect combinations (e.g. `/:a{.:b}{-:c}` on `/x-y.z` → `/{a+c}` instead of path-to-regexp's `/{a+b}`). Now they return null. - resolvePresenceFromParams counts a present-but-empty param value as present (`!== undefined` instead of truthiness). - Document that a root request arrives as route '' → null, intentionally matching http.route (also omitted for the empty route). Perf (optional-route matcher hot path only; common precomputed route unchanged at ~4ns/call): - Precompute a per-segment wildcardIndex instead of rescanning tokens twice per matcher step; drop the now-unused segmentWildcard helper. - Store prebuilt marker-name strings in the presence list (no per-read rebuild). - Skip the rollback-array allocation for segments with no intra-optional groups. - Split the URL path in one pass instead of split().filter(Boolean). Minimalism: - Drop the tokensToSegments/compileRoute test-only exports and their implementation-detail tests; keep normalizeRouteExpress as the single core seam (the dispatcher is covered by the express integration spec). - Note the process-global parse adapter and route-cache growth bounds. Unit coverage 96% lines; behavior verified against path-to-regexp v8's match(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): reject param/wildcard intra-segment optional groups; drop dead guards Second review round follow-up. Correctness (reject rather than mis-normalize): - A param or wildcard inside an intra-segment optional group is now rejected (returns null). Our delimiter-agnostic '[^/]+?' matcher diverged from path-to-regexp for these: '/photos/:id{.:format}' on '/photos/1..' assigns { id: '1..' } (format absent) but we emitted '/{id+format}'; and an optional group before a same-segment wildcard ('/:a{.:b}-*rest') dropped the optional's presence because the wildcard branch bypasses the segment matcher. Static-only intra-segment optional groups (e.g. '/foo{bar}') stay supported. Whole-segment optionals ('/items{/:id}', '/posts{/:id.:format}', nested '/a{/:b{/:c}}') and mandatory multi-param segments are unaffected. Dead code: - Remove renderRoute's precomputed short-circuit (unreachable: both callers pass precomputed===null) and its 'present segment after a catch-all' bail (unreachable given the compile-time non-terminal-wildcard guard); renderRoute now never returns null (return type + variants map tightened accordingly). - Fix an orphaned JSDoc block: splitPathSegments had been inserted between normalizeRouteExpress's doc comment and its definition. Verified against path-to-regexp v8 match(): rejected shapes → null, all retained shapes match. 85 unit + 101 appsec-index tests pass; 96% unit line coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): fix wildcard/multi-segment presence bugs; match literal; trim Final review-round follow-up (differential fuzz vs path-to-regexp v8 match()). Correctness: - A required terminal wildcard no longer matches zero URL segments, which had let a preceding optional be marked present. '/files{/:id}/*rest' on '/files/x' now yields '/files/{rest}' (was '/files/{id}/{rest}'). - Reject an optional group that directly spans >1 URL segment ('{/:a/:b}'): it is atomic in path-to-regexp but our matcher toggled its segments independently, so a sibling optional could steal one ('{/:a}{/:b/:c}' on '/B/C' gave '/{a}'; now null). - Match static segments against the LITERAL route text (what Express/ path-to-regexp match against the raw URL) instead of the re-encoded form; the encoded form is only for rendering. Fixes double-encoding ('/x{/a%40b}') and literal non-ASCII statics. - Reject optional trailing/interior slash groups ('/users{/}', '/items{/:id/}') and adjacent dynamic tokens with no static between ('/:a:b', '/:a*rest', which Express itself rejects at registration). Efficiency: - Strip the URL query string lazily inside normalizeRouteExpress, past the precomputed early-return (no slice on the common cached path). - Resolve the request context once in normalizeRoute (was web.root + getContext). - Store the per-segment matcher/prefix regex on the segment object instead of two module-level Maps. Dead code: - Remove getSegmentMatcher's unreachable RegExp-construction try/catch (pattern can't throw; the appsec hook already wraps the call) and groupActive's unreachable `g === undefined` guard. Verified against path-to-regexp v8 match(); 89 unit + 101 appsec-index tests pass; 98% unit line coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): reject multi-segment/prefixed-wildcard optional shapes; reuse http.route; trim Follow-up to the latest multi-agent review (differential fuzz vs path-to-regexp v8). Correctness (reject rather than mis-normalize): - Reject an optional group that spans more than one URL segment, counting a group as present in a segment when it is the segment's group OR any token's group. This now also catches a group that owns a token in one segment and a full later segment ('/a{b/:c}{/:d}' on '/a/d' gave '/ab/{c}'; now null). - Reject a wildcard that has a prefix in its segment ('v*rest', 'p{q}*rest') once the route also has optional groups: the backtracking matcher then runs and the wildcard-prefix regex can't resolve presence consistently with path-to-regexp ('/files{/:opt}/v*rest', '/a{/:id}/p{q}*rest' → null). Without optional groups the route is precomputed and a prefixed wildcard is fine. This makes getWildcardPrefixRegex dead, so it and the matcher's prefix branch are removed. Efficiency: - normalizeRoute now reuses the http.route tag (set by setRouteOrEndpointTag just before this hook) instead of re-deriving the route from context.paths, and resolves the span with a single web.root() lookup (was web.root + web.getContext). Removes a per-request join allocation for nested routers and a duplicate reconstruction of the route rule. Dead code: - Unnamed params/wildcards are impossible in path-to-regexp v8, so drop the null-name handling (typedef, the `?? null`, the `!= null` guards in renderRoute pass 2) and refresh the stale comment. - Drop renderRoute's always-true pass-1 per-token group-active check (a dynamic token can't sit in a deeper group than its segment). Doc-only: - Note that interior '//' URL segments are collapsed (a malformed-URL edge) and keep the process-global getParse note. Verified against path-to-regexp v8 match(); 89 unit + 101 appsec-index tests pass; 98% unit line coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): keep v8 parser on re-hook; support multiple static optional groups Addresses two Codex review comments. - path-to-regexp: probe `parse()` at hook time and adopt it only if it returns the v8 TokenData shape ({ tokens: [...] }). Previously, a later-loaded older major (6.x/7.x `parse()` returns a bare array) re-ran the hook and overwrote the working v8 adapter with one that always returns undefined, silently disabling normalization (and caching null) for the rest of the process. - normalized-route: allow any number of *static* intra-segment optional groups ('/a{b}{c}'). They are literal, so the named-marker segment matcher resolves their presence exactly (verified against path-to-regexp match() over all presence combinations). Only param/wildcard intra-segment optional groups — which need delimiter-aware matching we can't replicate — remain rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(appsec): drop optional chaining on the API Security gate config is guaranteed non-null when incomingHttpEndTranslator runs (enable() sets it before any event-loop turn; disable() nulls it and unsubscribes the handler in the same synchronous call), and config.appsec.DD_API_SECURITY_ENABLED is always a boolean. Address review feedback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(appsec): resolve optional-group presence via path-to-regexp match Replace the custom backtracking route matcher with path-to-regexp v8's own match() to resolve which optional groups a request filled. This reuses the framework's matching instead of re-implementing it (per review feedback) and cuts normalized-route.js from 721 to 448 lines. Behavior changes, both toward "omit rather than mis-normalize": - Static-only optional groups (/posts{/draft}, /a{b}{c}) have no capture key, so match() cannot report their presence -> the route is omitted. - An optional group sharing a param name with another token collapses to one key in match()'s output -> presence is ambiguous, so the route is omitted. - Intra-segment and multi-segment optional param groups that the old matcher rejected (/photos/:id{.:format}, /x{/:a/:b}, {/:a}{/:b/:c}) are now resolved correctly. getMatch() is added to the path-to-regexp instrumentation as a version-probed v8 match() factory, mirroring getParse(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): table-drive normalized-route spec (495 → 249 lines) Replace the one-assertion-per-it boilerplate with a check(route, url, expected, params) helper that registers one test per case, named after its inputs so a failure still names the exact route. Same coverage (95%/89%), 105 cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(appsec): trim comments in normalized-route Shorten multi-line inline comments to their non-obvious core and drop pure narration; keep the RFC-rules module doc and JSDoc. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): drop static-only optional groups instead of omitting the route A static-only optional group (/posts{/draft}, /a{b}{c}, a bare optional slash /users{/}) carries no param for match() to resolve. Rather than omit the whole route, render it as absent — a stable, minimal normalized route. This also rescues mixed routes: /a{/:id}/p{q}*rest now yields /a/{id}/{rest} (static group dropped, param resolved) instead of null. Only a param group whose presence is genuinely ambiguous (a shadowed name) still omits the route. Also lower MAX_OPTIONAL_GROUPS from 12 to 8: path-to-regexp's match() builds a regexp exponential in the optional-group count (first-call ~3s at 11 groups), which timed out the many-optionals guard test on CI. At 8 the route is omitted before a matcher is ever built. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): drop 5 coverage-redundant normalized-route cases Remove cases that exercise a branch already covered by another: undefined/42 inputs (same non-string guard as null), two ASCII-encode statics (covered by the dedicated encoding describe), and :path+ (same :name-modifier reject as :path*). Coverage unchanged (95.7%/89.4%, identical uncovered lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): keep mandatory static that shares a segment with an optional group Two correctness fixes found in review: 1. A group-0 (mandatory) static token can share a URL segment with an optional group when no top-level slash separates them (e.g. '.json' in '/files{/:id}.json', or the 'y' in '/x{/a}{/b}y'). renderRoute gated each output element on the segment's leading-slash group, so when that group was absent the whole segment — including the mandatory static — was dropped ('/files.json' rendered '/files'). renderRoute now flushes an element only at a present leading slash and merges an absent-slash segment's still-present tokens into the current element. 2. A group whose presence is detectable only via a param in a NESTED optional subgroup (e.g. '/a{/b{/:c}}') mis-rendered '/a/b' as '/a'. Detectability now requires a unique param the group holds directly, so such routes are omitted rather than mis-normalized. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): cap total optional groups and tie match adapter to v8 parse Two issues surfaced by a Codex review round: 1. The exponential-regexp guard counted only resolvable (detectable) groups, but path-to-regexp's match() regexp is exponential in the route's TOTAL optional-group count. A route like /r{a}...{t}{/:id} (20 static + 1 param) passed the cap (1 detectable) yet took ~4s to build. Cap on the total group count (groupParent.size) before building the matcher instead. Static-only routes with no resolvable group still precompute cheaply (no matcher). 2. The path-to-regexp match() probe (`probe?.params`) can't distinguish v8 from v6/v7 (all share the { params } shape), so a later-loaded older major could clobber the v8 matcher. Capture parse() and match() together, gated on the same v8 TokenData probe, so only a confirmed-v8 module installs either. Also init the variants Map when building the matcher entry (drop the per-request lazy check; precomputed entries never reach it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(appsec): skip param decoding and gate normalized route on the v8 dialect Review feedback from BridgeAR: - match(route, { decode: false }): presence resolution reads which params matched, never their values, so per-param decodeURIComponent is waste. ~55% faster per match() call (218ns -> 99ns; 320ns -> 194ns per request). It also stops a malformed escape ('/x/%ZZ') from throwing URIError inside the matcher, which read as "no match" and silently degraded presence resolution to the req.params fallback on attacker-controlled input. - Expose the Express route grammar and require it to be v8. Previously a loaded v8 path-to-regexp stood in for "this is Express 5", which is unsound twice over: an Express 4 app can pull v8 in through an unrelated dependency (path-to-regexp is hooked for any requirer), and a process can serve both majors (see express-multi-version.spec.js). The grammars disagree — '/a{2}' is a regex quantifier matching '/aa' in v4 but an optional group in v8 — so v4 routes were tagged '/a'. getExpressRouteDialect() now reports 'v8' | 'legacy' | 'mixed' | undefined and only 'v8' is normalized, so a v4 or mixed process emits no tag rather than a wrong one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): only let path-to-regexp 8 install the parse/match adapters IlyasShabi spotted that 7.x also returns TokenData, so the `Array.isArray( probe.tokens)` shape probe accepted it. Its tokens are bare strings rather than typed nodes, so tokensToSegments matched no branch, produced no segments, and every route normalized to '/' — a wrong tag on every request in any Express 5 app that also had path-to-regexp 7 in its tree. Gate the capture on `versions: ['>=8']` instead, per their suggestion. Version matching is the loader's job, so the probe and its try/catch are gone. Added a regression spec that drives the registered hooks through the loader's own semifies matching and asserts 7.x cannot install or replace the adapters (verified failing against the previous ['*'] registration). Also rename the route-dialect values to 'express5' | 'express4' | 'mixed' and drop "v8" from prose: it read as the V8 JS engine rather than path-to-regexp 8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * revert(appsec): drop the process-global Express-5 route dialect gate The gate refused to normalize in any process where both Express majors were loaded. That is sound in principle, but a process-global flag is set by any express require anywhere in the process — including mocha's collection phase — so in a shared test process every Express 5 request saw 'mixed' and lost the tag. AppSec/express was green at d81decb and failed from e52d102 for exactly this reason. Reverting to the parser-availability check restores that behaviour and leaves the known gap documented in place: an Express 4 app that pulls path-to-regexp 8 in through a dependency is read with Express 5 grammar. Closing it properly needs the dialect of the router that recorded the route, resolved per request (datadog-plugin-router's web.setRoute call site knows it), not a global flag. Keeps the two independent fixes: the >=8 hook gating and match({decode:false}). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): classify express major by installed version, not the range withVersions can hand the spec a range ('>=4') that intersects both majors while the folder actually installs express 5, so semver.intersects(version, '<5.0.0') reported express 4 and the app registered the wrong route syntax — the server then 404'd on '/tree/main'. Resolve the installed version with .version() and classify with satisfies(), as the next/mysql2 specs do. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: exercise the path-to-regexp instrumentation spec verify-exercised-tests failed because no workflow glob reached the new packages/datadog-instrumentations/test/path-to-regexp.spec.js. Add it to the router job, whose dependency it is, rather than spend a runner on two tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): move the path-to-regexp spec under the service-free misc glob The previous placement needed a PLUGINS entry to be exercised, and adding one made install_plugin_modules demand a version range ("Latest version for 'path-to-regexp' needs to be defined in versions/package.json"), provisioning module versions this spec never loads — it fabricates 7.x/8.x shaped modules. test:instrumentations:misc globs test/*/**/*.spec.js and runs without yarn services, which is what a pure unit test wants. Reverts the workflow edit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): mirror decode:false in the spec matcher, drop "v8" wording Review feedback from IlyasShabi: the spec's makeMatcher mirror had drifted from the instrumentation adapter, which now passes { decode: false }, and "v8" reads as the V8 engine rather than path-to-regexp 8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(appsec): pass client:false to the http plugin, not express `client` is an http-plugin option (datadog-plugin-http/src/index.js:29), so in the positional config array it was landing on express while http got {} — client spans were never actually disabled. Spotted by IlyasShabi. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(appsec): decide the Express major per request, not per process AppSec/express went red once the client:false fix landed. That fix was correct: the http client spans it had been leaving enabled were satisfying the Express 4 "must not set the tag" assertion on their own, so it had been passing vacuously. With the mask gone, the real defect showed: the parse/match adapters are process-wide, so an Express 4 block running after an Express 5 one gets its routes read with Express 5 grammar. In CI the >=4 folder resolves to 5.2.1 and runs third, so the 4.2.0 and 4.3.0 blocks after it were tagged. Decide the major per request from the serving app instead, via the legacy app.del alias Express 5 removed. Verified against every provisioned version (4.0.0-4.22.2 keep it, 5.0.0/5.2.1 do not). Being per-request, this also covers a v4 sub-app mounted inside a v5 process, which no process-wide flag can. The full fix remains the router's own dialect at the web.setRoute call site, resolved per request; this is the small version of it, kept local to appsec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 7a37622 commit b9b4e0b

6 files changed

Lines changed: 1121 additions & 1 deletion

File tree

packages/datadog-instrumentations/src/path-to-regexp.js

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@ const { addHook } = require('./helpers/instrument')
55
/** @type {((pattern: string | RegExp) => RegExp | undefined) | undefined} */
66
let compileToRegexp
77

8+
/** @type {((pattern: string) => { tokens: object[] } | undefined) | undefined} */
9+
let parseTokens
10+
11+
/** @type {((route: string) => ((url: string) => object | undefined) | undefined) | undefined} */
12+
let makeMatcher
13+
814
addHook({ name: 'path-to-regexp', versions: ['*'] }, moduleExports => {
915
// 0.1.x and 6.x: `module.exports = (path, ...) => RegExp`.
1016
// 7.x: `module.exports = { pathToRegexp(path, ...) => RegExp }`.
@@ -29,6 +35,47 @@ addHook({ name: 'path-to-regexp', versions: ['*'] }, moduleExports => {
2935
return moduleExports
3036
})
3137

38+
// 8.x only: earlier majors also expose `parse`/`match`, but with incompatible shapes — 7.x returns
39+
// `{ tokens }` too, yet its tokens are bare strings rather than typed nodes. Gating on the version
40+
// keeps those from installing adapters the consumers would misread.
41+
addHook({ name: 'path-to-regexp', versions: ['>=8'] }, moduleExports => {
42+
const { parse, match } = moduleExports
43+
44+
if (typeof parse === 'function' && typeof match === 'function') {
45+
parseTokens = pattern => {
46+
let result
47+
try {
48+
result = parse(pattern)
49+
} catch {
50+
return
51+
}
52+
if (Array.isArray(result?.tokens)) return result
53+
}
54+
55+
makeMatcher = route => {
56+
let matcher
57+
try {
58+
// Callers read which params matched, never their values: skipping decode is faster and
59+
// stops a malformed escape ('%ZZ') from throwing URIError, which would read as "no match".
60+
matcher = match(route, { decode: false })
61+
} catch {
62+
return
63+
}
64+
return url => {
65+
let result
66+
try {
67+
result = matcher(url)
68+
} catch {
69+
return
70+
}
71+
return result ? result.params : undefined
72+
}
73+
}
74+
}
75+
76+
return moduleExports
77+
})
78+
3279
/**
3380
* Returns whatever path-to-regexp compile adapter the host most recently
3481
* loaded. Capture this once at addHook fire time so each express/router
@@ -41,4 +88,23 @@ function getCompileToRegexp () {
4188
return compileToRegexp
4289
}
4390

44-
module.exports = { getCompileToRegexp }
91+
/**
92+
* Returns the host's path-to-regexp `parse` adapter (8.x token tree), or `undefined` when the
93+
* host has not loaded an 8.x path-to-regexp (e.g. Express 4 ships 0.1.x, which has no `parse`).
94+
* @returns {((pattern: string) => { tokens: object[] } | undefined) | undefined}
95+
*/
96+
function getParse () {
97+
return parseTokens
98+
}
99+
100+
/**
101+
* Returns the host's path-to-regexp 8.x `match` adapter: a factory `route => (url => params)` that
102+
* compiles a route once and returns a matcher yielding the captured params (or undefined on no
103+
* match / unusable route). `undefined` when the host has not loaded an 8.x path-to-regexp.
104+
* @returns {((route: string) => ((url: string) => object | undefined) | undefined) | undefined}
105+
*/
106+
function getMatch () {
107+
return makeMatcher
108+
}
109+
110+
module.exports = { getCompileToRegexp, getParse, getMatch }
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
5+
const { describe, it } = require('mocha')
6+
7+
const satisfies = require('../../../../vendor/dist/semifies')
8+
const instrumentations = require('../../src/helpers/instrumentations')
9+
const { getParse, getMatch } = require('../../src/path-to-regexp')
10+
11+
// Run the hooks the loader would run for `version`, exactly as register.js selects them.
12+
function applyHooksFor (version, moduleExports) {
13+
for (const { versions, hook } of instrumentations['path-to-regexp']) {
14+
if (!versions || versions.some(range => satisfies(version, range))) hook(moduleExports)
15+
}
16+
}
17+
18+
// 7.x `parse()` also returns `{ tokens }`, but its tokens are bare strings rather than typed nodes,
19+
// so adopting it would make every route normalize to '/'. Only 8.x may install the adapters.
20+
const v7Module = {
21+
parse: () => ({ tokens: ['/a/', { name: 'id' }], delimiter: '/' }),
22+
match: () => () => ({ params: {} }),
23+
pathToRegexp: () => ({ regexp: /^\/a\/([^/]+)$/, keys: [] }),
24+
}
25+
26+
const v8Module = {
27+
parse: pattern => ({ tokens: [{ type: 'text', value: pattern }] }),
28+
match: () => () => ({ params: { id: '1' } }),
29+
pathToRegexp: () => ({ regexp: /^\/a\/([^/]+)$/, keys: [] }),
30+
}
31+
32+
describe('path-to-regexp instrumentation', () => {
33+
it('does not let 7.x install the parse/match adapters', () => {
34+
const parseBefore = getParse()
35+
const matchBefore = getMatch()
36+
37+
applyHooksFor('7.2.0', v7Module)
38+
39+
assert.equal(getParse(), parseBefore)
40+
assert.equal(getMatch(), matchBefore)
41+
})
42+
43+
it('installs the adapters for 8.x', () => {
44+
applyHooksFor('8.4.2', v8Module)
45+
46+
assert.equal(typeof getParse(), 'function')
47+
assert.equal(typeof getMatch(), 'function')
48+
assert.deepStrictEqual(getParse()('/x'), { tokens: [{ type: 'text', value: '/x' }] })
49+
assert.deepStrictEqual(getMatch()('/a/:id')('/a/1'), { id: '1' })
50+
})
51+
})

0 commit comments

Comments
 (0)