jdaugherty commented on code in PR #15967: URL: https://github.com/apache/grails-core/pull/15967#discussion_r4088375589
########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersAutoConfiguration.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.ConditionalOnMissingClass; +import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; + +import org.grails.web.config.http.GrailsFilters; + +/** + * Registers {@link GrailsSecurityHeadersFilter} to apply baseline browser-hardening + * response headers. + * + * <p>Backs off entirely when Spring Security's header-writing infrastructure + * ({@code HeaderWriterFilter}) is on the classpath: that filter chain runs after this + * one would and only writes a header when it is still absent, so an eagerly-applied + * Grails default would silently win over an application's explicit Spring Security + * header configuration. Spring Security already ships secure header defaults of its + * own, so this auto-configuration only fills the gap for applications that don't have + * it.</p> + */ +@AutoConfiguration +@ConditionalOnWebApplication(type = ConditionalOnWebApplication.Type.SERVLET) +@ConditionalOnBooleanProperty(name = "grails.security.headers.enabled", matchIfMissing = true) +@ConditionalOnMissingClass("org.springframework.security.web.header.HeaderWriterFilter") Review Comment: This back-off makes things worse for Spring Security users rather than fixing the precedence problem. The Grails Spring Security Core plugin (`grails-spring-security/plugin` in this repo) never registers a `HeaderWriterFilter`. `SecurityFilterPosition` reserves a `HEADERS_FILTER` slot, but nothing in the plugin fills it, so plugin users get no security headers from Spring Security today. The plugin declares `spring-security-web` as an `api` dependency, which means `HeaderWriterFilter` is always on their classpath and this condition now disables the Grails filter for every one of them. The net result is zero security headers for every Grails Spring Security application, which is the opposite of what the PR description and the upgrade note promise. Two further problems with a class-presence condition: - Even in apps that do use the `HttpSecurity` DSL, `HeaderWriterFilter` only runs on requests matched by a `SecurityFilterChain`. Requests outside a `securityMatcher`, or chains with `headers.disable()`, get nothing, and this filter is no longer there to fill the gap. - The condition sits on the whole auto-configuration, so an explicitly configured value such as `grails.security.headers.content-security-policy.value` silently does nothing whenever `spring-security-web` is present. Explicit config that no-ops without any signal is a defect on its own. The precedence problem is real, but the fix belongs in the filter (write on commit so later writers win; see the comment there), not in a classpath condition. Please remove this. ########## 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: Still a silent no-op behind a TLS-terminating proxy unless `server.forward-headers-strategy` happens to be configured. Documenting the workaround is not a fix for an explicitly enabled setting that does nothing. When `request.isSecure()` is false, fall back to the forwarded scheme: `X-Forwarded-Proto: https` or RFC 7239 `Forwarded: proto=https`. Trust is not a concern for this particular header: RFC 6797 section 8.1 requires user agents to ignore `Strict-Transport-Security` received over a non-secure transport, so a client spoofing a forwarded header on a plain-HTTP direct connection gains nothing. There is already precedent for reading `X-Forwarded-Proto` directly in `GrailsWebRequest.getBaseUrl`. When `ForwardedHeaderFilter` or `RemoteIpValve` is configured, `isSecure()` is already correct and the fallback is never consulted, so the two approaches compose. ########## grails-doc/src/en/guide/security.adoc: ########## @@ -31,3 +31,65 @@ Grails has a few built in safety mechanisms by default. * The default link:scaffolding.html[scaffolding] templates HTML escape all data fields when displayed * Grails link creating tags (link:{gspTagsRef}link.html[link], link:{gspTagsRef}form.html[form], link:{gspTagsRef}createLink.html[createLink], link:{gspTagsRef}createLinkTo.html[createLinkTo] and others) all use appropriate escaping mechanisms to prevent code injection * Grails provides <<codecs,codecs>> to let you trivially escape data when rendered as HTML, JavaScript and URLs to prevent injection attacks here. + +==== HTTP Security Headers + +Grails servlet web applications send a small set of browser hardening headers by default: + +[cols="1,1", options="header"] +|=== +| Header +| Default value + +| `X-Content-Type-Options` +| `nosniff` + +| `X-Frame-Options` +| `SAMEORIGIN` + +| `Referrer-Policy` +| `strict-origin-when-cross-origin` + +| `X-XSS-Protection` +| `0` +|=== + +HSTS and Content Security Policy are not enabled by default because their correct values depend on deployment topology and application assets. +When HSTS is enabled, Grails sends `Strict-Transport-Security` only for secure requests (`request.isSecure()`). + +If your application is deployed behind a TLS-terminating reverse proxy (the typical nginx/haproxy topology), the connection between the proxy and the application is plain HTTP, so `request.isSecure()` is `false` unless the container is told to trust the proxy's forwarded headers. +Configure the Spring Boot `server.forward-headers-strategy: framework` (or `native`) property so `X-Forwarded-Proto` is honored, or set HSTS at the proxy instead, where TLS actually terminates. +Without one of these, enabling HSTS here is a silent no-op behind most reverse proxies. + +If Spring Security's own header-writing filter (`HeaderWriterFilter`) is on the classpath, this filter does not apply at all - Spring Security already provides its own configurable header defaults, and layering Grails' eager defaults underneath would let them win over an application's explicit Spring Security header configuration. + +You can disable the whole filter, disable individual headers, or override values from `application.yml`: + +[source,yaml] +.grails-app/conf/application.yml +---- +grails: + security: + headers: + enabled: true + frame-options: + value: DENY + referrer-policy: + value: no-referrer-when-downgrade + hsts: + enabled: true + value: max-age=31536000; includeSubDomains + content-security-policy: + enabled: true + value: default-src 'self' +---- + +Set `grails.security.headers.<header>.enabled` to `false` to omit a single header. +For example, `grails.security.headers.xss-protection.enabled: false` omits `X-XSS-Protection`. +If an application or another filter has already set one of these headers on the response, Grails leaves that value unchanged. + +This "already set" check can only see headers set inside the servlet container - it has no visibility into headers a reverse proxy adds to the response after it leaves the application. Review Comment: The duplication problem needs a behavioral fix, not a paragraph telling users to turn the feature off after they discover it. This is on by default, so a proxy-managed deployment changes what the browser receives the moment it upgrades, and nothing in the app logs or tests will tell them. The application cannot see headers the proxy appends on the way out, but it can detect that the request came through a proxy. Signals available per request: `Forwarded`, `X-Forwarded-For`, `X-Forwarded-Proto`, `X-Forwarded-Host`, `Via`, `X-Real-IP`. Signals available from configuration: `server.forward-headers-strategy` set to `framework` or `native`, or an active Spring Boot `CloudPlatform` (Boot itself uses that signal to turn forwarded-header handling on). Suggested behavior, as a tri-state setting defaulting to `auto`: when a proxy is detected, apply only the headers the application configured explicitly and skip the implicit defaults, logging once so operators can see why. Direct-exposed apps, the ones that most need these defaults, keep them. Proxied apps send exactly what they sent on 7.x unless they opt in with `always`, so there is no upgrade regression in either direction. `never` skips detection entirely for apps that know the proxy does not touch these headers. One caveat worth writing into the design: a client can add a forwarded header to its own direct request and suppress the defaults for that one response. That is not a practical bypass for clickjacking or MIME sniffing, since the attacker does not control the victim browser's request headers, but it should be stated. ########## 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()); Review Comment: Applying eagerly before the chain is the root cause of the Spring Security precedence problem, and the objection to the `finally`-block suggestion (redirects / `sendError` / streaming commit during the chain) has a standard answer: write the headers at commit time. Wrap the response in a small `HttpServletResponseWrapper` that intercepts the commit points (`sendError`, `sendRedirect`, `flushBuffer`, `getWriter().flush()/close()`, `getOutputStream().flush()/close()`, and content-length reached) and fills any still-missing headers immediately before delegating. If the chain returns without committing, fill them then. This is exactly what Spring Security's `HeaderWriterFilter` does with `OnCommittedResponseWrapper`, and when both filters are active the nesting works out naturally: the later filter's wrapper fires first and writes its configured values, then this wrapper fills only what is still absent. Anything set by a controller, an interceptor, or Spring Security wins; Grails only supplies defaults for headers nobody else set. With that in place the `@ConditionalOnMissingClass` back-off in the auto-configuration is unnecessary and should go. The existing redirect-committed and `DispatcherType.ERROR` tests should keep passing, and there should be a new test that a downstream filter setting `X-Frame-Options: DENY` after this filter runs ends up with `DENY`, not `SAMEORIGIN`. -- 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]
