matrei commented on PR #15967:
URL: https://github.com/apache/grails-core/pull/15967#issuecomment-5816659834
Re-reviewed at head `8da7b05c58` (base `8.0.x`, merge-base `7ea4cb9d8e`;
merges cleanly into current `8.0.x` at `a441334507`). This round only looks at
what changed since `5c8fc21206`.
What I ran locally:
- `:grails-controllers:cleanTest :grails-controllers:test --no-build-cache`:
199 tests, 0 failures (100 in `GrailsSecurityHeadersAutoConfigurationSpec`, 10
in `GrailsSecurityHeadersFilterTomcatSpec`).
- `:grails-controllers:codeStyle`: clean.
- My throwaway Tomcat specs from the last round, re-run at the new head with
more cases added (results below).
Everything from the last round is addressed, and I have nothing blocking.
Both behaviour problems are fixed on the wire. The PR description now matches
the code. One optional nit is below.
### Re-run of the previous findings
Relaxed path chars (`relaxedPathChars='|{}[]^`'`) with `hsts.enabled: true`:
```
request status nosniff
HSTS
GET /p|x 200 sent
- (was 500)
GET /p{x} 200 sent
- (was 500)
GET /p|x X-Forwarded-Proto: https 200 sent
sent (was 500)
GET /p|x X-Forwarded-Proto: http 200 sent -
GET /plain X-Forwarded-Host: [::1, Proto: https 200 sent
- (malformed host: IAE caught, HSTS skipped)
GET /plain X-Forwarded-Port: abc, Proto: https 200 sent -
GET /plain X-Forwarded-Proto: https:x / "ht tps" 200 sent -
GET /plain Content-Type: text/plain;charset=bogus 200 sent
sent
```
Writer output with `setBufferSize(32768)`, every row showing `nosniff` /
`X-Frame-Options` on the wire (all were checked at least once; the two CJK rows
twice each, interleaved so the enlarged buffer is recycled into a
default-buffer request):
```
400 × write('日' * 50) UTF-8 60 000 B sent (was
<missing>)
400 × write('日' * 50), default buf UTF-8 60 000 B sent (was
<missing> after a 32 KB request)
400 × write('ж' * 50) UTF-8 40 000 B sent
400 × write('😀' * 25) UTF-8 40 000 B sent
(surrogate pairs)
20000 × write((int) '日') UTF-8 60 000 B sent
400 × printf('%s', '日' * 50) UTF-8 60 000 B sent
400 × write('日' * 50) Shift_JIS 40 000 B sent
400 × write('日' * 50) UTF-16 40 002 B sent
1000 × write('é' * 50) ISO-8859-1 50 000 B sent
```
### Nit (optional)
`GrailsSecurityHeadersFilter.java:196-197`: the `catch` comment says "A
forwarded port that is not a number", but it also catches
`ForwardedHeaderUtils` rejecting a malformed forwarded host ("Invalid IPv4
address", the `[::1` row above) and `URI.create` rejecting the scheme.
"Malformed forwarded headers: no trustworthy scheme to go on" would cover all
three.
### Verified as correct
- `isSecure` builds the base URI from `request.getScheme()` alone, so the
request path and query never reach `java.net.URI`. `URI.create` is inside the
`try`, and a container scheme of `https` on a connection that isn't marked
secure still counts. This is covered by the new spec.
- Writer byte counting: `utf8Length` counts 1, 2 or 3 bytes, and 2 for each
half of a surrogate pair, so 4 for the pair. Any other encoding counts
`ceil(maxBytesPerChar)` per character, which can only over-count, so the
callback fires early rather than late. The charset is read after
`super.getWriter()`, once the servlet API has locked the encoding. An unknown
or decode-only charset falls back to UTF-8, and the container fails that writer
anyway. Once the callback has fired, the counting loop is skipped.
`CharBuffer.wrap(buf)` with `off` as an absolute index is right. `print`,
`append`, `printf`/`format` and `println` all go through the counted methods,
and the `printf` and `write(int)` rows confirm it on Tomcat.
- The new Tomcat `/multi-byte-writer` row fails at `5c8fc21206` (headers
missing in every run last round) and passes now, so it pins the fix.
- The Spring Security ordering paragraph in `security.adoc` describes the
trade-off accurately. At Tomcat's default buffer, UTF-8 text from about 8 KB of
encoded bytes now gets the Grails defaults before Spring Security's
character-counting wrapper fires. With Spring Security's default headers this
changes nothing: `X-Frame-Options` is written unconditionally, the `nosniff`
and `X-XSS-Protection: 0` values are the same, and Grails HSTS is off by
default. Only a non-default Spring Security value for a skip-if-present header
(for example `Referrer-Policy`) is affected, and the paragraph says how to hand
that header over.
- The docs changes: the character-encoding claim is corrected in all three
places. The "explicitly configured" wording now matches the
`explicit`/`enabled`/value logic, including `enabled: false` and the CSP/HSTS
`enabled: true` requirement. Appending the upgrade note as section 73 instead
of renumbering is reasonable, given the numbered cross-references such as
"section 53".
- `REVERSE_PROXY_REQUEST_HEADERS` is package-private. The Tomcat spec cleans
up with `@Shared @TempDir` and now checks the exact values of all four headers.
Keeping its own client instead of `HttpClientSupport` makes sense, because the
new multi-byte row compares the raw byte length.
- CI was still running at the time of review (5 passed, 66 pending, 0
failed).
--
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]
