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 }}", + ) + } +}