-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Standardize url bar icon state to match other platforms #7436
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
base: develop
Are you sure you want to change the base?
Conversation
common/common-utils/src/main/java/com/duckduckgo/common/utils/UriExtension.kt
Outdated
Show resolved
Hide resolved
common/common-utils/src/main/java/com/duckduckgo/common/utils/UriExtension.kt
Outdated
Show resolved
Hide resolved
| val host = this.host?.lowercase() ?: return false | ||
| if (host == "localhost") return true | ||
|
|
||
| val ipv4Parts = host.split(".") | ||
| if (ipv4Parts.size != 4) return false | ||
|
|
||
| val octets = ipv4Parts.mapNotNull { it.toIntOrNull() } | ||
| if (octets.size != 4 || octets.any { it !in 0..255 }) return false | ||
|
|
||
| val (first, second) = octets | ||
| if (first == 127) return true | ||
| if (first == 10) return true | ||
| if (first == 172 && second in 16..31) return true | ||
| if (first == 192 && second == 168) return true |
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.
Probably simpler to rely on the libraries themselves (will support ipv6 too). Assuming we don't already have a method like this?
val Uri.isLocalUrl: Boolean
get() {
if (scheme == "file") return true
val h = host?.lowercase() ?: return false
if (h == "localhost") return true
val addr = runCatching { InetAddresses.parseNumericAddress(h) }.getOrNull() ?: return false
return addr.isLoopbackAddress || addr.isSiteLocalAddress
}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.
(there's a bunch of others here folk use like blah.local and localhost.localhost)
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.
@aitorvs do you think we could add this as a shared method to https://github.com/duckduckgo/url_predictor as a is_local?
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.
@laghee The current solution you have only considers ipv4. Seem like what @jonathanKingston is suggesting in https://github.com/duckduckgo/Android/pull/7436/files#r2662628187 is a more complete solution so I think we should update to that instead.
Task/Issue URL: https://app.asana.com/1/137249556945/project/1201870266890790/task/1210936487262413?focus=true
Description
Our other browsers handle these states similarly for the most part, so we should adapt Android to match other platforms. As it stands, we get breakage reports from Android users whenever the privacy shield is showing the red warning dot, which means we get a lot of non-actionable feedback for people's router IPs and for sites we're trying to speculatively mitigate breakage on (turning off blocking to see if it reduces reports).
Steps to test this PR
noaprints.comandmarvel.com(both have protection disabled in the config)UI changes
Note
Standardizes omnibar icon and privacy shield behavior to align with other platforms.
Globeicon for localhost/private network URLs by checkingUri.isLocalUrl; falls back toPrivacyShieldonly for non-local URLsSiteMonitor.privacyProtectionnow returnsUNPROTECTEDonly for user-allowlisted domains or non-HTTPS; remote-config exceptions no longer show as unprotectedUri.isLocalUrl(localhost, 127.0.0.0/8, 10/8, 172.16/12, 192.168/16) with comprehensive unit testsWritten by Cursor Bugbot for commit 2bd886f. This will update automatically on new commits. Configure here.