[bugfix] Replace CachingResponseWrapper's header-name allowlist with buffer+replay

exist-core/src/main/java/org/exist/http/servlets/HeaderPassthroughResponse.java

@@ -0,0 +1,54 @@
+/*
+ * eXist-db Open Source Native XML Database
+ * Copyright (C) 2001 The eXist-db Authors
+ *
+ * info@exist-db.org
+ * http://www.exist-db.org
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+package org.exist.http.servlets;
+
+/**
+ * Capability of a response wrapper that buffers its output (and so cannot let a header take
+ * effect on the real, underlying response immediately) to still accept a header that must
+ * bypass that buffering and reach the real response right away, regardless of whether this
+ * particular wrapper instance ever gets flushed.
+ * <p>
+ * {@code org.exist.http.urlrewrite.XQueryURLRewrite$CachingResponseWrapper} is the sole
+ * implementer: it buffers everything a dispatched pipeline step writes -- including headers a
+ * step sets itself, whether via the underlying servlet's own resource handling or the XQuery
+ * {@code response:set-header()} function -- because that step's output may be discarded in favor
+ * of a later {@code <exist:view>} step's, in which case its buffered writes are correctly never
+ * replayed. A {@code controller.xql} {@code <exist:set-header>} directive is different: it is
+ * pipeline-level configuration, not step output, and must survive regardless of which step's
+ * output eventually wins -- so it needs a way to skip buffering entirely, tunnelling through
+ * however many wrapper layers are stacked (a forward step may itself trigger a nested dispatch,
+ * wrapping one {@code CachingResponseWrapper} around another) until it reaches a response that
+ * isn't one of these wrappers at all, and applying itself there, immediately.
+ *
+ * @see org.exist.http.urlrewrite.URLRewrite#setHeaders
+ */
+public interface HeaderPassthroughResponse {
+
+    /**
+     * Set a header on the real, underlying response immediately, bypassing any output buffering
+     * this response wrapper (or any wrapper it in turn wraps) would otherwise apply.
+     *
+     * @param name the header name.
+     * @param value the header value.
+     */
+    void setPassthroughHeader(String name, String value);
+}

exist-core/src/main/java/org/exist/http/servlets/HttpResponseWrapper.java

@@ -180,6 +180,29 @@ public void setHeader(final String name, final String value) {
 		response.setHeader(name, encode(value));
 	}
 
