refactor(asm): remove RASP wrapper frames from customer tracebacks - #19989
Open
avara1986 wants to merge 11 commits into
Open
refactor(asm): remove RASP wrapper frames from customer tracebacks#19989avara1986 wants to merge 11 commits into
avara1986 wants to merge 11 commits into
Conversation
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 4a056d7 | Docs | View more details | Give us feedback! |
avara1986
force-pushed
the
alberto.vara/APPSEC-69877-rasp-wrapping-context
branch
from
September 1, 2026 16:06
bea10c1 to
86379c9
Compare
BenchmarksBenchmark execution time: 2026-09-04 15:34:26 Comparing candidate commit 4a056d7 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 82 metrics, 0 unstable metrics.
|
avara1986
changed the title
avara1986
marked this pull request as ready for review
September 2, 2026 07:27
avara1986
requested review from
christophe-papazian
and removed request for
a team
September 2, 2026 07:27
avara1986
marked this pull request as draft
September 2, 2026 07:28
Base automatically changed from
alberto.vara/APPSEC-69951-remove-raise-without-wrapper-frame
to
main
September 2, 2026 09:06
…C-69877] The RASP hooks use wrapt, so the wrapper is a Python function with its own stack frame. That frame lands in the traceback of any ordinary application exception passing through it, and crash intake tags a report crash_datadog as soon as a ddtrace path appears in the frames. Datadog gets blamed for customer bugs. WrappingContext rewrites the target's own bytecode into a with-block around its body, so __enter__ has returned before the body runs and no ddtrace frame is left behind. Measured on the reported reproduction: before open -> wrapped_open_ED4CF71136E15EBF@ddtrace -> urlopen after open -> urlopen Note that ddtrace.internal.wrapping.wrap does not help here: the wrapper still owns a frame and it adds a trampoline frame on top. Adds try_wrap_context / try_unwrap_context alongside try_wrap_function_wrapper, reusing the same ModuleWatchdog bookkeeping so unpatch stays symmetric. They guard on is_wrapped because registering the same context type twice raises, where wrapt's apply_patch silently no-ops. Converts urllib.request.OpenerDirector.open to _SsrfOpenerDirectorOpen. The core context that used to be a with-block spanning the call is now opened in __enter__ and released from both __return__ and __exit__, which together cover the return and error paths; it must be released before a response-side block raises, because a raising __return__ suppresses __exit__. Arguments are read by name via _arg, replacing the args/kwargs index juggling; _arg also falls back to a **kwargs bag, which the urllib3 hooks will need for urllib3 v1. The remaining hooks in this module keep using wrapt and are migrated in follow-ups. builtins.open and os.system are C functions with no code object, so they cannot be bytecode-wrapped at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…EC-69877]
Converts the two http.client hooks to wrapping contexts so no ddtrace frame
is left in the traceback of application exceptions passing through them:
wrapped_request_A7F2C6E4D3B10958 -> _SsrfHttpConnectionRequest (__enter__)
wrapped_response -> _SsrfHttpConnectionGetresponse (__return__)
The crop anchor for the SSRF_REQ stack moves from the wrapper name to the
wrapped function's own name. A context runs inside the target's frame, so
there is no wrapper frame left to crop at; anchoring on the target keeps
report_stack cropping at the same place, and frames[0] stays the target's
caller. Dropping the anchor instead would have shifted the reported stack by
one frame, because _INTERNAL_FRAMES does not cover the stdlib.
Also fixes a real defect in try_wrap_context introduced with the urllib hook.
A contrib integration may already hold a wrapt wrapper on the attribute, and
binding the context to that proxy is unrecoverable:
- getattr(cls, "method") builds a fresh BoundFunctionWrapper on every access,
so the registration can never be found again;
- isinstance(proxy, FunctionType) is True because wrapt forwards __class__,
so the "peel until it is a function" loop never peeled.
unwrap() then silently no-opped while the code object stayed rewritten, and the
next patch rewrote on top of it, growing the code every patch/unpatch cycle
(request 42 -> 94 bytes) until the bytecode library raised KeyError while
parsing - surfacing as failures in unrelated tests far from the cause. Resolve
through the owner's __dict__ with a type() check, and unwrap the retained
context instance rather than re-resolving the attribute. Wrap failures are now
swallowed too: losing a RASP hook is acceptable, breaking patching is not.
The urllib3 hooks stay on wrapt. contrib/internal/urllib3/patch.py installs the
same appsec wrappers on the same targets, deduplicated today only by
apply_patch's FunctionWrapper guard, which a wrapping context bypasses; moving
them would double-invoke the WAF. Scoped separately.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
avara1986
force-pushed
the
alberto.vara/APPSEC-69877-rasp-wrapping-context
branch
from
September 2, 2026 09:28
86379c9 to
62a53e5
Compare
Reviewer feedback: keep the migrated logic as close to the current code as possible so the diff reads as a move. There will be time to refactor later. _SsrfHttpConnectionRequest.__enter__ keeps the original guard nesting and the method/body/headers locals in their original order. The only differences from wrapped_request_A7F2C6E4D3B10958 are the three argument reads going through _arg, the crop anchor, and returning self. Same treatment for _SsrfOpenerDirectorOpen: the _analyze_response and _analyze_http_error helpers are inlined back into __return__ and __exit__, so those bodies read like the original try/except block. A cleanup pass over the migration then removed what it had accumulated: - _RaspContext._VARKWARGS and the varkwargs branch in _arg. It existed for urllib3 v1, where _make_request keeps body/headers in **httplib_request_kw, but the urllib3 hooks are exactly what this change does not migrate. None of the three migrated targets has a **kwargs parameter, so the branch was unreachable. _arg is now a default-tolerant f_locals read; the fallback comes back with the urllib3 work, alongside tests that exercise it. - The rasp_active flag, which was a second encoding of core_ctx is not None. Replaced by a _rasp_active() predicate over the single source of truth. - _SsrfHttpConnectionGetresponse's _RaspContext base. It inspects only the return value and used neither _arg nor the core context, but inherited an __enter__ that wrote two storage keys nobody read on every getresponse call. Tests: the core-context release test asserted nothing, because RASP is inactive in that suite so __enter__ never opened a context. It now drives the lifecycle directly and checks the full_url item appears and then disappears on both the return and the error path; neutering _close_core_context fails it. The httplib coexistence probe uses a plain nested subclass instead of a dynamically built class, which also drops a super(type(self), self) that would have recursed infinitely had it ever been subclassed. Also merges the two release notes into one, since both hooks are the same customer-visible fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
avara1986
force-pushed
the
alberto.vara/APPSEC-69877-rasp-wrapping-context
branch
from
September 2, 2026 10:09
62a53e5 to
616798a
Compare
Member
Author
|
@codex review |
![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: 616798a16e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… hooks The migration PR had no coverage for the paths most likely to regress silently. Adds nine tests, each verified to fail when the behaviour it guards is broken: - BlockingException must stay BaseException-derived. A registered context's __enter__ runs inside `except Exception: continue`, so a block deriving from Exception would be swallowed and the request would proceed. Nothing else would catch that change. - A SSRF_REQ block still propagates out of the migrated http.client hook, and the WAF call carries crop_trace="request". The crop anchor moved from the wrapper name to the wrapped function's name and had no direct test. - The API10 down-response WAF call, which used to sit in a `with` block spanning the call and now lives in __return__, and its HTTPError counterpart in __exit__. Neither was covered. - A 3xx response must not be inspected here, since getresponse already reports it; otherwise the WAF is called twice per redirect. - http.client tracebacks carry no ddtrace frame, the same guarantee already asserted for urlopen. The httplib integration is unpatched in that test so it measures appsec's hook alone. - A failing hook must not surface to the customer, and a wrap failure must not break patching. Mutation-checked: removing the block, leaving the stale crop anchor, dropping either API10 call, dropping the 3xx guard, or narrowing the wrap except each make the corresponding test fail. Verified on Python 3.9, 3.13 and 3.14, avoiding the version-specific stdlib details that broke an earlier assertion on 3.9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Sep 3, 2026
…20016) ## Description A wrapping context that raises is skipped by the machinery that would have popped its per-call ContextVar storage. The storage chains one dict per call through `_STORAGE_PREV` and is never released, so on a long-lived worker thread it grows without bound, and on two of the three paths the universal storage is among the retained dicts — so it keeps hold of `__frame__`. Measured on Python 3.13, before the fix: ``` __enter__ raises: after 1 call concrete chain=1 universal chain=0 frame_retained=False after 8 calls concrete chain=8 universal chain=0 frame_retained=False __return__ raises: after 1 call concrete chain=1 universal chain=1 frame_retained=True after 5 calls concrete chain=5 universal chain=5 frame_retained=True __exit__ raises: after 5 calls concrete chain=5 universal chain=5 frame_retained=True ``` **Scope:** only the `__enter__` path is fixed here. `__return__` / `__exit__` and the 3.15 monitoring path leak the same way, but releasing the storage there is not a matter of adding a pop — `_exit` and `on_py_unwind` read `_SKIP_EXIT_KEY` back off that storage, so popping first makes them act on the enclosing call's storage. Documented in place and tracked in [APPSEC-69961](https://datadoghq.atlassian.net/browse/APPSEC-69961), with the two test parameters covering them marked `xfail(strict=True)`. Two distinct causes, both on `__enter__`: - **`__enter__`** — `_UniversalWrappingContext.__enter__` appends to `entered` only *after* a successful call, so a context that raises is absent from the list `__exit__` later iterates. Its `__enter__` has usually already pushed storage via `super().__enter__()`. From 3.11 the universal storage is fine here, because the injected exception handler still reaches `_exit()`. - **`__enter__` below 3.11** — those versions enter through a real `with` statement, and Python does not call `__exit__` when `__enter__` raises. So the universal storage, which holds `__frame__`, and the storage of contexts that *did* enter are left behind as well. Gated on the version, because popping here on 3.11+ would double-pop and discard an outer re-entrant call's storage. The fix releases storage on all three paths, comparing storage identity against a snapshot taken before the call. **That identity check is the load-bearing part**: popping unconditionally would discard an outer re-entrant call's storage, since a context that already popped now holds a different object. Existing semantics are deliberately unchanged: an `Exception` from `__enter__` is still swallowed and logged so a broken context cannot break the call, and a `BaseException` still propagates. ### Impact Latent today. No shipped `WrappingContext` subclass raises by design, so triggering it needs a bug in a context's `__enter__` / `__return__` / `__exit__` — which an `Exception`-derived failure does, even though it is swallowed and logged. A debugger probe or code-origin context hitting an unexpected error would leak. It becomes reachable by design in #19989, where `BlockingException` is raised from `__enter__` to stop a blocked outgoing request, i.e. one leaked dict per blocked request. Jira: https://datadoghq.atlassian.net/browse/APPSEC-69960 Blocks: #19989 ## Risks Medium — this is shared instrumentation used by the debugger, code origin, selenium and the lazy module loader. - The re-entrancy hazard is the thing to review closely. `_release_storage` pops only when the context still holds the exact object it held before the call, so an already-popped context is left alone. - Cleanup is wrapped so a failure to release cannot mask the original exception. - The `except Exception` in `__enter__` widened to `except BaseException` in order to run cleanup, then re-raises non-`Exception` unchanged. Behaviour for `Exception` is identical to before. ## Additional Notes Split out of #19989 at review request: the change is in shared wrapping infrastructure and wants its own review rather than riding along in an AppSec PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) [APPSEC-69961]: https://datadoghq.atlassian.net/browse/APPSEC-69961?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: alberto.vara <alberto.vara@datadoghq.com>
…EC-69877] Review feedback. try_wrap_context's module hook returned early whenever (module_name, name) was already in the registry, treating the key as proof that the current target is wrapped. It is not: when http.client or urllib.request is reloaded, or dropped from sys.modules and imported again, ModuleWatchdog fires the hook for freshly defined functions while the registry still holds a context bound to the previous ones. The new HTTPConnection and OpenerDirector methods then ran without SSRF and API10 instrumentation until a full unpatch/patch cycle. This was a regression against the wrapt path it replaced, which handled the case for free: wrap_object runs on every hook fire, and apply_patch wraps whatever the attribute holds at that point. Compare the installed context's __wrapped__ against the freshly resolved target instead, and rebind when they differ, releasing the stale context first. A failure to release is logged and does not stop the rebind. Also adds the regression test for the blocking-path storage leak that review raised against this PR. The framework fix landed separately in #20016 (APPSEC-69960), but this is the code that raises BlockingException from __enter__, so the assertion belongs here: three blocked requests must leave both the concrete and the universal ContextVar unset, or each block chains another storage dict onto the worker thread. Both are mutation-checked: removing the rebind, or removing the storage release in _UniversalWrappingContext.__enter__, fails the corresponding test. appsec::appsec on py3.13: 708 passed, 1 xfailed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…[APPSEC-69877] Comparing the three WrappingContext users, the debugger is the one that gets the lifecycle right: it guards its own work in __enter__/__return__/__exit__ and always chains to super(). The RASP contexts did neither consistently. - __enter__ ran unguarded. open_rasp_subcontext_scope() is called after the core context is opened, so if it raised, the universal context swallowed the exception and left this context out of its entered list. Neither __return__ nor __exit__ then runs, so core_ctx.__exit__ never happens: _CURRENT_CONTEXT stays pinned to an orphaned url_open_analysis context for the rest of the thread's life and later core.set_item/find_item land in the orphan. The old "with core.context_with_data(...)" block could not leak this way. The body moves to _handle_enter(), mirroring selenium's _handle_enter and the debugger's _open_signals, and a failure now closes the core context. - __return__ and __exit__ had try/finally but no except, so a bug in the response analysis surfaced to the customer and, via __return__, reached the universal context, which sets the skip flag and strands this call's storage (APPSEC-69961). Both now swallow and log, as the other two products do. - try_wrap_context read installed.__wrapped__, which is a weakref property that raises RuntimeError once the old function is collected - exactly the reload case the rebind was added for. Inside the blanket try that aborted the hook and left the stale entry, so the hook would stay unwrapped permanently. Read _wrapped_ref() instead and treat a dead ref as stale. - _arg re-resolved __frame__ per call, which takes the wrapping registry lock and scans co_consts. The http.client hook read three arguments, so every instrumented downstream request paid that three times. Resolve once. Both new behaviours are mutation-checked: making __enter__ or __return__ stop swallowing fails the corresponding test. appsec::appsec 710 passed / 1 xfailed on py3.13; test_common_modules 95 passed on py3.9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
avara1986
marked this pull request as ready for review
September 4, 2026 09:25
There was a problem hiding this comment.
💡 Codex Review
dd-trace-py/tests/appsec/appsec/test_common_modules.py
Lines 668 to 669 in 8470c7e
Use plain prose in the test docstring
Remove the reStructuredText markup around HTTPConnectionPool.urlopen, “relative,” and the literal examples in this test-only docstring. Test docstrings are not rendered by Sphinx, and the repository convention explicitly requires plain prose without reStructuredText emphasis or double-backtick literals so these comments remain readable in editors.
AGENTS.md reference: AGENTS.md:L43-L50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…apper [APPSEC-69877] _wrap_request and _wrap_putrequest guard the wrapped call with "except Exception", but BlockingException derives from BaseException so that Exploit Prevention's block cannot be swallowed by an intermediate handler. The span was therefore created and activated by tracer.trace(), then never finished: it is never flushed, it stays current for the rest of the thread's life, and every later span on that thread is parented to it. This is a pre-existing defect that was order-dependent, and this branch removes the order that avoided it. Measured with the httplib integration and appsec both patched, blocking from HTTPConnection.request: patch order main this branch httplib contrib first clean leaked appsec first leaked leaked On main, an appsec wrapt wrapper installed after the integration sat outside _wrap_request, so the block raised before the span existed. A wrapping context lives in the target's own bytecode, so it is always innermost and that escape is gone. Catching BaseException here matches what the rest of the tracer already does for span cleanup - redis_utils, valkey_utils, elasticsearch, asgi, pyramid, tornado and tracer.py all do the same. _wrap_getresponse already used try/finally and was unaffected. Also applies two review findings on the appsec side: plain prose instead of reStructuredText in a test docstring, and an AIDEV-NOTE anchor on the __enter__ lifecycle invariant. The new test covers both patch orders and fails on both without the fix. test_common_modules 97 passed on py3.13; contrib::httplib green on py3.13. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 4, 2026
…877] The httplib span leak is a pre-existing bug in shared integration code, not part of this migration: on main it already strands the span whenever appsec patched http.client before the integration did. Only the second order was safe, because appsec's wrapt wrapper then sat outside _wrap_request and the block raised before the span existed. It belongs on its own, so it moves to #20073 (APPSEC-70027) with tests that raise a real BlockingException from inside the wrapped call and therefore need neither the network nor appsec. The regression test added here goes with it - its contract is the same one those two cover, and keeping a copy that cannot pass until #20073 lands would only make this branch red. This branch now depends on #20073: a wrapping context lives in the target's own bytecode, so it is always innermost and the block always raises inside the span, which removes the order that used to avoid the leak. Merge main in once #20073 has landed. test_common_modules 95 passed on py3.13. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Sep 4, 2026
…apper (#20073) ## Summary Jira: [APPSEC-70027](https://datadoghq.atlassian.net/browse/APPSEC-70027) Exploit Prevention blocks an outgoing request by raising `BlockingException`, which derives from `BaseException` so that no intermediate handler can swallow a blocking decision. `_wrap_request` and `_wrap_putrequest` guard the wrapped call with `except Exception`: ```python try: return func_to_call(*args, **kwargs) except Exception: span = getattr(instance, "_datadog_span", None) ... ``` The block goes straight past that handler. The span has already been created and activated by `tracer.trace()` and attached to the connection, so it is never finished: it is never flushed, it stays current for the rest of the thread's life, and every later span on that thread is parented to it. Measured with the integration and appsec both patched, blocking from `HTTPConnection.request`: ``` span created: True span finished: False still current: True Shutting down tracer with 1 spans. These spans will not be sent to Datadog: name=http.client.request ``` ## Order dependence Reachable on `main` today, but only in one patch order: | patch order | `main` | |---|---| | httplib contrib patched first | clean, no span created | | appsec patched first | **leaked** | When appsec patches the attribute *after* the integration, its wrapt wrapper sits outside `_wrap_request` and the block raises before the span exists. When appsec patches first, the integration wraps appsec's wrapper and the block happens inside the span. ## Why now The AppSec RASP migration to `WrappingContext` (#19989, [APPSEC-69878](https://datadoghq.atlassian.net/browse/APPSEC-69878)) removes the order that avoids this. A wrapping context lives in the target's own bytecode, so it is always innermost and the block always raises inside the span. **#19989 depends on this landing first.** Found by Codex review on that PR, split out here because it is a pre-existing bug in shared integration code rather than part of the migration. ## Fix Catch `BaseException` around the wrapped call in both wrappers and re-raise. That is what the rest of the tracer already does for span cleanup — `redis_utils`, `valkey_utils`, `elasticsearch`, `asgi`, `pyramid`, `tornado` and `tracer.py`. `_wrap_getresponse` already used `try/finally` and was unaffected. ## Testing Two tests in `tests/contrib/httplib/test_httplib.py` raise a real `BlockingException` from inside the wrapped call, so they need no network and no appsec setup: - `test_span_is_finished_when_the_request_raises_a_base_exception` - `test_span_is_finished_when_putrequest_raises_a_base_exception` Both assert the span is finished and no span is left current. Mutation-checked — reverting to `except Exception` fails both. `contrib::httplib` green on py3.12. `scripts/lint fmt` clean. ## Checklist - [x] PR author has checked that all the criteria below are met - [x] The PR description includes an overview of the change - [x] The PR description articulates the motivation for the change - [x] The change includes tests OR the PR description describes a testing strategy - [x] The PR description notes risks associated with the change, if any - [x] Newly-added code is easy to change - [x] The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - [x] The change includes or references documentation updates if necessary - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [ ] Reviewer has checked that all the criteria below are met - [ ] Title is accurate - [ ] All changes are related to the pull request's stated goal - [ ] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [ ] Testing strategy adequately addresses listed risks - [ ] Newly-added code is easy to change - [ ] Release note makes sense to a user of the library - [ ] If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [ ] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) 🤖 Generated with [Claude Code](https://claude.com/claude-code) [APPSEC-70027]: https://datadoghq.atlassian.net/browse/APPSEC-70027?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [APPSEC-69878]: https://datadoghq.atlassian.net/browse/APPSEC-69878?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: alberto.vara <alberto.vara@datadoghq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Unhandled exceptions are shipped to the crashtracker with their full traceback
(
ddtrace/internal/core/crashtracking.py:185), and intake tags a reportcrash_datadog:trueassoon as a ddtrace path appears in the frames. The AppSec RASP hooks use wrapt, so the wrapper is a
Python function with its own stack frame. That frame appears in the traceback of ordinary
application exceptions passing through it — even when RASP is inactive and the wrapper only does
return original(...)— so tracer-unrelated customer errors get attributed to Datadog, inflatingdd.instrumentation_telemetry_data.datadog_crashes.WrappingContext(ddtrace/internal/wrapping/context.py) rewrites the target's own bytecode into awith-block around its body, so
__enter__has returned before the body runs and no ddtrace frame isleft behind. Measured on the reported reproduction:
Worth noting for reviewers:
ddtrace.internal.wrapping.wrapdoes not help here. The wrapperstill owns a frame, and it adds a trampoline frame on top:
Migrated in this PR:
urllib.request.OpenerDirector.openwrapped_open_ED4CF71136E15EBF_SsrfOpenerDirectorOpenhttp.client.HTTPConnection.requestwrapped_request_A7F2C6E4D3B10958_SsrfHttpConnectionRequesthttp.client.HTTPConnection.getresponsewrapped_response_SsrfHttpConnectionGetresponseAdds
try_wrap_context/try_unwrap_contextin_patch_utils.pyalongsidetry_wrap_function_wrapper, reusing the sameModuleWatchdogbookkeeping. wrapt stays in thatmodule:
apply_patch/patchable_builtin/patch_builtinsexist for the C targets, and IASTstill uses
FunctionWrapper.Points that needed care
A
with core.context_with_data(...)block spanned the wrapped call.__enter__and__return__/__exit__are separate calls, so it becomes a manual open in__enter__releasedfrom both
__return__and__exit__. It must be released before a response-side blockraises, because a raising
__return__sets_SKIP_EXIT_KEYand suppresses__exit__.RASP blocking still works because
BlockingExceptionderives fromBaseException(
ddtrace/internal/_exceptions.py:5)._UniversalWrappingContext.__enter__swallowsexcept Exceptionper registered context (context.py:759-765), so anException-derived blockwould be silently dropped. Load-bearing, and now covered by a test.
The
crop_traceanchor moved from the wrapper name to the wrapped function's own name. Acontext runs inside the target's frame, so there is no wrapper frame left to crop at. Anchoring on
the target keeps
report_stackcropping in the same place, soframes[0]remains the target'scaller. Dropping the anchor would have shifted the reported stack by one frame, because
_INTERNAL_FRAMESdoes not cover the stdlib.The migrated bodies are deliberately kept as a move rather than a rewrite — same guard nesting,
same locals in the same order — so the diff reads as relocation. The only differences from the
original wrapper functions are the argument reads going through
_arg, the crop anchor, andreturning
self. Refactoring can come later.Binding a context to a wrapt proxy is unrecoverable, which matters because
contrib/internal/httplib/patch.py:229wrapt-wraps the same twoHTTPConnectionattributes:getattr(cls, "method")builds a freshBoundFunctionWrapperon every access, so theregistration can never be found again;
isinstance(proxy, FunctionType)isTrue— wrapt forwards__class__— so a "peel until it isa function" loop never peels.
unwrap()then silently no-ops while the code object stays rewritten, and the next patch rewrites ontop of it, growing the code every patch/unpatch cycle (
request42 → 94 bytes) until thebytecodelibrary raises
KeyErrorwhile parsing. Resolved by reading the owner's__dict__with atype()check and unwrapping the retained context instance instead of re-resolving the attribute. Wrap
failures are also swallowed now — losing a RASP hook is acceptable, breaking customer patching is
not.
Not migrated
contrib/internal/urllib3/patch.py:55-60installs the same appsecwrappers on the same targets as
patch_common_modules(), deduplicated today only byapply_patch'sFunctionWrapperguard, which a wrapping context bypasses. Migrating appsec's sidealone would double-invoke the WAF and double-count
downstream_requests. Doing it properly meansremoving the contrib registrations, which crosses the boundary
.cursor/rules/isolated-responsibility.mdcgoverns. Also needs urllib3 v1 handling, where_make_requestkeepsbody/headersin**httplib_request_kwrather than as namedparameters, so
_argwill need a varkwargs fallback added alongside tests that exercise it.builtins.open,os.system,os.fork,builtins.eval, hashlib IAST sinks. C callables withno
__code__;WrappingContextcannot wrap them at all.wrapped_request_D8CB81E472AF98A2is called inlinefrom five contrib patch modules and keeps its wrapt signature.
This does not close APPSEC-69877 on its own. Those targets keep contributing ddtrace frames.
Jira: https://datadoghq.atlassian.net/browse/APPSEC-69877
Testing
New tests in
tests/appsec/appsec/test_common_modules.py:traceback;
_argreadsfullurlby name and the hook actually runs (get the name wrong and RASP silentlystops inspecting outgoing requests, which no other assertion would catch);
full_urlitem survives a failed request;both, asserted via the arguments it observed;
len(fn.__code__.co_code)returns to baseline after 6 mixed-order patch/unpatch cycles — this isthe assertion that catches the proxy-binding defect above;
is_wrapped-style checks do not.Verified on Python 3.13:
appsec::appsec695 passed / 1 xfailed / 0 failed;appsec::appsec_threats_flask_no_iast2900 passed / 2 skipped / 8 xfailed / 0 failed, which coversthe
ssrfexploit-prevention cases and theirtop_functionsassertions on the cropped RASP stack.scripts/lint fmtandscripts/lint typingclean.Risks
Medium — bytecode rewriting replaces wrapt for three hooks on an outbound HTTP path.
HTTPConnectionandOpenerDirectorsubclassestoo. Intended, and patch/unpatch symmetry is now asserted by the bytecode-size test.
pyproject.tomlis>=3.9,<3.15andcontext.pyhas a branch per version, so there is no gap today.but see the skips in
tests/wrapping/test_tstrings_py314.pyandtest_async.py.__enter__plus__return__instead of one wrapt call,against network I/O. Happy to run
scripts/lint-adjacent benchmarks if reviewers want a number.Additional Notes
The
wrapped_response→_SsrfHttpConnectionGetresponsemove also drops a pre-existingoriginal(*args, *kwargs)typo (*kwargs, not**kwargs), which was harmless only becausegetresponsetakes no keyword arguments.🤖 Generated with Claude Code