You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@struts.apache.org by "metsw24-max (via GitHub)" <gi...@apache.org> on 2026/05/27 19:18:47 UTC
[PR] Fix CSP style-src nonce policy and escape unsafe HTML output paths [struts]
metsw24-max opened a new pull request, #1720:
URL: https://github.com/apache/struts/pull/1720
This PR fixes three framework-side output safety issues related to CSP policy generation and HTML escaping.
All existing tests pass after the changes.
## Fix 1: radiomap.ftl attribute escaping
### Problem
`radiomap.ftl` used:
```ftl
${attributes.name?no_esc}
```
without pre-sanitizing `"` characters.
Unlike other form templates, this bypassed FreeMarker auto-escaping entirely and allowed a double quote to break out of the HTML attribute context.
### Fix
Escape only double quotes before `?no_esc`:
```ftl
${attributes.name?replace('"', '"')?no_esc}
```
Single quotes are intentionally preserved because Struts OGNL map syntax may legitimately contain them:
```text
myMap['key']
```
Files changed:
* `template/simple/radiomap.ftl`
* `template/html5/radiomap.ftl`
---
## Fix 2: CSP policy missing `style-src`
### Problem
The framework propagates CSP nonces to generated `<link>` and `<script>` tags, but the default CSP policy only defined:
```text
script-src 'nonce-...'
```
No `style-src` directive existed, meaning style nonces were not enforced by browsers.
### Fix
Added:
* `STYLE_SRC` constant to `CspSettings`
* `style-src 'nonce-...' ...` directive generation in `DefaultCspSettings`
Also updated CSP interceptor tests to validate the new policy format.
Files changed:
* `CspSettings.java`
* `DefaultCspSettings.java`
* `CspInterceptorTest.java`
---
## Fix 3: unescaped redirect body output
### Problem
`ServletRedirectResult` wrote the raw redirect URL directly into the HTML response body when using non-302 status codes:
```java
response.getWriter().write(finalLocation);
```
Since `finalLocation` may contain OGNL-evaluated values, framework-controlled HTML output should always be escaped before rendering.
### Fix
Escape the response body output using Apache Commons Text:
```java
StringEscapeUtils.escapeHtml4(finalLocation)
```
The `Location` response header itself remains unchanged.
Files changed:
* `ServletRedirectResult.java`
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSP style-src nonce policy and escape unsafe HTML output paths [struts]
Posted by "metsw24-max (via GitHub)" <gi...@apache.org>.
metsw24-max commented on PR #1720:
URL: https://github.com/apache/struts/pull/1720#issuecomment-4602842073
@lukaszlenart i have requested for jira account ....after approval i will raise a jira ticket
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSP style-src nonce policy and escape unsafe HTML output paths [struts]
Posted by "metsw24-max (via GitHub)" <gi...@apache.org>.
metsw24-max commented on PR #1720:
URL: https://github.com/apache/struts/pull/1720#issuecomment-4956914207
NO
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSP style-src nonce policy and escape unsafe HTML output paths [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #1720:
URL: https://github.com/apache/struts/pull/1720#issuecomment-4950984105
Were you able to request the JIRA account?
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix CSP style-src nonce policy and escape unsafe HTML output paths [struts]
Posted by "lukaszlenart (via GitHub)" <gi...@apache.org>.
lukaszlenart commented on PR #1720:
URL: https://github.com/apache/struts/pull/1720#issuecomment-4573704807
Thanks for the contribution and for clearly separating the three changes — that made review much easier.
I went through each item against current `main` (7.2.0-SNAPSHOT). Summary up front: **none of the three is an exploitable framework vulnerability**, so this is fine to discuss/handle in the open as hardening/quality work — but it needs a Jira ticket and the radiomap change needs to be reworked. Details below.
A general process note: when something *is* believed to be an exploitable framework vulnerability, please don't open a public PR for it — send it to `security@struts.apache.org` first (see [`SECURITY.md`](https://github.com/apache/struts/blob/main/SECURITY.md)). In this case the items are hardening rather than vulns, so a public PR is acceptable.
---
### Fix 1 — `radiomap.ftl` `name` escaping
You're correct that `radiomap.ftl` is the **only** template that renders `name` with `?no_esc` (`template/simple/radiomap.ftl:66`, `template/html5/radiomap.ftl:67`), bypassing the auto-escaping that every sibling applies (`checkbox.ftl:21`, `file.ftl:22`, `hidden.ftl:22`, `form-common.ftl:33`). Auto-escaping is on globally (`FreemarkerManager` sets `HTMLOutputFormat.INSTANCE` + `ENABLE_IF_DEFAULT_AUTO_ESCAPING_POLICY`). The `?no_esc` is a leftover from the WW-4972 auto-escaping migration (commit `3d20e8095`) and looks like an oversight rather than a deliberate exception.
Two caveats:
1. **Scope** — `attributes.name` is the developer-supplied field name from the tag (`<s:radio name="…"/>`), not a request value. So this is a consistency/hardening fix, not a framework vulnerability; it only matters if an application injects user input into a field name.
2. **The proposed fix is incomplete.** Escaping only `"` → `"` still leaves `<`, `>`, `&` un-escaped. The single-quote rationale ("OGNL map syntax `myMap['key']`") isn't needed: browsers decode HTML entities in attribute values before submission, so `name="foo['bar']"` is still submitted as `foo['bar']` — full escaping does not break OGNL map names (`checkbox.ftl` fully auto-escapes `name` and supports them today).
**Recommendation:** just remove `?no_esc` so radiomap matches every other template (plain `${attributes.name}`), rather than adding a partial hand-rolled escape.
### Fix 2 — CSP missing `style-src`
Accurate observation: `DefaultCspSettings.createPolicyFormat()` emits `object-src` / `script-src` (with nonce) / `base-uri` but no `style-src`, while `link.ftl` already stamps a nonce on stylesheet `<link>` tags (`link.ftl` includes `nonce.ftl`, default `rel="stylesheet"`). So that style nonce is currently inert.
This is a defense-in-depth gap, not an injection in framework code — so it's hardening, not a vulnerability. More importantly, adding `style-src 'nonce-…' …` **starts enforcing** style-src and will break any app using inline `style=`/`<style>` without a nonce. That's a behavioral change that needs its own discussion + changelog note and probably shouldn't ride along silently with the other two. Please split it into a dedicated ticket so we can weigh the compatibility impact.
### Fix 3 — `ServletRedirectResult` body escaping
`ServletRedirectResult.java:251` does write `finalLocation` raw into the body, but only on the non-302 branch, which always also sets a `3xx` status + `Location` header. Browsers follow the `Location` header on every redirect status (301/303/307/308) and don't render the body, and `finalLocation` is developer-configured and passed through `encodeRedirectURL`. So escaping here is harmless but effectively cosmetic — not a reachable XSS. No objection to the change itself; it's low-value.
---
### To move this forward
- Please file a Jira ticket and retitle the PR `WW-XXXX <description>` (every PR needs a ticket — https://issues.apache.org/jira/projects/WW), and drop the "security fix" framing since these are hardening/quality changes.
- Fix 1: remove `?no_esc` instead of the partial escape.
- Fix 2: split into its own ticket; it's a compatibility-affecting behavior change.
- Fix 3: fine as-is, low priority.
Happy to help shepherd these once the ticket(s) are in place.
--
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: notifications-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org