+	/**
+	 * Set a header the same way {@link #setHeader(String, String)} does, except that if the
+	 * wrapped response buffers its output (see {@link HeaderPassthroughResponse}), the header
+	 * bypasses that buffering and applies to the real response immediately. Used exclusively by
+	 * {@code URLRewrite#setHeaders} to apply a controller.xql {@code <exist:set-header>}
+	 * directive -- pipeline configuration, not step output -- so it survives regardless of which
+	 * pipeline step's buffered output eventually gets flushed. Deliberately not used by
+	 * {@link #setHeader(String, String)} itself, which backs the XQuery
+	 * {@code response:set-header()} function: a step's own programmatic header call is step
+	 * output and must stay subject to the same buffering as everything else that step writes.
+	 *
+	 * @param name the name of the header.
+	 * @param value the header value.
+	 */
+	public void setPassthroughHeader(final String name, final String value) {
+		final String encoded = encode(value);
+		if (response instanceof HeaderPassthroughResponse passthroughResponse) {
+			passthroughResponse.setPassthroughHeader(name, encoded);
+		} else {
+			response.setHeader(name, encoded);
+		}
+	}
+
 	@Override
 	public void setIntHeader(final String name, final int value) {
 		response.setIntHeader(name, value);

exist-core/src/main/java/org/exist/http/urlrewrite/URLRewrite.java

@@ -251,7 +251,12 @@ public void prepareRequest(final XQueryURLRewrite.RequestWrapper request) {
     protected void setHeaders(final HttpResponseWrapper response) {
         if (headers != null) {
             for (final Map.Entry<String, String> entry : headers.entrySet()) {
-                response.setHeader(entry.getKey(), entry.getValue());
+                // setPassthroughHeader(), not setHeader(): an <exist:set-header> directive is
+                // pipeline configuration, not a step's own output, and must survive regardless of
+                // which step's buffered output eventually gets flushed. See
+                // HeaderPassthroughResponse for why this can't just be setHeader()'s default
+                // behavior.
+                response.setPassthroughHeader(entry.getKey(), entry.getValue());
             }
         }
     }

exist-core/src/main/java/org/exist/http/urlrewrite/XQueryURLRewrite.java

@@ -35,6 +35,7 @@
 import org.exist.http.Descriptor;
 import org.exist.http.servlets.Authenticator;
 import org.exist.http.servlets.BasicAuthenticator;
+import org.exist.http.servlets.HeaderPassthroughResponse;
 import org.exist.http.servlets.HttpRequestWrapper;
 import org.exist.http.servlets.HttpResponseWrapper;
 import org.exist.security.AuthenticationException;
@@ -1329,7 +1330,8 @@ public long getDateHeader(final String s) {
         }
     }
 
-    private static class CachingResponseWrapper extends HttpServletResponseWrapper {
+    private static class CachingResponseWrapper extends HttpServletResponseWrapper
+            implements HeaderPassthroughResponse {
 
         /**
          * Tracks which of {@link #getWriter()} / {@link #getOutputStream()} this response has
@@ -1418,38 +1420,64 @@ public String getContentType() {
         }
 
         /**
-         * Headers that describe an intermediate resource -- not the final output -- and so must not
-         * leak onto the real response while buffering. A static resource forward step (e.g. Jetty's
-         * default/ResourceServlet serving a plain file) sets these from the FILE's own properties
-         * (its length, its modification time, its identity, whether it supports byte ranges), through
-         * any of setHeader/addHeader/setIntHeader/addIntHeader/setDateHeader/addDateHeader --
-         * HttpServletResponseWrapper's defaults for all of these delegate straight to the real
-         * underlying response with no cache awareness. Without this guard they leak onto the real
-         * response before a later <exist:view> step's own, different output is flushed to it --
-         * Content-Length doing exactly this is what broke https://github.com/eXist-db/exist/issues/6669;
-         * Last-Modified/ETag/Accept-Ranges are the same shape of bug, just without (yet) a hard
-         * failure mode of their own to force the issue.
+         * A header write this step made while buffering, recorded so it can be replayed -- in the
+         * exact original order, through the exact original typed method -- onto the real response
+         * from {@link #flush()}, but only if this wrapper is the one that actually gets flushed.
+         * If a later {@code <exist:view>} step replaces this step's output, this wrapper (and
+         * everything buffered in it) is simply discarded without ever being replayed: correct,
+         * because the header described a step whose output turned out not to be the final one.
          * <p>
-         * Deliberately narrower than "buffer every header while caching": headers set explicitly via
-         * controller.xql's {@code <exist:set-header>} ({@link URLRewrite#setHeaders}, called before
-         * {@code doRewrite()}'s {@code dispatcher.forward()}) must keep going straight through
-         * immediately, since {@code applyViews()} discards each step's wrapper for a fresh one around
-         * the real response on the next view step and only ever flushes the last one -- buffering
-         * those too would silently drop them.
+         * Replaying the original call verbatim (rather than pre-computing a final name-to-value
+         * map here) means the real response's own semantics decide what "wins" when the same
+         * header is set multiple times (setHeader replaces, addHeader appends) -- exactly as they
+         * would have if these calls had never been buffered at all.
          */
-        private static final Set<String> BUFFERED_RESOURCE_METADATA_HEADERS =
-                Set.of("content-length", "last-modified", "etag", "accept-ranges");
+        private sealed interface BufferedHeader {
+            record Set(String name, String value) implements BufferedHeader {
+            }
+
+            record Add(String name, String value) implements BufferedHeader {
+            }
+
+            record SetInt(String name, int value) implements BufferedHeader {
+            }
+
+            record AddInt(String name, int value) implements BufferedHeader {
+            }
+
+            record SetDate(String name, long value) implements BufferedHeader {
+            }
 
-        private boolean isBufferedResourceMetadataHeader(final String name) {
-            return cache && BUFFERED_RESOURCE_METADATA_HEADERS.contains(name.toLowerCase(Locale.ROOT));
+            record AddDate(String name, long value) implements BufferedHeader {
+            }
+        }
+
+        private final List<BufferedHeader> bufferedHeaders = new ArrayList<>();
+
+        /**
+         * Set a header on the real response immediately, bypassing buffering, tunnelling through
+         * however many {@code CachingResponseWrapper} layers are stacked (a forward step may
+         * itself trigger a nested dispatch) until reaching a response that isn't one of these
+         * wrappers. See {@link HeaderPassthroughResponse}: this exists solely for
+         * {@link URLRewrite#setHeaders}'s controller.xql {@code <exist:set-header>} directives,
+         * which are pipeline configuration, not step output, and so must survive regardless of
+         * which step's buffered output eventually wins.
+         */
+        @Override
+        public void setPassthroughHeader(final String name, final String value) {
+            if (getResponse() instanceof HeaderPassthroughResponse passthroughResponse) {
+                passthroughResponse.setPassthroughHeader(name, value);
+            } else {
+                super.setHeader(name, value);
+            }
         }
 
         @Override
         public void setHeader(final String name, final String value) {
             if ("Content-Type".equals(name)) {
                 setContentType(value);
-            } else if (isBufferedResourceMetadataHeader(name)) {
-                return;
+            } else if (cache) {
+                bufferedHeaders.add(new BufferedHeader.Set(name, value));
             } else {
                 super.setHeader(name, value);
             }
@@ -1460,46 +1488,51 @@ public void addHeader(final String name, final String value) {
             if ("Content-Type".equals(name)) {
                 // Route through setContentType() like setHeader() does -- otherwise a step that sets
                 // Content-Type via addHeader() (e.g. Jetty's default/ResourceServlet serving a static
-                // file) bypasses the contentType tracking entirely and leaks straight onto the real
-                // response, so a later <exist:view> step's actual declared Content-Type never sticks.
+                // file) bypasses the contentType tracking entirely, which would leak it straight onto
+                // the real response even though a later <exist:view> step's own Content-Type should
+                // win instead.
                 setContentType(value);
-            } else if (isBufferedResourceMetadataHeader(name)) {
-                return;
+            } else if (cache) {
+                bufferedHeaders.add(new BufferedHeader.Add(name, value));
             } else {
                 super.addHeader(name, value);
             }
         }
 
         @Override
         public void setIntHeader(final String name, final int value) {
-            if (isBufferedResourceMetadataHeader(name)) {
-                return;
+            if (cache) {
+                bufferedHeaders.add(new BufferedHeader.SetInt(name, value));
+            } else {
+                super.setIntHeader(name, value);
             }
-            super.setIntHeader(name, value);
         }
 
         @Override
         public void addIntHeader(final String name, final int value) {
-            if (isBufferedResourceMetadataHeader(name)) {
-                return;
+            if (cache) {
+                bufferedHeaders.add(new BufferedHeader.AddInt(name, value));
+            } else {
+                super.addIntHeader(name, value);
             }
-            super.addIntHeader(name, value);
         }
 
         @Override
         public void setDateHeader(final String name, final long date) {
-            if (isBufferedResourceMetadataHeader(name)) {
-                return;
+            if (cache) {
+                bufferedHeaders.add(new BufferedHeader.SetDate(name, date));
+            } else {
+                super.setDateHeader(name, date);
             }
-            super.setDateHeader(name, date);
         }
 
         @Override
         public void addDateHeader(final String name, final long date) {
-            if (isBufferedResourceMetadataHeader(name)) {
-                return;
+            if (cache) {
+                bufferedHeaders.add(new BufferedHeader.AddDate(name, date));
+            } else {
+                super.addDateHeader(name, date);
             }
-            super.addDateHeader(name, date);
         }
 
         @Override
@@ -1557,13 +1590,18 @@ public void flush() throws IOException {
             if (cache && contentType != null) {
                 super.setContentType(contentType);
             }
+            if (cache) {
+                replayBufferedHeaders();
+            }
             if (sos != null) {
                 final byte[] data = sos.getData();
                 // Set the real Content-Length explicitly rather than leaving it to the container to
-                // infer (e.g. via chunked transfer encoding). The buffered step's own Content-Length
-                // headers are deliberately suppressed above (see isBufferedContentLength()) precisely
-                // because they'd be wrong for this, the final, output -- this is where the correct
-                // value actually gets set, from the bytes that are really about to be written.
+                // infer (e.g. via chunked transfer encoding). Deliberately not part of the generic
+                // replay above -- any buffered Content-Length (from this step's own setHeader/
+                // setIntHeader/etc. call) is skipped there and superseded by this, because it's the
+                // one header whose correctness is independently checkable against the bytes actually
+                // being written, and trusting a self-reported value over that is exactly how #6669
+                // happened.
                 if (cache) {
                     super.setContentLengthLong(data.length);
                 }
@@ -1572,6 +1610,32 @@ public void flush() throws IOException {
                 out.flush();
             }
         }
+
+        private void replayBufferedHeaders() {
+            for (final BufferedHeader header : bufferedHeaders) {
+                final String name = switch (header) {
+                    case BufferedHeader.Set(final String n, final String ignored) -> n;
+                    case BufferedHeader.Add(final String n, final String ignored) -> n;
+                    case BufferedHeader.SetInt(final String n, final int ignored) -> n;
+                    case BufferedHeader.AddInt(final String n, final int ignored) -> n;
+                    case BufferedHeader.SetDate(final String n, final long ignored) -> n;
+                    case BufferedHeader.AddDate(final String n, final long ignored) -> n;
+                };
+                if ("Content-Length".equalsIgnoreCase(name)) {
+                    // See flush(): the real Content-Length is derived from the actual buffered byte
+                    // count, never trusted from a step's own self-reported value.
+                    continue;
+                }
+                switch (header) {
+                    case BufferedHeader.Set(final String n, final String value) -> super.setHeader(n, value);
+                    case BufferedHeader.Add(final String n, final String value) -> super.addHeader(n, value);
+                    case BufferedHeader.SetInt(final String n, final int value) -> super.setIntHeader(n, value);
+                    case BufferedHeader.AddInt(final String n, final int value) -> super.addIntHeader(n, value);
+                    case BufferedHeader.SetDate(final String n, final long value) -> super.setDateHeader(n, value);
+                    case BufferedHeader.AddDate(final String n, final long value) -> super.addDateHeader(n, value);
+                }
+            }
+        }
     }
 
     private static class CachingServletOutputStream extends ServletOutputStream {