fix: decode only percent-encoded escapes in decodeURIComponent - #25672
Conversation
UrlUtil.decodeURIComponent reassembled UTF-8 bytes from every character with a code point above 127, including characters that were never percent-encoded. A literal non-ASCII character was thus reinterpreted as a UTF-8 byte and replaced with U+FFFD. A servlet container decodes the path info, so the initial server side navigation sees literal characters. Static route segments containing a non-ASCII character therefore never matched, and @WildcardParameter values were corrupted. Percent-encoded escapes are now collected into a byte sequence that is decoded as UTF-8, while the surrounding characters are kept as they are. Fixes #25671
|
Isn't what truly is wrong here that a decoded string is passed to decodeURIComponent? |
A jar URL does not have to be percent-encoded, so ResourceFolderUtil reads folder entry names that have the characters they have in the jar. The test fails without the decodeURIComponent fix, where the resources of such a folder are silently not found.
|
@Artur- Yes — and that is a better framing than the one in the issue. The double decode is the cause of the routing symptom; this PR only stops the second decode from destroying data. Worth splitting into two questions. Is Is it the fix for the reported issue? No, it is a narrowing of it. Route resolution has three input sources with two conventions:
Fixing that properly means picking one canonical form for the path that reaches the router, and both choices have consequences:
Either one changes |
|
Please create an issue about the cases where it is decoded even though it should not be, so that can be fixed separately. |
|
@claude review and check that decodeURIComponent now works as in the browser |
|
Claude finished @Artur-'s task in 3m 12s —— View job Review: does
|
| Input | Browser | This PR | Match |
|---|---|---|---|
test%20file.txt |
test file.txt |
test file.txt |
✅ |
test+file.txt |
test+file.txt |
test+file.txt |
✅ (+ not treated as space, like the browser) |
%C3%A5%C3%A4%C3%B6.txt |
åäö.txt |
åäö.txt |
✅ |
grüße |
grüße |
grüße |
✅ (the bug being fixed) |
日本 |
日本 |
日本 |
✅ (code point > U+00FF, was truncated before) |
emoji 😀 |
emoji 😀 |
emoji 😀 |
✅ |
gr%C3%BC%C3%9Fe-ü-%C3%A4x%C3%B6 |
grüße-ü-äxö |
grüße-ü-äxö |
✅ (literal + encoded mixed) |
%F0%9F%98%80 |
😀 |
😀 |
✅ (4‑byte char across 4 escapes) |
100%25 |
100% |
100% |
✅ |
%00 |
`` | `` | ✅ |
% |
throws URIError |
% |
|
%2 |
throws URIError |
%2 |
|
%zz |
throws URIError |
%zz |
|
%C3 |
throws URIError |
� |
|
%C3%28 |
throws URIError |
�( |
|
a%b%C3%BC |
throws URIError |
a%bü |
|
%ED%A0%80 (surrogate) |
throws URIError |
��� |
What's correct
- The core fix is right and well implemented. Collecting consecutive
%XXescapes into one byte buffer and flushing it whenever a literal run interrupts (UrlUtil.java:178-191) is the correct way to keep multi‑byte UTF‑8 whole while never re‑interpreting literal characters as bytes. This is precisely what the old(byte) ccast got wrong for code points above U+00FF (日本→�). - The early
return encodedwhen there is no escape (UrlUtil.java:167-170) matches the browser's pass‑through of already‑decoded strings. +is preserved rather than turned into a space, matchingdecodeURIComponent(and unlikeURLDecoder) — confirmed above.
The one divergence — malformed input
The browser's decodeURIComponent throws URIError: URI malformed on every malformed escape; this method instead degrades gracefully (keeps the literal %xx text when the two hex digits are absent, or emits U+FFFD � when the bytes aren't valid UTF‑8, because ByteArrayOutputStream.toString(UTF_8) substitutes rather than throws).
For the internal routing / resource‑lookup use case this is arguably the better behaviour — you don't want a stray % in a path to blow up navigation with an exception — so I would not change the behaviour. But two small things are worth doing:
- Javadoc accuracy. The doc says it "Corresponds to decodeURIComponent in JavaScript" (
UrlUtil.java:144-145). That's now true for valid input but not for malformed input, where the browser throws and this doesn't. A one‑line note ("malformed or incomplete escapes are left as‑is / replaced rather than throwing, unlike the browser") would make the contract honest. Fix this → - Test the gap. Add a test pinning the malformed cases above (
%,%2,%zz,%C3,%C3%28) so the graceful‑degradation contract is intentional and can't silently regress into throwing. Fix this →
Note on scope
This confirms what you and the author already discussed: the change is a sound, self‑contained data‑corruption fix and correctly aligns decodeURIComponent with the browser for valid input. The remaining double‑decode contract issue (e.g. a literal 50%20off still decoding to 50 off) is separate and, per your last comment, belongs in its own issue — it is not something this method can fix in isolation, since the caller passes an already‑decoded string.
· branch fix/decode-uri-component-keeps-literal-non-ascii
Records where the assumption is made, as the callers that pass a path that is already decoded are fixed separately in #25690.
|
… (CP: 25.3) (#25703) This PR cherry-picks changes from the original PR #25672 to branch 25.3. --- #### Original PR description > ## Summary > `UrlUtil.decodeURIComponent` treated every non-ASCII character as a raw UTF-8 byte, even when it was never percent-encoded. Because of that, paths that already contain literal characters like `ü` or `日` were corrupted into `�`, so routes with non-ASCII segments did not match and wildcard parameters lost their text. Now only real `%XX` escapes are decoded and all other characters are left untouched. > > ## What changed > **Behavior change:** `UrlUtil.decodeURIComponent` no longer rewrites non-ASCII characters that are not percent-encoded. This affects anyone who passes an already decoded (or partly decoded) string: before it came back mangled, now it comes back unchanged. Strings that only contain `%XX` escapes decode exactly as before, so normal encoded input is unaffected. > > - `decodeURIComponent` now collects consecutive `%XX` escapes into a byte sequence and decodes that sequence as UTF-8. Text between escapes is copied as-is, so a multi-byte character split over several escapes still decodes to one character. > - Input without any escape is returned directly. > - Javadoc now states that unescaped characters are kept as they are. > > Why this matters in practice: a servlet container decodes the path info, so the first server-side navigation sees literal characters. Static route segments with a non-ASCII character never matched, and `@WildcardParameter` values were corrupted. Jar URLs are also not required to be percent-encoded, so `ResourceFolderUtil` silently found no resources in a folder whose entry name contains a non-ASCII character. > > No public or protected API was added, removed, or changed. > > Fixes #25671 > > ## Test summary > > | # | Status | What the test verifies | Why it matters | > |---|--------|------------------------|----------------| > | 1 | ✅ | A string with literal non-ASCII characters (`grüße`, `日本`, an emoji) is returned unchanged | This is the bug: such input used to become `�` | > | 2 | ✅ | A string mixing `%XX` escapes and literal characters decodes to `grüße-ü-äxö` | Both forms must work in the same string; also pins that multi-byte escapes still decode | > | 3 | ✅ | A static route `grüße` matches both the literal and the percent-encoded location | Server-side navigation sees literal text, client-side sees encoded text | > | 4 | ✅ | A `@WildcardParameter` value keeps `grüße` for literal input and decodes it for encoded input | Corrupted parameter values were the visible symptom for apps | > | 5 | ✅ | `PathUtil.getSegmentsListWithDecoding` keeps literal UTF-8 segments and splits them correctly | Route resolution is built on this splitting step | > | 6 | ✅ | `ResourceFolderUtil.visitFiles` finds files in a jar folder named `thèmes/` | Resources in such folders were silently skipped | > | 7 | ❗ **gap** | Behaviour for a malformed or truncated escape (for example a lone `%C3`) | Invalid input should degrade predictably, not throw | > > - `UrlUtilTest.decodeURIComponent_literalNonAsciiCharacters_returnedUnchanged` → 1 > - `UrlUtilTest.decodeURIComponent_literalAndEncodedNonAsciiCharacters_bothDecoded` → 2 > - `RouterTest.static_route_with_non_ascii_character` → 3 > - `RouterTest.wildcard_parameter_with_non_ascii_characters` → 4 > - `PathUtilTest.getSegmentsListWithDecoding_handlesUtf8Characters` (extended) → 5 > - `ResourceFolderUtilTest.folderPathContainsLiteralNonAsciiCharacter_filesInTheJarAreVisited` → 6 > > Deliberately not tested: the new private `appendDecoded` helper, which is covered through the public method, and plain ASCII or `%2F` decoding, which existing tests in `UrlUtilTest` and `PathUtilTest` already pin. Co-authored-by: totally-not-ai[bot] <290682512+totally-not-ai[bot]@users.noreply.github.com> Co-authored-by: Artur Signell <artur@vaadin.com>
|
@Artur- are there any plans to backport this one to 25.2 or even 24.10 ? |
|
We can do that |
|
Hi @totally-not-ai[bot] and @Artur-, when i performed cherry-pick to this commit to 25.2, i have encountered the following issue. Can you take a look and pick it manually? |
|
Hi @totally-not-ai[bot] and @Artur-, when i performed cherry-pick to this commit to 24.10, i have encountered the following issue. Can you take a look and pick it manually? |
|
@claude backport this to 25.2 |
|
Claude finished @Artur-'s task in 8m 25s —— View job • Backport PR #25672 to 25.2 — done ✅
Why the automatic cherry-pick failed: the original PR also added a test to What was backported (5 of the 6 files):
All three affected test classes pass:
|
… (CP: 25.2) (#25735) UrlUtil.decodeURIComponent treated every non-ASCII character as a raw UTF-8 byte, even when it was never percent-encoded, so paths that already contain literal characters like ü or 日 were corrupted into the replacement character. Now only real %XX escapes are decoded and all other characters are left untouched. Backport of #25672 to 25.2. The ResourceFolderUtilTest case from the original PR is omitted because ResourceFolderUtil does not exist on 25.2. Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Artur Signell <260340+Artur-@users.noreply.github.com>
|
This ticket/PR has been released with Vaadin 25.4.0-alpha1. |
|
Picked into 25.2 by #25735 |
totally-not-ai
Bot
added
cherry-picked-24.10
and removed
need to pick manually 24.10
labels
|
This PR has been cherry-picked to 24.10: #26102 |
… (CP: 24.10) (#26102) This PR cherry-picks changes from the original PR #25672 to branch 24.10. --- #### Original PR description > ## Summary > `UrlUtil.decodeURIComponent` treated every non-ASCII character as a raw UTF-8 byte, even when it was never percent-encoded. Because of that, paths that already contain literal characters like `ü` or `日` were corrupted into `�`, so routes with non-ASCII segments did not match and wildcard parameters lost their text. Now only real `%XX` escapes are decoded and all other characters are left untouched. > > ## What changed > **Behavior change:** `UrlUtil.decodeURIComponent` no longer rewrites non-ASCII characters that are not percent-encoded. This affects anyone who passes an already decoded (or partly decoded) string: before it came back mangled, now it comes back unchanged. Strings that only contain `%XX` escapes decode exactly as before, so normal encoded input is unaffected. > > - `decodeURIComponent` now collects consecutive `%XX` escapes into a byte sequence and decodes that sequence as UTF-8. Text between escapes is copied as-is, so a multi-byte character split over several escapes still decodes to one character. > - Input without any escape is returned directly. > - Javadoc now states that unescaped characters are kept as they are. > > Why this matters in practice: a servlet container decodes the path info, so the first server-side navigation sees literal characters. Static route segments with a non-ASCII character never matched, and `@WildcardParameter` values were corrupted. Jar URLs are also not required to be percent-encoded, so `ResourceFolderUtil` silently found no resources in a folder whose entry name contains a non-ASCII character. > > No public or protected API was added, removed, or changed. > > Fixes #25671 > > ## Test summary > > | # | Status | What the test verifies | Why it matters | > |---|--------|------------------------|----------------| > | 1 | ✅ | A string with literal non-ASCII characters (`grüße`, `日本`, an emoji) is returned unchanged | This is the bug: such input used to become `�` | > | 2 | ✅ | A string mixing `%XX` escapes and literal characters decodes to `grüße-ü-äxö` | Both forms must work in the same string; also pins that multi-byte escapes still decode | > | 3 | ✅ | A static route `grüße` matches both the literal and the percent-encoded location | Server-side navigation sees literal text, client-side sees encoded text | > | 4 | ✅ | A `@WildcardParameter` value keeps `grüße` for literal input and decodes it for encoded input | Corrupted parameter values were the visible symptom for apps | > | 5 | ✅ | `PathUtil.getSegmentsListWithDecoding` keeps literal UTF-8 segments and splits them correctly | Route resolution is built on this splitting step | > | 6 | ✅ | `ResourceFolderUtil.visitFiles` finds files in a jar folder named `thèmes/` | Resources in such folders were silently skipped | > | 7 | ❗ **gap** | Behaviour for a malformed or truncated escape (for example a lone `%C3`) | Invalid input should degrade predictably, not throw | > > - `UrlUtilTest.decodeURIComponent_literalNonAsciiCharacters_returnedUnchanged` → 1 > - `UrlUtilTest.decodeURIComponent_literalAndEncodedNonAsciiCharacters_bothDecoded` → 2 > - `RouterTest.static_route_with_non_ascii_character` → 3 > - `RouterTest.wildcard_parameter_with_non_ascii_characters` → 4 > - `PathUtilTest.getSegmentsListWithDecoding_handlesUtf8Characters` (extended) → 5 > - `ResourceFolderUtilTest.folderPathContainsLiteralNonAsciiCharacter_filesInTheJarAreVisited` → 6 > > Deliberately not tested: the new private `appendDecoded` helper, which is covered through the public method, and plain ASCII or `%2F` decoding, which existing tests in `UrlUtilTest` and `PathUtilTest` already pin. Co-authored-by: totally-not-ai[bot] <290682512+totally-not-ai[bot]@users.noreply.github.com> Co-authored-by: Artur Signell <artur@vaadin.com>
4 participants
Summary
UrlUtil.decodeURIComponenttreated every non-ASCII character as a raw UTF-8 byte, even when it was never percent-encoded. Because of that, paths that already contain literal characters likeüor日were corrupted into�, so routes with non-ASCII segments did not match and wildcard parameters lost their text. Now only real%XXescapes are decoded and all other characters are left untouched.What changed
Behavior change:
UrlUtil.decodeURIComponentno longer rewrites non-ASCII characters that are not percent-encoded. This affects anyone who passes an already decoded (or partly decoded) string: before it came back mangled, now it comes back unchanged. Strings that only contain%XXescapes decode exactly as before, so normal encoded input is unaffected.decodeURIComponentnow collects consecutive%XXescapes into a byte sequence and decodes that sequence as UTF-8. Text between escapes is copied as-is, so a multi-byte character split over several escapes still decodes to one character.Why this matters in practice: a servlet container decodes the path info, so the first server-side navigation sees literal characters. Static route segments with a non-ASCII character never matched, and
@WildcardParametervalues were corrupted. Jar URLs are also not required to be percent-encoded, soResourceFolderUtilsilently found no resources in a folder whose entry name contains a non-ASCII character.No public or protected API was added, removed, or changed.
Fixes #25671
Test summary
grüße,日本, an emoji) is returned unchanged�%XXescapes and literal characters decodes togrüße-ü-äxögrüßematches both the literal and the percent-encoded location@WildcardParametervalue keepsgrüßefor literal input and decodes it for encoded inputPathUtil.getSegmentsListWithDecodingkeeps literal UTF-8 segments and splits them correctlyResourceFolderUtil.visitFilesfinds files in a jar folder namedthèmes/%C3)UrlUtilTest.decodeURIComponent_literalNonAsciiCharacters_returnedUnchanged→ 1UrlUtilTest.decodeURIComponent_literalAndEncodedNonAsciiCharacters_bothDecoded→ 2RouterTest.static_route_with_non_ascii_character→ 3RouterTest.wildcard_parameter_with_non_ascii_characters→ 4PathUtilTest.getSegmentsListWithDecoding_handlesUtf8Characters(extended) → 5ResourceFolderUtilTest.folderPathContainsLiteralNonAsciiCharacter_filesInTheJarAreVisited→ 6Deliberately not tested: the new private
appendDecodedhelper, which is covered through the public method, and plain ASCII or%2Fdecoding, which existing tests inUrlUtilTestandPathUtilTestalready pin.