Skip to content

Add default configurable Grails security response headers - #15967

Open
jamesfredley wants to merge 7 commits into
8.0.xfrom
feat/default-security-headers
Open

Add default configurable Grails security response headers#15967
jamesfredley wants to merge 7 commits into
8.0.xfrom
feat/default-security-headers

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Description

What was found

Problem Impact
Grails apps do not set common security headers by default Missing X-Content-Type-Options, X-Frame-Options, Referrer-Policy, etc.
Google Doc 2.1 listed framework default security headers as not covered No existing open PR for this
Apps that use Spring Security still benefit from baseline headers when plugin is absent Framework should provide safe defaults with opt-out

What changed

Area Change
Filter GrailsSecurityHeadersFilter (OncePerRequestFilter)
Auto-config GrailsSecurityHeadersAutoConfiguration with @ConditionalOnMissingBean
Defaults X-Content-Type-Options: nosniff, X-Frame-Options: SAMEORIGIN, Referrer-Policy: strict-origin-when-cross-origin, X-XSS-Protection: 0
HSTS Only applied on secure requests when configured
CSP Disabled by default; set via config when desired
Config grails.security.headers.* enable/disable and per-header values
Docs / tests Security guide, upgrade notes, auto-config specs

Review feedback addressed

Source Item Resolution
Copilot Apply headers in a finally block after the chain Declined: for security headers that skips redirect/error/streaming responses that commit during the chain. Kept eager before-chain application with containsHeader gap-fill (downstream setHeader still wins) + added a redirect-committed regression test
Review pass ERROR redispatches lacked headers Overrode shouldNotFilterErrorDispatch() to false + DispatcherType.ERROR test

Out of scope / follow-up

Topic Status
Full CSP policy templates Follow-up
Permissions-Policy defaults Follow-up
Spring Security ordering (eager defaults vs SS skip-if-present writers; needs on-commit wrapper) Follow-up (SS is an optional plugin)

Related MD topics

Source Topic
Google Doc 2.1 Default framework security headers
Status map Not covered

Contributor Checklist

Issue and Scope

  • Background explains the security-headers gap.
  • Scoped to default response headers only.
  • Single focused feature seed.
  • Targets 8.0.x.

Code Quality

  • Tests cover enable/disable and defaults.
  • Focused controllers tests intended for CI.
  • No mass reformatting.
  • AI starting point labeled.

Licensing and Attribution

  • Apache License 2.0.
  • Contributor rights confirmed.
  • ai-generated-starting-point label applied.

Documentation

  • User-facing docs updated.
  • PR description explains what changed and why.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Register a OncePerRequestFilter for X-Content-Type-Options,
X-Frame-Options, Referrer-Policy, optional HSTS on secure requests,
and optional CSP. Configurable via grails.security.headers.* with
opt-out and ConditionalOnMissingBean.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a default, configurable servlet filter in grails-controllers that applies baseline browser-hardening response headers for Grails 8 servlet web applications, along with auto-configuration, configuration metadata, tests, and documentation updates in the user guide and upgrade notes.

Changes:

  • Added GrailsSecurityHeadersFilter plus GrailsSecurityHeadersProperties and a servlet-only Boot auto-configuration to register it by default (with opt-out via grails.security.headers.*).
  • Added configuration metadata for IDE/property hinting and documented the defaults + customization in the Security guide and Grails 8 upgrade notes.
  • Added a Spock spec verifying auto-config behavior, default headers, per-header disablement, overrides, and secure-request HSTS behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
grails-doc/src/en/guide/upgrading/upgrading80x.adoc Adds Grails 8 upgrade note about the new default security headers filter and how to disable/configure it.
grails-doc/src/en/guide/security.adoc Documents default headers, rationale for HSTS/CSP being opt-in, and example application.yml configuration.
grails-controllers/src/test/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersAutoConfigurationSpec.groovy Adds test coverage for auto-configuration and header application behavior.
grails-controllers/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports Registers the new auto-configuration class for Spring Boot discovery.
grails-controllers/src/main/resources/META-INF/additional-spring-configuration-metadata.json Adds Spring configuration metadata entries for grails.security.headers.* properties.
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersProperties.java Introduces bindable configuration properties and defaults for each supported header.
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersFilter.java Implements the OncePerRequestFilter that applies headers conditionally based on configuration and existing response headers.
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersAutoConfiguration.java Adds servlet-only auto-config with conditional registration and a FilterRegistrationBean for ordering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bito-code-review

Copy link
Copy Markdown

The proposed change to move header application into a finally block is technically sound for ensuring that downstream components have the first opportunity to set headers. By applying defaults only if the headers are missing after the filter chain completes, you prevent the filter from prematurely setting values that might conflict with or override specific requirements set by controllers or other filters. This approach effectively balances providing sensible defaults with allowing granular overrides.

grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/GrailsSecurityHeadersFilter.java

@Override
    protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
            throws ServletException, IOException {
        try {
            filterChain.doFilter(request, response);
        } finally {
            applyHeader(response, "X-Content-Type-Options", properties.getContentTypeOptions());
            // ... apply other headers
        }
    }

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.21212% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.3591%. Comparing base (e88612c) to head (b2f970d).
⚠️ Report is 6 commits behind head on 8.0.x.

