matrei commented on PR #15967:
URL: https://github.com/apache/grails-core/pull/15967#issuecomment-5816037537
Reviewed at head `5c8fc21206` (base `8.0.x`, merge-base `7ea4cb9d8e`; merges
cleanly into current `8.0.x` at `a441334507`).
What I ran locally:
- `:grails-controllers:cleanTest :grails-controllers:test --no-build-cache`:
182 tests, 0 failures (84 in `GrailsSecurityHeadersAutoConfigurationSpec`, 9 in
`GrailsSecurityHeadersFilterTomcatSpec`).
- `:grails-controllers:codeStyle`: clean.
- Throwaway specs (not committed) that put the filter in front of servlets
on the embedded Tomcat from the test classpath, to try commit paths and request
shapes the PR's specs don't cover. Results are below.
The commit-time wrapper, the move to `GrailsFilters.FIRST` and the `always`
default fix the reverse-proxy, Spring Security precedence and asset-pipeline
problems raised in the earlier reviews by @jdaugherty and @codeconsole. The
latest commit adds re-arming on `reset()`, the typed-registration back-off and
the blank-value warning, which all look right. Three things are left before
merge: the switch to `ForwardedHeaderUtils` turns some requests into 500s when
HSTS is enabled, one commit path still drops the headers, and the PR
description is stale. The rest are small.
### 1. With HSTS enabled, URLs Tomcat accepts but `java.net.URI` rejects now
fail with 500
`GrailsSecurityHeadersFilter.java:185-191`: `isSecure` now builds a
`ServletServerHttpRequest` and calls `getURI()` so it can hand the request to
`ForwardedHeaderUtils`. `getURI()` throws `IllegalStateException` ("Could not
resolve HttpServletRequest as URI") when the request URL isn't a valid
`java.net.URI`, and only `IllegalArgumentException` is caught. Tomcat lets such
paths through when `relaxedPathChars` is configured
(`server.tomcat.relaxed-path-chars`), which apps set for legacy links
containing `|`, `{}` or `[]`. I ran it on Tomcat with
`relaxedPathChars='|{}[]^`'` and `hsts.enabled: true`:
```
request status nosniff HSTS
GET /plain 200 sent -
GET /p|x 500 - -
GET /p{x} 500 - -
GET /plain?a=|b 200 sent -
(getURI re-encodes a bad query)
GET /p|x X-Forwarded-Proto: https 500 - -
GET /plain X-Forwarded-Proto: https 200 sent sent
```
With HSTS disabled (the default), `isSecure` isn't called and these requests
return 200, so the failure only shows up after someone turns HSTS on. The
previous hand-written parser didn't touch the URI. Only the scheme is needed
here, so it can be resolved without the request URL at all, for example:
```java
HttpHeaders headers = new ServletServerHttpRequest(request).getHeaders();
String scheme =
ForwardedHeaderUtils.adaptFromForwardedHeaders(URI.create("http://localhost";),
headers)
.build().getScheme();
```
Catching `IllegalStateException` as well would also stop the 500s, but it
would silently skip HSTS on those requests. A spec row with a relaxed path char
and HSTS enabled would cover it.
### 2. Multi-byte text written through the writer can commit before the
wrapper fires
`SecurityHeadersResponseWrapper.java:229`: the writer counts characters, but
the buffer it is compared against (`getBufferSize()`) is in bytes. On Tomcat
the default 8 KB char buffer usually hides this. Once the response buffer is
larger than that, UTF-8 text with 3-byte characters fills the byte buffer, and
commits, at about a third of the character count the wrapper is waiting for:
```
servlet nosniff /
X-Frame-Options
setBufferSize(32768), 400 × write('日' * 50) (60 KB) <missing> /
<missing> (every run, at both heads)
setBufferSize(32768), 400 × write('ж' * 50) (40 KB) sent
setBufferSize(32768), 1000 × write('a' * 50) (50 KB) sent
default buffer, 400 × write('日' * 50) sent (4 of 4
runs)
```
It also leaked from one request to the next. In my first run, a plain
default-buffer `/cjk-chunks` request lost its headers because it came right
after a request on the same keep-alive connection that had called
`setBufferSize(32768)`. Tomcat had kept the enlarged buffer on the recycled
response. It happened again at `5c8fc21206`. So an app doesn't have to call
`setBufferSize` on the affected request itself.
The class Javadoc (`SecurityHeadersResponseWrapper.java:43-47`) says a
multi-byte encoding reaches the buffer "a little before the count does" and
that "the callback still runs no later than the chain returning". It does run
then, but the response has already committed, so the `setHeader` calls are
dropped without any error. The comment describes this as safe when it isn't.
Spring Security's `OnCommittedResponseWrapper` makes the same approximation,
so this isn't a regression compared with Spring Security. Still, CJK sites are
exactly the users who would hit it. Options:
- Count writer output conservatively as `chars *
charsetEncoder.maxBytesPerChar()` for the response's character encoding. This
fires early rather than late. The trade-off is that an inner skip-if-present
writer that counts characters (Spring Security's `HeaderWriterFilter`) may fire
after this one on large multi-byte bodies and lose precedence. Given the
choice, sending the default is better than sending nothing.
- At minimum, correct the Javadoc and document the limitation in
`security.adoc` next to the "body large enough to fill the container's response
buffer" sentence.
The latest commit caches the buffer size and refreshes it on
`setBufferSize`/`reset`/`resetBuffer`, which doesn't change this: the count is
still in characters.
Either way, a Tomcat spec case with `setBufferSize(32768)` and chunked
3-byte characters would pin it down.
### 3. The PR description no longer matches the code
The squash commit will most likely be built from the description, and it
currently says:
- "Kept eager before-chain application with `containsHeader` gap-fill" /
Copilot `finally` suggestion "Declined". The filter now writes at commit time.
- "Spring Security ordering (… needs on-commit wrapper) — Follow-up". That
is done in this PR.
- "`GrailsSecurityHeadersAutoConfiguration` with
`@ConditionalOnMissingBean`". There is now also a custom condition that backs
off on a typed `FilterRegistrationBean<GrailsSecurityHeadersFilter>`.
- Nothing about `grails.security.headers.defaults`
(`always`/`auto`/`never`), forwarded-scheme HSTS, `GrailsFilters.FIRST`, or the
Tomcat spec.
- References to "Google Doc 2.1" and a "Status map", which readers of the
public PR can't see.
Please rewrite it to describe the final behaviour before merging.
### 4. The upgrade note is under the wrong heading
`upgrading80x.adoc:500`: the note is a paragraph inside "13. Spring Security
Filter Ordering". This change alters the response headers of every servlet
application, not just Spring Security users, so someone skipping the Spring
Security section will miss it. It should be its own numbered section (for
example "14. Default Security Response Headers", renumbering the ones after it).
### 5. The "explicitly configured is always sent" wording is too broad
`security.adoc:97`: "Any header you configure explicitly (setting either its
`enabled` or its `value` key) is always sent". That isn't true for:
- `content-security-policy.value` or `hsts.value` set on its own. Both keep
`enabled: false`, so nothing is sent.
- `<header>.enabled: false`, which also counts as explicit.
Suggest wording along the lines of "Explicitly configured headers are not
affected by `defaults`: a header you have enabled and given a value is sent
whether or not a proxy is detected." It would also help to say directly that
CSP and HSTS need `enabled: true` as well as a value. (The latest commit's
blank-value warning covers the `enabled: true` without a value case, and the
new sentence in `security.adoc` explains it. The "always sent" sentence itself
is unchanged.)
### 6. Minor
- The new ordering paragraph says Spring Boot's "character-encoding" filter
runs outside the Grails filter (`security.adoc:65`, `upgrading80x.adoc:503`,
`GrailsSecurityHeadersAutoConfiguration.java:61`). In a Grails app it doesn't:
`ControllersAutoConfiguration` registers its own `CharacterEncodingFilter` at
`GrailsFilters.CHARACTER_ENCODING_FILTER` (-180), which is inside `FIRST`, and
Boot's `HttpEncodingAutoConfiguration` backs off. Boot's `ErrorPageFilter` is
only registered for WAR deployments, which is worth saying too. Only the
forwarded-header filter is outside in a default Grails app.
- `GrailsSecurityHeadersFilter.java:71`: `REVERSE_PROXY_REQUEST_HEADERS` is
`public static final`, which adds to the public API a list that only the filter
and the spec's `where:` block use. Package-private would do, and it avoids
having to keep the list stable.
- `GrailsSecurityHeadersFilterTomcatSpec.groovy:72,75`: the two temp
directories are never deleted. Spock's `@TempDir` on a `@Shared` field would
clean them up.
- Optional: `GrailsSecurityHeadersFilterTomcatSpec` could implement
`HttpClientSupport` from `grails-testing-support-http-client` instead of its
own `get()` helper and `firstValue(...).present` checks. `assertHeaders(...,
status)` would then check all four default values and the status in one call;
today only two of the four values are checked. This needs `testImplementation
project(':grails-testing-support-http-client')`, which doesn't create a cycle
(`grails-gsp` already uses the testing-support modules in its tests). There's
no Spring context to inject `local.server.port`, so the spec overrides
`getHttpBaseUrl()`. The shared client follows redirects, so the `/redirect` row
needs a client that doesn't, passed through `http(path, client)`. I tried this
locally on the previous head (`37b983c60c`): all 8 features pass and
`codeStyle` is clean. The `/reset` row added since then carries over with the
same `referrerPolicy` default. The unused `DEFAULT_HEADER_NAMES` and the
`get()` hel
per go away, and the `java.net.http.HttpRequest`/`HttpResponse` imports are no
longer needed:
```groovy
class GrailsSecurityHeadersFilterTomcatSpec extends Specification
implements HttpClientSupport {
@Shared
HttpClient noRedirects =
HttpClient.newBuilder().followRedirects(HttpClient.Redirect.NEVER).build()
@Override
String getHttpBaseUrl() {
"http://localhost:${port}";
}
@Unroll
void 'headers are on the wire for #path (#description)'() {
when:
def response = http(path, noRedirects)
then:
response.assertHeaders([
'X-Content-Type-Options': 'nosniff',
'X-Frame-Options' : 'SAMEORIGIN',
'Referrer-Policy' : referrerPolicy,
'X-XSS-Protection' : '0'
], status)
bodyLength == null || response.body().length() == bodyLength
where:
path | description
| status | bodyLength | referrerPolicy
'/small-stream' | 'body below the response buffer'
| 200 | 100 |
'strict-origin-when-cross-origin'
'/large-stream' | 'output stream body outgrowing the buffer, no
flush' | 200 | 50 * 1024 |
'strict-origin-when-cross-origin'
'/large-writer' | 'writer body outgrowing the buffer, no flush'
| 200 | 50 * 1024 |
'strict-origin-when-cross-origin'
'/println' | 'Content-Length reached by println'
| 200 | 2 + LINE_SEPARATOR_LENGTH | 'no-referrer'
'/redirect' | 'redirect committed inside the chain'
| 302 | null |
'strict-origin-when-cross-origin'
'/error' | 'sendError committed inside the chain'
| 404 | null |
'strict-origin-when-cross-origin'
'/reset' | 'reset after the buffer filled but before it
flushed' | 500 | 5 |
'strict-origin-when-cross-origin'
'/assets/a.js' | 'served by an inner filter that never continues
the chain' | 200 | 14 |
'strict-origin-when-cross-origin'
}
void 'a header set by the servlet wins over the Grails default'() {
expect:
http('/println').hasHeaderValue('Referrer-Policy', 'no-referrer')
}
```
The `referrerPolicy` column is needed because the exact-value check
catches something the current `present` check misses: the `/println` servlet
sets `Referrer-Policy: no-referrer` itself, so that row can't expect the
default. `body()` returns a String, so its length only equals the byte count
for ASCII bodies. A multi-byte case for item 2 would need to compare byte
length.
### Verified as correct
- Commit-time writing: the wrapper intercepts `sendError`/`sendRedirect`
(all four overloads), `flushBuffer`, writer and stream `flush`/`close`,
declared `Content-Length` (including via
`setHeader`/`addHeader`/`setIntHeader`/`addIntHeader` and when declared after
the body), buffer-full, and the `println` line separator. The Tomcat spec shows
each of these on the wire.
- The Servlet 6.1 `ServletOutputStream.write(ByteBuffer)` default goes
through the wrapper's `write(byte[])`, so it is counted. Checked on Tomcat for
a 100 B and a 50 KB `ByteBuffer`: headers sent in both cases.
- `println` is not double-counted. `PrintWriter.print`/`append`/`format` all
funnel through the overridden `write` methods, and only `newLine()` bypasses
them.
- Ordering: at `FIRST` (-200) the filter is outside the asset-pipeline
filter (-190), the character encoding filter, SiteMesh, and Spring Security at
its default -100. Innermost-first firing means every one of them wins over the
Grails defaults, and the spec covers Spring Security's `HeaderWriterFilter` on
both sides.
- The `@ConditionalOnMissingClass` back-off is gone. With
`spring-security-web` on the classpath the filter stays active, which fixes the
problem that the Grails Spring Security plugin never registers a
`HeaderWriterFilter`.
- HSTS behind a TLS-terminating proxy: apart from item 1, reading the scheme
through `ForwardedHeaderUtils` matches `ForwardedHeaderFilter` (first
`Forwarded` element, `X-Forwarded-Proto`, `X-Forwarded-Ssl`). A forwarded host
with a space doesn't break it, and the scheme is only resolved when HSTS is
enabled. `ForwardedHeaderFilter` and `RemoteIpValve` run earlier, so they
compose with it.
- The new Spring Security ordering docs are right about `X-Frame-Options`:
in spring-security-web 7.1.1, `XFrameOptionsHeaderWriter` in
`DENY`/`SAMEORIGIN` mode calls `setHeader` unconditionally, while the
`Referrer-Policy`, HSTS and static writers (including `X-Content-Type-Options`)
skip a header that is already present.
- `reset()` re-arms the callback and `resetBuffer()` restarts the count, and
the Tomcat `/reset` row shows the replacement response gets the headers.
`super.reset()` throws on a committed response before any state is cleared.
- The custom `OnMissingSecurityHeadersFilterRegistration` condition matches
only on the declared generic type without eager init, and the specs cover
another name, a registration that builds the filter itself, and registrations
for other filters.
- `defaults: always` is the default, and `auto`/`never` behave as
documented, including the config-level signals
(`server.forward-headers-strategy`, active `CloudPlatform`) and the log line
emitted once.
- `explicit` tracking works through Boot binding: nested `Header` instances
are bound in place through the getters, so only keys that are actually set flip
the flag.
- `ERROR` dispatches are filtered (`shouldNotFilterErrorDispatch() ==
false`). Spring Boot always registers an error page, so an exception that
resets the response still gets headers on the error dispatch.
- The `whatsNew.adoc` entry and `@since 8.0` on the three public classes
that @codeconsole asked for are in. There are no `@author` tags, Apache headers
are present, and the Spring Security and Tomcat test dependencies are test-only
and BOM-managed.
- CI was still running at the time of review (24 passed, 50 pending). The
one failure, "Update Release Draft", is the release-drafter job and doesn't run
the PR's code.
--
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]
