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 `[email protected]` 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
