Skip to content

Prevent servlet snippet injection from corrupting recycled responses - #21104

Draft
trask with Copilot wants to merge 10 commits into
mainfrom
copilot/task-fix-snippet-injection-issue
Draft

trask with Copilot wants to merge 10 commits into
mainfrom
copilot/task-fix-snippet-injection-issue

Conversation

Copilot AI commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Prevent recycled Jetty output streams from carrying stale snippet state into later requests. A cached five-byte Content-Length could truncate a 228-byte JSP to 37 bytes.

Scope stream state to the request lifetime in both javax and jakarta instrumentation, while keeping stream and writer matching independent and preserving late content-type support.

Fixes #21096.

@opentelemetry-pr-dashboard

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-10-31 18:17 UTC

Move out of draft to request review.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

Copilot AI and others added 2 commits October 31, 2026 18:32
…etime

Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix advice failure with snippet injection in Jetty smoke tests Prevent servlet snippet injection from corrupting recycled responses Oct 31, 2026
Copilot AI requested a review from trask October 31, 2026 18:48
@trask
trask requested a balanced review from Copilot October 31, 2026 18:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Mock lifetime and synchronous keep-alive tests do not establish shared-body lifecycle behavior when forwards or async code reset the raw container response, so installed-agent validation is needed.

No open issues 🎉

What changed in this PR

This PR fixes servlet snippet injection state leaking across recycled responses, preventing stale Content-Length values from truncating later responses in both javax and jakarta instrumentation.

Changes:

  • Bind injection state to the request and response body while preserving async redispatch and late-content-type support.
  • Share writer and stream tracking, and reset abandoned body state without double-adjusting Content-Length.
  • Add lifecycle regression tests and a Jetty smoke test that reuses one HTTP/1.1 connection.
File Description
smoke-tests/​src/​test/​java/​io/​opentelemetry/​smoketest/​appserver/​JettySmokeTest.java Tests complete JSP output after a non-HTML response on the same connection.
instrumentation/​servlet/​servlet-common/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​common/​snippet/​SnippetInjectingPrintWriter.java Accepts shared injection state and marks writer ownership.
instrumentation/​servlet/​servlet-common/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​common/​snippet/​OutputStreamSnippetInjectionHelper.java Skips stream injection when the writer owns injection.
instrumentation/​servlet/​servlet-common/​bootstrap/​src/​main/​java/​io/​opentelemetry/​javaagent/​bootstrap/​servlet/​InjectionState.java Tracks request lifetime, output ownership, resets, and Content-Length adjustments.
instrumentation/​servlet/​servlet-5.0/​javaagent/​src/​unitTests/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v5_0/​snippet/​InjectionStateLifetimeTest.java Covers jakarta response reuse, redispatch, errors, and resets.
instrumentation/​servlet/​servlet-5.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v5_0/​snippet/​ServletOutputStreamInjectionState.java Shares body state and clears expired jakarta stream ownership.
instrumentation/​servlet/​servlet-5.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v5_0/​snippet/​Servlet5SnippetInjectingResponseWrapper.java Manages request-bound state and body resets for jakarta responses.
instrumentation/​servlet/​servlet-5.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v5_0/​service/​JakartaServletServiceAdvice.java Passes the request into the jakarta response wrapper.
instrumentation/​servlet/​servlet-3.0/​javaagent/​src/​unitTests/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v3_0/​snippet/​InjectionStateLifetimeTest.java Covers javax response reuse, redispatch, errors, and resets.
instrumentation/​servlet/​servlet-3.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v3_0/​snippet/​ServletOutputStreamInjectionState.java Shares body state and clears expired javax stream ownership.
instrumentation/​servlet/​servlet-3.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v3_0/​snippet/​Servlet3SnippetInjectingResponseWrapper.java Manages request-bound state and body resets for javax responses.
instrumentation/​servlet/​servlet-3.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v3_0/​Servlet3Advice.java Passes the request into the javax response wrapper.

Review effort: Balanced

Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot AI requested a review from trask October 31, 2026 19:53
@trask
trask requested a balanced review from Copilot October 31, 2026 20:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Both servlet response wrappers disable stream injection before writer creation succeeds, causing snippet injection to be lost when callers catch a getWriter() failure and continue using the output stream.

1 open issue

Review effort: Balanced

Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot AI requested a review from trask October 31, 2026 20:51
@trask
trask requested a balanced review from Copilot October 31, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The ERROR-dispatch behavior lacks installed-agent regression coverage for Servlet 3 and Servlet 5, leaving wrapper-guard and reset-advice interactions, replacement-body completeness, single-snippet injection, and Content-Length consistency unverified.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Add installed-agent regression for ERROR-dispatch response replacement

instrumentation/​servlet/​servlet-3.0/​javaagent/​src/​main/​java/​io/​opentelemetry/​javaagent/​instrumentation/​servlet/​v3_0/​snippet/​Servlet3SnippetInjectingResponseWrapper.java:74

[Testing] The new ERROR-dispatch behavior lacks an installed-agent regression: the unit test constructs the wrapper directly, leaving its interaction with the existing-wrapper guard and reset advice untested. Add a regression for both Servlet 3 and Servlet 5 that writes an uncommitted fixed-length body, dispatches to an HTML error servlet, and checks the complete replacement body, exactly one snippet, and matching Content-Length when present. The same gap applies to Servlet5SnippetInjectingResponseWrapper.java:66.

1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

An unpublished finding about stale cached writers after raw async response resets warrants human assessment because it can bypass container ownership checks and disable active-stream injection.

No open issues 🎉

Review effort: Balanced

Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot AI requested a review from trask October 31, 2026 22:53
@trask
trask requested a balanced review from Copilot October 31, 2026 23:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed because an unpublished finding identifies an asynchronous span-export race that may contaminate subsequent tests and undermine their assertions.

No open issues 🎉

Review effort: Balanced

Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot AI requested a review from trask November 2, 2026 06:16
@trask
trask requested a balanced review from Copilot November 2, 2026 06:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Both servlet versions unnecessarily clear an already-empty virtual field on every output-stream write, adding volatile-write overhead even when snippet injection is disabled.

1 open issue

Review effort: Balanced

Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot AI requested a review from trask November 2, 2026 06:48
@trask
trask requested a balanced review from Copilot November 2, 2026 06:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the changes tying servlet snippet-injection state to the request lifetime.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Advice failure with snippet injection in jetty smoke tests

4 participants