-
Notifications
You must be signed in to change notification settings - Fork 337
docs(appsec): add AppSec developer reference docs and module AGENTS.md files #11454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| # AppSec Module | ||
|
|
||
| Extended reference: [docs/appsec/](../../docs/appsec/) | ||
|
|
||
| ## Adding a new WAF address: 4-file checklist | ||
|
|
||
| Every new WAF address requires changes in exactly 4 files. Missing any one causes a silent no-op: | ||
|
|
||
| 1. `Events.java` -- declare the event type with a unique sequential ID | ||
| 2. `InstrumentationGateway.java` -- register the callback type in `getCallback()` | ||
| 3. `KnownAddresses.java` -- declare the `Address<T>` constant and add to `fromString()` switch | ||
| 4. `GatewayBridge.java` -- subscriber field, `init()` registration, `reset()` null, handler method, `DATA_DEPENDENCIES` entry | ||
|
|
||
| See [docs/appsec/ig-events.md](../../docs/appsec/ig-events.md) for the full handler pattern including the retry-on-expiry loop. | ||
|
|
||
| ## `KnownAddressesSpecificationForkedTest` instance count | ||
|
|
||
| Every new `Address<?>` declared in `KnownAddresses.java` must be accompanied by incrementing the | ||
| expected count in `KnownAddressesSpecificationForkedTest`: | ||
|
|
||
| ```groovy | ||
| Address.instanceCount() == 47 // increment by 1 per new Address | ||
| ``` | ||
|
|
||
| The test fails if this is not updated. | ||
|
|
||
| ## WAF API quick reference | ||
|
|
||
| - `server.response.status` must be `Integer`, not `String` (changed in libddwaf v1.28.0) | ||
| - gRPC messages must use `runEphemeral()`, not `run()` -- persistent data caches only the first message | ||
| - Processor outputs (`_dd.appsec.s.req.body`, fingerprints) come from `ResultWithData.attributes` -- never pass them as input addresses | ||
| - `server.request.headers.no_cookies` must not include cookies -- cookies go in `server.request.cookies` | ||
|
|
||
| Full type mapping and limits: [docs/appsec/waf-api.md](../../docs/appsec/waf-api.md) | ||
|
|
||
| ## Callback guard ordering in advice | ||
|
|
||
| Fetch ALL callbacks before any early-return guard. A callback fetched after a conditional return | ||
| is silently skipped when only that callback is registered: | ||
|
|
||
| ```java | ||
| // Correct | ||
| BiFunction<...> bodyCallback = cbp.getCallback(EVENTS.requestBodyProcessed()); | ||
| BiFunction<...> filenamesCallback = cbp.getCallback(EVENTS.requestFilesFilenames()); | ||
| BiFunction<...> contentCallback = cbp.getCallback(EVENTS.requestFilesContent()); | ||
| if (bodyCallback == null && filenamesCallback == null && contentCallback == null) return; | ||
| ``` | ||
|
|
||
| ## Blocking model per framework | ||
|
|
||
| | Framework | Call `effectivelyBlocked()`? | Notes | | ||
| |---|---|---| | ||
| | Netty | NEVER | `BlockingResponseHandler` does it internally | | ||
| | Tomcat / Jersey / Jetty | YES, after `t = new BlockingException(...)` | Inside `if (brf != null)` | | ||
| | Undertow | YES, but `tryCommitBlockingResponse` is not idempotent | Guard `t == null` before each call | | ||
| | Vert.x | NO | Handler-based model | | ||
|
|
||
| Full details: [docs/appsec/blocking-patterns.md](../../docs/appsec/blocking-patterns.md) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| # Akka HTTP AppSec Instrumentation | ||
|
|
||
| Extended reference: [docs/appsec/ig-events.md](../../../../docs/appsec/ig-events.md), | ||
| [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md) | ||
|
|
||
| ## Two multipart routes must both be instrumented | ||
|
|
||
| Akka HTTP has two independent entry points for multipart form data. Any new WAF address that | ||
| captures multipart body data must instrument both: | ||
|
|
||
| | Route | Entry point | Notes | | ||
| |---|---|---| | ||
| | Route 1 | `handleMultipartStrictFormData(Multipart$FormData$Strict)` | Has `reqCtx` as local variable; iterates via `getStrictParts()` | | ||
| | Route 2 | `handleStrictFormData(StrictForm)` | No `reqCtx` in scope; must obtain via `activeSpan()` | | ||
|
|
||
| If only Route 1 is instrumented, multipart requests processed via `formFieldMultiMap` (Route 2) | ||
| silently miss the WAF event. | ||
|
|
||
| ## Do not extract filenames callback dispatch into `UnmarshallerHelpers` | ||
|
|
||
| Extracting the `requestFilesFilenames` callback dispatch into a shared helper method in | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which problems does it cause? Unless more detail is given, this risks becoming cargo cult and nobody remembers why we do it. |
||
| `UnmarshallerHelpers` is known to cause problems. Keep dispatch inline in each advice class. | ||
|
|
||
| ## No `effectivelyBlocked()` in Akka advice | ||
|
|
||
| Akka HTTP uses the Netty-style blocking model. Do not call `effectivelyBlocked()` in advice. | ||
| See [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md#netty-never-effectivelyblocked). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # Jetty AppSec Instrumentation | ||
|
|
||
| Extended reference: [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md), | ||
| [docs/appsec/multipart-frameworks.md](../../../../docs/appsec/multipart-frameworks.md) | ||
|
|
||
| ## Jetty 8.x: no `getSubmittedFileName()`, manual parsing required | ||
|
|
||
| `Part.getSubmittedFileName()` (Servlet 3.1) is not available in Jetty 8.x (Servlet 3.0). Filenames | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better documneted in the code, not here. |
||
| must be extracted by manually parsing the `Content-Disposition` header from each `Part`. | ||
|
|
||
| Exit advice on `getParts()` is the correct instrumentation point for Jetty 8.x. There is no | ||
| `parseParts()` equivalent to intercept. | ||
|
|
||
| ## Jetty 9.4/10: muzzle discriminator via `_dispatcherType` | ||
|
|
||
| The field `_dispatcherType: Ljavax/servlet/DispatcherType;` distinguishes Jetty 9.4/10.x | ||
| (javax namespace) from Jetty 11+ (jakarta namespace). Use this field as the muzzle reference | ||
| rather than trying to match on API version strings alone. | ||
|
|
||
| ## Blocking: Jetty uses Servlet container model | ||
|
|
||
| Jetty follows the Servlet blocking pattern. `effectivelyBlocked()` must be called explicitly | ||
| after creating `BlockingException`, inside `if (brf != null)`. See | ||
| [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md#servlet-containers-tomcat-jersey-resteasy). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # Netty 4.1 AppSec Instrumentation | ||
|
|
||
| Extended reference: [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md) | ||
|
|
||
| ## NEVER call `effectivelyBlocked()` in Netty advice | ||
|
|
||
| `effectivelyBlocked()` must not appear in any Netty advice class or helper. `BlockingResponseHandler` calls it internally when the blocking response is committed. Calling it in advice causes a double-invocation and closes the span prematurely. | ||
|
|
||
| This applies even to the urlencoded body-processed path in `HttpPostRequestDecoderInstrumentation` -- the span is already closed synchronously by `tryCommitBlockingResponse()` before advice can reach the mark. | ||
|
|
||
| Correct pattern: | ||
| ```java | ||
| // Netty advice — blocking path | ||
| if (brf != null) { | ||
| brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); | ||
| thr = new BlockingException("..."); | ||
| // do NOT call ctx.getTraceSegment().effectivelyBlocked() | ||
| } | ||
| ``` | ||
|
|
||
| ## `thr` creation must be inside `if (brf != null)` | ||
|
|
||
| If `thr = new BlockingException(...)` is placed outside the `brf != null` guard, it is created even when no blocking action exists. The exception is then thrown unconditionally in `@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)`, producing spurious 500 responses. | ||
|
|
||
| ## FileUpload content: two-branch read | ||
|
|
||
| `HttpData` stores uploads either in memory or on disk depending on size. Always branch on `isInMemory()`: | ||
|
|
||
| ```java | ||
| if (fileUpload.isInMemory()) { | ||
| ByteBuf buf = fileUpload.getByteBuf(); | ||
| // read from buf | ||
| } else { | ||
| File f = fileUpload.getFile(); | ||
| try (InputStream is = new FileInputStream(f)) { | ||
| // read from stream | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Never call `getFile()` on an in-memory upload -- the file does not exist on disk and the call throws. | ||
|
|
||
| ## `@RequiresRequestContext` + `Config.get()` causes muzzle failure | ||
|
|
||
| Do not declare `static final` fields initialized from `Config.get()` in any `@RequiresRequestContext`-annotated advice inner class. Muzzle validates those classes against the instrumented library's classpath (Netty), where `Config` is absent, causing `MuzzleValidationException` in CI. | ||
|
|
||
| Move such constants to the helper class declared in `helperClassNames()`: | ||
|
|
||
| ```java | ||
| // Wrong -- in @RequiresRequestContext advice inner class | ||
| private static final int MAX_FILES = Config.get().getAppSecMaxFileContentCount(); | ||
|
|
||
| // Correct -- in NettyMultipartHelper or similar helper | ||
| public class NettyMultipartHelper { | ||
| static final int MAX_FILES_TO_INSPECT = Config.get().getAppSecMaxFileContentCount(); | ||
| } | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # Tomcat AppSec Instrumentation | ||
|
|
||
| Extended reference: [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md), | ||
| [docs/appsec/file-content.md](../../../../docs/appsec/file-content.md), | ||
| [docs/appsec/multipart-frameworks.md](../../../../docs/appsec/multipart-frameworks.md) | ||
|
|
||
| ## MUST call `effectivelyBlocked()` in Tomcat blocking path | ||
|
|
||
| Unlike Netty, Tomcat advice must call `effectivelyBlocked()` explicitly. The call must come AFTER | ||
| `t = new BlockingException(...)` and both must be inside `if (brf != null)`: | ||
|
|
||
| ```java | ||
| if (brf != null) { | ||
| brf.tryCommitBlockingResponse(ctx.getTraceSegment(), rba); | ||
| t = new BlockingException("..."); // 1. create first | ||
| ctx.getTraceSegment().effectivelyBlocked(); // 2. mark after | ||
| } | ||
| ``` | ||
|
|
||
| If `effectivelyBlocked()` throws (span already finished), the exception object must already exist | ||
| so the advice can still return a non-null value and let the container propagate it. | ||
|
|
||
| ## `Part` access via reflection: GlassFish restriction does NOT apply here | ||
|
|
||
| Tomcat's `ParameterCollector` uses reflection to avoid bytecode references to `javax` vs `jakarta` | ||
| Part types. This is safe for Tomcat because the agent classloader is not subject to Java 9+ module | ||
| access restrictions for Tomcat's classloader. | ||
|
|
||
| For GlassFish, direct cast via `javax.servlet.http.Part` interface is required instead. | ||
| See [docs/appsec/multipart-frameworks.md](../../../../docs/appsec/multipart-frameworks.md#glassfishpayara-no-reflection-via-parametercollector). | ||
|
|
||
| ## `inspectContent` flag must be evaluated before iterating parts | ||
|
|
||
| Check whether the `requestFilesContent` callback is registered BEFORE iterating parts, not inside | ||
| the loop. This avoids calling `getInputStream()` on every file when no rule uses `files_content`: | ||
|
|
||
| ```java | ||
| boolean inspectContent = cbp.getCallback(EVENTS.requestFilesContent()) != null; | ||
| ParameterCollector collector = new ParameterCollector.ParameterCollectorImpl(inspectContent); | ||
| for (Object part : parts) { | ||
| collector.addPart(part); | ||
| } | ||
| ``` | ||
|
|
||
| ## `helperClassNames()` must list all nested inner classes | ||
|
|
||
| ByteBuddy does not auto-discover nested classes. Every `$Inner` class used at runtime must be | ||
| explicitly listed in `helperClassNames()` or a `NoClassDefFoundError` will occur silently. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # Undertow AppSec Instrumentation | ||
|
|
||
| Extended reference: [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md), | ||
| [docs/appsec/ig-events.md](../../../../docs/appsec/ig-events.md) | ||
|
|
||
| ## `tryCommitBlockingResponse` is not idempotent in Undertow | ||
|
|
||
| Unlike Tomcat/Jersey, Undertow's `tryCommitBlockingResponse` commits the response immediately. | ||
| Calling it twice sends a duplicate response and can throw an `IllegalStateException`. | ||
|
|
||
| When an advice has two blocking paths (body + filenames), the `t == null` guard must be checked | ||
| BEFORE each `tryCommitBlockingResponse` call, not after: | ||
|
|
||
| ```java | ||
| // Wrong -- commits response even if already blocked | ||
| if (bodyCallback != null && !map.isEmpty()) { | ||
| Flow<Void> flow = bodyCallback.apply(ctx, map); | ||
| t = tryBlock(flow, ctx); | ||
| } | ||
| if (filenamesCallback != null && !filenames.isEmpty()) { | ||
| Flow<Void> flow = filenamesCallback.apply(ctx, filenames); | ||
| t = tryBlock(flow, ctx); // tryBlock calls tryCommitBlockingResponse again | ||
| } | ||
|
|
||
| // Correct -- guard prevents second commit | ||
| if (bodyCallback != null && !map.isEmpty()) { | ||
| Flow<Void> flow = bodyCallback.apply(ctx, map); | ||
| t = tryBlock(flow, ctx); | ||
| } | ||
| if (t == null && filenamesCallback != null && !filenames.isEmpty()) { | ||
| Flow<Void> flow = filenamesCallback.apply(ctx, filenames); | ||
| t = tryBlock(flow, ctx); | ||
| } | ||
| ``` | ||
|
|
||
| See [docs/appsec/ig-events.md](../../../docs/appsec/ig-events.md#undertow-trycommitblockingresponse-is-not-idempotent) | ||
| for the full explanation of why this differs from other Servlet containers. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # Vert.x Web AppSec Instrumentation | ||
|
|
||
| Extended reference: [docs/appsec/ig-events.md](../../../../docs/appsec/ig-events.md), | ||
| [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md) | ||
|
|
||
| ## Advice classes are inlined, not injected | ||
|
|
||
| `RoutingContextImplInstrumentation` has no `helperClassNames()` override -- it uses the empty | ||
| default. Advice classes (`RoutingContextJsonAdvice`, `RoutingContextFilenamesAdvice`, etc.) are | ||
| inlined by ByteBuddy, not injected as helpers. Only runtime-instantiated handlers go in | ||
| `helperClassNames()` (e.g. `WafPublishingBodyHandler`). | ||
|
|
||
| New `@RequiresRequestContext` classes for `RoutingContextImplInstrumentation` must NOT be added | ||
| to `helperClassNames()`. | ||
|
|
||
| ## `fileUploads()` requires prior `BodyHandler` execution | ||
|
|
||
| `RoutingContext.fileUploads()` returns an empty set unless a `BodyHandler` has previously parsed | ||
| the body. `setExpectMultipart(true)` + `endHandler` populates `formAttributes()` but NOT file | ||
| uploads. | ||
|
|
||
| ## Use distinct `CallDepthThreadLocalMap` keys per advice class | ||
|
|
||
| Each advice class in `RoutingContextImplInstrumentation` must use a distinct key in | ||
| `CallDepthThreadLocalMap` to avoid re-entrancy interference. Using the same key across the | ||
| filenames advice and the JSON advice causes one to suppress the other. | ||
|
|
||
| ## No `effectivelyBlocked()` in Vert.x advice | ||
|
|
||
| Do not call `effectivelyBlocked()` in Vert.x advice. See | ||
| [docs/appsec/blocking-patterns.md](../../../../docs/appsec/blocking-patterns.md#vertx). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing any one causes a silent no-op-> Maybe we could add a test that checks all these 4 files are coherent in their registered addresses?Then we wouldn't need to document it at all. If you miss them, you get a test error explaining the issue.