You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by "BalusC (via GitHub)" <gi...@apache.org> on 2026/07/29 13:11:57 UTC
[PR] Keep the buffered response writer usable across responseReset() [myfaces]
BalusC opened a new pull request, #1033:
URL: https://github.com/apache/myfaces/pull/1033
`ServletExternalContextImpl.getResponseOutputWriter()` caches a `BufferedWriter` since 0e84278c5. `responseReset()` and `responseSendError()` drop that field, but a `ResponseWriter` created earlier in the render phase keeps writing into the instance it obtained back then. Everything rendered after the reset — typically an error page — therefore lands in a buffer nobody drains, and the client gets HTTP 200 with an empty body.
This is not limited to third party exception handlers: `AjaxExceptionHandlerImpl` (line 205) and `ErrorPageWriter` (line 427) follow the same reset-then-reuse-the-cached-writer pattern.
There is a second break from the same commit: `release()` flushes the `BufferedWriter`, which flushes the container writer and thereby commits the response. A render exception on a non-ajax request can then no longer be reset and forwarded to the error page.
### Fix
`ResettableBufferedWriter` replaces the plain `BufferedWriter`:
- it keeps its identity across `responseReset()` / `responseSendError()`, which now discard the buffered chars instead of dropping the writer;
- it resolves the container writer per drain instead of holding on to it, so it survives `HttpServletResponse.reset()`;
- `flush()` drains into the container writer but does not flush it — committing stays the container's decision, or that of an explicit `responseFlushBuffer()`.
The buffering and its performance benefit are unchanged: same 8 KB buffer, same coalescing of the many small `HtmlResponseWriterImpl` writes.
### Verified
- New `ServletExternalContextImplTest#testResponseResetDiscardsBufferedOutputButKeepsWriterUsable`. On current `main` it fails with `expected: <error page> but was: <aborted responseerror page>`, which also shows aborted output being re-committed.
- Full `mvn clean verify` on the reactor is green.
- Real world reproduction: OmniFaces `FullAjaxExceptionHandlerIT` on Tomcat 11 + MyFaces 5.0.0-SNAPSHOT goes from 7 errors to 14/14 passing.
🤖 Generated with [Claude Code](https://claude.com/claude-code) (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the buffered response writer usable across responseReset() [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko commented on PR #1033:
URL: https://github.com/apache/myfaces/pull/1033#issuecomment-5118677547
@BalusC checkstyle failed
thanks for the PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the buffered response writer usable across responseReset() [myfaces]
Posted by "BalusC (via GitHub)" <gi...@apache.org>.
BalusC commented on PR #1033:
URL: https://github.com/apache/myfaces/pull/1033#issuecomment-5119268341
> checkstyle failed
Fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the buffered response writer usable across responseReset() [myfaces]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #1033:
URL: https://github.com/apache/myfaces/pull/1033#discussion_r3674905172
##########
impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java:
##########
@@ -1122,4 +1126,108 @@
}
return _currentFacesContext;
}
+
+ /**
+ * Coalesces the many small render-time writes into few large writes to the container writer, like a
+ * {@link java.io.BufferedWriter} does, but additionally allows the buffered chars to be discarded again by
+ * {@link #reset()} and resolves the container writer per drain instead of holding on to it.
+ * <p>
+ * Both are required because a {@link jakarta.faces.context.ResponseWriter} keeps writing into the very instance it
+ * obtained from {@link jakarta.faces.context.ExternalContext#getResponseOutputWriter()}. Replacing this instance on
+ * {@link jakarta.faces.context.ExternalContext#responseReset()} would therefore silently route everything rendered afterwards, such as an
+ * error page, into a buffer which is never drained.
+ */
+ static final class ResettableBufferedWriter extends Writer
+ {
+ private static final int BUFFER_SIZE = 8192;
+
+ private final ServletResponse response;
+ private final char[] buffer = new char[BUFFER_SIZE];
+ private int count;
+
+ ResettableBufferedWriter(ServletResponse response)
+ {
+ this.response = response;
+ }
+
+ @Override
+ public void write(int c) throws IOException
+ {
+ if (count >= BUFFER_SIZE)
+ {
+ drain();
+ }
+ buffer[count++] = (char) c;
+ }
+
+ @Override
+ public void write(char[] chars, int offset, int length) throws IOException
+ {
+ if (length >= BUFFER_SIZE)
+ {
+ drain();
+ response.getWriter().write(chars, offset, length);
+ return;
+ }
+ if (length > BUFFER_SIZE - count)
+ {
+ drain();
+ }
+ System.arraycopy(chars, offset, buffer, count, length);
+ count += length;
+ }
+
+ @Override
+ public void write(String string, int offset, int length) throws IOException
+ {
+ if (length >= BUFFER_SIZE)
+ {
+ drain();
+ response.getWriter().write(string, offset, length);
Review Comment:
## CodeQL / Cross-site scripting
Cross-site scripting vulnerability due to a [user-provided value](1).
Cross-site scripting vulnerability due to a [user-provided value](2).
Cross-site scripting vulnerability due to a [user-provided value](3).
Cross-site scripting vulnerability due to a [user-provided value](4).
Cross-site scripting vulnerability due to a [user-provided value](5).
Cross-site scripting vulnerability due to a [user-provided value](6).
Cross-site scripting vulnerability due to a [user-provided value](7).
Cross-site scripting vulnerability due to a [user-provided value](8).
Cross-site scripting vulnerability due to a [user-provided value](9).
Cross-site scripting vulnerability due to a [user-provided value](10).
Cross-site scripting vulnerability due to a [user-provided value](11).
Cross-site scripting vulnerability due to a [user-provided value](12).
Cross-site scripting vulnerability due to a [user-provided value](13).
Cross-site scripting vulnerability due to a [user-provided value](14).
Cross-site scripting vulnerability due to a [user-provided value](15).
Cross-site scripting vulnerability due to a [user-provided value](16).
Cross-site scripting vulnerability due to a [user-provided value](17).
Cross-site scripting vulnerability due to a [user-provided value](18).
Cross-site scripting vulnerability due to a [user-provided value](19).
Cross-site scripting vulnerability due to a [user-provided value](20).
Cross-site scripting vulnerability due to a [user-provided value](21).
Cross-site scripting vulnerability due to a [user-provided value](22).
Cross-site scripting vulnerability due to a [user-provided value](23).
Cross-site scripting vulnerability due to a [user-provided value](24).
Cross-site scripting vulnerability due to a [user-provided value](25).
Cross-site scripting vulnerability due to a [user-provided value](26).
Cross-site scripting vulnerability due to a [user-provided value](27).
[Show more details](https://github.com/apache/myfaces/security/code-scanning/118)
##########
impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java:
##########
@@ -1122,4 +1126,108 @@
}
return _currentFacesContext;
}
+
+ /**
+ * Coalesces the many small render-time writes into few large writes to the container writer, like a
+ * {@link java.io.BufferedWriter} does, but additionally allows the buffered chars to be discarded again by
+ * {@link #reset()} and resolves the container writer per drain instead of holding on to it.
+ * <p>
+ * Both are required because a {@link jakarta.faces.context.ResponseWriter} keeps writing into the very instance it
+ * obtained from {@link jakarta.faces.context.ExternalContext#getResponseOutputWriter()}. Replacing this instance on
+ * {@link jakarta.faces.context.ExternalContext#responseReset()} would therefore silently route everything rendered afterwards, such as an
+ * error page, into a buffer which is never drained.
+ */
+ static final class ResettableBufferedWriter extends Writer
+ {
+ private static final int BUFFER_SIZE = 8192;
+
+ private final ServletResponse response;
+ private final char[] buffer = new char[BUFFER_SIZE];
+ private int count;
+
+ ResettableBufferedWriter(ServletResponse response)
+ {
+ this.response = response;
+ }
+
+ @Override
+ public void write(int c) throws IOException
+ {
+ if (count >= BUFFER_SIZE)
+ {
+ drain();
+ }
+ buffer[count++] = (char) c;
+ }
+
+ @Override
+ public void write(char[] chars, int offset, int length) throws IOException
+ {
+ if (length >= BUFFER_SIZE)
+ {
+ drain();
+ response.getWriter().write(chars, offset, length);
+ return;
+ }
+ if (length > BUFFER_SIZE - count)
+ {
+ drain();
+ }
+ System.arraycopy(chars, offset, buffer, count, length);
+ count += length;
+ }
+
+ @Override
+ public void write(String string, int offset, int length) throws IOException
+ {
+ if (length >= BUFFER_SIZE)
+ {
+ drain();
+ response.getWriter().write(string, offset, length);
Review Comment:
## CodeQL / Information exposure through a stack trace
[Error information](1) can be exposed to an external user.
[Show more details](https://github.com/apache/myfaces/security/code-scanning/119)
##########
impl/src/main/java/org/apache/myfaces/context/servlet/ServletExternalContextImpl.java:
##########
@@ -1122,4 +1126,108 @@
}
return _currentFacesContext;
}
+
+ /**
+ * Coalesces the many small render-time writes into few large writes to the container writer, like a
+ * {@link java.io.BufferedWriter} does, but additionally allows the buffered chars to be discarded again by
+ * {@link #reset()} and resolves the container writer per drain instead of holding on to it.
+ * <p>
+ * Both are required because a {@link jakarta.faces.context.ResponseWriter} keeps writing into the very instance it
+ * obtained from {@link jakarta.faces.context.ExternalContext#getResponseOutputWriter()}. Replacing this instance on
+ * {@link jakarta.faces.context.ExternalContext#responseReset()} would therefore silently route everything rendered afterwards, such as an
+ * error page, into a buffer which is never drained.
+ */
+ static final class ResettableBufferedWriter extends Writer
+ {
+ private static final int BUFFER_SIZE = 8192;
+
+ private final ServletResponse response;
+ private final char[] buffer = new char[BUFFER_SIZE];
+ private int count;
+
+ ResettableBufferedWriter(ServletResponse response)
+ {
+ this.response = response;
+ }
+
+ @Override
+ public void write(int c) throws IOException
+ {
+ if (count >= BUFFER_SIZE)
+ {
+ drain();
+ }
+ buffer[count++] = (char) c;
+ }
+
+ @Override
+ public void write(char[] chars, int offset, int length) throws IOException
+ {
+ if (length >= BUFFER_SIZE)
+ {
+ drain();
+ response.getWriter().write(chars, offset, length);
+ return;
+ }
+ if (length > BUFFER_SIZE - count)
+ {
+ drain();
+ }
+ System.arraycopy(chars, offset, buffer, count, length);
+ count += length;
+ }
+
+ @Override
+ public void write(String string, int offset, int length) throws IOException
+ {
+ if (length >= BUFFER_SIZE)
+ {
+ drain();
+ response.getWriter().write(string, offset, length);
Review Comment:
## CodeQL / Information exposure through an error message
[Error information](1) can be exposed to an external user.
[Error information](2) can be exposed to an external user.
[Error information](3) can be exposed to an external user.
[Error information](4) can be exposed to an external user.
[Show more details](https://github.com/apache/myfaces/security/code-scanning/120)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the buffered response writer usable across responseReset() [myfaces]
Posted by "tandraschko (via GitHub)" <gi...@apache.org>.
tandraschko merged PR #1033:
URL: https://github.com/apache/myfaces/pull/1033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@myfaces.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org