Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions agent/src/main/java/dev/aikido/agent/Wrappers.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package dev.aikido.agent;

import dev.aikido.agent.wrappers.*;
import dev.aikido.agent.wrappers.executor.AbstractExecutorServiceWrapper;
import dev.aikido.agent.wrappers.executor.DelegatedExecutorServiceWrapper;
import dev.aikido.agent.wrappers.executor.ForkJoinPoolWrapper;
import dev.aikido.agent.wrappers.executor.ScheduledThreadPoolExecutorWrapper;
import dev.aikido.agent.wrappers.executor.ThreadPoolExecutorWrapper;
import dev.aikido.agent.wrappers.file.FileConstructorMultiArgumentWrapper;
import dev.aikido.agent.wrappers.file.FileConstructorSingleArgumentWrapper;
import dev.aikido.agent.wrappers.javalin.*;
Expand All @@ -17,6 +22,13 @@ public final class Wrappers {
private Wrappers() {}
public static final List<Wrapper> WRAPPERS = Arrays.asList(
new PostgresWrapper(),

new DelegatedExecutorServiceWrapper(),
new ThreadPoolExecutorWrapper(),
new AbstractExecutorServiceWrapper(),
new ForkJoinPoolWrapper(),
new ScheduledThreadPoolExecutorWrapper(),

new SpringMVCJakartaWrapper(),
new SpringMVCJavaxWrapper(),
new SpringWebfluxWrapper(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package dev.aikido.agent.wrappers.executor;

import dev.aikido.agent.wrappers.Wrapper;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

import java.util.concurrent.AbstractExecutorService;
import java.util.concurrent.Callable;

import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;

public class AbstractExecutorServiceWrapper implements Wrapper {
@Override
public String getName() {
return SubmitAdvice.class.getName();
}

@Override
public ElementMatcher getMatcher() {
return isMethod()
.and(named("submit"))
.and(
takesArguments(Runnable.class)
.or(takesArguments(Callable.class))
.or(takesArguments(Runnable.class, Object.class))
);
}

@Override
public ElementMatcher getTypeMatcher() {
return isSubTypeOf(AbstractExecutorService.class);
}

public static class SubmitAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void before(
@Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task
) {
if (task instanceof Runnable) {
task = ExecutorContextPropagation.wrap((Runnable) task);
} else if (task instanceof Callable) {
task = ExecutorContextPropagation.wrap((Callable) task);
Comment on lines +45 to +48

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium - Bootstrap executor advice calls an agent-only helper that JDK classes cannot resolve

The new AbstractExecutorService, ThreadPoolExecutor, and ForkJoinPool advices inject direct calls to ExecutorContextPropagation.wrap(...) into java.util.concurrent classes even though this JVM setup never injects agent helper classes into the bootstrap classloader. The project already handles bootstrap-loaded JDK wrappers via reflective loading for that reason, and these advices also suppress any linkage error, so submit/execute on core executors silently runs without propagated request context. As a result, async work scheduled onto common JDK executors loses the request metadata that Zen uses to attribute and enforce protections on downstream sinks.

Show fix

Do not reference dev.aikido.agent... helpers directly from advice woven into bootstrap-loaded JDK classes. Either move these executor wrappers to the same reflective bridge pattern already used for bootstrap JDK wrappers, or explicitly append the helper classes to the bootstrap classloader search before instrumenting java.util.concurrent so the injected calls can actually resolve.

More info - Reply on this comment to give feedback or ignore the issue.

}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package dev.aikido.agent.wrappers.executor;

import dev.aikido.agent.wrappers.Wrapper;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

import java.lang.reflect.Method;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.concurrent.Callable;

import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;

public class DelegatedExecutorServiceWrapper implements Wrapper {
@Override
public String getName() {
return DelegatedExecutorAdvice.class.getName();
}

@Override
public ElementMatcher getMatcher() {
return isMethod()
.and(named("execute").or(named("submit")))
.and(
takesArguments(Runnable.class)
.or(takesArguments(Callable.class))
.or(takesArguments(Runnable.class, Object.class))
);
}

@Override
public ElementMatcher getTypeMatcher() {
return nameStartsWith("java.util.concurrent.Executors$");
}

public static class DelegatedExecutorAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void before(
@Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task
) throws Exception {
if (task == null) {
return;
}

// This advice is applied to JDK classes loaded by the bootstrap classloader.
// Load agent_api reflectively because bootstrap classes cannot directly reference agent classes.
String jarFilePath = System.getProperty("AIK_agent_api_jar");
if (jarFilePath == null || jarFilePath.isBlank()) {
return;
}

// The wrapper returned by wrap() resolves through the parent (system) classloader,
// so this per-call loader can be closed once wrapping is done.
URLClassLoader classLoader = new URLClassLoader(new URL[] { new URL(jarFilePath) });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating and closing a URLClassLoader for every delegated executor submission adds substantial repeated class-loading overhead on a high-frequency task-submission path.

Details

✨ AI Reasoning
​Task submission can occur at high throughput. Each invocation constructs a class loader, loads the context propagation class, reflects its wrapping method, and closes the loader. This repeated resource creation is directly introduced by the advice and is avoidable through cached initialization.

🔧 How do I fix it?
Move constant work outside loops. Use StringBuilder instead of string concatenation in loops. Cache compiled regex patterns. Use hash-based lookups instead of nested loops. Batch database operations instead of N+1 queries.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit try/finally around classLoader.close() is unnecessarily verbose for a scoped resource. Use try-with-resources to express the same lifecycle more directly.

Details

✨ AI Reasoning
​The loader is created, used within one block, and always closed afterward. Java's try-with-resources construct expresses this lifecycle directly while preserving exception behavior and cleanup.

🔧 How do I fix it?
Rewrite the snippet in the simpler, behavior-equivalent form: return a boolean expression directly instead of if cond return true else return false, avoid using lists when they are guaranteed to contain one element, etc.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

try {
Class<?> contextPropagationClass = classLoader.loadClass(
"dev.aikido.agent_api.context.ContextPropagation"
);

if (task instanceof Runnable) {
Method wrapRunnable = contextPropagationClass.getMethod("wrap", Runnable.class);
task = wrapRunnable.invoke(null, task);
Comment on lines +58 to +68

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium - Delegated and scheduled executor wrappers reopen agent_api.jar on every task submission

Both bootstrap-safe executor wrappers now allocate a fresh URLClassLoader, load ContextPropagation, reflect the wrap(...) method, and close the loader for every submit/execute or schedule call. These methods are on the hot path for common asynchronous work, so the change adds repeated JAR parsing and reflective lookup overhead to every task dispatch instead of amortizing it once per JVM. Under request-driven executor usage this can materially reduce throughput and increase allocation pressure for the very async workloads this feature targets.

Show fix

Cache the reflected ContextPropagation class and wrap methods across calls instead of constructing a new URLClassLoader per task. If bootstrap isolation prevents direct helper references, initialize the reflective bridge lazily once in bootstrap-safe code and reuse the cached Method/MethodHandle objects for all subsequent submissions and schedules.

More info - Reply on this comment to give feedback or ignore the issue.

} else if (task instanceof Callable) {
Method wrapCallable = contextPropagationClass.getMethod("wrap", Callable.class);
task = wrapCallable.invoke(null, task);
}
} finally {
classLoader.close();
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package dev.aikido.agent.wrappers.executor;

import java.lang.reflect.Method;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.concurrent.Callable;

// Bridges the executor advice (woven into java.util.concurrent classes) to ContextPropagation in
// agent_api, which lives on a different classloader, and caches the reflected methods so the lookup
// happens once. A missing AIK_agent_api_jar is treated as "not ready yet" (early startup) and
// retried on the next call, rather than disabling propagation for the whole JVM; only a genuine
// load failure once the path is set disables it.
public final class ExecutorContextPropagation {
private static volatile Method wrapRunnableMethod;
private static volatile Method wrapCallableMethod;
private static volatile boolean disabled;

private ExecutorContextPropagation() {}

public static Runnable wrap(Runnable task) {
if (task == null) {
return task;
}
Method wrap = wrapRunnableMethod;
if (wrap == null) {
init();
wrap = wrapRunnableMethod;
}
if (wrap == null) {
return task;
}
try {
return (Runnable) wrap.invoke(null, task);
} catch (Throwable ignored) {
return task;
}
}

@SuppressWarnings("unchecked")
public static <T> Callable<T> wrap(Callable<T> task) {
if (task == null) {
return task;
}
Method wrap = wrapCallableMethod;
if (wrap == null) {
init();
wrap = wrapCallableMethod;
}
if (wrap == null) {
return task;
}
try {
return (Callable<T>) wrap.invoke(null, task);
} catch (Throwable ignored) {
return task;
}
}

private static synchronized void init() {
if (disabled || wrapRunnableMethod != null) {
return;
}
String jarFilePath = System.getProperty("AIK_agent_api_jar");
if (jarFilePath == null || jarFilePath.isBlank()) {
return; // not set yet during early startup - retry on a later call
}
try {
URLClassLoader classLoader = new URLClassLoader(new URL[] { new URL(jarFilePath) });
Class<?> clazz = classLoader.loadClass("dev.aikido.agent_api.context.ContextPropagation");
wrapCallableMethod = clazz.getMethod("wrap", Callable.class);
wrapRunnableMethod = clazz.getMethod("wrap", Runnable.class);
} catch (Throwable ignored) {
disabled = true;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package dev.aikido.agent.wrappers.executor;

import dev.aikido.agent.wrappers.Wrapper;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

import java.util.concurrent.Callable;
import java.util.concurrent.ForkJoinPool;

import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;

public class ForkJoinPoolWrapper implements Wrapper {
@Override
public String getName() {
return ForkJoinAdvice.class.getName();
}

@Override
public ElementMatcher getMatcher() {
return isMethod()
.and(named("execute").or(named("submit")))
.and(
takesArguments(Runnable.class)
.or(takesArguments(Callable.class))
.or(takesArguments(Runnable.class, Object.class))
);
}

@Override
public ElementMatcher getTypeMatcher() {
return isSubTypeOf(ForkJoinPool.class);
}

public static class ForkJoinAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void before(
@Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task
) {
if (task instanceof Runnable) {
task = ExecutorContextPropagation.wrap((Runnable) task);
} else if (task instanceof Callable) {
task = ExecutorContextPropagation.wrap((Callable) task);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package dev.aikido.agent.wrappers.executor;

import dev.aikido.agent.wrappers.Wrapper;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

import java.lang.reflect.Method;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.concurrent.Callable;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

import static net.bytebuddy.implementation.bytecode.assign.Assigner.Typing.DYNAMIC;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;

public class ScheduledThreadPoolExecutorWrapper implements Wrapper {
@Override
public String getName() {
return ScheduleAdvice.class.getName();
}

@Override
public ElementMatcher getMatcher() {
return isMethod()
.and(named("schedule"))
.and(
takesArguments(Runnable.class, long.class, TimeUnit.class)
.or(takesArguments(Callable.class, long.class, TimeUnit.class))
);
Comment on lines +29 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium - Periodic scheduled executor methods are left uninstrumented

ScheduledThreadPoolExecutorWrapper only matches the one-shot schedule(...) overloads, so scheduleAtFixedRate(...) and scheduleWithFixedDelay(...) still enqueue the original runnable without ContextPropagation.wrap(...). Work moved onto those periodic APIs therefore runs with no propagated request context, and Zen's request-scoped detection and blocking logic loses the metadata it needs to attribute and enforce protections on downstream async operations. Applications that fan request work out through recurring scheduled tasks can silently bypass the new executor propagation coverage.

Show fix
Suggested change
public ElementMatcher getMatcher() {
return isMethod()
.and(named("schedule"))
.and(
takesArguments(Runnable.class, long.class, TimeUnit.class)
.or(takesArguments(Callable.class, long.class, TimeUnit.class))
);
return isMethod()
.and(
named("schedule").and(
takesArguments(Runnable.class, long.class, TimeUnit.class)
.or(takesArguments(Callable.class, long.class, TimeUnit.class))
)
.or(
named("scheduleAtFixedRate")
.and(takesArguments(Runnable.class, long.class, long.class, TimeUnit.class))
)
.or(
named("scheduleWithFixedDelay")
.and(takesArguments(Runnable.class, long.class, long.class, TimeUnit.class))
)
);

More info - Reply on this comment to give feedback or ignore the issue.

}

@Override
public ElementMatcher getTypeMatcher() {
return isSubTypeOf(ScheduledThreadPoolExecutor.class);
}

public static class ScheduleAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void before(
@Advice.Argument(value = 0, readOnly = false, typing = DYNAMIC) Object task
) throws Exception {
if (task == null) {
return;
}

// This advice is applied to JDK classes loaded by the bootstrap classloader.
// Load agent_api reflectively because bootstrap classes cannot directly reference agent classes.
String jarFilePath = System.getProperty("AIK_agent_api_jar");
if (jarFilePath == null || jarFilePath.isBlank()) {
return;
}

// The wrapper returned by wrap() resolves through the parent (system) classloader,
// so this per-call loader can be closed once wrapping is done.
URLClassLoader classLoader = new URLClassLoader(new URL[] { new URL(jarFilePath) });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating and closing a URLClassLoader for every scheduled task adds substantial repeated class-loading overhead on a high-frequency scheduling path.

Details

✨ AI Reasoning
​Scheduling operations may be frequent in applications using periodic or delayed tasks. Every call performs class-loader construction, class loading, reflective method lookup, and cleanup, even when the same propagation class and methods are repeatedly needed. This is newly introduced in the advice.

🔧 How do I fix it?
Move constant work outside loops. Use StringBuilder instead of string concatenation in loops. Cache compiled regex patterns. Use hash-based lookups instead of nested loops. Batch database operations instead of N+1 queries.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit try/finally around classLoader.close() is unnecessarily verbose for a scoped resource. Use try-with-resources to express the same lifecycle more directly.

Details

✨ AI Reasoning
​The loader is created, used within one block, and always closed afterward. Java's try-with-resources construct expresses this lifecycle directly while preserving exception behavior and cleanup.

🔧 How do I fix it?
Rewrite the snippet in the simpler, behavior-equivalent form: return a boolean expression directly instead of if cond return true else return false, avoid using lists when they are guaranteed to contain one element, etc.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

try {
Class<?> contextPropagationClass = classLoader.loadClass(
"dev.aikido.agent_api.context.ContextPropagation"
);

if (task instanceof Runnable) {
Method wrapRunnable = contextPropagationClass.getMethod("wrap", Runnable.class);
task = wrapRunnable.invoke(null, task);
} else if (task instanceof Callable) {
Method wrapCallable = contextPropagationClass.getMethod("wrap", Callable.class);
task = wrapCallable.invoke(null, task);
}
} finally {
classLoader.close();
}
}
}
}
Loading
Loading