lukaszlenart commented on PR #1777:
URL: https://github.com/apache/struts/pull/1777#issuecomment-4951796976
Thanks for this. Dropping the second `URLDecoder.decode()` in `buildPath()`
is the right call — the container has already decoded `getServletPath()`, so
the extra decode was redundant, and removing it (plus the now-unnecessary
`throws UnsupportedEncodingException`) is a clean correctness fix. Adding
explicit segment rejection and tests is a good direction too.
A few things I'd tighten — happy for you to handle them here, or I can file
a WW ticket and pick them up in a follow-up, whichever you prefer:
1. **Prefer per-segment checks over substring matching.**
`containsMalformedPathSegment` uses `path.contains("..")`, which also rejects
legitimate names like `foo..bar.js`. `DefaultWebJarUrlProvider.split()` already
walks segments and rejects `..`/`.` precisely — cleaner to unify on one
per-segment helper than to add a parallel `contains`-based one, and it avoids
the false positives.
2. **Denylist vs. canonicalize.** Enumerating encoded forms (`%2e`) is
fragile. Normalizing the path (collapse `.`/`..`, `\`→`/`) and confirming it
stays within the intended prefix is more robust and self-documenting.
3. **Behavior change to note:** the WebJar path previously rejected `..`
per-segment (`equals("..")`); routing it through the shared helper broadens
that to `contains`. Intentional, but worth calling out.
4. **Minor/structural:** the helper needed a brace fix; indentation of the
added block and the inline `java.util.Locale.ROOT` (vs. an import) are slightly
off — worth a tidy pass. And `encoding` is now unused once the decode is gone.
Overall a good cleanup — mostly about making the guard precise and
consistent with what the WebJar resolver already does.
--
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]
