Migrate dd-trace-core groovy files to java part 12#11662
Open
jpbempel wants to merge 139 commits into
Open
Conversation
jpbempel
force-pushed
the
jpbempel/g2j-core-pt12
branch
from
2cc67b3 to
35a718a
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
…he kafka-connect-0.11 muzzle range: 7.5.15, 7.6.12, 7.7.10, 7.8.9, 7.9.8 (both -ce and -ccs). Same as we did in #11377. (#11668) Skip the latest broken org.apache.kafka:connect-runtime versions in the kafka-connect-0.11 muzzle range: 7.5.15, 7.6.12, 7.7.10, 7.8.9, 7.9.8 (both -ce and -ccs). Same as we did in #11377. Co-authored-by: alexey.kuznetsov <alexey.kuznetsov@datadoghq.com>
…REPORT_TIMEOUT config keys (#11664) remove(appsec): delete dead DD_APPSEC_REPORTING_INBAND and DD_APPSEC_REPORT_TIMEOUT config keys Both config keys were introduced in 2021 (reporting inband in #1d1fc13, report timeout in #2fd53d8) but the classes that consumed them (InbandReportServiceImpl and ReportServiceImpl) were subsequently removed. The getters isAppSecReportingInband(), getAppSecReportMinTimeout() and getAppSecReportMaxTimeout() are now dead code — no caller exists in the codebase. Remove the constants, defaults, fields, assignments and getters. Refs: APPSEC-68459 remove(appsec): delete dead DD_APPSEC_REPORTING_INBAND and DD_APPSEC_REPORT_TIMEOUT from supported-configurations metadata Follow-up to the previous commit removing these keys from Config.java. The metadata/supported-configurations.json entries for DD_APPSEC_REPORTING_INBAND and DD_APPSEC_REPORT_TIMEOUT were still advertising these config keys as supported, which caused GeneratedSupportedConfigurations to include them even though Config no longer reads them. Merge branch 'master' into alejandro.gonzalez/APPSEC-68459-remove-dead-appsec-config Merge branch 'master' into alejandro.gonzalez/APPSEC-68459-remove-dead-appsec-config Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
fix muzzle guard in vertx5 to not exclude 5.1 also exclude iast forked latest tests Merge branch 'master' into vandonr/vertx51 Co-authored-by: raphael.vandon <raphael.vandon@datadoghq.com>
Improved report to be more human-readable. Co-authored-by: alexey.kuznetsov <alexey.kuznetsov@datadoghq.com>
…ps all other elements. (#11672) Fix bug where calling 'context.with((ImplicitContextKeyed) null)' drops all other elements. Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
refactor: Apply normalization on every project
perf: Insulate the BuildTimeInstrumentationPlugin from changes using a ClasspathNormalizer
BuildTimeInstrumentationPlugin install a post-processing step onto
normal compile tasks.
Flow:
1. It creates a resolvable `buildTimeInstrumentationPlugin`
configuration
2. For each main `compileJava` / `compileGroovy` / `compileScala` task,
it registers that configuration as a **compile-task input**
3. Then appends a `doLast("instrumentClasses", ...)` action to
post-process compiled classes. That action runs Byte Buddy plugins
against the compiled class output.
If any input for that task changes, the whole compile task reruns.
In particular the Byte Buddy plugins are tracked via
```kotlin
inputs.files(project.configurations.named(BUILD_TIME_INSTRUMENTATION_PLUGIN_CONFIGURATION))
```
In particular instrumentation plugins rely on instrumentation plugins
coming from `agent-tooling` project which also happens to have a version
file
```Gradle
subProj.configurations.named('buildTimeInstrumentationPlugin') {
it.dependencies.add(subProj.dependencies.project(
path: ':dd-java-agent:agent-tooling',
configuration: 'buildTimeInstrumentationToolingPlugins'
))
```
Before the normalizer, Gradle hashed the full jar contents of the
build-time instrumentation plugin classpath. And as such the jars on
that configuration changed only because their generated *.version
changed after a new Git commit. And as such invalidated the task output.
Adding the normalizer to this input only teaches Gradle to use the
normalizaton declared in the project to ignore the version file on
this input.
Important nuance: `ClasspathNormalizer` is better than
`CompileClasspathNormalizer` here. These are Byte Buddy plugins executed
at build time, so method-body/resource changes in those plugin jars
can change generated bytecode. We only want to ignore the known
volatile version resource.
---
Before this change the compile task amounted to a significant part when
only the commit version changed
```
327.901s compile*
10.486s codenarcTest
9.576s *Jar
8.217s process*Resources
2.208s expandAgentShadowJar*
2.032s generate*Index
1.241s writeVersionNumberFile
```
After this change
```
14.780s compile* # up-to-date checking, not recompilation
7.469s process*Resources
6.194s *Jar
5.056s codenarcTest
2.233s writeVersionNumberFile
1.847s generate*Index
0.899s expandAgentShadowJar*
```
Co-authored-by: brice.dutheil <brice.dutheil@datadoghq.com>
Ignore capturing connection continuation for armeria Co-authored-by: andrea.marziali <andrea.marziali@datadoghq.com>
…e PR. (#11676) Fix new instrumentation lock files leaking into core dependency-update PR. Co-authored-by: alexey.kuznetsov <alexey.kuznetsov@datadoghq.com>
chore(build): Bump Gradle to 9.6.0 fix(test): update dependencies report assertion for Gradle 9.6 project path quoting Gradle 9.6 wraps project paths in single quotes in the dependencies report output: `project ':dd-java-agent:agent-bootstrap'` instead of the previous `project :dd-java-agent:agent-bootstrap`. Drop the `project` prefix from the assertion so the check matches both formats. fix(sofarpc): force netty-all to 4.1.79 to resolve classpath conflict under Gradle 9.6 The `sofa-rpc-all:5.14.2` brings `netty-all:4.1.44.Final`, which is a fat jar, while `grpc-netty:1.53.0` brings individual `netty-*:4.1.79.Final` modules. Gradle 9.6.0 introduced a regression in the classpath ordering, as they are working on revamping the dependency traversal. The result is that the `4.1.44` fat jar classes appears earlier and its classes are loaded from it ; This produces a `NoSuchMethodError` at runtime in netty's `AbstractReferenceCountedByteBuf`. The workaround is to force `netty-all` to `4.1.79` so the fat jar and the individual modules are aligned on the same version. This should be fixed in 9.7.0 and consequently removed from the `resolutionStrategy`. See gradle/gradle#38057 refactor(build): Migrate away from property delegate (deprecated in 9.6.0) Gradle 9.6.0 deprecates a few old API, unfortunately Kotlin property delegates are part of the bag. gradle/gradle#37555 gradle/gradle#37556 https://docs.gradle.org/current/userguide/upgrading_version_9.html#kotlin_dsl_delegated_properties chore(build): Bump Gradle version in GitLab CI cache config to 9.6.0 Co-authored-by: brice.dutheil <brice.dutheil@datadoghq.com>
chore: Update Gradle dependencies Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: bruce.bujon <bruce.bujon@datadoghq.com>
feat(core): Improve http codec extractor tests Extract the ContextInterpreter tests related to AppSec to remove duplication. Handle extractor initialization in a new parent test class Deduplicate constants and values into the new parent test class Co-authored-by: bruce.bujon <bruce.bujon@datadoghq.com>
Improve crash report message when signal is absent Co-authored-by: andrea.marziali <andrea.marziali@datadoghq.com>
Remove testcontainer dependencies for buildId tests Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
chore: Update smoke test latest tool versions Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> fix: filter RC and milestone versions for maven ci: bump surefire version to 3.5.6 Co-authored-by: daniel-mohedano <daniel.mohedano@datadoghq.com>
feat(core): Improve http codec injector tests Co-authored-by: bruce.bujon <bruce.bujon@datadoghq.com>
Instrument Jetty for server.request.body.filenames Add GetFilenamesAdvice to all three Jetty AppSec modules to collect uploaded file names from multipart requests and fire the requestFilesFilenames() IG callback: - jetty-appsec-8.1.3: intercepts getParts() return value; includes Content-Disposition header fallback for Servlet 3.0 (Jetty 9.0) where getSubmittedFileName() is not available - jetty-appsec-9.2: intercepts no-arg getParts() for Servlet 3.1+ - jetty-appsec-9.3: same, applies to Jetty 9.3, 10, 11 Enable testBodyFilenames() in Jetty 9.x, 10 and 11 server tests. Fix GetFilenamesAdvice double-firing and extend coverage to getParts(MultiMap) path - jetty-appsec-9.3: add call-depth guard (Collection.class) to GetFilenamesAdvice to prevent double callback invocation when getParts() calls getParts(MultiMap) internally - jetty-appsec-9.2: extend GetFilenamesAdvice matcher to all getParts overloads (not just no-arg) to cover getParameter*()/getParameterMap() code paths, guarded with same call-depth mechanism to avoid double-firing spotless Extend testBodyFilenamesCalledOnce coverage to Jetty 9.x and 10.x - Add BODY_MULTIPART_REPEATED case to TestServlet3 (javax) so Jetty 9.x/10.x test modules can exercise the repeated getParts() scenario - Enable testBodyFilenamesCalledOnce() for Jetty 9.0, 9.0.4, 9.3, 9.4.21, and 10.0 Add BODY_MULTIPART_COMBINED test to cover GetFilenamesFromMultiPartAdvice path - New BODY_MULTIPART_COMBINED endpoint: calls getParameterMap() first (triggers GetFilenamesFromMultiPartAdvice via extractContentParameters -> getParts(MultiMap)), then getParts() explicitly (GetFilenamesAdvice must not double-fire since _contentParameters is already set) - New test 'file upload filenames called once via parameter map' verifies the callback fires exactly once across both advice paths - Enabled in Jetty 9.0, 9.0.4, 9.3, 9.4.21, 10.0 and 11.0 spotless Fix missing static imports for BODY_MULTIPART_REPEATED and BODY_MULTIPART_COMBINED Fix GetFilenamesAdvice double-fire for Jetty 9.4+ where _multiParts replaces _contentParameters as the getParts() cache In Jetty 9.3, getParts(MultiMap) sets _contentParameters, so the map==null guard prevents re-firing on repeated getParts() calls. In Jetty 9.4+, getParts() delegates to getParts(null) and caches the result in _multiParts instead, leaving _contentParameters null on every call. Add _multiParts==null as an additional guard (optional=true handles Jetty 9.3 where the field does not exist). Fix GetFilenamesAdvice double-fire in jetty-appsec-8.1.3 In Jetty 8.x/9.0, _multiPartInputStream is null only on the first getParts() call. Add OnMethodEnter guard to skip the WAF callback on subsequent calls which return the cached multipart result. Fix GetFilenamesAdvice double-fire for all Jetty 9.3–11 versions @advice.FieldValue(optional=true) is not supported in ByteBuddy 1.11.22. Replace it with @Advice.This + inline reflection to detect whether getParts() has already been called on this request: - Jetty 9.4+: checks _multiParts (set after first getParts() call) - Jetty 9.3.x: falls back to _multiPartInputStream (the cache field in 9.3.x, where _multiParts does not exist and _contentParameters is only set by the getParameterMap() → extractContentParameters() path, not by getParts()) Covers all forkedTest and latestDepForkedTest suites for Jetty 9.0–11. Simplify GetFilenamesAdvice in jetty-appsec-8.1.3: remove dead Servlet 3.1+ branch Jetty 8 implements only Servlet 3.0, so getSubmittedFileName() is never present on the Part objects. The reflection probe (try { getMethod("getSubmittedFileName") }) and the Servlet 3.1+ code path were dead code. Remove them and always parse filenames from the Content-Disposition header directly. Remove unnecessary casts in Jetty AppSec GetFilenamesAdvice Type @Advice.Return as Collection<Part> so the loop variable can be Part directly, eliminating the (Part) cast on each iteration. Extract Content-Disposition parsing to MultipartHelper + unit tests Move the filename extraction logic from GetFilenamesAdvice into a new MultipartHelper helper class so it can be unit tested in isolation. Add 12 Spock test cases covering quoted/unquoted filenames, empty values, whitespace, null input, and edge cases. Extract filename extraction to MultipartHelper in jetty-appsec-9.2 + unit tests Move the getSubmittedFileName() loop from GetFilenamesAdvice into a new MultipartHelper helper class (injected via helperClassNames) so it can be unit tested in isolation. Add 8 Spock test cases covering null/empty collections, null/empty filenames, multiple parts, and special characters. Split jetty-appsec-9.3 [9.3,12) into three clean modules: 9.3, 9.4, 11.0 Eliminates all reflection from the multipart filename instrumentation by creating version-specific modules with compile-time type safety: - jetty-appsec-9.3 [9.3,9.4): javax.servlet, uses _multiPartInputStream field - jetty-appsec-9.4 [9.4,11.0): javax.servlet, uses _multiParts field - jetty-appsec-11.0 [11.0,12.0): jakarta.servlet, uses _multiParts field Each module uses muzzle references as version discriminators instead of runtime reflection, and delegates filename extraction to a testable MultipartHelper class with 8 Spock unit tests each. Server test modules updated to reference the correct appsec module per Jetty version range. Add Jetty 8.x integration tests for multipart body and filenames - Add Jetty8LatestDepForkedTest: runs against Jetty 8.x (latestDepForkedTest task) and enables testBodyMultipart/testBodyFilenames coverage. Gated by 'test.dd.filenames' system property so it is skipped when running against Jetty 7.6. - Add testCompileOnly dep on org.eclipse.jetty.orbit:javax.servlet so MultipartConfigElement compiles without pulling in the excluded javax.servlet:javax.servlet-api artifact. - Fix ParameterCollector.put to accept (Object, Object) and cast internally: Jetty 8.x MultiMap.add uses (Object, Object) descriptor while Jetty 9.x uses (String, Object), so the ASM bytecode visitor was silently skipping all form field captures on Jetty 8. - Update GetPartsMethodVisitor to match both (String,Object) and (Object,Object) MultiMap.add descriptors and emit the INVOKEINTERFACE with (Object, Object). Fix muzzle range for jetty-appsec-9.3/9.4: split at 9.4.10 _multiPartInputStream was replaced by _multiParts in Jetty 9.4.10.v20180503. Early 9.4.x versions (9.4.0–9.4.9) still use _multiPartInputStream like 9.3.x, so extend jetty-appsec-9.3 to cover [9.3, 9.4.10) and narrow jetty-appsec-9.4 to [9.4.10, 11.0). The classLoaderMatcher in jetty-appsec-9.4 (checking for _multiParts) now correctly matches only versions >= 9.4.10. Replace MultiPartsFieldMatcher with typed module splits for Jetty 9.4–11.x The _multiParts field type changes multiple times across Jetty versions, making a single typed muzzle reference insufficient. Replace the ASM-based classLoaderMatcher with clean module splits using typed muzzle references: - jetty-appsec-9.4 [9.4.10, 10.0): _multiParts: MultiParts _queryEncoding: String (excludes 10.x) - jetty-appsec-10.0 [10.0, 10.0.10): _multiParts: MultiPartFormInputStream - jetty-appsec-10.0.10 [10.0.10, 11.0): _multiParts: MultiParts _queryEncoding: Charset (excludes 9.4.x) - jetty-appsec-11.0 [11.0, 11.0.10): _multiParts: MultiPartFormInputStream - jetty-appsec-11.0.10 [11.0.10, 12.0): _multiParts: MultiParts All six modules pass muzzle with assertInverse = true. Fix CI failures and reduce agent JAR size for Jetty filenames PR - Add jetty-appsec-9.4 and jetty-appsec-11.0 to armeria-jetty-1.24 testRuntimeOnly classpath; these are required after the muzzle split that tightened jetty-appsec-9.3 to [9.3, 9.4.10) - Merge jetty-appsec-10.0 functionality into jetty-appsec-9.4: extend the muzzle from [9.4.10, 10.0) to also cover [10.0.10, 11.0) using _multiParts:MultiParts + javax.servlet.http.Part as discriminators; delete the now-redundant jetty-appsec-10.0 module - Delete intermediate modules jetty-appsec-10.0.10 and jetty-appsec-11.0.10 that were split-by-point-release and now handled by the two modules above - Scope server.request.body.filenames to Jetty 9.3+: revert filenames additions from jetty-appsec-8.1.3 (Jetty 8, EOL 2015) and jetty-appsec-9.2 (Jetty 9.2, EOL 2019) to stay within the 32 MiB agent JAR limit; requestBodyProcessed support for those versions is unchanged Agent JAR: 33,551,402 bytes (3,030 bytes under the 32 MiB limit) Remove redundant .or(named("getParts")) from ExtractContentParametersAdvice matcher ExtractContentParametersAdvice applied to getParts() is always a no-op: it increments/decrements CallDepthThreadLocalMap but never fires since _contentParameters is always null at that point. GetFilenamesAdvice and GetFilenamesFromMultiPartAdvice handle getParts() exclusively. Disable testBodyFilenames in jetty-server-9.0 and jetty-server-9.0.4 jetty-appsec-8.1.3 (covers Jetty <9) and jetty-appsec-9.2 (covers Jetty [9.2,9.3)) were reverted to master state: they report requestBodyProcessed but not requestFilesFilenames. Jetty 9.0.x and 9.2.x therefore have no active appsec module that fires the filenames event, so testBodyFilenames() must stay false for those modules. Restore .or(named("getParts")) in ExtractContentParametersAdvice for jetty-appsec-9.3/9.4/11.0 In Armeria + Jetty 9.4.48 (and possibly other embedded/wrapped setups), getParts() is the application entry point for multipart parsing — it internally calls extractContentParameters(), which sets _contentParameters. Without the matcher on getParts(), ExtractContentParametersAdvice.after() never fires in that code path, so the requestBodyProcessed event (and request.body.converted tag) is never reported. The call-depth guard (Request.class key) already prevents double-firing when both methods are instrumented: getParts() increments depth to 1 before extractContentParameters() is entered, so the nested advice is a no-op. fix(appsec/jetty): use _dispatcherType bytecode discriminator for jetty-appsec-9.4 Replace the JAVAX_PART_REFERENCE muzzle check (class presence on classpath) with a field-level bytecode check on Request._dispatcherType descriptor: - Ljavax/servlet/DispatcherType; in Jetty 9.4/10 (javax namespace) - Ljakarta/servlet/DispatcherType; in Jetty 11+ (jakarta namespace) This reliably excludes Jetty 11+ even when both javax and jakarta servlet jars are on the test classpath simultaneously. Also remove assertInverse from 9_series muzzle block to avoid conflict with 10_series (both cover overlapping Jetty 10.0.x versions). Fixes armeria-jetty-1.24:jetty11Test ClassCastException where jetty94.MultipartHelper tried to cast jakarta.servlet.http.Part to javax.servlet.http.Part. Skip multipart test on Jetty 9.0.x where jetty-appsec-8.1.3 causes HTTP 500 jetty-appsec-8.1.3 muzzle range [8.1.3, 9.2.0.RC0) includes Jetty 9.0.x. When applied, it calls ParameterCollector.put(String, String) which does not exist in Jetty 9.0.x → HTTP 500 on multipart requests. Override multipart() with assumeTrue(false) in jetty-server-9.0 and jetty-server-9.0.4 test classes until the muzzle range is corrected. Use @ignore to skip multipart test on Jetty 9.0.x where jetty-appsec-8.1.3 causes HTTP 500 Replace assumeTrue(false) with Spock's @ignore annotation — more reliable for overriding void feature methods without Spock block labels. jetty-appsec-8.1.3 muzzle range [8.1.3, 9.2.0.RC0) includes Jetty 9.0.x. When applied, it calls ParameterCollector.put(String, String) which does not exist in Jetty 9.0.x → HTTP 500 on multipart requests. Add supportsMultipart() hook to skip multipart test on Jetty 9.0.x Spock AST-transforms feature methods, so @OverRide on a feature method override triggers a compile error. Use a boolean hook instead: - AppSecInactiveHttpServerTest.supportsMultipart() returns true by default and is guarded with assumeTrue() at the setup: block level - Jetty9InactiveAppSecTest overrides to false in both jetty-server-9.0 and jetty-server-9.0.4, where jetty-appsec-8.1.3 range [8.1.3, 9.2.0.RC0) causes ParameterCollector.put(String, String) to fail → HTTP 500. Verified locally: tests=8, skipped=3 (multipart correctly skipped), failures=0. fix(appsec/jetty): disable multipart test on Jetty 9.0.x and bump test version to 10.0.10 - jetty-server-9.0 and jetty-server-9.0.4: set testBodyMultipart() = false. jetty-appsec-8.1.3 covers [8.1.3, 9.2.0.RC0) which includes Jetty 9.0.x; its extractContentParameters() calls ParameterCollector.put(String, String) which does not exist in Jetty 9.0.x, causing HTTP 500 on multipart requests. - jetty-server-10.0: bump test dependency from 10.0.0 to 10.0.10. jetty-appsec-9.4 requires _multiParts: Lorg/eclipse/jetty/server/MultiParts; but that type was only introduced in 10.0.10 (previously MultiPartFormInputStream), so the muzzle check fails for Jetty 10.0.0-10.0.9, leaving tests uninstrumented. - Revert accidental modification of agent-jmxfetch/integrations-core submodule back to its master pointer. fix(appsec/jetty): replace bytecode injection in jetty-appsec-8.1.3 with exit-advice Replace the brittle GetPartsMethodVisitor ASM bytecode injection (intercepting MultiMap.add() calls inside getParts()) with a clean ByteBuddy exit-advice approach: - Remove ParameterCollector, GetPartsAdvice, GetPartsVisitorWrapper, RequestClassVisitor, and GetPartsMethodVisitor; drop HasTypeAdvice from the instrumentation module. - Add PartHelper with extractFormFields() (reads InputStream per part) and extractFilenames() (parses Content-Disposition manually, since Part.getSubmittedFileName() is Servlet 3.1+ and not available in Jetty 8.x). - Add GetFilenamesAdvice (@RequiresRequestContext / @ActiveRequestContext) on getParts():Collection that fires requestBodyProcessed() with form fields and requestFilesFilenames() with upload filenames. Uses CallDepthThreadLocalMap to guard against reentrant calls. - Jetty8LatestDepForkedTest: set testBodyFilenamesCalledOnce() and testBodyFilenamesCalledOnceCombined() to false because Jetty 8.x has no _multiParts / _contentParameters field guards to suppress duplicate firings. test(appsec/jetty): add unit tests for PartHelper (jetty-appsec-8.1.3) fix(appsec/jetty): cover Jetty 10.0.0-10.0.9 and close PartHelper InputStream - jetty-appsec-9.4: extend muzzle to also cover Jetty 10.0.0–10.0.9, which were previously a gap. In those versions _multiParts is typed MultiPartFormInputStream (not MultiParts); the primary Reference spec matches 9.4.10+ and 10.0.10+, and an OrReference alternative matches 10.0.0–10.0.9. The GetFilenamesAdvice already uses typing=DYNAMIC so no advice changes are needed. - jetty-appsec-8.1.3 PartHelper: wrap part.getInputStream() in try-with-resources to avoid leaking file descriptors on file-backed multipart form fields. fix(appsec/jetty): quote-aware Content-Disposition parser in PartHelper Splitting the header on ';' naively truncated filenames that contain semicolons inside a quoted value, e.g. filename="shell;evil.php" would produce "shell" instead of the full name. Replace the split() loop with a quote-aware state-machine parser that skips semicolons inside quoted strings and handles backslash-escaped characters. Add test cases for semicolons in filenames, escaped quotes, and filename appearing before other parameters. fix(appsec/jetty): use Request bytecode discriminator for jetty-appsec-11.0 Replace the JAKARTA_PART_REFERENCE classpath check with a _dispatcherType field descriptor check on Request.class bytecode, mirroring the approach already used by jetty-appsec-9.4. The classpath check passes on any Jetty 9.4/10 app that has jakarta.servlet-api as a dependency, causing double-instrumentation of extractContentParameters. The bytecode check is authoritative: in Jetty 11+ Request.class carries _dispatcherType as Ljakarta/servlet/DispatcherType;, while 9.4/10 carry the javax descriptor. feat(appsec): advise getPart(String) in Jetty 8 to catch single-part uploads In Jetty 8.x, getPart(String name) calls _multiPartInputStream.getPart(String) directly without delegating to getParts(). Applications that retrieve only one file via getPart() without ever calling getParts() would have their filename event missed. Add GetPartAdvice to cover this path. The charset fix (AI comment 2) was investigated and is not applicable: HTML5 form submissions always use UTF-8 and browsers never include charset= on individual part Content-Type headers, so the existing hardcoded UTF-8 is correct. fix(appsec): guard Jetty 8 getParts against repeated calls; disable filename tests in async suite 1. Add _multiPartInputStream == null guard to GetFilenamesAdvice.before() so that repeated getParts() calls on the same request (which Jetty caches) do not re-fire requestFilesFilenames/requestBodyProcessed WAF callbacks. The field is null before the first multipart parse and non-null on all subsequent cached calls, matching the pattern used in the 9.4/11.0 advice (_multiParts guard). 2. JettyAsyncHandlerTest already disabled testBodyFilenames() but neglected to disable testBodyFilenamesCalledOnce() and testBodyFilenamesCalledOnceCombined(), which are now enabled in the Jetty11Test parent. Override both to false in the async handler suite to prevent spurious test failures. fix(appsec): distinguish empty filename from absent filename in PartHelper filenameFromPart() was returning null for both 'no filename parameter' and 'filename=""', causing extractFormFields() to buffer the full body of file inputs submitted with no file chosen (filename=""). An empty <input type=file> is still a file part, not a form field. Return "" instead of null so that callers using != null correctly skip those parts without reading their content. Update tests to assert "" for empty-filename cases and add regression tests for extractFormFields/extractFilenames with empty-filename parts. Note: the second AI comment about getPart(String) double-firing was not implemented. The bytecode shows the internal call is to MultiPartInputStream.getParts() (not Request.getParts()), so GetFilenamesAdvice (which instruments Request.getParts()) is never triggered during a getPart() call. There is no double-firing. test(appsec): add earlyDep10ForkedTest to cover Jetty 10.0.0–10.0.9 The jetty-appsec-9.4 early_10_series muzzle pass covers [10.0.0, 10.0.10) where _multiParts is MultiPartFormInputStream (before the type changed to MultiParts in 10.0.10). The existing test suite used 10.0.10, leaving the early versions untested. Add earlyDep10ForkedTest (Jetty 10.0.9) to exercise the OR-muzzle reference and verify that multipart filename events fire correctly on the old field type. All three filename tests pass: filenames, filenames-called-once, filenames-combined. refactor(appsec/jetty): extract fire-event logic into helpers; fix ASM API constant Move duplicated IG-callback + blocking-commit blocks out of advice into helper methods (MultipartHelper.fireFilenamesEvent / PartHelper.fireBodyProcessedEvent + fireFilenamesEvent) that return BlockingException|null, exploiting @Advice.Thrown(readOnly=false) assignment behaviour. Also replace hardcoded Opcodes.ASM9 with OpenedClassReader.ASM_API in RequestGetPartsInstrumentation and fix extractFormFields to use computeIfAbsent instead of get+put. fix(appsec/jetty8): prevent double-firing filename event when getPart() delegates to getParts() In Jetty 9.0/9.1 (covered by jetty-appsec-8.1.3's [8.1.3,9.2.0.RC0) range), Request.getPart(String) delegates to getParts() internally. Without this fix, GetFilenamesAdvice fires for the full collection (via the internal getParts() call) and then GetPartAdvice fires again for the returned singleton, because the two advice classes use independent call-depth keys and cannot see each other. Fix: peek at the Part.class call depth in GetFilenamesAdvice.before(). If > 1, GetPartAdvice is active on the stack and will handle the event; skip to avoid double-firing. In Jetty 8.x (where getPart() does not delegate), the peek is always 1, so GetFilenamesAdvice continues to fire normally. fix(appsec/jetty8): correct partPeek condition — use == 0 instead of == 1 CallDepthThreadLocalMap.incrementCallDepth() uses post-increment (depth++) so it returns the depth BEFORE the increment. In the normal case (no GetPartAdvice active), Part.class depth is 0 before the peek, making partPeek == 0. The previous guard checked partPeek == 1 which is true only when GetPartAdvice IS already on the stack — exactly backwards. Result: GetFilenamesAdvice never fired for direct getParts() calls, breaking the filenames event on Jetty 8.x. Fix: use partPeek == 0 (proceed when GetPartAdvice is not active) so the condition is: - partPeek == 0: normal direct getParts() → proceed (fire filenames event) - partPeek == 1: getParts() called by getPart() in Jetty 9.0/9.1 → skip (GetPartAdvice handles it) fix(appsec/jetty8): inspect all cached parts in GetPartAdvice, not just the singleton When getPart("field") is the app's first multipart access, Jetty 8 parses the entire multipart stream and caches all parts in _multiPartInputStream, but only returns the one requested part. The previous advice forwarded just that singleton to AppSec, so any co-uploaded file parts were invisible to requestFilesFilenames — a WAF bypass if the app never called getParts() explicitly. Fix: read all cached parts via MultiPartInputStream.getParts() (reflected, cached handle) and fall back to the singleton only when reflection fails. Also remove the part==null early return: even if the requested field was not found, other file parts may have parsed. Add PartHelper.getAllParts(Object, Part) + FakeMpi unit tests. fix(appsec/jetty8): guard GetPartAdvice against repeated getPart() calls The Part.class depth check only prevents re-entry within a single getPart() invocation; after the call returns the depth is 0 again, so a second getPart("file") call on the same request would re-fire requestBodyProcessed and requestFilesFilenames with the same cached parts. Add the same _multiPartInputStream == null guard that GetFilenamesAdvice already uses: once the field is set the multipart body was parsed and events were already dispatched — skip. fix(appsec/jetty8): respect part Content-Type charset in extractFormFields Hard-coded UTF-8 in readPartContent() caused mojibake for fields with Content-Type: text/plain; charset=ISO-8859-1 (or any non-UTF-8 charset). Now parses the charset from the part's Content-Type header and falls back to UTF-8 only when absent or unrecognised. fix(appsec/jetty10): regenerate lockfile and force 10.0.9 for earlyDep10ForkedTest The lockfile was out of sync: testCompileClasspath was locked to 10.0.0 instead of 10.0.10, and earlyDep10ForkedTest configurations were missing entirely. Regenerated with --write-locks. Also added a resolutionStrategy to force Jetty 10.0.9 for earlyDep10ForkedTest: without it, Gradle picks 10.0.10 (inherited via testImplementation), defeating the purpose of the suite which exercises the MultiPartFormInputStream path present in [10.0.0, 10.0.10). fix(appsec/jetty): pendingBlock pattern, jetty9.2 filenames, tryCommit check, per-part try/catch - Fix body+filenames ordering bug in jetty8 GetFilenamesAdvice and GetPartAdvice: both callbacks now always fire even when body blocks (pendingBlock pattern, appsec-ig-events §7) - Add requestFilesFilenames support for jetty-appsec-9.2 ([9.2, 9.3)): new MultipartHelper, GetFilenamesAdvice, GetFilenamesFromMultiPartAdvice, and integration test Jetty92LatestDepForkedTest - Fix tryCommitBlockingResponse() return not checked before effectivelyBlocked() in PartHelper (jetty8) and MultipartHelper (jetty9.3, 9.4, 11.0) - Add per-part try/catch in extractFilenames() and extractFormFields() loops in PartHelper and all MultipartHelper implementations test(appsec/jetty): migrate 8 new Groovy test files to Java/JUnit 5 Satisfies the enforce-groovy-migration CI check. All 8 Groovy test files introduced in this PR are replaced by Java equivalents: - jetty-appsec-8.1.3: PartHelperTest (36 unit tests via Mockito) - jetty-appsec-9.2/9.3/9.4: MultipartHelperTest (8 unit tests each) - jetty-appsec-11.0: MultipartHelperTest using jakarta.servlet.http.Part - jetty-server-7.6: Jetty8LatestDepForkedTest (extends Jetty76Test) - jetty-server-9.0.4: Jetty92LatestDepForkedTest (extends Jetty9Test) - jetty-server-10.0: Jetty10EarlyDepForkedTest (extends Jetty10Test) Integration tests extending Groovy Spock base classes are placed in src/test/groovy/ as .java files so groovyc compiles them together with the Groovy classes they extend, avoiding the compileTestJava ordering issue. This pattern is already used elsewhere in the repo. test(appsec/jetty): delete the 8 replaced Groovy test files fix(appsec/jetty): add catch comments and remove dead call-depth decrement - Add explanatory comments to all catch(Exception ignored) blocks in PartHelper and MultipartHelper across jetty8/9.2/9.3/9.4/11.0 modules, consistent with the pattern used in Jersey and GlassFish helpers - Remove spurious CallDepthThreadLocalMap.decrementCallDepth(Request.class) from jetty92 GetPartsAdvice.after() which had no matching increment and was corrupting the thread-local counter refactor(appsec/jetty): address review comments from amarziali - ArrayList(parts.size()) in extractFilenames() across all 5 modules to pre-size the list and avoid rehashing - Callback-before-extract optimization in fireFilenamesEvent() and fireBodyProcessedEvent(): check callback != null before calling extractFilenames()/extractFormFields() so we skip Part iteration when AppSec is not listening - WithTypeStructure.structureMatcher() in jetty-appsec-11.0 to replace additionalMuzzleReferences() for javax vs jakarta DispatcherType discrimination; evaluated per-class at transformation time - Skip-increment optimization in ExtractContentParametersAdvice and GetFilenamesAdvice (jetty-appsec-11.0): return false early when guard fields are already non-null, avoiding CallDepthThreadLocalMap overhead on cached repeat calls - IODH + MethodHandle in PartHelper (jetty-appsec-8.1.3) replacing volatile Method for MultiPartInputStream.getParts() resolution; inner class MpiGetPartsHolder added to helperClassNames() fix(appsec/jetty8): duck-typed reflection fallback in getAllParts() when IODH handle is null MpiGetPartsHolder resolves MultiPartInputStream.getParts() at class-load time via Class.forName(). In test environments where jetty-server is not on the test classpath (compileOnly only), GET_PARTS is null and the MethodHandle path is unavailable. Add duck-typed reflection as fallback: when GET_PARTS is null, call getParts() via getClass().getMethod("getParts").invoke(...). In production the fast MethodHandle path always wins; the reflection fallback only activates when the class could not be resolved. This restores getAllPartsReturnsAllPartsFromMultiPartInputStream and getAllPartsPrefersFullCollectionOverSingleton unit tests that regressed after the IODH refactor. fix(appsec/jetty11): revert GetFilenamesAdvice skip-increment optimization The skip-increment optimization introduced in the amarziali review commit broke the shared Collection.class depth counter that GetFilenamesAdvice and GetFilenamesFromMultiPartAdvice rely on to coordinate. In Jetty 11.0.0, public getParts() delegates to the private getParts(MultiMap) overload. GetFilenamesAdvice increments Collection depth before the public method runs, which raises the depth so GetFilenamesFromMultiPartAdvice (also applied to the private overload) sees depth > 0 and skips firing. When the optimization skipped the increment because _multiParts != null, the private getParts(MultiMap) call found Collection depth = 0 on the second and third repeated calls and fired the requestFilesFilenames callback again, causing _dd.appsec.filenames.cb.calls = 3 (BODY_MULTIPART_REPEATED) and = 2 (BODY_MULTIPART_COMBINED) instead of the expected 1. Fix: restore the original always-increment/always-decrement pattern in GetFilenamesAdvice so the depth counter remains balanced on every code path. refactor(appsec/jetty8): remove duck-typed fallback from getAllParts, use real MultiPartInputStream in tests The reflection fallback (getClass().getMethod("getParts").invoke(...)) was only needed because MultiPartInputStream was not on the test classpath, causing the IODH MethodHandle to initialize to null. Fix the root cause instead: add testImplementation for jetty-server 8.1.3.v20120416 and replace the FakeMpi stub with mock(MultiPartInputStream.class). fix(appsec/jetty8): add log.debug to all PartHelper catch blocks - getAllParts: log.debug on MethodHandle invocation failure - extractFilenames: log.debug when skipping a malformed part - extractFormFields: log.debug when skipping a malformed part - readPartContent: log.debug on IOException before returning null IODH static block and charsetFromContentType left comment-only: static initializer context and attacker-controlled input respectively. fix(appsec/jetty8): replace increment+decrement peek with getCallDepth Use CallDepthThreadLocalMap.getCallDepth(Part.class) to read the current depth without modifying it, instead of the equivalent incrementCallDepth + decrementCallDepth pair. fix(appsec/jetty): use MultipartHelper.class as CallDepthThreadLocalMap key Replace Collection.class with MultipartHelper.class in all four jetty appsec modules (9.2, 9.3, 9.4, 11.0). Collection.class is a generic JDK class that any other instrumentation could use as a key on the same thread, silently corrupting call depth tracking. MultipartHelper.class is specific to each module and cannot collide with other instrumentations. Merge branch 'master' into alejandro.gonzalez/APPSEC-61873-3 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
feat: Expand the jar checks to stricter agent jar validation * Maintains the size check. * Verify some required entries * Ensure there's a minimum number of classes in the whole jar * Ensure products are correctly included and have at least one class * Light size check on the indexes * Fixed list of packages that should not appear in the jar * Run checks as part of the build job, to catch issues earlier Co-authored-by: brice.dutheil <brice.dutheil@datadoghq.com>
… values (#11604) fix: waf.init and waf.updates metrics emit per-event count 1 instead of sequential counters Removes wafInitCounter and wafUpdatesCounter AtomicInteger fields from WafMetricCollector. Both wafInit() and wafUpdates() now use literal 1L, matching RFC semantics (COUNT-1-per-event). The old sequential counters (1, 2, 3, ...) caused success:N/A when the rawMetricsQueue was full and a metric was dropped — the counter advanced but the offer() silently failed, creating gaps the backend interpreted as success:N/A. Also updates currentRuleVersion from e.wafDiagnostics.rulesetVersion in the InvalidRuleSetException catch block of handleWafUpdateResultReport(), so wafUpdates() emits with the correct event_rules_version even on partial WAF config errors. Raises log level from debug to warn. fix: only update statsReporter rule version when rules were actually loaded on error On InvalidRuleSetException, always update currentRuleVersion and setRuleVersion() on modules so wafUpdates() emits with the correct event_rules_version. But only update statsReporter (trace tagging) if rules.getLoaded() is non-empty, mirroring the success path guard. This avoids tagging traces with a version whose rules failed to load. Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
… (#11694) fix: Forbid OkHostnameVerifier to prevent accidental use, CVE-2021-0341 See #11631 Co-authored-by: brice.dutheil <brice.dutheil@datadoghq.com>
…11693) Cleanup of Spotbugs annotations. Removed Spotbugs from test scope. Fixed review. Co-authored-by: alexey.kuznetsov <alexey.kuznetsov@datadoghq.com>
Add missing repositories to Update Gradle dependencies workflow Co-authored-by: sarah.chen <sarah.chen@datadoghq.com>
Add SCA benchmark gitlab config Co-authored-by: sarah.chen <sarah.chen@datadoghq.com>
Pre-construct TagMap.Entry objects in InternalTagsAdder InternalTagsAdder set base.service / version via TagMap.set(tag, value), allocating a fresh TagMap.Entry per span. Both values are fixed for the life of the tracer, and TagMap.Entry objects are safe to share across maps (the OptimizedTagMap collision design relies on it), so build the two Entry objects once in the constructor and reuse them via set(entry). A JFR profile of petclinic (2026-06-03) attributed ~52 allocation samples to InternalTagsAdder.processTags (one Entry per span); this drops them to zero. Re-applies the change from the stale PR #10965 (415 commits behind master, drifted signature) onto current master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Fix empty-DD_SERVICE version drop; migrate test to JUnit 5 Addresses the Codex review comment on #11555: pre-building the base.service Entry must not change behavior for an explicitly-empty DD_SERVICE. Entry.create rejects empty values, so baseServiceEntry is null in that case; the processTags branch now falls back to set(BASE_SERVICE, ddService) to preserve byte-identical behavior, and the version branch is still reached when the span service also matches the empty configured service. Migrate InternalTagsAdderTest from Groovy/Spock to JUnit 5 (parameterized with @MethodSource) and add regression coverage for the empty-DD_SERVICE case (9 migrated cases + 2 new = 11). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Trim verbose generated comments in InternalTagsAdder per review feedback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Resolve merge conflict: keep @TableTest structure from master, add regression test Preserves the @TableTest versions of the two existing tests that landed on master, and adds the empty-DD_SERVICE regression test (from the PR) as a plain @test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Fix empty-ddService: exit early + drop redundant Entry.create guard - Extend the processTags null guard to also exit when ddService.length()==0, which prevents writing _dd.base_service="" via the TagMap.set path that has no empty-value guard (unlike Entry.create). Empty ddService now behaves the same as null/unset. - Remove the manual null+length>0 pre-check before TagMap.Entry.create in the constructor; Entry.create already returns null for null or empty values, so the guard was redundant. - Update the regression test to assert the new early-return behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Merge branch 'master' into dougqh/preconstruct-internal-tags Accept review feedback: @nonnull ddService, drop empty guard and regression test Config.getServiceName() always returns a non-null non-empty string (defaults to "unnamed-java-app"), so the null/@empty guard in processTags and the corresponding regression test for empty DD_SERVICE are unnecessary. Replaced @nullable with @nonnull on the constructor param to document the actual contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Merge branch 'master' into dougqh/preconstruct-internal-tags Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
chore: Update instrumentation Gradle dependencies Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Rolled back armeria 1.40.0 to pass the build on GitLab. Co-authored-by: AlexeyKuznetsov-DD <alexey.kuznetsov@datadoghq.com>
feat(smoke): support Maven smoke test apps The smoke-test-app plugin now has separate Gradle and Maven application entry points. OpenLiberty smoke tests can use the shared nested-build wiring instead of bespoke Exec tasks, while existing Gradle smoke-test apps keep the same task model. Maven apps still launch through the checked-in root mvnw instead of an installed mvn. Keeping mvnw preserves the Maven version pinned by .mvn/wrapper/maven-wrapper.properties and gives CI and local builds the same cross-platform launcher. CI no longer rewrites maven-wrapper.properties to use MASS. Apache Maven Wrapper documents MVNW_REPOURL as the repository-manager override, so GitLab exports it from MAVEN_REPOSITORY_PROXY and trims the trailing slash before mvnw appends the Maven distribution path: https://maven.apache.org/tools/wrapper/#Using_a_Maven_Repository_Manager The Maven path is covered by smoke-test plugin tests. ProjectBuilder verifies mavenApp task wiring, and the TestKit end-to-end test runs a fake mvnw with MAVEN_REPOSITORY_PROXY. It also asserts the nested Maven process receives MVNW_REPOURL. fix(smoke): address Maven app review feedback fix(smoke): make nested build timeouts opt-in Only wire Gradle stop and Maven build timeouts when the smoke app DSL sets them explicitly. Add Maven cache coverage that deletes the app output directory and verifies the jar is restored from the outer build cache. Mirror the output assertion for the existing Gradle cache test. Co-authored-by: brice.dutheil <brice.dutheil@datadoghq.com>
jpbempel
requested review from
dd-oleksii,
jandro996,
mcculls,
randomanderson,
sameerank,
sarahchen6,
tylfin and
ygree
and removed request for
a team
dd-octo-sts
Bot
added
the
tag: ai generated
spotless
jpbempel
force-pushed
the
jpbempel/g2j-core-pt12
branch
from
4ea79e9 to
8796f31
Compare
mcculls
approved these changes
Jul 7, 2026
mcculls
left a comment
mcculls
left a comment
Contributor
There was a problem hiding this comment.
I reviewed the part 12 changes, LGTM
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
20 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.
we migrate 20 tests:
TraceMapperV04PayloadTest
TraceMapperV05PayloadTest
TraceMapperV1PayloadTest
DDEvpProxyApiTest
DDIntakeApiTest
DDIntakeTraceInterceptorTest
DDIntakeTrackTypeResolverTest
DDAgentApiTest
DDAgentWriterCombinedTest
DDAgentWriterTest
DDIntakeWriterCombinedTest
DDIntakeWriterTest
MultiWriterTest
PayloadDispatcherImplTest
PrioritizationTest
SerializationTest
SpanSamplingWorkerTest
TraceMapperTest
TraceProcessingWorkerTest
WriterFactoryTest
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
part1: #11053
part2: #11062
part3: #11085
part4: #11146
part5: #11217
part6: #11362
part7: #11374
part8: #11437
part9: #11488
part10: #11543
part11: #11566 Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]