From 4f2430d3541ce4ae1702fd6199da7ae4df854716 Mon Sep 17 00:00:00 2001
From: Pluto <pluto@hobbs.farm>
Date: Tue, 3 Feb 2026 09:11:06 -0600
Subject: [PATCH] fix(security): Whitelist bypass vulnerability
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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>
---
 IMPROVEMENTS.md                               | 46 ++++++++++++++
 .../farm/hobbs/kioskbrowser/MainActivity.kt   |  5 +-
 .../farm/hobbs/kioskbrowser/WhitelistTest.kt  | 62 +++++++++++++++++++
 3 files changed, 111 insertions(+), 2 deletions(-)
 create mode 100644 IMPROVEMENTS.md
 create mode 100644 app/src/test/kotlin/farm/hobbs/kioskbrowser/WhitelistTest.kt

diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md
new file mode 100644
index 0000000..1fa5290
--- /dev/null
+++ b/IMPROVEMENTS.md
@@ -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 🪐*
diff --git a/app/src/main/kotlin/farm/hobbs/kioskbrowser/MainActivity.kt b/app/src/main/kotlin/farm/hobbs/kioskbrowser/MainActivity.kt
index c9ec3a8..f81210f 100644
--- a/app/src/main/kotlin/farm/hobbs/kioskbrowser/MainActivity.kt
+++ b/app/src/main/kotlin/farm/hobbs/kioskbrowser/MainActivity.kt
@@ -128,8 +128,9 @@ class MainActivity : AppCompatActivity() {
     }
 
     private fun isUrlAllowed(url: String): Boolean {
-        return whitelist.any { domain -> 
-            url.contains(domain, ignoreCase = true) 
+        val host = android.net.Uri.parse(url).host?.lowercase() ?: return false
+        return whitelist.any { domain ->
+            host == domain || host.endsWith(".$domain")
         }
     }
 
diff --git a/app/src/test/kotlin/farm/hobbs/kioskbrowser/WhitelistTest.kt b/app/src/test/kotlin/farm/hobbs/kioskbrowser/WhitelistTest.kt
new file mode 100644
index 0000000..7a2c705
--- /dev/null
+++ b/app/src/test/kotlin/farm/hobbs/kioskbrowser/WhitelistTest.kt
@@ -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"))
+    }
+}