fix(security): Whitelist bypass vulnerability
- Fixed URL whitelist to use proper domain boundary matching - Added WhitelistTest.kt with 6 test cases - Added IMPROVEMENTS.md with code review notes Security fix identified by Spirit 👻 Co-authored-by: Spirit <spirit@hobbs.farm> Co-authored-by: Margaret <margaret@hobbs.farm>
This commit is contained in:
@@ -0,0 +1,46 @@
|
|||||||
|
# Improvements & Code Review Notes
|
||||||
|
|
||||||
|
## Security Fixes Applied
|
||||||
|
|
||||||
|
### P0: Whitelist Bypass Vulnerability (FIXED)
|
||||||
|
**Issue:** Original whitelist check used `url.contains(domain)` which would allow bypass:
|
||||||
|
- `https://hobbs.farm.evil.com` would match `hobbs.farm` ❌
|
||||||
|
|
||||||
|
**Fix:** Use proper domain boundary matching:
|
||||||
|
```kotlin
|
||||||
|
val host = Uri.parse(url).host?.lowercase() ?: return false
|
||||||
|
return whitelist.any { domain ->
|
||||||
|
host == domain || host.endsWith(".$domain")
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
## Recommended Future Improvements
|
||||||
|
|
||||||
|
### P1: PIN Storage Security
|
||||||
|
- Current: Plain text in DataStore
|
||||||
|
- Recommended: SHA-256 hash with salt
|
||||||
|
- Implementation: Use `MessageDigest` to hash before storing
|
||||||
|
|
||||||
|
### P2: UX Enhancements
|
||||||
|
- Add loading indicator (ProgressBar) during page loads
|
||||||
|
- Add network error screen with retry button
|
||||||
|
- Consider swipe-to-refresh in admin settings
|
||||||
|
|
||||||
|
### P2: Code Quality
|
||||||
|
- Add unit tests for whitelist logic (added: WhitelistTest.kt)
|
||||||
|
- Add instrumented tests for WebView behavior
|
||||||
|
- Consider Kotlin coroutines for async operations
|
||||||
|
|
||||||
|
## Test Coverage
|
||||||
|
|
||||||
|
| Component | Tests | Status |
|
||||||
|
|-----------|-------|--------|
|
||||||
|
| Whitelist matching | 6 | ✅ |
|
||||||
|
| Idle timer | TBD | ⏳ |
|
||||||
|
| Admin PIN | TBD | ⏳ |
|
||||||
|
| Boot receiver | TBD | ⏳ |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
*Code review by Spirit 👻*
|
||||||
|
*Coordinated by Pluto 🪐*
|
||||||
@@ -128,8 +128,9 @@ class MainActivity : AppCompatActivity() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private fun isUrlAllowed(url: String): Boolean {
|
private fun isUrlAllowed(url: String): Boolean {
|
||||||
return whitelist.any { domain ->
|
val host = android.net.Uri.parse(url).host?.lowercase() ?: return false
|
||||||
url.contains(domain, ignoreCase = true)
|
return whitelist.any { domain ->
|
||||||
|
host == domain || host.endsWith(".$domain")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,62 @@
|
|||||||
|
package farm.hobbs.kioskbrowser
|
||||||
|
|
||||||
|
import android.net.Uri
|
||||||
|
import org.junit.Assert.*
|
||||||
|
import org.junit.Test
|
||||||
|
import org.junit.runner.RunWith
|
||||||
|
import org.robolectric.RobolectricTestRunner
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whitelist URL matching tests
|
||||||
|
* Tests the domain boundary checking to prevent bypass attacks
|
||||||
|
*/
|
||||||
|
@RunWith(RobolectricTestRunner::class)
|
||||||
|
class WhitelistTest {
|
||||||
|
|
||||||
|
private val whitelist = listOf("hobbs.farm", "example.com")
|
||||||
|
|
||||||
|
private fun isUrlAllowed(url: String): Boolean {
|
||||||
|
val host = Uri.parse(url).host?.lowercase() ?: return false
|
||||||
|
return whitelist.any { domain ->
|
||||||
|
host == domain || host.endsWith(".$domain")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `exact domain match allowed`() {
|
||||||
|
assertTrue(isUrlAllowed("https://hobbs.farm"))
|
||||||
|
assertTrue(isUrlAllowed("https://example.com/page"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `subdomain allowed`() {
|
||||||
|
assertTrue(isUrlAllowed("https://www.hobbs.farm"))
|
||||||
|
assertTrue(isUrlAllowed("https://sub.example.com"))
|
||||||
|
assertTrue(isUrlAllowed("https://deep.sub.hobbs.farm/path"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `bypass attempt blocked - domain as substring`() {
|
||||||
|
// Critical security test: hobbs.farm.evil.com should NOT pass
|
||||||
|
assertFalse(isUrlAllowed("https://hobbs.farm.evil.com"))
|
||||||
|
assertFalse(isUrlAllowed("https://fakehobbs.farm"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `unrelated domain blocked`() {
|
||||||
|
assertFalse(isUrlAllowed("https://google.com"))
|
||||||
|
assertFalse(isUrlAllowed("https://malware.site"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `invalid URL returns false`() {
|
||||||
|
assertFalse(isUrlAllowed("not-a-url"))
|
||||||
|
assertFalse(isUrlAllowed(""))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `case insensitive matching`() {
|
||||||
|
assertTrue(isUrlAllowed("https://HOBBS.FARM"))
|
||||||
|
assertTrue(isUrlAllowed("https://HoBbS.FaRm/Page"))
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user