WW-5645 Canonicalise static content paths and remove redundant URL decode

[arunmanni-ai]

The buildPath() method in DefaultStaticContentLoader applied URLDecoder.decode() to resource paths that the servlet container had already decoded. This redundant decode conflicted with the servlet spec's path-handling contract.

This patch:

• Removes the unnecessary URLDecoder.decode() call from buildPath() and drops the now-unused encoding field and its setter
• Adds a Validator.canonicalisePath() utility that normalises the path (resolves . and .., converts \ to /) and returns Optional.empty() when the resolved path escapes above root
• Wires the canonical path through both findStaticResource() and the WebJar split() so downstream lookups use the normalised form
• Documents the encoding contract on RequestUtils.getServletPath()
• Adds test coverage for literal traversal, encoded traversal, backslash paths, and the WebJar serving path

[lukaszlenart]

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.

[arunmanni-ai]

Thanks again — pushed two commits addressing points 1, 3, and the Locale import part of 4 (per-segment matching in
containsMalformedPathSegment, and removed the now-redundant dot-checkin DefaultWebJarUrlProvider.split()).

Still to follow in one more commit:

• Small indentation cleanup and removing the now-unused encoding field (rest of point 4)
• Replacing the denylist with a canonicalize approach (point 2) — taking a bit more care with this one since it also means wiring the resolved path through buildPath()/split() rather than just validating and discarding it, so it'll take a little longer to get right.

Will update this thread once that's up.

[lukaszlenart]

Thanks for the quick turnaround — both changes look good:

• Per-segment matching (cd51fc690): containsMalformedPathSegment now walks segments with equals("..")/equals(".") instead of contains(".."), so the foo..bar.js false positive is gone. 👍
• WebJar consolidation (cace037c8): dropping the duplicate segment loop in split() and relying on the shared helper keeps the behavior and gives a single source of truth. Also nice that Locale is now imported rather than inlined.

Two optional nits, non-blocking:

1. encoding (DefaultStaticContentLoader#encoding) is now dead — it's still @Inject'd but nothing reads it once the URLDecoder.decode is gone. Could be dropped along with setEncoding.
2. The } closing validateStaticContentPath in StaticContentLoader.Validator is indented at 7 spaces instead of 8 — trivial formatting.

Neither blocks; happy with the direction.

[arunmanni-ai]

Pushed the two nits — indentation on the validateStaticContentPath closing brace, and removed the unused encoding field/setter from DefaultStaticContentLoader.

Working on the canonicalize approach for point 2 now (wiring the resolved path through buildPath()/split() rather than just validating and discarding it, so it'll take a bit more care to getright). Would appreciate holding off on merging until that's up —should have it here soon.

[arunmanni-ai]

I replaced the denylist with a canonicalise-then-verify approach. containsMalformedPathSegment is now canonicalisePath, which normalises the path (resolves . and .., converts \ to /) and returns Optional.empty() if it escapes above root. Both findStaticResource and the WebJar split() now use the canonical form for downstream lookups. Dropped the %2e denylist and its test, and removed the now-dead setEncoding call from the test setUp.

All four points addressed -- ready for review when you get a chance.

[arunmanni-ai]

Fixed the build failure — removed stale setEncoding() calls from DefaultStaticContentLoaderWebJarTest and StaticContentLoaderTest that referenced the method removed in this PR. CI should be green now.

[arunmanni-ai]

Hi @lukaszlenart,

The CI failures are from two new tests I added for encoded traversal (%2e%2e) in DefaultWebJarUrlProvider. The canonicalisePath() method currently catches literal .. but not percent-encoded variants.

I'm planning to add a check at the top of canonicalisePath() to reject paths containing %2e (case-insensitive) before the segment splitting. This keeps buildPath() untouched and doesn't change its contract.

Wanted to check with you first — does that approach align with the design intent, or would you prefer handling it differently?

[lukaszlenart]

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.

[arunmanni-ai]

Makes sense, thanks for the detailed walkthrough. Went with option 1 — removed both tests since the end-to-end 404 via webJarEncodedTraversalReturns404 already covers it. CI should be clean now.