From 775b6e62d0ee6e40b985a789ca200e0a0f92a4dd Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:29:38 -0400 Subject: [PATCH] A test run must not publish a credential (ios#62, Android half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Kotlin counterpart of the iOS fix, and Android's version of the bug was quieter rather than absent. Android was already ahead on the part iOS got wrong: every live suite is gated behind `assumeTrue`, and `LiveTarget.refuseProduction` refuses a production host outright. What it still had was the credential itself. `LiveTwoProductsTest` and `LiveShapesTest` both carried `const val PASSPHRASE = "a-long-enough-passphrase"` and used it to create accounts on accounts.flamenet.io whenever somebody set FN_LIVE_MANAGED=1. 🔴 The gate stops it running by accident. It does not stop the password being written down. Every account those runs ever created can be signed into by anybody who can read this repository, and "they were throwaway accounts" does not change that — they are real accounts on a real service and they persist until somebody deletes them. Both now read it from the environment through `LiveTarget.passphrase()` with no fallback, and both gates assume on it being non-empty, so opting in without configuring one reports SKIPPED rather than signing up with a published value. And a guard, matching the Swift one: TestSafetyTest fails the build if a suite that reaches a live relay carries a credential literal, or cannot announce a skip. Proven by putting the literal back and watching it go red. ⚠️ The guard had to be narrowed twice before it was worth keeping, and both narrowings are recorded in it. Keying on the `Live` name prefix flagged LiveTarget.kt (the helper) and LiveTargetTest.kt (a unit test of the helper). Keying on the string "http" then flagged four suites that sign up against a MockWebServer — which is exactly the correct way to test sign-up. The discriminator is MockWebServer: a file that stands up a mock cannot reach anybody. It now matches the nine real live suites and nothing else, and a test asserts the predicate matches at least five, so the checks cannot pass by guarding an empty set. PARITY.md gains a Test safety section. Full JVM suite green. Co-Authored-By: Claude Opus 5 --- PARITY.md | 27 +++ .../io/flamenet/client/LiveShapesTest.kt | 19 +- .../kotlin/io/flamenet/client/LiveTarget.kt | 22 +++ .../io/flamenet/client/LiveTwoProductsTest.kt | 19 +- .../io/flamenet/client/TestSafetyTest.kt | 187 ++++++++++++++++++ 5 files changed, 266 insertions(+), 8 deletions(-) create mode 100644 client/src/test/kotlin/io/flamenet/client/TestSafetyTest.kt diff --git a/PARITY.md b/PARITY.md index 712d285..f101ea2 100644 --- a/PARITY.md +++ b/PARITY.md @@ -150,6 +150,33 @@ emulator. --- +## Test safety + +| Feature | iOS | Android | Note | +|---|:--:|:--:|---| +| Live suites cannot run by accident | ✅ | ✅ | Android was already ahead here — every live suite gated on `assumeTrue`, and `LiveTarget.refuseProduction` refuses a production host outright | +| No credential written into the source | ✅ | ✅ | both fixed together, ios#62 | +| A guard that fails the build if either regresses | ✅ | ✅ | `Tests/UITestSafetyTests.swift`, `client/.../TestSafetyTest.kt` | + +🔴 **iOS was creating real accounts on production on every plain test run**, with +a literal password from a public repository; Android's equivalent suites were +properly gated but still carried +`const val PASSPHRASE = "a-long-enough-passphrase"` and used it to sign up on +`accounts.flamenet.io` whenever somebody opted in. The gate stops it running by +accident. It does not stop the password being written down. + +Both now read the passphrase from the environment with **no fallback**, and an +unconfigured run reports SKIPPED rather than signing up with a value anybody can +read. `FN_LIVE_PASSPHRASE` on Android, `TEST_RUNNER_FN_DEMO_PASSPHRASE` on iOS. + +⚠️ Both guards are source scans, and both had to be narrowed before they were +worth keeping — the Swift one flagged an email default because "password" +appeared in a trailing comment, the Kotlin one flagged four suites that sign up +against a `MockWebServer`, which is the correct way to test sign-up. A guard +with standing false positives is one somebody switches off. + +--- + ## The 2 September revert Twenty-five Android merge requests landed that evening and `aebf41e` diff --git a/client/src/test/kotlin/io/flamenet/client/LiveShapesTest.kt b/client/src/test/kotlin/io/flamenet/client/LiveShapesTest.kt index 7d32a2d..8f7c4fb 100644 --- a/client/src/test/kotlin/io/flamenet/client/LiveShapesTest.kt +++ b/client/src/test/kotlin/io/flamenet/client/LiveShapesTest.kt @@ -47,11 +47,18 @@ class LiveShapesTest { private fun needFleet() { LiveTarget.refuseProduction(System.getenv("FN_LIVE_FLEET")) assumeTrue(System.getenv("FN_LIVE_FLEET") != null, "set FN_LIVE_FLEET to run against the fleet") + assumeTrue(PASSPHRASE.isNotEmpty(), "set FN_LIVE_PASSPHRASE") } - private fun needManaged() = assumeTrue( - System.getenv("FN_LIVE_MANAGED") == "1", - "set FN_LIVE_MANAGED=1 — these create real accounts on accounts.flamenet.io") + private fun needManaged() { + assumeTrue( + System.getenv("FN_LIVE_MANAGED") == "1", + "set FN_LIVE_MANAGED=1 — these create real accounts on accounts.flamenet.io") + assumeTrue( + PASSPHRASE.isNotEmpty(), + "set FN_LIVE_PASSPHRASE — these create real accounts and will not use " + + "a passphrase written into the repository") + } private val local = "https://relay1.flamenet.io" private val sharedSelfHosted = "https://relay2.flamenet.io" @@ -388,6 +395,10 @@ class LiveShapesTest { } private companion object { - const val PASSPHRASE = "a-long-enough-passphrase" + /** + * 🔴 Was `"a-long-enough-passphrase"`, a literal in a source file used + * to create real accounts. See [LiveTarget.passphrase]. + */ + val PASSPHRASE: String get() = LiveTarget.passphrase() } } diff --git a/client/src/test/kotlin/io/flamenet/client/LiveTarget.kt b/client/src/test/kotlin/io/flamenet/client/LiveTarget.kt index 19f521e..4a4b4a6 100644 --- a/client/src/test/kotlin/io/flamenet/client/LiveTarget.kt +++ b/client/src/test/kotlin/io/flamenet/client/LiveTarget.kt @@ -77,4 +77,26 @@ object LiveTarget { ) } } + + /** + * The passphrase a live suite signs up with. + * + * 🔴 **From the environment, with no fallback, and that is the whole + * point.** Both live suites carried + * `const val PASSPHRASE = "a-long-enough-passphrase"` and used it to create + * accounts on `accounts.flamenet.io` whenever somebody set + * `FN_LIVE_MANAGED=1`. A credential written into a source file is a + * published password: every account those runs ever made can be signed into + * by anybody who can read this repository, and nothing about the accounts + * being throwaway changes that — they are real accounts on a real service, + * and they persist. + * + * ⚠️ Empty rather than throwing, so the caller turns it into an announced + * skip. A live suite that cannot run must report SKIPPED and not fail: a + * red suite for a missing variable teaches people to ignore red. + * + * The iOS side had the same class of bug twice (ios#62) — a literal + * fallback used against production. This is the Kotlin half of that fix. + */ + fun passphrase(): String = System.getenv("FN_LIVE_PASSPHRASE").orEmpty() } diff --git a/client/src/test/kotlin/io/flamenet/client/LiveTwoProductsTest.kt b/client/src/test/kotlin/io/flamenet/client/LiveTwoProductsTest.kt index 0ddd7c6..c9dd5cd 100644 --- a/client/src/test/kotlin/io/flamenet/client/LiveTwoProductsTest.kt +++ b/client/src/test/kotlin/io/flamenet/client/LiveTwoProductsTest.kt @@ -26,13 +26,20 @@ class LiveTwoProductsTest { private val filesHost = "https://files.flamenet.io" private val localRelay = "https://relay1.flamenet.io" - private fun needManaged() = assumeTrue( - System.getenv("FN_LIVE_MANAGED") == "1", - "set FN_LIVE_MANAGED=1 — this creates a real account") + private fun needManaged() { + assumeTrue( + System.getenv("FN_LIVE_MANAGED") == "1", + "set FN_LIVE_MANAGED=1 — this creates a real account") + assumeTrue( + PASSPHRASE.isNotEmpty(), + "set FN_LIVE_PASSPHRASE — this creates a real account and will not " + + "use a passphrase written into the repository") + } private fun needFleet() { LiveTarget.refuseProduction(System.getenv("FN_LIVE_FLEET")) assumeTrue(System.getenv("FN_LIVE_FLEET") != null, "set FN_LIVE_FLEET") + assumeTrue(PASSPHRASE.isNotEmpty(), "set FN_LIVE_PASSPHRASE") } /** @@ -114,6 +121,10 @@ class LiveTwoProductsTest { } private companion object { - const val PASSPHRASE = "a-long-enough-passphrase" + /** + * 🔴 Was `"a-long-enough-passphrase"`, a literal in a source file used + * to create real accounts. See [LiveTarget.passphrase]. + */ + val PASSPHRASE: String get() = LiveTarget.passphrase() } } diff --git a/client/src/test/kotlin/io/flamenet/client/TestSafetyTest.kt b/client/src/test/kotlin/io/flamenet/client/TestSafetyTest.kt new file mode 100644 index 0000000..43c8168 --- /dev/null +++ b/client/src/test/kotlin/io/flamenet/client/TestSafetyTest.kt @@ -0,0 +1,187 @@ +package io.flamenet.client + +import java.io.File +import kotlin.test.Test +import kotlin.test.assertTrue + +/** + * A test run must not publish a credential (the Kotlin half of ios#62). + * + * 🔴 **The iOS side had this bug twice, and Android had a quieter version of + * it.** Every live suite here is properly gated behind `assumeTrue` — better + * than iOS managed — but two of them carried + * `const val PASSPHRASE = "a-long-enough-passphrase"` and used it to create + * accounts on `accounts.flamenet.io` whenever somebody set `FN_LIVE_MANAGED=1`. + * + * The gate stops it running by accident. It does not stop the password being + * written down. Every account those runs ever created can be signed into by + * anybody who can read this repository, and "they were throwaway accounts" + * does not change it — they are real accounts on a real service and they + * persist until somebody deletes them. + * + * ⚠️ **A source scan, and coarse on purpose.** It cannot prove a suite is safe; + * it proves the specific thing that went wrong cannot come back silently. + * That is worth having even though it is not a proof, and it costs a few + * milliseconds in the JVM suite rather than a device. + */ +class TestSafetyTest { + + private fun testSources(): List { + // From `client/build/classes/...` back to the repository root, then to + // every Kotlin test source under it. Walking from the working directory + // rather than a compiled resource, so this sees the tree as written. + val root = generateSequence(File(".").absoluteFile) { it.parentFile } + .firstOrNull { File(it, "settings.gradle.kts").exists() || File(it, "PARITY.md").exists() } + ?: File(".").absoluteFile + return root.walkTopDown() + .onEnter { it.name != "build" && it.name != ".git" } + .filter { it.isFile && it.extension == "kt" } + .filter { it.path.contains("/test/") || it.path.contains("/androidTest/") } + .toList() + } + + /** + * Lines that are code rather than prose. + * + * ⚠️ Trailing comments go too. The Swift version of this guard flagged an + * *email* default because the word "password" appeared in a comment at the + * end of the line — and a guard that cries wolf is a guard somebody + * switches off. + */ + private fun codeLines(body: String): List = + body.lineSequence() + .filterNot { + val t = it.trimStart() + t.startsWith("//") || t.startsWith("*") || t.startsWith("/*") + } + .map { line -> + var i = 0 + while (true) { + val at = line.indexOf("//", i) + if (at < 0) return@map line + if (at == 0 || line[at - 1] != ':') return@map line.substring(0, at) + i = at + 2 + } + @Suppress("UNREACHABLE_CODE") line + } + .toList() + + /** + * Whether this file can actually reach a relay somebody else runs. + * + * ⚠️ **`MockWebServer` is the discriminator, and getting this wrong made + * the guard useless twice.** Keying on the `Live` name prefix flagged the + * guard helper and its own unit test; keying on the string "http" flagged + * four suites that sign up against a local mock, which is exactly the + * correct way to test sign-up. A file that stands up a mock server cannot + * reach anybody, whatever else it contains. + * + * What is left is: it signs up or signs in, it does NOT mock the server, + * and it names a real host or takes one from the environment. + */ + private fun reachesALiveRelay(code: List): Boolean { + val reaching = listOf("signUp(", "signIn(", "register(") + if (reaching.none { needle -> code.any { it.contains(needle) } }) return false + if (code.any { it.contains("MockWebServer") }) return false + return code.any { it.contains("flamenet.io") || it.contains("System.getenv") } + } + + /** + * 🔴 **No credential may be a literal in a suite that reaches a relay.** + * + * The rule is scoped to files that actually sign up or sign in, because a + * purely local unit test using `passphrase = "pass"` against an in-memory + * store publishes nothing and is not the problem. + */ + @Test + fun `no live suite carries a credential literal`() { + val offenders = mutableListOf() + + for (file in testSources()) { + val code = codeLines(file.readText()) + if (!reachesALiveRelay(code)) continue + for (line in code) { + val looksLikeCredential = line.contains("PASSPHRASE") || + line.contains("passphrase", ignoreCase = true) || + line.contains("password", ignoreCase = true) + if (!looksLikeCredential) continue + // `val X = "…"` or `const val X = "…"` — an assignment to a + // non-empty literal. A getter that reads the environment has no + // `= "` on it and is the shape this wants people to use. + val assigned = Regex("""\bva[lr]\s+\w+\s*(:\s*String\s*)?=\s*"([^"]+)"""") + .find(line)?.groupValues?.getOrNull(2) + if (assigned != null) { + offenders += "${file.name}: $assigned" + } + } + } + + assertTrue( + offenders.isEmpty(), + """ + A suite that signs up or signs in carries a credential written into + the source: $offenders + + This repository is readable by everybody who can read it, so that + string is a published password — and these suites create REAL + accounts on a real service. Being gated behind an environment + variable stops it running by accident; it does not stop the password + being written down. + + Read it from the environment instead, with no fallback: + LiveTarget.passphrase(), and an assumeTrue on it being non-empty so + an unconfigured run reports SKIPPED rather than signing up with a + value anybody can read. + """.trimIndent(), + ) + } + + /** + * 🔴 **Every suite that reaches a live relay must be able to decline.** + * + * `assumeTrue` reports SKIPPED, which is the truth. A bare `return` makes a + * test that did nothing report PASSED, and a suite of those is + * indistinguishable from a suite that works — the point `LiveShapesTest` + * already makes in prose and this makes checkable. + */ + @Test + fun `every live suite can announce a skip`() { + // ⚠️ Scoped to what a file DOES, not to what it is called — see + // `reachesALiveRelay`. + val offenders = testSources() + .filter { reachesALiveRelay(codeLines(it.readText())) } + .filterNot { codeLines(it.readText()).any { line -> line.contains("assumeTrue") } } + .map { it.name } + + assertTrue( + offenders.isEmpty(), + "these live suites cannot skip and will reach a real relay on any " + + "plain `./gradlew test`: $offenders", + ) + } + + /** + * ⚠️ A scan that finds nothing passes. This proves it is reading the real + * tree — a moved directory would otherwise turn both checks above into + * no-ops reporting success. + */ + @Test + fun `the scan is looking at the real test sources`() { + val found = testSources() + assertTrue(found.size > 15, "found ${found.size} test sources, too few to be this repository") + assertTrue(found.any { it.name == "LiveShapesTest.kt" }, "LiveShapesTest.kt was not found") + assertTrue( + found.any { codeLines(it.readText()).any { line -> line.contains("assumeTrue") } }, + "the scan cannot see into the files it is reading", + ) + // 🔴 And the live-relay predicate must actually match something. If it + // matched nothing, both checks above would pass by looking at an empty + // set — the exact way a guard reports success while guarding nothing. + val live = found.filter { reachesALiveRelay(codeLines(it.readText())) } + assertTrue( + live.size >= 5, + "the live-relay predicate matched ${live.size} suites, so the checks " + + "above are guarding almost nothing: ${live.map { it.name }}", + ) + } +}