Context propagation primitives for async task execution - #342
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| if (task == null || task instanceof ContextPropagatingRunnable) { | ||
| return task; |
There was a problem hiding this comment.
🟡 Medium - Re-wrapping a retained task keeps the first request's context
ContextPropagation.wrap(...) returns an existing ContextPropagatingRunnable/Callable unchanged instead of rebinding it to the current request. If an application keeps a task instance and calls wrap() before submitting it for later requests, the wrapper continues to install the original ContextObject, so later async work inherits stale route, user, IP, and forcedProtectionOff state. That can misattribute attack reports to the wrong request and, when the first request had protection forced off, skip vulnerability scanning for subsequent requests.
Show fix
Do not treat already-wrapped tasks as safe to reuse across requests. Either always create a fresh wrapper for each wrap() call, or unwrap and rebind the delegate when the current ContextObject differs from the one captured previously so each submission propagates the caller's current request context.
More info - Reply on this comment to give feedback or ignore the issue.
| ContextObject context = Context.get(); | ||
| if (context == null) { | ||
| return task; | ||
| } | ||
|
|
||
| return new ContextPropagatingRunnable(task, context); |
There was a problem hiding this comment.
🟡 Medium - Async fan-out shares one mutable request context between worker threads
wrap(...) captures the live ContextObject reference and installs that same object in every worker thread instead of snapshotting it for each task. When one request submits multiple wrapped tasks in parallel, they concurrently mutate shared state such as redirectStartNodes, cached extracted strings, user, and rate-limit metadata without synchronization. This can corrupt redirect tracking or cached request data, leading to missed SSRF/vulnerability detections and occasional collector failures under concurrent async work.
Show fix
Propagate an immutable snapshot of the request context to each task instead of sharing the original mutable ContextObject. At minimum, clone or otherwise isolate mutable collections/caches before wrapping, or make the context and all of its mutable substructures thread-safe if concurrent async use is intended.
More info - Reply on this comment to give feedback or ignore the issue.
Introduce ContextPropagatingRunnable/Callable and the ContextPropagation factory that captures the current request Context at wrap time and restores it around task execution, restoring the worker's previous context afterwards. Wrapping is idempotent and passes through null / already-wrapped / no-context tasks. Unit-tested in isolation without agent weaving.
c0d0a2a to
75593a3
Compare
First of three stacked PRs adding request-context propagation across async boundaries. This one is just the primitives — no instrumentation yet — so it's small and easy to review on its own.
When a request is handled, its context lives in a ThreadLocal. The moment work is handed to another thread that's gone, so any detector (SQLi, SSRF, …) running there sees no context. The building block for fixing that is a small wrapper that snapshots the current context when a task is created and restores it around the task's run.
ContextPropagation.wrap(...)takes aRunnable/Callableand returns a wrapper that:Contextat wrap time,It stays a no-op where it should:
null, already-wrapped, and no-active-context tasks are returned untouched, so wrapping twice is harmless.No executor is touched here — that's the next PR. Covered by
ContextPropagationTest(18 cases: capture, restore, restore-previous, clear-when-none, exception safety, idempotency, delegate execution).