codeconsole commented on code in PR #15967: URL: https://github.com/apache/grails-core/pull/15967#discussion_r4089483838
########## grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersProperties.java: ########## @@ -0,0 +1,173 @@ +/* + * 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 org.springframework.boot.context.properties.ConfigurationProperties; + +@ConfigurationProperties(prefix = "grails.security.headers") +public class GrailsSecurityHeadersProperties { + + private boolean enabled = true; + + private Defaults defaults = Defaults.AUTO; Review Comment: With `AUTO` as the default, most production deployments get none of the default headers. Every request through a load balancer or ingress carries `X-Forwarded-For`/`X-Real-IP`, and every Kubernetes pod has `KUBERNETES_SERVICE_HOST`/`KUBERNETES_SERVICE_PORT` set, which makes Boot report `CloudPlatform.KUBERNETES`. I ran the auto-configured filter (`securityHeadersFilter(properties, new StandardEnvironment())`) on embedded Tomcat 11.0.26: ``` environment request nosniff / X-Frame-Options / Referrer-Policy plain host direct sent plain host X-Forwarded-For: 203.0.113.7 <missing> KUBERNETES_SERVICE_* set direct <missing> KUBERNETES_SERVICE_* set X-Forwarded-For: 203.0.113.7 <missing> ``` The headers only appear on requests that hit the app directly, which in practice means local dev and tests. The production traffic they're meant to protect doesn't get them, and the only trace is one INFO line. `AUTO` assumes the proxy supplies these headers, but ingress-nginx, AWS ALB, the GKE ingress and Heroku's router add none of them unless someone configures them to. The risk `AUTO` guards against, a proxy that already sends one of these headers, is small for these four defaults: - `X-Content-Type-Options`: browsers only look at the first value, and `nosniff` twice is still `nosniff`. - `X-Frame-Options`: identical values collapse to one. Conflicting values (proxy `DENY`, app `SAMEORIGIN`) make the browser block framing, which is the stricter outcome the proxy asked for. - `Referrer-Policy`: the last valid token wins, and nginx's `add_header` goes after the upstream headers, so the proxy's policy still applies. - `X-XSS-Protection`: no current browser implements the XSS auditor, so this header does nothing either way. CSP is the header where duplicates really hurt (multiple policies are intersected), and it isn't a default. Explicitly configured headers are sent whether or not a proxy is detected, so `AUTO` doesn't prevent that case anyway. I'd default to `ALWAYS` and keep `AUTO` as the opt-in for deployments whose proxy owns these headers, which the upgrade note's nginx `add_header` guidance already describes. ```suggestion private Defaults defaults = Defaults.ALWAYS; ``` The metadata JSON default, the `security.adoc` table, the upgrade note and the defaults specs would need to follow. -- 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]
