chore(asm): add normalized HTTP route span tag for Django#18209
Conversation
|
Codeowners resolved as |
BenchmarksBenchmark execution time: 2026-05-28 13:15:26 Comparing candidate commit 83224a0 in PR branch Found 0 performance improvements and 4 performance regressions! Performance is the same for 465 metrics, 9 unstable metrics. scenario:iast_aspects-re_search_aspect
scenario:iastaspectsospath-ospathbasename_aspect
scenario:span-start
scenario:telemetryaddmetric-1-count-metric-1-times
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b561bc27f
ℹ️ 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".
Extends RFC-1103 ``_dd.appsec.normalized_route`` to Django request spans, following the FastAPI/Starlette implementation. Handles ``path()`` converters (``<int:id>``, ``<path:file_path>``, ...), ``re_path()`` regex with named ``(?P<name>...)`` and unnamed ``(...)`` captures, and ``include()`` joins. Optional regex groups that didn't match are dropped per-request (rule 6); unnamed groups emit auto-numbered ``paramN`` placeholders (rule 4) with collision avoidance against converter-supplied names. A regex fast path matches the FastAPI/Starlette implementation's ~85 ns warm-path cost on common ``path()``-only routes; the slow path is ~1.5 µs warm for regex routes after caching. Plumbing: shared ``ddtrace.contrib.internal.django.utils._request_path_params`` returns ``resolver_match.kwargs or resolver_match.args or None`` across the three Django ``set_http_meta`` callsites, falling back to a fresh ``resolver.resolve(request.path_info)`` when ``request.resolver_match`` isn't yet set. Wrapped in a broad try/except + debug log so a raising third-party ``ResolverMatch`` can't break request tagging. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Per PR review: ``/?`` strip is now gated on ``is_regex`` so ``path("foo/?", view)``
(which Django escapes the ``?`` as literal via ``_route_to_regex``) doesn't get
its trailing literal collapsed onto ``path("foo", view)``'s normalized tag.
Char-class scanner now skips a leading ``^]`` / ``]`` literal so ``[]a)b]`` is
correctly recognized as one class instead of an empty class plus stray ``)``
that bumped ``capture_idx`` past Python re's actual group count.
Both bugs are exotic in real Django URLconf (literal ``?`` in ``path()`` and
``]`` as first char in ``[...]`` regex routes) but the fixes are localized.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
``traced_get_response_async`` calls ``_after_request_tags`` directly and never dispatches ``django.finalize_response.pre`` — which is the only sync-path hook that forwarded ``route=`` to ``set_http_meta``. The AppSec normalized-route listener short-circuits on missing ``route``, so Django served via ASGI was silently missing ``_dd.appsec.normalized_route``. Have ``_on_django_after_request_headers_post`` (the hook both sync and async paths fire) read ``http.route`` from the span — ``_set_resolver_tags`` already set it earlier in ``_after_request_tags`` — and forward it as ``route=``. Sync requests fire the listener twice with the same value (idempotent). Added a focused unit test for the listener's route forwarding. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
c2bb2cb to
b939ff0
Compare
…compute The ASGI route forwarding (c2bb2cb) makes ``_on_django_after_request_headers_post`` also fire the AppSec normalized-route listener, which on Django's sync path collides with the existing ``finalize_response.pre`` dispatch. Add a per-request ``normalized_route_emitted`` flag on ``ASM_Environment`` so the second dispatch short-circuits before re-running the normalizer. Also gate the listener on an active ASM context — without one, appsec/api-security is inactive for the request and there's nothing to do. The four ASM snapshot tests in ``test_django_appsec_snapshots.py`` hit ``/``, which matches Django's ``^$`` route; ``normalize_route`` maps that to ``/``. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
No circular-import constraint (``_asm_request_context`` doesn't import from ``_handlers``); the lazy form was inherited from ``_on_set_http_meta`` but isn't needed here. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Address review feedback: the unit-level `_on_django_after_request_headers_post` regression test relied on extensive mocking. Replace it with an end-to-end test that drives the dd-trace-wrapped `get_asgi_application()` directly, exercising the production TraceMiddleware → ASGIHandler → traced_get_response_async → _after_request_tags pipeline against a parameterized route. Also drop an AI-flavored docstring on `_django_request_path_params`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ormalized-route-for-django
Django's ``ASGIHandler.handle`` races ``listen_for_disconnect(receive)`` against ``process_request`` via ``asyncio.wait(..., return_when=FIRST_COMPLETED)``. Returning http.disconnect on the second receive lets the disconnect listener win, which cancels process_request before it can call send(), leaving the response unsent and the test asserting against an empty message list. Hang instead so process_request wins; Django cancels the disconnect awaiter once the response has been sent. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
``_taint_django_func_call`` unconditionally accessed ``http_req.environ``, which only exists on WSGI ``HttpRequest``. Django's ``ASGIRequest`` builds an equivalent ``META`` dict from the ASGI scope and does not expose ``environ``, so the access raised ``AttributeError`` and turned every ASGI-served request through Django's middleware chain into a 500. Wrap the assignment in a try/except matching the existing pattern used for ``http_req._body`` / ``http_req.body`` a few lines above; ``META`` tainting (next line) still runs on both transports. Surfaced by ``test_normalized_route_asgi``, which drives the TraceMiddleware-wrapped ``get_asgi_application()`` against a real Django ASGI handler. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
bd27a71
into
main
Extends RFC-1103 ``_dd.appsec.normalized_route`` to Flask, mirroring the FastAPI/Starlette (#17920) and Django (#18209) implementations. - ``ddtrace/appsec/_api_security/_normalized_route.py``: adds ``normalize_route_flask`` with a fast path (single-converter params, no catch-all) and a slow path covering multi-param-in-segment, ``<path:name>`` catch-all, ``any(...)`` converters, and static URL-encoding. - ``ddtrace/appsec/_handlers.py``: adds ``"flask"`` to ``_NORMALIZED_ROUTE_BY_INTEGRATION``; resolves the DispatcherMiddleware mount prefix from ``flask.resource.full`` so sub-app routes include their full assembled path. - Test app (flat + subapp): adds ``/multi-param/<first>.<last>/`` and ``/files/<path:file_path>`` routes. - ``tests/appsec/contrib_appsec/utils.py``: removes Flask from ``xfail_interface`` on the two normalized-route tests. - Unit tests: 50 new parametrized cases for ``normalize_route_flask``, fast-path eligibility, and invalid-input rejection. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
Extends RFC-1103 ``_dd.appsec.normalized_route`` to Flask, mirroring the FastAPI/Starlette (#17920) and Django (#18209) implementations. - ``ddtrace/appsec/_api_security/_normalized_route.py``: adds ``normalize_route_flask`` with a fast path (single-converter params, no catch-all) and a slow path covering multi-param-in-segment, ``<path:name>`` catch-all, ``any(...)`` converters, and static URL-encoding. - ``ddtrace/appsec/_handlers.py``: adds ``"flask"`` to ``_NORMALIZED_ROUTE_BY_INTEGRATION``; resolves the DispatcherMiddleware mount prefix from ``flask.resource.full`` so sub-app routes include their full assembled path. - Test app (flat + subapp): adds ``/multi-param/<first>.<last>/`` and ``/files/<path:file_path>`` routes. - ``tests/appsec/contrib_appsec/utils.py``: removes Flask from ``xfail_interface`` on the two normalized-route tests. - Unit tests: 50 new parametrized cases for ``normalize_route_flask``, fast-path eligibility, and invalid-input rejection. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
Extends RFC-1103 ``_dd.appsec.normalized_route`` to Flask, mirroring the FastAPI/Starlette (#17920) and Django (#18209) implementations. - ``ddtrace/appsec/_api_security/_normalized_route.py``: adds ``normalize_route_flask`` with a fast path (single-converter params, no catch-all) and a slow path covering multi-param-in-segment, ``<path:name>`` catch-all, ``any(...)`` converters, and static URL-encoding. - ``ddtrace/appsec/_handlers.py``: adds ``"flask"`` to ``_NORMALIZED_ROUTE_BY_INTEGRATION``; resolves the DispatcherMiddleware mount prefix from ``flask.resource.full`` so sub-app routes include their full assembled path. - Test app (flat + subapp): adds ``/multi-param/<first>.<last>/`` and ``/files/<path:file_path>`` routes. - ``tests/appsec/contrib_appsec/utils.py``: removes Flask from ``xfail_interface`` on the two normalized-route tests. - Unit tests: 50 new parametrized cases for ``normalize_route_flask``, fast-path eligibility, and invalid-input rejection. Co-Authored-By: Claude Sonnet 4.6 (1M context) <[email protected]>
APPSEC-65477 ## Description Extends RFC-1103 `_dd.appsec.normalized_route` to Flask, following the FastAPI/Starlette (#17920) and Django (#18209) implementations. - **Normalizer** (`ddtrace/appsec/_api_security/_normalized_route.py`): `normalize_route_flask()` with fast path (single-converter params) and slow path (multi-param segments, `<path:name>` catch-all, `any()` converters, static URL-encoding). - **Handler** (`ddtrace/appsec/_handlers.py`): adds `"flask"` to `_NORMALIZED_ROUTE_BY_INTEGRATION`; resolves `DispatcherMiddleware` mount prefix from `FLASK_RESOURCE_FULL` so sub-app routes include their full assembled path. ## Testing - 56 new unit test cases for `normalize_route_flask` (fast path, slow path, invalid inputs, DM assembly, edge cases). - Integration tests un-xfailed for Flask on `test_normalized_route` and `test_normalized_route_disabled_when_api_security_off`. - Covers both flat-app (Blueprint) and subapp (DispatcherMiddleware) interface variants. ## Risks None — all production code is in `ddtrace/appsec/`. No changes to integration code or public API. ## Additional Notes Non-terminal `<path:...>` routes (e.g. `/<path:wiki>/edit`, valid in Werkzeug) return `None` — omit rather than guess, per RFC-1103 rule 5 which mandates catch-alls be terminal. Co-authored-by: christophe.papazian <[email protected]>
## Description Extends RFC-1103 `_dd.appsec.normalized_route` to Tornado, following the FastAPI/Starlette (#17920), Django (#18209), and Flask (#18343) implementations. Tornado's ddtrace integration produces `http.route` strings with `%s` as a placeholder for every capturing group (via `_regex_to_route`). The new normalizer maps those placeholders to parameter names sourced from `path_params`: a dict for named groups (`(?P<name>...)`), a list for positional groups, or `{}` for static routes. Optional trailing-slash patterns (`/?`) are treated as not declaring a trailing slash per RFC-1103 rule 1, consistent with the Django `^asm/?$` → `/asm` convention. **Changes:** - **`_normalized_route.py`** — adds `_normalize_route_tornado_cached` (LRU-cached, keyed on route + param-names tuple) and `normalize_route_tornado` (public entry point). Handles `%s` placeholder mapping, `/?` stripping, multi-param-in-segment combining with `+` (rule 5), and static-segment URL-encoding (rule 3). - **`_handlers.py`** — registers `"tornado": normalize_route_tornado` in `_NORMALIZED_ROUTE_BY_INTEGRATION`. - **`tornado_app/app.py`** — converts the `/asm/` route from positional to named capturing groups so `path_params` arrives as a dict with proper parameter names; adds `MultiParamHandler` (`/multi-param/`) and `FilesHandler` (`/files/`) for integration test coverage. - **`test_tornado.py`** — overrides `test_normalized_route` and `test_normalized_route_disabled_when_api_security_off` with Tornado-specific expected values (no trailing slash for `/?` routes); expands `ENDPOINT_DISCOVERY_EXPECTED_PATHS`. - **`test_normalized_route.py`** — 27 new unit tests for `normalize_route_tornado` covering all code paths. - **`utils.py`** — documents why Tornado is skipped in `test_normalized_route_survives_request_span_name_override` (span name is hard-coded via `schematize_url_operation`). ## Testing - 27 unit tests in `tests/appsec/appsec/api_security/test_normalized_route.py` covering named groups, positional groups, multi-param combining (`+`), `/?` stripping, explicit trailing-slash preservation, URL-encoding, mismatch detection, and `lru_cache` identity. - Integration tests in `test_tornado.py` exercise the full request path: named-group params (`/asm/{param_int}/{param_str}`), multi-param-in-segment (`/multi-param/{first+last}`), catch-all (`/files/{file_path}`), root (`/`), ASM-disabled gate, and API-Security-disabled gate. - `test_normalized_route_disabled_when_api_security_off` verifies the tag is absent when API Security is off while ASM is on. ## Risks None — all production code is confined to `ddtrace/appsec/`. No changes to integration code or public API. The `/asm/` route change (positional → named groups) is backward-compatible: Tornado passes named groups as keyword arguments that match the existing handler signature. ## Additional Notes Tornado routes using positional groups (no `(?P<name>...)`) produce auto-numbered placeholders (`{param1}`, `{param2}`, …). Users who want named parameters in `_dd.appsec.normalized_route` should use named capturing groups in their route patterns. Co-authored-by: christophe.papazian <[email protected]>
APPSEC-65476
Extends RFC-1103
_dd.appsec.normalized_routeto Django, mirroring the FastAPI/Starlette PR (#17920): emits the tag on every Django request span carryinghttp.routewhen API Security is active, on both sync (WSGI) and async (ASGI) request paths.Coverage
path()converters (<int:id>,<path:file_path>,<str:name>,<name>).re_path()regex with named(?P<name>...)and unnamed(...)captures.include()joins (parent route assembled with child route).+, catch-all tail.resolver_match.kwargs/args.paramNplaceholders; pre-scan reserves anyparamKtaken by a named group OR apath()converter so they never collide.Routes that can't be safely normalized return
None(omit-rather-than-guess): top-level(?:...), lookarounds, inline-flag groups, comments, backreferences, conditionals,{n,m}quantifiers on regex routes, character classes outside named groups, and any capture spanning/.Implementation
ddtrace/appsec/_api_security/_normalized_route.py:normalize_route_django(route, path_params). Regex fast path (singlere.sub) for the commonpath()-only shape — ~80 ns warm-path cost, parity with the existing Starlette/FastAPI fast path. Slow path tokenizes per segment with capture-index tracking matching Pythonre's group numbering.ddtrace/appsec/_handlers.py:_NORMALIZED_ROUTE_BY_INTEGRATIONdispatch table replaces the hard-coded allowlist.ddtrace.contrib.internal.django.utils._request_path_params(request)resolves the polymorphickwargs or args or Noneshape across all Djangoset_http_metacallsites, with a fresh-resolve fallback for pre-view hooks. Wrapped in try/except + debug log so a raising third-partyResolverMatchcan't break tagging.🤖 Generated with Claude Code