[VFS-863] Content inside brackets in directory/file names is not decoded - #773
Conversation
Only set ipv6Host to true in UriParser.decode() if it is in the authority of the URI. Otherwise, leave it false so that content inside of brackets in the path will still be decoded.
90f020f to
d22d69d
Compare
nicktarallo
changed the title
|
Hi @garydgregory, this was discussed briefly in the user mailing list, made a PR to try to address the issue. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue in Commons VFS 2.10 where percent-encoded content inside [...] in paths was not decoded because the decoder treated any bracketed section as an IPv6 host. The change narrows IPv6-host handling to bracketed content that appears within the RFC 3986 authority portion of a URI, and adds regression tests.
Changes:
- Updated
UriParser.decode(StringBuilder, ...)to only treat[...]specially when it occurs in the detected authority segment. - Added unit tests covering percent-decoding inside brackets in paths and preserving
%25inside IPv6 hosts. - Added an end-to-end local filesystem round-trip test to ensure
getURL()paths containing brackets resolve consistently.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/UriParser.java | Refines decode logic to avoid treating path [...] as IPv6 host unless within authority. |
| commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/UriParserTest.java | Adds focused regression tests for decoding behavior inside brackets in path vs IPv6 host. |
| commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/local/UrlTests.java | Adds round-trip integration test for local file URLs with bracketed directory names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ch == '/') { | ||
| consecutiveSlashes++; | ||
| if (consecutiveSlashes == 2) { | ||
| inAuthority = true; | ||
| authorityDetected = true; | ||
| } |
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
Hello @nicktarallo
Thank you for the PR.
Please review the comments from copilot. You might need to add a test in check the main suggestion. The test suggestion seems like a test bug.
…ter :// Addresses review feedback: - Authority detection previously triggered on any "//" in the input, so a "//" later in a path (e.g. "file:/a//[inside%25text]") would incorrectly treat following brackets as an IPv6 host. Detection now requires a leading "//" (network-path reference) or "://" with no earlier "/" (scheme delimiter). - Close the Files.walk stream in UrlTests with try-with-resources. - Add regression tests for "//" mid-path, "://" embedded in a path after the authority, a network-path reference IPv6 host, and a single URI mixing an IPv6 zone ID with brackets in the path.
Hi @garydgregory, thanks for the review. I addressed both comments:
Also verified all 4 new test methods fail against the pre-fix code. |
|
@nicktarallo PR merged 🚀 Thank you! |
In VFS 2.10, content inside brackets in the file URL is not decoded. This is because it is detected to be part of an IPv6 host, although this may not be desired behavior in all circumstances (local file paths that happen to have content in brackets are also affected, despite being unrelated to IPv6).
The fix here is to ensure that we only detect a host inside of the authority as defined by RFC 3986. We do this by detecting the authority as the content between the first instance of
"//"and the next"/", since the URL takes the formatscheme://<authority>/<path>. Bracketsoutsideof the authority hold no special meaning, so content inside of these brackets will now be decoded.Bug was most likely introduced by this PR
There appears to be another old, stale PR out for the same issue with a regex approach, but I could not find an associated ticket: #562
AI (Claude Code) was used to write most of the code (I reviewed all output). The fix approach and the design of the test cases were provided by me to the tool.