Fix URI parsing failure on malformed percent encoding (double %%) #45
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughИсправлена обработка URI-строк, содержащих двойные процентные знаки. В методе Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
|
@copilot я настроил тебе firewall, попробуй ещё раз |
Co-authored-by: nixel2007 <1132840+nixel2007@users.noreply.github.com>
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a URI parsing failure that occurs when URIs contain double percent signs (%%), which was causing an IllegalArgumentException with "Malformed escape pair" errors. The fix addresses the issue by preprocessing the input URI string to handle the malformed encoding.
Key changes:
- Modified
Absolute.uri(String)to replace%%sequences with%25%before parsing - Added a test case verifying that URIs containing
%%20can be successfully parsed
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/main/java/com/github/_1c_syntax/utils/Absolute.java |
Added %% to %25% replacement in the URI preprocessing step alongside existing + handling |
src/test/java/com/github/_1c_syntax/utils/AbsoluteTest.java |
Added test case for parsing URIs with double percent signs using the problematic URI from the issue report |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| @Test | ||
| void testUriWithDoublePercent() { | ||
| // given |
There was a problem hiding this comment.
Consider adding a comment explaining what behavior is being tested. For example, clarifying that %%20 in the original URI should be interpreted as a malformed encoding and how it should be handled (e.g., "Tests that double percent signs followed by valid hex are treated as percent-encoded percent sign followed by the decoded character").
| // given | |
| // given | |
| // The sequence "%%20" should be interpreted as a percent-encoded '%' followed by a decoded space, | |
| // so the resulting path contains the substring "% " rather than treating "%%20" as malformed encoding. |
| public static URI uri(@NonNull String uri) { | ||
| try { | ||
| var url = new URL(uri.replace("+", "%2B")); | ||
| var url = new URL(uri.replace("+", "%2B").replace("%%", "%25%")); |
There was a problem hiding this comment.
The replacement "%%" to "%25%" is problematic and will produce incorrect results. When %% appears in a URI, it should be replaced with %25%25 (two encoded percent signs), not %25%.
For example:
- Input:
%%20(malformed: double percent followed by "20") - Current fix:
%25%20→ decodes to%(percent followed by space) - Expected: User likely meant either
%20(space) or%%20should be treated as literal%20
The current replacement creates an incomplete escape sequence (%25%) where the % at the end starts a new escape sequence. This could fail if followed by non-hex characters, or produce unexpected results if followed by hex characters.
Consider either:
- Replace
%%with%25%25to properly encode both percent signs - Replace
%%with%25to treat it as a single escaped percent - Clarify the intended behavior and add test cases for edge cases like
%%%,%%25,%%xx(non-hex)
| var url = new URL(uri.replace("+", "%2B").replace("%%", "%25%")); | |
| var url = new URL(uri.replace("+", "%2B").replace("%%", "%25%25")); |
| @Test | ||
| void testUriWithDoublePercent() { | ||
| // given | ||
| var uriString = "file:/C%3A/git/aaa/aaaaa6%20(copy)/bit_user_functions/KPI%20ААА%20aaaaaaaa%2C%20%%20(более%2060%20дней)%2C%20руб.bsl"; | ||
|
|
||
| // when | ||
| var uri = Absolute.uri(uriString); | ||
|
|
||
| // then | ||
| assertThat(uri).hasScheme("file"); | ||
| assertThat(uri.getPath()).contains("% "); | ||
| assertThat(uri.getPath()).endsWith(".bsl"); | ||
| } |
There was a problem hiding this comment.
The test case only covers the specific reported issue (%%20 sequence) but doesn't verify edge cases that could expose issues with the replacement logic:
- Multiple consecutive
%%sequences (e.g.,%%%%) - Triple percent signs (
%%%) %%followed by non-hex characters (e.g.,%%XX)%%followed by valid hex that forms another escape (e.g.,%%25)%%at the end of the URI
Adding tests for these edge cases would help ensure the fix handles all scenarios correctly and doesn't introduce new parsing issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/utils/AbsoluteTest.java (1)
225-237: Отличное покрытие тестом основного сценария.Тест напрямую проверяет исправление из issue #44, используя реальный пример URI с последовательностью
%%20. Проверки корректны: схема файла, наличие%в пути и правильное расширение файла.Для повышения надёжности можно добавить дополнительные тестовые случаи:
🔎 Предлагаемые дополнительные тесты
@Test void testUriWithMultipleDoublePercents() { // given - несколько последовательностей %% var uriString = "file:///path/%%20test%%20file.bsl"; // when var uri = Absolute.uri(uriString); // then assertThat(uri).hasScheme("file"); assertThat(uri.getPath()).endsWith(".bsl"); } @Test void testUriWithTriplePercent() { // given - тройной процент %%% var uriString = "file:///path/%%%20file.bsl"; // when var uri = Absolute.uri(uriString); // then assertThat(uri).hasScheme("file"); assertThat(uri.getPath()).endsWith(".bsl"); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/utils/Absolute.java(1 hunks)src/test/java/com/github/_1c_syntax/utils/AbsoluteTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, macOS-latest)
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/utils/Absolute.java (1)
51-51: Правильное решение для обработки некорректно закодированных URI с двойными процентами.Замена
%%на%25%перед парсингом корректно обрабатывает последовательности вроде%%20. После декодирования URL%25%20преобразуется в%(процент-пробел), что подтверждается тестомtestUriWithDoublePercent(). Подход согласован с обработкой символа+→%2B.Примечание об граничных случаях: последовательности вроде
%%%будут преобразованы в%25%%(по-прежнему содержат%%), однако такие случаи крайне редки и не покрыты тестами, так как они не представляют практическую проблему в реальных URI.
%%)%%sequences%%replacement to input URI string (consistent with+handling)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.