Files with missing lines Patch % Lines
...b/controllers/GrailsSecurityHeadersProperties.java 58.5366% 17 Missing ⚠️
...s/web/controllers/GrailsSecurityHeadersFilter.java 88.2353% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #15967        +/-   ##
==================================================
+ Coverage     52.3514%   52.3591%   +0.0077%     
- Complexity      18299      18310        +11     
==================================================
  Files            2036       2039         +3     
  Lines           96347      96413        +66     
  Branches        16829      16831         +2     
==================================================
+ Hits            50439      50481        +42     
- Misses          38485      38507        +22     
- Partials         7423       7425         +2     
Files with missing lines Coverage Δ
...ollers/GrailsSecurityHeadersAutoConfiguration.java 100.0000% <100.0000%> (ø)
...s/web/controllers/GrailsSecurityHeadersFilter.java 88.2353% <88.2353%> (ø)
...b/controllers/GrailsSecurityHeadersProperties.java 58.5366% <58.5366%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The Copilot suggestion to apply the headers in a post-chain finally
block would skip every security header on responses that commit during
the chain (sendRedirect, sendError, flushBuffer, streaming), which is
exactly where the headers are needed. Keep applying the headers eagerly
before the filter chain (values already present still win, explicit
downstream setHeader still replaces) and add a regression test proving
the headers are present on a redirect-committed response.

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]
OncePerRequestFilter.shouldNotFilterErrorDispatch() defaults to true, so
the filter was skipped on ERROR redispatches even though the ERROR
dispatcher type is registered, leaving error responses without the
security headers. Override it to return false and cover the behavior
with a DispatcherType.ERROR test.

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]
@jamesfredley

Copy link
Copy Markdown
Contributor Author

Review feedback addressed

Merged the latest 8.0.x and pushed follow-up commits.

Copilot review comment - apply headers in a finally block after the chain: evaluated and intentionally not adopted, because for security headers it regresses committed responses. If a downstream sendRedirect / sendError / flushBuffer / streaming write commits the response during the chain, isCommitted() is already true on return and a post-chain block would skip all hardening headers on exactly the redirect/error/streaming responses that need them. Instead the filter keeps applying the defaults eagerly before the chain with a containsHeader gap-fill (so a value set earlier wins, and an explicit downstream setHeader still replaces it), and I added:

  • a regression test that a redirect-committed response still carries the default headers, and
  • an override of shouldNotFilterErrorDispatch() to false plus a DispatcherType.ERROR test, so container ERROR redispatches (which reset the response) also get the headers.

Follow-up (documented, not changed here): when the optional Spring Security plugin is installed, its header writers skip headers already present, so eagerly-set Grails defaults can win over a Spring-Security-configured policy. The robust fix is an on-commit response wrapper that fills only-missing headers after downstream writers run; that is a larger change and Spring Security is an optional plugin, so I left it as a follow-up rather than expand this starter filter's scope.

Local verification: :grails-controllers:test --tests GrailsSecurityHeadersAutoConfigurationSpec (9 features) passes.

@jdaugherty jdaugherty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look at this with reverse-proxy deployments (nginx/haproxy) in mind, since that's how most production Grails apps run. A few concerns inline — the common thread is that the containsHeader guard only protects against headers set inside the servlet container, but in real deployments these headers are frequently owned by the edge proxy or by Spring Security, and in both cases the current behavior is surprising.

Comment thread grails-doc/src/en/guide/upgrading/upgrading80x.adoc Outdated
@jdaugherty

Copy link
Copy Markdown
Contributor

The reverse proxy scenarios are real regressions that need addressed.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
… gaps

Addresses jdaugherty's 3 unresolved review findings, all confirmed
against the current code before fixing:

- Spring Security precedence (verified inverted doc claim): this filter
  registers at GrailsFilters.LAST (order -110), before Spring Security's
  chain (-100), and applied its defaults eagerly - so Spring Security's
  header writers (which skip a header that's already present) never got
  a chance to win, contrary to what the upgrade notes claimed. Fixed by
  adding @ConditionalOnMissingClass("...HeaderWriterFilter") to
  GrailsSecurityHeadersAutoConfiguration so the whole auto-configuration
  backs off when Spring Security's header-writing infrastructure is on
  the classpath, per jdaugherty's suggested option - Spring Security
  already ships its own configurable header defaults. Added a
  spring-security-web test-only dependency and a
  FilteredClassLoader-based helper so the existing "Spring Security
  absent" tests keep exercising that path explicitly, plus a new test
  confirming the back-off.
- HSTS silently never sent behind a TLS-terminating reverse proxy
  (request.isSecure() is false unless server.forward-headers-strategy
  is configured): this isn't a code bug - request.isSecure() is the
  correct check, and Spring Boot's forward-headers-strategy is the
  standard way to make it proxy-aware - so documented the proxy case
  and the mitigation in the security guide instead of changing behavior.
- Reverse-proxy header duplication (nginx add_header appends rather than
  replaces, so the client can receive a header twice once the app also
  sends it): also not something a code change here can generally detect
  or fix, since the proxy operates entirely outside the request
  pipeline. Documented the risk (with the concrete Chrome/CSP/
  Referrer-Policy duplicate-header failure modes jdaugherty identified)
  and the mitigation (disable the affected header(s) at the app and let
  the proxy own them, or strip the app's copy at the proxy).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 27, 2026
@testlens-app

testlens-app Bot commented Aug 6, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: b2f970d
▶️ Tests: 57436 executed
⚪️ Checks: 60/60 completed


Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants