Test the three bugs that reached the living room, and script the release
The rate limit, the version compare and the download check were all wrapped in Android — a Context, a socket, a file — so none of them had a test, and all three shipped broken: the remote thrown out after five reconnects (0.11.1), a truncated APK handed to the installer (0.11.5), and a version compare that would read 0.11.10 as older than 0.11.9 the moment we get there. They are now plain Kotlin in AttemptBudget and UpdateRules, with the clock injected, and 20 tests covering the failures themselves. AttemptBudget also prunes expired entries before evicting: dropping only empty queues let the map grow without bound while every tracked address held one fresh failure. tests/smoke.sh installs the debug APK on the headless emulator, walks the first screen with the D-pad and fails on a crash or ANR — the class of bug unit tests cannot see, and the reason 0.11.0 crashed on startup. Forty seconds end to end. tools/release.sh does the bump, tests, signed build, tag, apk branch and Gitea release in one command. It refuses an unsigned build and never writes the release notes itself; commit subjects are offered as a draft. tests/helpers/jdk.sh picks a JDK Gradle can run on: Ubuntu moved default-java to 25, which fails the build with a bare "IllegalArgumentException: 25.0.4". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
78
app/src/main/java/dev/castarr/tv/server/AttemptBudget.kt
Normal file
78
app/src/main/java/dev/castarr/tv/server/AttemptBudget.kt
Normal file
@@ -0,0 +1,78 @@
|
||||
package dev.castarr.tv.server
|
||||
|
||||
/**
|
||||
* Per-address budget for failed authentication attempts.
|
||||
*
|
||||
* Kept free of Android and of the clock so the rule itself can be tested:
|
||||
* this is where the phone remote was thrown out in 0.11.0, because every
|
||||
* connection cost budget instead of only the failed ones. A remote
|
||||
* reconnects on every network hiccup, and five reconnects a minute are
|
||||
* normal traffic, not an attack.
|
||||
*/
|
||||
class AttemptBudget(
|
||||
private val windowMs: Long = WINDOW_MS,
|
||||
private val maxFailures: Int = MAX_FAILURES,
|
||||
private val maxTrackedAddresses: Int = MAX_TRACKED_ADDRESSES,
|
||||
private val now: () -> Long = System::currentTimeMillis,
|
||||
) {
|
||||
|
||||
private val failures = HashMap<String, ArrayDeque<Long>>()
|
||||
|
||||
/** True while this address may still try. Never consumes budget. */
|
||||
@Synchronized
|
||||
fun allows(address: String): Boolean {
|
||||
val queue = failures[address] ?: return true
|
||||
prune(queue, now())
|
||||
if (queue.isEmpty()) failures.remove(address)
|
||||
return queue.size < maxFailures
|
||||
}
|
||||
|
||||
/** Only a *failed* authentication costs budget. */
|
||||
@Synchronized
|
||||
fun recordFailure(address: String) {
|
||||
val timestamp = now()
|
||||
val queue = failures.getOrPut(address) { ArrayDeque() }
|
||||
queue.addLast(timestamp)
|
||||
if (failures.size > maxTrackedAddresses) evict(timestamp)
|
||||
}
|
||||
|
||||
/** A successful authentication wipes the address clean. */
|
||||
@Synchronized
|
||||
fun clear(address: String) {
|
||||
failures.remove(address)
|
||||
}
|
||||
|
||||
@Synchronized
|
||||
fun trackedAddresses(): Int = failures.size
|
||||
|
||||
private fun prune(queue: ArrayDeque<Long>, timestamp: Long) {
|
||||
while (queue.isNotEmpty() && timestamp - queue.first() > windowMs) {
|
||||
queue.removeFirst()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Expired entries first, and only then the oldest addresses. Dropping
|
||||
* just the empty queues left the map growing without bound as long as
|
||||
* every tracked address still held one fresh failure.
|
||||
*/
|
||||
private fun evict(timestamp: Long) {
|
||||
val iterator = failures.entries.iterator()
|
||||
while (iterator.hasNext()) {
|
||||
val entry = iterator.next()
|
||||
prune(entry.value, timestamp)
|
||||
if (entry.value.isEmpty()) iterator.remove()
|
||||
}
|
||||
if (failures.size <= maxTrackedAddresses) return
|
||||
failures.entries
|
||||
.sortedBy { it.value.firstOrNull() ?: 0L }
|
||||
.take(failures.size - maxTrackedAddresses)
|
||||
.forEach { failures.remove(it.key) }
|
||||
}
|
||||
|
||||
companion object {
|
||||
const val WINDOW_MS = 60_000L
|
||||
const val MAX_FAILURES = 5
|
||||
const val MAX_TRACKED_ADDRESSES = 64
|
||||
}
|
||||
}
|
||||
@@ -59,40 +59,9 @@ class ControlServer(
|
||||
var running = false
|
||||
private set
|
||||
|
||||
// Failed authentication attempts per remote address. Counting every
|
||||
// failure (not just the ones carrying a code) keeps a hostile client
|
||||
// from spending someone else's budget.
|
||||
private val attempts = HashMap<String, ArrayDeque<Long>>()
|
||||
|
||||
/** True while this address may still try; does not consume budget. */
|
||||
@Synchronized
|
||||
private fun attemptAllowed(address: String): Boolean {
|
||||
val now = System.currentTimeMillis()
|
||||
val queue = attempts[address] ?: return true
|
||||
while (queue.isNotEmpty() && now - queue.first() > ATTEMPT_WINDOW_MS) {
|
||||
queue.removeFirst()
|
||||
}
|
||||
return queue.size < ATTEMPT_MAX
|
||||
}
|
||||
|
||||
/**
|
||||
* Only *failed* authentication costs budget. Counting successes too
|
||||
* threw out honest clients: the phone remote reconnects on every
|
||||
* network hiccup, and five reconnects a minute are normal.
|
||||
*/
|
||||
@Synchronized
|
||||
private fun recordFailure(address: String) {
|
||||
val queue = attempts.getOrPut(address) { ArrayDeque() }
|
||||
queue.addLast(System.currentTimeMillis())
|
||||
if (attempts.size > MAX_TRACKED_ADDRESSES) {
|
||||
attempts.entries.removeAll { it.value.isEmpty() }
|
||||
}
|
||||
}
|
||||
|
||||
@Synchronized
|
||||
private fun clearFailures(address: String) {
|
||||
attempts.remove(address)
|
||||
}
|
||||
// Failed authentication attempts per remote address. The rule itself
|
||||
// lives in AttemptBudget, where it is unit-tested.
|
||||
private val attempts = AttemptBudget()
|
||||
|
||||
fun startServer() {
|
||||
// A busy port must not take the whole app down — the remote is
|
||||
@@ -312,7 +281,7 @@ class ControlServer(
|
||||
}
|
||||
|
||||
private fun handleHello(msg: JSONObject) {
|
||||
if (!attemptAllowed(remoteAddress)) {
|
||||
if (!attempts.allows(remoteAddress)) {
|
||||
trySend(JSONObject().put("type", "error").put("error", "rate_limited").toString())
|
||||
runCatching { close(WebSocketFrame.CloseCode.PolicyViolation, "rate limited", false) }
|
||||
return
|
||||
@@ -320,12 +289,12 @@ class ControlServer(
|
||||
val tokenOk = Pairing.isValidToken(context, msg.optString("token"))
|
||||
val codeOk = !tokenOk && Pairing.isValidCode(context, msg.optString("code"))
|
||||
if (!tokenOk && !codeOk) {
|
||||
recordFailure(remoteAddress)
|
||||
attempts.recordFailure(remoteAddress)
|
||||
trySend(JSONObject().put("type", "error").put("error", "bad_code").toString())
|
||||
runCatching { close(WebSocketFrame.CloseCode.PolicyViolation, "bad code", false) }
|
||||
return
|
||||
}
|
||||
clearFailures(remoteAddress)
|
||||
attempts.clear(remoteAddress)
|
||||
authorized = true
|
||||
deviceName = msg.optString("name").ifEmpty { "Handy" }
|
||||
// A code-authenticated client gets its own revocable token, never
|
||||
@@ -360,9 +329,6 @@ class ControlServer(
|
||||
const val TAG = "ControlServer"
|
||||
const val PING_INTERVAL_MS = 8_000L
|
||||
const val SOCKET_TIMEOUT_MS = 40_000
|
||||
const val ATTEMPT_WINDOW_MS = 60_000L
|
||||
const val ATTEMPT_MAX = 5
|
||||
const val MAX_TRACKED_ADDRESSES = 64
|
||||
const val MAX_CLIENTS = 8
|
||||
const val HANDSHAKE_TIMEOUT_MS = 10_000L
|
||||
val PING_PAYLOAD = byteArrayOf(0x6e, 0x63)
|
||||
|
||||
@@ -45,7 +45,7 @@ object UpdateChecker {
|
||||
}
|
||||
}
|
||||
if (url.isEmpty() && tag.isNotEmpty()) url = APK_FALLBACK
|
||||
if (url.isNotEmpty() && isNewer(tag, BuildConfig.VERSION_NAME)) {
|
||||
if (url.isNotEmpty() && UpdateRules.isNewer(tag, BuildConfig.VERSION_NAME)) {
|
||||
apkUrl = url
|
||||
val version = "v$tag"
|
||||
withContext(Dispatchers.Main) { state.updateAvailable = version }
|
||||
@@ -93,8 +93,7 @@ object UpdateChecker {
|
||||
} finally {
|
||||
connection.disconnect()
|
||||
}
|
||||
require(part.length() > 0) { "empty download" }
|
||||
require(expected <= 0 || part.length() == expected) {
|
||||
require(UpdateRules.isComplete(part.length(), expected)) {
|
||||
"truncated: ${part.length()} of $expected"
|
||||
}
|
||||
file.delete()
|
||||
@@ -123,18 +122,6 @@ object UpdateChecker {
|
||||
}
|
||||
}
|
||||
|
||||
private fun isNewer(remote: String, local: String): Boolean {
|
||||
fun parts(v: String) = v.split(".").mapNotNull { it.toIntOrNull() }
|
||||
val r = parts(remote)
|
||||
val l = parts(local)
|
||||
for (i in 0 until maxOf(r.size, l.size)) {
|
||||
val a = r.getOrElse(i) { 0 }
|
||||
val b = l.getOrElse(i) { 0 }
|
||||
if (a != b) return a > b
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
private fun get(url: String): String {
|
||||
val connection = URL(url).openConnection() as HttpURLConnection
|
||||
return try {
|
||||
|
||||
45
app/src/main/java/dev/castarr/tv/update/UpdateRules.kt
Normal file
45
app/src/main/java/dev/castarr/tv/update/UpdateRules.kt
Normal file
@@ -0,0 +1,45 @@
|
||||
package dev.castarr.tv.update
|
||||
|
||||
/**
|
||||
* The two decisions the updater makes, without Android or a network in the
|
||||
* way: is the offered release newer, and did the download arrive whole.
|
||||
*
|
||||
* Both shipped as bugs once — a version compare that reads "0.11.10" as
|
||||
* older than "0.11.9", and a half-written APK handed to the package
|
||||
* installer, which then sits on a spinner with nothing to report.
|
||||
*/
|
||||
object UpdateRules {
|
||||
|
||||
/**
|
||||
* Compares dotted numeric versions segment by segment, missing segments
|
||||
* counting as zero ("0.12" == "0.12.0"). A leading "v" is tolerated on
|
||||
* either side; anything non-numeric is ignored rather than throwing,
|
||||
* because a release tag is user input.
|
||||
*/
|
||||
fun isNewer(remote: String, local: String): Boolean {
|
||||
val r = segments(remote)
|
||||
val l = segments(local)
|
||||
for (i in 0 until maxOf(r.size, l.size)) {
|
||||
val a = r.getOrElse(i) { 0 }
|
||||
val b = l.getOrElse(i) { 0 }
|
||||
if (a != b) return a > b
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
/**
|
||||
* True when the bytes on disk match what the server announced.
|
||||
*
|
||||
* A server that announces nothing (chunked transfer, `announced <= 0`)
|
||||
* cannot be checked against — then any non-empty file has to pass, which
|
||||
* is the honest answer rather than a guess.
|
||||
*/
|
||||
fun isComplete(actualBytes: Long, announcedBytes: Long): Boolean {
|
||||
if (actualBytes <= 0) return false
|
||||
if (announcedBytes <= 0) return true
|
||||
return actualBytes == announcedBytes
|
||||
}
|
||||
|
||||
private fun segments(version: String) =
|
||||
version.removePrefix("v").split(".").mapNotNull { it.toIntOrNull() }
|
||||
}
|
||||
Reference in New Issue
Block a user