fix(servlet): cover Servlet 6.1 3-arg sendRedirect in jakarta IAST instrumentation#11599
fix(servlet): cover Servlet 6.1 3-arg sendRedirect in jakarta IAST instrumentation#11599wconti27 wants to merge 1 commit into
Conversation
Add a call-depth guard to the jakarta sendRedirect IAST advice so the Servlet 6.1 sendRedirect(String,int,boolean) overload and the 1-arg form (which 6.1 delegates to the 3-arg method) yield exactly one unvalidated- redirect detection. Migrate the Groovy response instrumentation test to JUnit 5 and add 3-arg overload coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
|
PR Blocked - Invalid Label The pull request introduced unexpected labels:
This PR is blocked until:
Note: Simply removing labels from the pull request is not enough - a maintainer must delete this comment then remove the label to remove the block. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f8378ae67
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| final UnvalidatedRedirectModule module = InstrumentationBridge.UNVALIDATED_REDIRECT; | ||
| if (module != null) { | ||
| if (null != location && !location.isEmpty()) { | ||
| module.onRedirect(location); |
There was a problem hiding this comment.
Ensure call depth is reset when the redirect sink throws
When UNVALIDATED_REDIRECT.onRedirect(location) throws, the @Advice.OnMethodEnter(suppress = Throwable.class) swallows the exception after the call depth has already been incremented but before onEnter can return true; the exit advice then sees the default false enter value and skips CallDepthThreadLocalMap.reset(...). In that failure scenario the servlet response call-depth remains nonzero on the request thread, so subsequent sendRedirect calls on the same thread are treated as nested and the IAST redirect sink stops firing until the thread is reused/reset elsewhere.
Useful? React with 👍 / 👎.
🟢 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. |
Summary
Servlet 6.1 (Jakarta EE 11, Spring Boot 4 / Tomcat 11) adds a new
sendRedirect(String location, int sc, boolean clearBuffer)overload. The existingjakarta-servlet-5.0instrumentation already covers it — its matcher (named("sendRedirect").and(takesArgument(0, String.class))) is arity-agnostic and the muzzle range[5.0.0,)loads on 6.1.0 — so no new module is needed.However, Tomcat 11's 1-arg
sendRedirectinternally delegates to the 3-arg overload. With both methods woven, the IASTonRedirectcallback fired twice per redirect. This PR adds aCallDepthThreadLocalMapguard (mirroring the APMSendAdvicepattern) to ensure exactly one callback per redirect regardless of the internal delegation chain.Changes
JakartaHttpServletResponseInstrumentation.java: AddedCallDepthThreadLocalMapenter/exit guard toSendRedirectAdviceDummyResponse.java: Added 3-argsendRedirectstub + made 1-arg delegate to it (models Tomcat 11 / Servlet 6.1 behavior for tests)JakartaHttpServletResponseInstrumentationTest.groovy→.java: Full JUnit 5 migration (CLAUDE.md requirement), adding new cases: 3-arg fires exactly once, 1-arg delegation fires exactly once, null/empty locations fire nothingVerification
./gradlew :dd-java-agent:instrumentation:servlet:jakarta-servlet-5.0:test— BUILD SUCCESSFULPart of: #11597
🤖 Generated with Claude Code