codeconsole commented on code in PR #15967: URL: https://github.com/apache/grails-core/pull/15967#discussion_r4097507723
########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/SecurityHeadersResponseWrapper.java: ########## @@ -0,0 +1,402 @@ +/* + * 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 java.nio.CharBuffer; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; + +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, writes that reach a + * declared {@code Content-Length}, and writes that fill the container's response + * buffer (which the container flushes, and thereby commits, on its own). If the + * wrapped chain returns without committing, the owner is expected to invoke + * {@link #beforeCommit()} itself. A {@link #reset()} discards the headers along with the + * body, so it re-arms the callback for the response that replaces them. + * + * <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> + * + * <p>Body size is counted in bytes, the unit of both the container's buffer and + * {@code Content-Length}. Output stream writes count as written. Writer output counts as + * its encoded length in the response's character encoding: exactly for UTF-8 and + * single-byte encodings, and at the encoding's maximum bytes per character for any + * other, so the count never falls behind the bytes the container holds. Spring Security's + * {@code OnCommittedResponseWrapper} counts writer output in characters instead, so for a + * multi-byte body large enough to fill the buffer this callback can run before Spring + * Security's, and a header Spring Security only writes when it is absent then keeps the + * value written here.</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; + + /** + * The container's buffer size, read once content is being written. The servlet API + * forbids changing it after that point, so the value is cached and only refreshed by + * {@link #setBufferSize(int)}, {@link #reset()} or {@link #resetBuffer()}. + */ + private int bufferSize = -1; + + 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(); + } + + /** + * Clears the status, headers and body. The headers the callback wrote are gone with + * it, so the callback is armed again for whatever is written next. + */ + @Override + public void reset() { + super.reset(); + this.fired = false; + this.contentLength = -1; + this.contentWritten = 0; + this.bufferSize = -1; + } + + @Override + public void resetBuffer() { + super.resetBuffer(); + this.contentWritten = 0; + this.bufferSize = -1; + } + + @Override + public void setBufferSize(int size) { + super.setBufferSize(size); + this.bufferSize = -1; + } + + @Override + public void setContentLength(int len) { + trackContentLength(len); + super.setContentLength(len); + } + + @Override + public void setContentLengthLong(long len) { + trackContentLength(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 void setIntHeader(String name, int value) { + trackContentLengthHeader(name, value); + super.setIntHeader(name, value); + } + + @Override + public void addIntHeader(String name, int value) { + trackContentLengthHeader(name, value); + super.addIntHeader(name, value); + } + + @Override + public ServletOutputStream getOutputStream() throws IOException { + return new CommitAwareOutputStream(super.getOutputStream()); + } + + @Override + public PrintWriter getWriter() throws IOException { + PrintWriter writer = super.getWriter(); + return new CommitAwareWriter(writer, getCharacterEncoding()); + } Review Comment: Nit (optional): each call builds a new `CommitAwareWriter` and resolves the charset again (plus `newEncoder()` for anything but UTF-8), whereas the container hands back the same writer every time. Caching it keeps `response.writer.is(response.writer)` true like the unwrapped response. `reset()` has to clear it, since the encoding can change after a reset: ```java private PrintWriter writer; @Override public PrintWriter getWriter() throws IOException { if (this.writer == null) { this.writer = new CommitAwareWriter(super.getWriter(), getCharacterEncoding()); } return this.writer; } ``` plus `this.writer = null;` in `reset()`. I tried it: the same instance comes back on repeated calls, a new one after `reset()`, and the Tomcat commit-path cases above still send the headers. `getOutputStream()` could get the same treatment. ########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersFilter.java: ########## @@ -0,0 +1,226 @@ +/* + * 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.net.URI; +import java.util.List; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.springframework.http.HttpHeaders; +import org.springframework.http.server.ServletServerHttpRequest; +import org.springframework.util.StringUtils; +import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.util.ForwardedHeaderUtils; + +/** + * Applies baseline browser hardening response headers. + * + * <p>Headers are written immediately before the response commits (or when the filter + * chain returns, whichever comes first) and only when nothing further down the chain + * has already set them, so a controller, an interceptor, another filter or Spring + * Security's header writers running inside this filter win over the Grails defaults. + * A filter that runs <em>outside</em> this one and, like most of Spring Security's header + * writers, only fills headers that are still absent finds the Grails value already in + * place; see {@link GrailsSecurityHeadersAutoConfiguration} for the ordering.</p> + * + * <p>When {@code grails.security.headers.defaults} is {@code auto} and the request is + * detected as having come through a reverse proxy, the built-in default values are + * suppressed and only headers the application configured explicitly are applied, for + * deployments where the proxy owns these headers and the application cannot see what it + * adds. See {@link GrailsSecurityHeadersProperties.Defaults} for the controlling + * setting.</p> + * + * <p>When {@link GrailsSecurityHeadersProperties#isEnabled()} is {@code false} the filter + * passes every request through untouched. The auto-configuration does not register the + * filter at all in that case; the check here covers filters an application constructs + * itself.</p> + * + * @since 8.0 + */ +public class GrailsSecurityHeadersFilter extends OncePerRequestFilter { + + /** + * Request headers whose presence indicates the request was relayed by a reverse + * proxy or load balancer. + */ + static final List<String> REVERSE_PROXY_REQUEST_HEADERS = List.of( + "Forwarded", "X-Forwarded-For", "X-Forwarded-Proto", "X-Forwarded-Host", "Via", "X-Real-IP"); + + private static final Logger logger = LoggerFactory.getLogger(GrailsSecurityHeadersFilter.class); + + private static final String HTTPS = "https"; + + private final GrailsSecurityHeadersProperties properties; + + private final boolean reverseProxyConfigured; + + private final AtomicBoolean reverseProxyLogged = new AtomicBoolean(); + + private final Set<String> blankValueLogged = ConcurrentHashMap.newKeySet(); + + public GrailsSecurityHeadersFilter(GrailsSecurityHeadersProperties properties) { + this(properties, false); + } + + /** + * @param properties the header configuration + * @param reverseProxyConfigured whether the deployment is known to sit behind a reverse + * proxy from configuration alone (for example a forwarded-headers strategy or an active + * cloud platform), independent of any per-request signal + */ + public GrailsSecurityHeadersFilter(GrailsSecurityHeadersProperties properties, boolean reverseProxyConfigured) { + this.properties = properties; + this.reverseProxyConfigured = reverseProxyConfigured; + } + + @Override + protected boolean shouldNotFilterErrorDispatch() { + return false; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) + throws ServletException, IOException { + if (!this.properties.isEnabled()) { + filterChain.doFilter(request, response); + return; + } + boolean applyDefaults = shouldApplyDefaults(request); + boolean secure = isEnabled(this.properties.getHsts()) && isSecure(request); + SecurityHeadersResponseWrapper wrapped = new SecurityHeadersResponseWrapper(response, + () -> writeHeaders(response, applyDefaults, secure)); + try { + filterChain.doFilter(request, wrapped); + } + finally { + wrapped.beforeCommit(); + } + } + + private void writeHeaders(HttpServletResponse response, boolean applyDefaults, boolean secure) { + applyHeader(response, "X-Content-Type-Options", "content-type-options", + this.properties.getContentTypeOptions(), applyDefaults); + applyHeader(response, "X-Frame-Options", "frame-options", this.properties.getFrameOptions(), applyDefaults); + applyHeader(response, "Referrer-Policy", "referrer-policy", this.properties.getReferrerPolicy(), applyDefaults); + applyHeader(response, "X-XSS-Protection", "xss-protection", this.properties.getXssProtection(), applyDefaults); + if (secure) { + applyHeader(response, "Strict-Transport-Security", "hsts", this.properties.getHsts(), applyDefaults); + } + applyHeader(response, "Content-Security-Policy", "content-security-policy", + this.properties.getContentSecurityPolicy(), applyDefaults); + } + + private boolean shouldApplyDefaults(HttpServletRequest request) { + return switch (this.properties.getDefaults()) { + case ALWAYS -> true; + case NEVER -> false; + case AUTO -> !isBehindReverseProxy(request); + }; + } + + private boolean isBehindReverseProxy(HttpServletRequest request) { + String signal = this.reverseProxyConfigured ? "server configuration" : detectReverseProxyHeader(request); + if (signal == null) { + return false; + } + if (this.reverseProxyLogged.compareAndSet(false, true)) { + logger.info("Reverse proxy detected via {} and grails.security.headers.defaults is 'auto': the default " + + "Grails security headers are not applied and only explicitly configured " + + "grails.security.headers.* values are sent. Set grails.security.headers.defaults to 'always' " + + "to apply the defaults behind the proxy, or to 'never' to rely solely on explicit " + + "configuration.", signal); + } + return true; + } + + private static String detectReverseProxyHeader(HttpServletRequest request) { + for (String name : REVERSE_PROXY_REQUEST_HEADERS) { + if (StringUtils.hasText(request.getHeader(name))) { + return name + " request header"; + } + } + return null; + } + + /** + * Whether the client connection is secure. Falls back to the scheme a reverse proxy + * forwards in the RFC 7239 {@code Forwarded} header or in {@code X-Forwarded-Proto} + * (and {@code X-Forwarded-Ssl}), read the same way Spring's {@code ForwardedHeaderFilter} + * reads them, when the container itself saw plain HTTP. That is the case behind a + * TLS-terminating proxy that has not been configured through + * {@code server.forward-headers-strategy}. Trusting the forwarded scheme is safe for + * HSTS: user agents ignore a {@code Strict-Transport-Security} header received over a + * non-secure transport (RFC 6797, section 8.1), so a spoofed header on a plain + * connection has no effect. + */ + private static boolean isSecure(HttpServletRequest request) { + if (request.isSecure()) { + return true; + } + HttpHeaders headers = new ServletServerHttpRequest(request).getHeaders(); + try { + // Only the scheme is needed, so the forwarded headers are applied to the request's + // scheme alone rather than to its URL: java.net.URI rejects request paths the + // container may accept, such as those allowed through Tomcat's relaxedPathChars. + URI base = URI.create(request.getScheme() + "://localhost"); + String scheme = ForwardedHeaderUtils.adaptFromForwardedHeaders(base, headers).build().getScheme(); + return HTTPS.equalsIgnoreCase(scheme); + } + catch (IllegalArgumentException malformedForwardedHeaders) { + // A forwarded port that is not a number: no trustworthy scheme to go on. Review Comment: Nit (optional): this also catches `ForwardedHeaderUtils` rejecting a malformed forwarded host and `URI.create` rejecting the scheme, not only a non-numeric port. ```suggestion // Malformed forwarded headers: no trustworthy scheme to go on. ``` -- 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]
