-
Notifications
You must be signed in to change notification settings - Fork 864
[PM-14846] Improve IP Address and Port Handling in StringExtensions #5118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-14846] Improve IP Address and Port Handling in StringExtensions #5118
Conversation
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5118 +/- ##
==========================================
- Coverage 83.83% 82.86% -0.97%
==========================================
Files 636 966 +330
Lines 49358 57256 +7898
Branches 6702 7160 +458
==========================================
+ Hits 41377 47444 +6067
- Misses 5745 7353 +1608
- Partials 2236 2459 +223 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
URI("https://$this") | ||
} else { | ||
URI(this) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered replacing URI
with android.net.Uri
, but that introduces different problems and changes the behavior of this function.
Uri.parse()
is too lenient. It accepts URI's we consider invalid; e.g.,not a uri
.Uri
does not return the correct port when the URI consists of{IP}:{PORT}
, without the scheme.Uri
is an Android class, so we cannot use it in unit tests. We would rely on theRoboelectric
implementation ofUri
, which may or may not accurately reflect the actual implementation from Google.
app/src/main/java/com/x8bit/bitwarden/data/platform/util/StringExtensions.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/com/x8bit/bitwarden/data/util/StringExtensionsTest.kt
Outdated
Show resolved
Hide resolved
- **New Function:** Added `isIpAddress()` to check if a string is a valid IPv4 address, optionally with a port and scheme. - **Regex Update:** Added the `IP_ADDRESS_WITH_OPTIONAL_PORT` regex. It validates IPv4 addresses with or without ports (0-65535 range), with an optional `http://` or `https://` prefix. - **URI Parsing:** Modified `toUriOrNull()` to handle IP addresses without schemes by adding a default "https://" scheme before parsing. - **Tests update**: Added new tests for `isIpAddress` function.
09ddef8
to
ffcae91
Compare
assertEquals("192.168.1.1:8080", uriString.getHostWithPortOrNull()) | ||
} | ||
|
||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's merged, though why not use @ParameterizedTest
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First time I've seen ParameterizedTest
. Looks interesting. I'll keep it in mind for future use. Thanks for pointing it out!
🎟️ Tracking
PM-14846
PM-20513
Relates to #5084
📔 Objective
Correct parsing of IP addresses with and without scheme and port.
URI
cannot parse an IP address with a port when it does not also include the scheme (http, https), so autofill matching triggered from the Quick Tile was not working properly.isIpAddress()
to check if a string is a valid IPv4 address, optionally with a port and scheme.IP_ADDRESS_WITH_OPTIONAL_PORT
regex. It validates IPv4 addresses with or without ports (0-65535 range), with an optionalhttp://
orhttps://
prefix.toUriOrNull()
to handle IP addresses without schemes by adding a default "https://" scheme before parsing.isIpAddress
function.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes