lukaszlenart commented on PR #1777:
URL: https://github.com/apache/struts/pull/1777#issuecomment-4955252139
Hi @arunmanni-ai — thanks for checking first. I'd hold off on the `%2e`
reject: it isn't needed, and `canonicalisePath()` is the wrong place for it.
I ran the two failing tests plus the loader-level one to be sure of the
behaviour:
- `resolveResourcePath("jquery/%2e%2e/%2e%2e/etc/passwd")` returns
`META-INF/resources/webjars/jquery/3.7.1/%2e%2e/%2e%2e/etc/passwd` — non-empty.
The WebJar locator resolves the **version** and passes the `%2e%2e` segments
through without checking the file exists. `canonicalisePath()` correctly leaves
them alone, because `%2e%2e` is not a literal `..`.
- End-to-end it's already a 404: `DefaultStaticContentLoaderWebJarTest`
(incl. `webJarEncodedTraversalReturns404`) passes. The bogus resolved path is
handed to `getResource`, which does **not** URL-decode, so `%2e%2e` is a
literal segment that matches nothing → 404.
So there's no traversal to defend against — the old `URLDecoder.decode` was
the only thing that turned `%2e%2e` into `..`, and that's gone. The two failing
tests assert a stricter-than-reality contract (resolver must return empty for
encoded-dot paths) for a case that can't traverse.
Why not add `%2e` to `canonicalisePath()`:
1. It's a pure path normaliser — mixing in content-filtering muddies its
contract and reintroduces the denylist we moved away from.
2. `%2e` is only dangerous if something decodes it, and nothing does
anymore, so rejecting it implies a risk that no longer exists.
3. It only patches `%2e`; the real looseness (the locator resolves *any*
non-existent filePath to a bogus path, e.g. `jquery/does-not-exist.js`) stays.
Suggested resolution, in order of preference:
1. Drop or rewrite the two failing tests to assert the real property — that
these paths aren't *servable* (404 end-to-end). That's already covered by
`webJarEncodedTraversalReturns404`, so they're largely redundant.
2. If you want resolution-layer strictness for hygiene, have
`resolveResourcePath` verify the resolved resource actually **exists** (a
positive check) rather than denylisting `%2e` — that rejects `%2e%2e` and every
other non-existent path generically. Keep `canonicalisePath()` pure.
Either is fine by me; I'd just avoid putting a denylist back into the
normaliser.
--
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]
