codeconsole commented on code in PR #15967: URL: https://github.com/apache/grails-core/pull/15967#discussion_r4089332206
########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/SecurityHeadersResponseWrapper.java: ########## @@ -0,0 +1,249 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.web.controllers; + +import java.io.IOException; +import java.io.PrintWriter; + +import jakarta.servlet.ServletOutputStream; +import jakarta.servlet.WriteListener; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpServletResponseWrapper; + +/** + * Response wrapper that runs a callback exactly once, immediately before the response + * is committed. Every commit path is intercepted: redirects, errors, explicit buffer + * flushes, flushing or closing the writer or output stream, and writes that reach a + * declared {@code Content-Length}. If the wrapped chain returns without committing, + * the owner is expected to invoke {@link #beforeCommit()} itself. + * + * <p>Writing headers this late lets anything further down the filter chain (a + * controller, an interceptor, Spring Security's header writers, another filter) set + * its own value first; the callback only fills what is still missing.</p> + */ +final class SecurityHeadersResponseWrapper extends HttpServletResponseWrapper { + + private static final String CONTENT_LENGTH = "Content-Length"; + + private final Runnable beforeCommit; + + private boolean fired; + + private long contentLength = -1; + + private long contentWritten; + + SecurityHeadersResponseWrapper(HttpServletResponse response, Runnable beforeCommit) { + super(response); + this.beforeCommit = beforeCommit; + } + + /** + * Runs the callback if it has not run yet. Safe to call repeatedly. + */ + void beforeCommit() { + if (!this.fired) { + this.fired = true; + this.beforeCommit.run(); + } + } + + @Override + public void sendError(int sc) throws IOException { + beforeCommit(); + super.sendError(sc); + } + + @Override + public void sendError(int sc, String msg) throws IOException { + beforeCommit(); + super.sendError(sc, msg); + } + + @Override + public void sendRedirect(String location) throws IOException { + beforeCommit(); + super.sendRedirect(location); + } + + @Override + public void sendRedirect(String location, int sc) throws IOException { + beforeCommit(); + super.sendRedirect(location, sc); + } + + @Override + public void sendRedirect(String location, boolean clearBuffer) throws IOException { + beforeCommit(); + super.sendRedirect(location, clearBuffer); + } + + @Override + public void sendRedirect(String location, int sc, boolean clearBuffer) throws IOException { + beforeCommit(); + super.sendRedirect(location, sc, clearBuffer); + } + + @Override + public void flushBuffer() throws IOException { + beforeCommit(); + super.flushBuffer(); + } + + @Override + public void setContentLength(int len) { + this.contentLength = len; + super.setContentLength(len); + } + + @Override + public void setContentLengthLong(long len) { + this.contentLength = len; + super.setContentLengthLong(len); + } + + @Override + public void setHeader(String name, String value) { + trackContentLengthHeader(name, value); + super.setHeader(name, value); + } + + @Override + public void addHeader(String name, String value) { + trackContentLengthHeader(name, value); + super.addHeader(name, value); + } + + @Override + public ServletOutputStream getOutputStream() throws IOException { + return new CommitAwareOutputStream(super.getOutputStream()); + } + + @Override + public PrintWriter getWriter() throws IOException { + return new CommitAwareWriter(super.getWriter()); + } + + private void trackContentLengthHeader(String name, String value) { + if (CONTENT_LENGTH.equalsIgnoreCase(name) && value != null) { + try { + this.contentLength = Long.parseLong(value.trim()); + } + catch (NumberFormatException ignored) { + // The container rejects or ignores a malformed Content-Length; nothing to track. + } + } + } + + private void trackWritten(long count) { + if (this.fired) { + return; + } + this.contentWritten += count; + if (this.contentLength >= 0 && this.contentWritten >= this.contentLength) { Review Comment: This misses the most common commit path: the container's response buffer filling up. With no `Content-Length` and no explicit flush, Tomcat commits as soon as the body outgrows `getBufferSize()` (8 KB by default). The filter's `finally` then calls `setHeader` on a committed response, and Tomcat silently ignores it. I put the filter in front of a servlet on embedded Tomcat 11.0.26: ``` path body X-Content-Type-Options X-Frame-Options /small-stream 100 B, OutputStream nosniff SAMEORIGIN /large-stream 50 KB, OutputStream <missing> <missing> /large-writer 50 KB, getWriter() <missing> <missing> ``` So large JSON responses, file downloads, and any big page not buffered by SiteMesh go out with no hardening headers. That contradicts the "streamed body" sentence in `security.adoc`. `OnCommittedResponseWrapper` checks the buffer size next to the content length for exactly this reason. Adding the same check fixed both large cases in the same run: ```suggestion int bufferSize = getBufferSize(); if ((this.contentLength >= 0 && this.contentWritten >= this.contentLength) || (bufferSize > 0 && this.contentWritten >= bufferSize)) { ``` The spec can't catch this because `MockHttpServletResponse` still accepts `setHeader` after it commits. A variant of the streaming test would: set `response.bufferSize = 16`, write 64 bytes with no flush, and assert the headers are already present inside the chain. ########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersAutoConfiguration.java: ########## @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.web.controllers; + +import java.util.EnumSet; + +import jakarta.servlet.DispatcherType; + +import org.springframework.boot.autoconfigure.AutoConfiguration; +import org.springframework.boot.autoconfigure.condition.ConditionalOnBooleanProperty; +import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; +import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; +import org.springframework.boot.cloud.CloudPlatform; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.core.env.Environment; + +import org.grails.web.config.http.GrailsFilters; + +/** + * Registers {@link GrailsSecurityHeadersFilter} to apply baseline browser-hardening + * response headers. The filter writes at response commit time and only fills headers + * that are still absent, so it coexists with Spring Security's header writers and any + * other filter or controller that sets these headers itself. + */ +@AutoConfiguration +@ConditionalOnWebApplication(type = ConditionalOnWebApplication.Type.SERVLET) +@ConditionalOnBooleanProperty(name = "grails.security.headers.enabled", matchIfMissing = true) +@EnableConfigurationProperties(GrailsSecurityHeadersProperties.class) +public class GrailsSecurityHeadersAutoConfiguration { + + static final String FORWARD_HEADERS_STRATEGY = "server.forward-headers-strategy"; + + @Bean + @ConditionalOnMissingBean(value = GrailsSecurityHeadersFilter.class, name = "grailsSecurityHeadersFilter") + public GrailsSecurityHeadersFilter securityHeadersFilter(GrailsSecurityHeadersProperties properties, + Environment environment) { + return new GrailsSecurityHeadersFilter(properties, isReverseProxyConfigured(environment)); + } + + /** + * Whether the deployment declares itself to be behind a reverse proxy: either a + * forwarded-headers strategy is configured (in which case the forwarding filter or + * valve strips the forwarded request headers before this filter could see them), or + * Spring Boot detected a cloud platform, where ingress through a proxy is the norm. + */ + static boolean isReverseProxyConfigured(Environment environment) { + String strategy = environment.getProperty(FORWARD_HEADERS_STRATEGY); + if (strategy != null && !"none".equalsIgnoreCase(strategy.trim())) { + return true; + } + CloudPlatform platform = CloudPlatform.getActive(environment); + return platform != null && platform != CloudPlatform.NONE; + } + + @Bean + @ConditionalOnMissingBean(name = "grailsSecurityHeadersFilter") + public FilterRegistrationBean<GrailsSecurityHeadersFilter> grailsSecurityHeadersFilter( + GrailsSecurityHeadersFilter securityHeadersFilter) { + FilterRegistrationBean<GrailsSecurityHeadersFilter> registrationBean = new FilterRegistrationBean<>(); + registrationBean.setFilter(securityHeadersFilter); + registrationBean.setDispatcherTypes(EnumSet.of(DispatcherType.REQUEST, DispatcherType.FORWARD, + DispatcherType.INCLUDE, DispatcherType.ERROR)); + registrationBean.addUrlPatterns("/*"); + registrationBean.setOrder(GrailsFilters.LAST.getOrder()); Review Comment: `LAST` (-110) nests this filter inside every filter registered earlier, so an earlier filter that writes the response itself without calling `chain.doFilter` never reaches it. Asset-pipeline does exactly that: `AssetPipelineGrailsPlugin` registers `AssetPipelineFilter` at `GrailsFilters.ASSET_PIPELINE_FILTER` (-190), and on a hit it streams the file and calls `flushBuffer()` without continuing the chain. So `/assets/**` JS and CSS, where `nosniff` matters most, never get these headers. With commit-time writing, the outermost slot is also the one that makes precedence work. Nested wrappers fire innermost-first, both on commit and when the chain returns, so the outermost filter fills last and everything inside it wins. At -110 that is inverted for anything registered before it. For example, with `spring.security.filter.order: -170` (security ahead of SiteMesh) and `frameOptions { deny() }`, this wrapper fires first and writes `SAMEORIGIN`, then `XFrameOptionsHeaderWriter` skips because the header is already present. That breaks the "regardless of filter ordering" promise in `upgrading80x.adoc`. Registering at `GrailsFilters.FIRST` (-200) or earlier would fix both cases. ########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/SecurityHeadersResponseWrapper.java: ########## @@ -0,0 +1,249 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.web.controllers; + +import java.io.IOException; +import java.io.PrintWriter; + +import jakarta.servlet.ServletOutputStream; +import jakarta.servlet.WriteListener; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpServletResponseWrapper; + +/** + * Response wrapper that runs a callback exactly once, immediately before the response + * is committed. Every commit path is intercepted: redirects, errors, explicit buffer + * flushes, flushing or closing the writer or output stream, and writes that reach a + * declared {@code Content-Length}. If the wrapped chain returns without committing, + * the owner is expected to invoke {@link #beforeCommit()} itself. + * + * <p>Writing headers this late lets anything further down the filter chain (a + * controller, an interceptor, Spring Security's header writers, another filter) set + * its own value first; the callback only fills what is still missing.</p> + */ +final class SecurityHeadersResponseWrapper extends HttpServletResponseWrapper { + + private static final String CONTENT_LENGTH = "Content-Length"; + + private final Runnable beforeCommit; + + private boolean fired; + + private long contentLength = -1; + + private long contentWritten; + + SecurityHeadersResponseWrapper(HttpServletResponse response, Runnable beforeCommit) { + super(response); + this.beforeCommit = beforeCommit; + } + + /** + * Runs the callback if it has not run yet. Safe to call repeatedly. + */ + void beforeCommit() { + if (!this.fired) { + this.fired = true; + this.beforeCommit.run(); + } + } + + @Override + public void sendError(int sc) throws IOException { + beforeCommit(); + super.sendError(sc); + } + + @Override + public void sendError(int sc, String msg) throws IOException { + beforeCommit(); + super.sendError(sc, msg); + } + + @Override + public void sendRedirect(String location) throws IOException { + beforeCommit(); + super.sendRedirect(location); + } + + @Override + public void sendRedirect(String location, int sc) throws IOException { + beforeCommit(); + super.sendRedirect(location, sc); + } + + @Override + public void sendRedirect(String location, boolean clearBuffer) throws IOException { + beforeCommit(); + super.sendRedirect(location, clearBuffer); + } + + @Override + public void sendRedirect(String location, int sc, boolean clearBuffer) throws IOException { + beforeCommit(); + super.sendRedirect(location, sc, clearBuffer); + } + + @Override + public void flushBuffer() throws IOException { + beforeCommit(); + super.flushBuffer(); + } + + @Override + public void setContentLength(int len) { + this.contentLength = len; + super.setContentLength(len); + } + + @Override + public void setContentLengthLong(long len) { + this.contentLength = len; + super.setContentLengthLong(len); + } + + @Override + public void setHeader(String name, String value) { + trackContentLengthHeader(name, value); + super.setHeader(name, value); + } + + @Override + public void addHeader(String name, String value) { + trackContentLengthHeader(name, value); + super.addHeader(name, value); + } + + @Override + public ServletOutputStream getOutputStream() throws IOException { + return new CommitAwareOutputStream(super.getOutputStream()); + } + + @Override + public PrintWriter getWriter() throws IOException { + return new CommitAwareWriter(super.getWriter()); + } + + private void trackContentLengthHeader(String name, String value) { + if (CONTENT_LENGTH.equalsIgnoreCase(name) && value != null) { + try { + this.contentLength = Long.parseLong(value.trim()); + } + catch (NumberFormatException ignored) { + // The container rejects or ignores a malformed Content-Length; nothing to track. + } + } + } + + private void trackWritten(long count) { + if (this.fired) { + return; + } + this.contentWritten += count; + if (this.contentLength >= 0 && this.contentWritten >= this.contentLength) { + beforeCommit(); + } + } + + private final class CommitAwareOutputStream extends ServletOutputStream { + + private final ServletOutputStream delegate; + + CommitAwareOutputStream(ServletOutputStream delegate) { + this.delegate = delegate; + } + + @Override + public void write(int b) throws IOException { + trackWritten(1); + this.delegate.write(b); + } + + @Override + public void write(byte[] b) throws IOException { + trackWritten(b.length); + this.delegate.write(b); + } + + @Override + public void write(byte[] b, int off, int len) throws IOException { + trackWritten(len); + this.delegate.write(b, off, len); + } + + @Override + public void flush() throws IOException { + beforeCommit(); + this.delegate.flush(); + } + + @Override + public void close() throws IOException { + beforeCommit(); + this.delegate.close(); + } + + @Override + public boolean isReady() { + return this.delegate.isReady(); + } + + @Override + public void setWriteListener(WriteListener listener) { + this.delegate.setWriteListener(listener); + } + } + + private final class CommitAwareWriter extends PrintWriter { + + CommitAwareWriter(PrintWriter delegate) { + super(delegate, false); + } + + @Override + public void write(int c) { + trackWritten(1); + super.write(c); + } + + @Override + public void write(char[] buf, int off, int len) { + trackWritten(len); + super.write(buf, off, len); + } + + @Override + public void write(String s, int off, int len) { + trackWritten(len); + super.write(s, off, len); + } + + @Override Review Comment: `PrintWriter.println()` writes the line separator straight to the wrapped `out` through its private `newLine()`, bypassing these overrides, so line separators never count toward `Content-Length`. In the same Tomcat run, `setContentLength(3)` + `writer.println('hi')` counted 2 of 3. Tomcat treats the response as committed once the declared length has been written, so the headers written in `finally` were dropped. Counting the separator fixes it (verified): ```suggestion @Override public void println() { trackWritten(System.lineSeparator().length()); super.println(); } @Override ``` A `println` variant of the content-length test would cover it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
