jdaugherty commented on code in PR #15967: URL: https://github.com/apache/grails-core/pull/15967#discussion_r3595447725
########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersFilter.java: ########## @@ -0,0 +1,65 @@ +/* + * 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 jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.springframework.util.StringUtils; +import org.springframework.web.filter.OncePerRequestFilter; + +public class GrailsSecurityHeadersFilter extends OncePerRequestFilter { + + private final GrailsSecurityHeadersProperties properties; + + public GrailsSecurityHeadersFilter(GrailsSecurityHeadersProperties properties) { + this.properties = properties; + } + + @Override + protected boolean shouldNotFilterErrorDispatch() { + return false; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) + throws ServletException, IOException { + applyHeader(response, "X-Content-Type-Options", properties.getContentTypeOptions()); + applyHeader(response, "X-Frame-Options", properties.getFrameOptions()); + applyHeader(response, "Referrer-Policy", properties.getReferrerPolicy()); + applyHeader(response, "X-XSS-Protection", properties.getXssProtection()); + if (request.isSecure()) { + applyHeader(response, "Strict-Transport-Security", properties.getHsts()); + } + applyHeader(response, "Content-Security-Policy", properties.getContentSecurityPolicy()); + filterChain.doFilter(request, response); + } + + private static void applyHeader(HttpServletResponse response, String name, + GrailsSecurityHeadersProperties.Header header) { + if (header != null && header.isEnabled() && StringUtils.hasText(header.getValue()) && + !response.containsHeader(name)) { Review Comment: The `containsHeader` guard can't see headers injected by a reverse proxy, because those are added *after* the response leaves the app. In the very common setup where nginx already sends these headers via `add_header` (which appends and never replaces upstream headers), upgrading to 8.0 means the client suddenly receives the header twice — e.g. proxy `X-Frame-Options: DENY` plus app `SAMEORIGIN`. Browsers handle conflicting duplicates inconsistently: duplicate conflicting `X-Frame-Options` causes Chrome to block framing outright, duplicate `Referrer-Policy` resolves to the last valid value, and multiple `Content-Security-Policy` headers are enforced as the intersection of all policies (strictly tighter, can break pages). haproxy `http-response set-header` replaces, so it's unaffected, but nginx `add_header` deployments will double-send by default. Since this is on by default, existing proxy-managed deployments change behavior silently on upgrade. At minimum the security guide and the 8.0 upgrade notes should call this out explicitly with the mitigation (set `grails.security.headers.enabled: false` — or per-header `enabled: false` — when the edge proxy owns these headers, or strip the app's copies at the proxy). Worth also considering whether on-by-default is the right trade-off for an upgrade release given this. ########## grails-doc/src/en/guide/upgrading/upgrading80x.adoc: ########## @@ -547,6 +547,10 @@ Spring Security is moving toward the fluent `HttpSecurity` configuration API for If your application references `DEFAULT_FILTER_ORDER` for custom filter positioning, replace it with the concrete value `-100` (computed as `OrderedFilter.REQUEST_WRAPPER_FILTER_MAX_ORDER - 100`). Longer term, consider migrating to the fluent `HttpSecurity` API for filter chain configuration. +Grails 8 also registers a servlet filter that sends default browser hardening headers for servlet web applications. +Applications that already set these headers through Spring Security or a custom filter keep their existing response values. Review Comment: I believe this claim is inverted for Spring Security. This filter registers at `GrailsFilters.LAST` (order -110), which runs *before* the Spring Security filter chain (default order -100), and it sets its headers eagerly on the way in. Spring Security's header writers (`XFrameOptionsHeaderWriter`, `HstsHeaderWriter`, `StaticHeadersWriter` — verified in spring-security-web 7.0.4 sources) all skip writing when the header is already present on the response. So an app that configures e.g. `frameOptions { deny() }` in its security config will still send the Grails default `SAMEORIGIN` after upgrading — the Grails filter wins, not Spring Security. Same for a customized HSTS policy. That's a behavioral regression for existing Spring Security users, and the opposite of what this sentence promises. Options: back off when Spring Security is on the classpath (it already provides secure defaults for these headers), or write the headers at commit time only when still absent (the `HeaderWriterFilter` pattern) so anything set later in the chain wins. If the current precedence is intentional, this doc needs to say the opposite of what it says now. ########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersFilter.java: ########## @@ -0,0 +1,65 @@ +/* + * 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 jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.springframework.util.StringUtils; +import org.springframework.web.filter.OncePerRequestFilter; + +public class GrailsSecurityHeadersFilter extends OncePerRequestFilter { + + private final GrailsSecurityHeadersProperties properties; + + public GrailsSecurityHeadersFilter(GrailsSecurityHeadersProperties properties) { + this.properties = properties; + } + + @Override + protected boolean shouldNotFilterErrorDispatch() { + return false; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) + throws ServletException, IOException { + applyHeader(response, "X-Content-Type-Options", properties.getContentTypeOptions()); + applyHeader(response, "X-Frame-Options", properties.getFrameOptions()); + applyHeader(response, "Referrer-Policy", properties.getReferrerPolicy()); + applyHeader(response, "X-XSS-Protection", properties.getXssProtection()); + if (request.isSecure()) { Review Comment: Behind a TLS-terminating proxy (the typical nginx/haproxy topology), the backend connection is plain HTTP, so `request.isSecure()` is `false` and HSTS is silently never sent — even when the user has explicitly set `grails.security.headers.hsts.enabled: true`. It only works if the app is also configured with `server.forward-headers-strategy: framework|native` so `X-Forwarded-Proto` is honored (the `ForwardedHeaderFilter` / `RemoteIpValve` run well before this filter, so ordering is fine once that's set). The docs currently just say HSTS is sent "only for secure requests" — that should spell out the proxy case: either configure `server.forward-headers-strategy`, or (usually better) set HSTS at the proxy where TLS actually terminates. Otherwise enabling HSTS here is a silent no-op for most real deployments. -- 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]
