diff --git a/app/proguard-rules.pro b/app/proguard-rules.pro index 16f03e2..a1eca00 100644 --- a/app/proguard-rules.pro +++ b/app/proguard-rules.pro @@ -1,4 +1,41 @@ -# Release builds are minified. Nothing project-specific is needed yet. +# Release builds are minified. # -# When a rule IS added here it must say what breaks without it — a proguard file +# When a rule is added here it must say what breaks without it — a proguard file # of unexplained -keep lines is a file nobody can ever safely shrink again. + +# --------------------------------------------------------------------------- +# PRODUCT_PLAN.md §45: "Disable verbose logging in release builds." +# --------------------------------------------------------------------------- +# +# `checkNoHealthLogging` in the root build.gradle.kts already fails the build on +# any logging call in a module that can see a cycle date, and that is the real +# protection because it stops the line being written at all. +# +# This is the second layer, and it exists for the two cases a source guard +# cannot reach: +# +# - **A dependency logging on our behalf.** Room, WorkManager and the AndroidX +# libraries log, and a stack trace or a query they print can carry a value +# that came from a cycle record. The guard cannot see inside a library; R8 +# can remove the call sites. +# - **A future module.** The guard reads a list of directories. Somebody adding +# a module and forgetting to list it gets no warning, because absence of a +# finding looks exactly like a clean result. +# +# `assumenosideeffects` lets R8 delete these calls entirely, arguments and all, +# rather than leaving a stripped string constant in the dex. It is safe here +# only because nothing in this app uses a Log return value — `Log.d` returns an +# int nobody reads. If that ever stops being true, R8 will assume it is zero. +# +# Warning removed deliberately: `-dontwarn` is not used, because a warning here +# would mean the class shape changed and this rule stopped matching, which is +# exactly what we would want to hear about. +-assumenosideeffects class android.util.Log { + public static int v(...); + public static int d(...); + public static int i(...); + public static int w(...); + public static int e(...); + public static int wtf(...); + public static int println(...); +} diff --git a/build.gradle.kts b/build.gradle.kts index 488098c..db34c6a 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -306,10 +306,136 @@ tasks.register("checkPermissions") { } } + +// --------------------------------------------------------------------------- +// PRODUCT_PLAN.md §45: no health data in logs, and not by good behaviour +// --------------------------------------------------------------------------- +// +// §45 forbids writing cycle dates to logs and requires verbose logging gone from +// release builds. Today the app honours that by simply not logging at all — two +// references to `android.util.Log` exist in the whole tree and neither is a log +// call. That is a property of nobody having typed one yet, not of anything +// stopping them. +// +// One `Log.d("cycle", record.toString())` added while chasing a defect writes +// menstrual dates to logcat, where a bug report, a crash reporter, and on older +// Android versions any app with log access can read them. It is one line, it +// looks harmless in review, and it is exactly what §45 is written about. +// +// ## The two things that make this guard rather than a grep +// +// Both are GUARDS.md §2, and both are live in this repository right now: +// +// - `PeriodApplication.kt` passes `android.util.Log.WARN` to WorkManager as a +// CONSTANT. It is not a log call and must not fail the build. So the check +// matches a method call — `Log.d(` — and never the class name alone. +// - `ReminderWorker.kt`'s KDoc says "a `Log.d` in a worker is the kind that +// survives", explaining why there isn't one. A naive grep fails on the +// comment that exists to prevent the thing. Comments are stripped first. +// +// A guard that punishes the clearest possible explanation gets the explanation +// deleted, which is a worse outcome than no guard. + +/** Modules that can see a cycle date. `core/designsystem` cannot, so it is absent. */ +val modulesSeeingHealthData: List = listOf( + "app", "core/data", "core/database", "core/datastore", + "core/notifications", "domain/cycle", "domain/prediction", +) + +/** + * Call shapes that put a value somewhere a person can read it later. + * + * `printStackTrace` is in the list for the same reason as the rest: the stack it + * prints carries whatever the exception message holds, and the easiest way to + * write an exception message is to interpolate the record that caused it. + */ +val forbiddenLoggingCalls: List = listOf( + "Log.v(", "Log.d(", "Log.i(", "Log.w(", "Log.e(", "Log.wtf(", "Log.println(", + "println(", "print(", "System.out", "System.err", "printStackTrace(", +) + +tasks.register("checkNoHealthLogging") { + group = "verification" + description = "No logging call may exist in a module that can see a cycle date (PRODUCT_PLAN §45)." + + val roots = modulesSeeingHealthData.map { layout.projectDirectory.dir(it).asFile } + val forbidden = forbiddenLoggingCalls + // Resolved at configuration time. Reaching for `layout` inside doLast + // captures the Project itself, which the configuration cache refuses to + // serialize — the build fails with a cache problem rather than a guard + // result, which reads like the guard is broken when it is not. + val repoRoot = layout.projectDirectory.asFile + + doLast { + // Comments go before anything is matched. Block comments first, so a + // KDoc spanning lines cannot leave its middle behind, then line + // comments. Kotlin has no nested block comments to worry about here. + fun codeOf(text: String): String = + text.replace(Regex("""/\*.*?\*/""", RegexOption.DOT_MATCHES_ALL), "") + .lines().joinToString("\n") { it.substringBefore("//") } + + var scanned = 0 + val hits = mutableListOf() + + roots.forEach { root -> + root.walkTopDown() + .filter { it.isFile && it.extension == "kt" } + // `/build/` and `/bin/` are outputs, not source. `bin/` is an + // IDE artefact, gitignored, and it holds stale COPIES of test + // files — the first run of this guard failed on a println in a + // copy of a test that the real tree exempts. A guard that fails + // on untracked build output is a guard somebody switches off. + .filterNot { it.path.contains("/build/") || it.path.contains("/bin/") } + // Test sources are exempt: a test that prints is a test being + // debugged, it never ships, and forbidding it would push people + // to debug by other means. + .filterNot { it.path.contains("/src/test/") || it.path.contains("/src/androidTest/") } + .forEach { file -> + scanned++ + val code = codeOf(file.readText()) + code.lines().forEachIndexed { i, line -> + forbidden.forEach { pattern -> + if (line.contains(pattern)) { + hits += "${file.relativeTo(repoRoot)}:${i + 1} $pattern ${line.trim()}" + } + } + } + } + } + + // Never a silent pass. A path typo in modulesSeeingHealthData would + // otherwise report a clean build having read nothing at all, which is + // how the module-boundary guard spent its first day green. + if (scanned == 0) { + throw GradleException( + "no Kotlin sources found, so no logging was checked. " + + "Check the paths in modulesSeeingHealthData.", + ) + } + + if (hits.isNotEmpty()) { + logger.error("") + logger.error("PRODUCT_PLAN.md §45: health data must never reach a log.") + logger.error("") + hits.forEach { logger.error(" $it") } + logger.error("") + logger.error("These modules can see a cycle date, so a log call in them can print one.") + logger.error("If you are debugging, delete the line before committing. If you genuinely") + logger.error("need diagnostics, emit a non-sensitive event NAME with no values, as §45's") + logger.error("debug logging rule shows, and add the call shape to an allowlist here with") + logger.error("the reason.") + throw GradleException("${hits.size} logging call(s) where health data is visible.") + } + + logger.lifecycle("health logging: $scanned Kotlin file(s) checked, no logging calls.") + } +} + // Wired into `check` so it runs with the tests rather than only when remembered. subprojects { tasks.matching { it.name == "check" }.configureEach { dependsOn(rootProject.tasks.named("checkModuleBoundaries")) if (project.path == ":app") dependsOn(rootProject.tasks.named("checkPermissions")) + dependsOn(rootProject.tasks.named("checkNoHealthLogging")) } } diff --git a/docs/architecture/README.md b/docs/architecture/README.md index eba8770..0979617 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -380,6 +380,7 @@ menu is. | `scripts/forgejo-issue.py` | files and closes issues in the tracker convention, with every rule of it as a check | | `scripts/prove-guard.sh` | breaks what a guard protects and requires the guard to go red | | `scripts/schema-guard.sh` | a Room entity may not change without the version changing with it — asks git, because Room overwrites the export during the build | +| `checkNoHealthLogging` (root `build.gradle.kts`) | no logging call may exist in a module that can see a cycle date — §45. Strips comments and matches a call rather than the class, so the `Log.WARN` constant and the KDoc explaining the rule both stay legal | | `.githooks/` | pre-commit, commit-msg, post-commit — see [githooks/README.md](githooks/README.md) | ## What does not belong here diff --git a/docs/history/DEVELOPMENT_LOG.md b/docs/history/DEVELOPMENT_LOG.md index d3100b5..0daabf5 100644 --- a/docs/history/DEVELOPMENT_LOG.md +++ b/docs/history/DEVELOPMENT_LOG.md @@ -32,6 +32,67 @@ written and stay true. It is exempt from review for the same reason a receipt is ## Entries +### 2026-08-18 — A logging guard, and the leak it was not looking for + +#38 asked for a guard that stops health data reaching a log. Writing it found a +worse problem than the one it was written for. + +**The guard.** `checkNoHealthLogging` fails the build on any logging call in a +module that can see a cycle date. Two things make it a guard rather than a grep, +and both were live in this repository already: `PeriodApplication` passes +`android.util.Log.WARN` to WorkManager as a **constant**, which is not a log +call, and `ReminderWorker`'s KDoc says *"a `Log.d` in a worker is the kind that +survives"* — the comment explaining why there isn't one. A naive grep fails the +build on the clearest possible explanation, and the obvious fix is to delete the +explanation. So it matches a call shape and strips comments first. + +Proved in both directions, per GUARDS.md §1: injecting a real `Log.d` into +`CycleRepository` produced **exactly one** failure, and a comment containing both +`Log.d(` and `println(` stayed green. + +It also failed its first run for a reason worth recording — it walked +`domain/*/bin/`, an IDE output directory that is gitignored and holds stale +copies of test files, and reported a `println` in one of them. A guard that +fails on untracked build output is a guard somebody switches off. + +**And the thing it could never have caught.** `Prediction`'s `init` block +interpolated dates into its `require` messages: + + require(!windowStart.isAfter(windowEnd)) { "window start $windowStart is after window end $windowEnd" } + +Five predicted dates across three messages, in an `IllegalArgumentException` — +the one string a crash reporter is guaranteed to collect without anybody +choosing to log it. §45 says "do not include raw cycle dates in crash reports", +and this was the app doing exactly that, with no logging statement anywhere near +it. + +The same shape applies to every data class: `toString()` renders every field +into any string that touches it. `PeriodRecord`, `SpottingRecord`, +`CycleRecord`, `Prediction` and `NotYetObservation` now override it — ids and +lengths survive, dates do not, because an id identifies a row without describing +a person and a cycle length says nothing about when. `NoDatesInDiagnosticsTest` +pins all seven cases, and was itself proved to fail. + +**The release build was driven, not reasoned about.** `assembleRelease` is +unsigned, so it was signed with the debug keystore and installed alongside the +debug build. Onboarding to a forecast, then logging a period: **zero ISO dates in +logcat, zero health words, and the only mentions of the package are the system's +own.** The screenshot confirms it reached a real forecast rather than failing +early, because "no logs" is trivially true of an app that did nothing. + +- **Closed:** #30, #38. +- **Next action:** Batch 06 has #34, #35 and #37 left. #37, the privacy promise + in Settings, is the smallest and needs only the screen that now exists. #35, + export, is the bigger one and its trap is §45 again — an export is health data + leaving the app, so it uses the Storage Access Framework rather than writing to + shared storage. #34, app lock, still carries the open product question: what + happens when somebody forgets their PIN, with an irreversible delete on the + same screen. +- **Blockers:** #9, the Command Center webhook, still needs a person. TalkBack + has still never been run and no real lock screen has been looked at. A + `docs/design/dist/splash.png` appeared in the tree during this session and is + untracked — it belongs to nobody's issue yet. + ### 2026-08-18 — Batch 06 starts, and two defects that only a device could show The designed Settings screen and Delete My Data both landed. Neither is the diff --git a/docs/security/SECURITY.md b/docs/security/SECURITY.md index cd753db..5b6545a 100644 --- a/docs/security/SECURITY.md +++ b/docs/security/SECURITY.md @@ -123,6 +123,27 @@ Event names without values. Verbose logging is off in release builds, and no raw cycle date may appear in a crash report. An error's *name* is almost always enough; its message often carries the thing you were trying not to log. +**This is enforced rather than remembered, in three places**, because the rule +has three separate ways to be broken: + +- **`checkNoHealthLogging`** in the root `build.gradle.kts` fails the build on + any logging call in a module that can see a cycle date. It runs in + `./gradlew check`, it distinguishes a `Log.d(` call from the `android.util.Log.WARN` + constant this app legitimately passes to WorkManager, and it strips comments + first so the KDoc explaining why there is no logging does not fail the build. +- **`-assumenosideeffects` in `app/proguard-rules.pro`** removes `android.util.Log` + calls from the release build entirely, which covers the two things a source + guard cannot reach: a dependency logging on our behalf, and a module somebody + adds without listing it in the guard. +- **The domain types do not render their own dates.** `PeriodRecord`, + `SpottingRecord`, `CycleRecord`, `Prediction` and `NotYetObservation` all + override `toString()`. This is the leak that needs no logging statement at + all: a data class prints every field into any string that touches it, and + `Prediction`'s own `require` messages used to interpolate five predicted dates + into an `IllegalArgumentException` — the one string a crash reporter is + guaranteed to collect without anybody choosing to log it. + `NoDatesInDiagnosticsTest` pins all of it. + Analytics, if adopted at all, collect product-level events only ([§46](../planning/PRODUCT_PLAN.md)) — never `cycle_length=31`, `fertility_status=high` or `prediction_error=`. Prediction accuracy is computed diff --git a/domain/cycle/src/main/kotlin/dev/privacyllc/period/domain/cycle/Cycle.kt b/domain/cycle/src/main/kotlin/dev/privacyllc/period/domain/cycle/Cycle.kt index 983c783..8e49d26 100644 --- a/domain/cycle/src/main/kotlin/dev/privacyllc/period/domain/cycle/Cycle.kt +++ b/domain/cycle/src/main/kotlin/dev/privacyllc/period/domain/cycle/Cycle.kt @@ -18,7 +18,23 @@ data class PeriodRecord( val endDate: LocalDate? = null, val source: PeriodRecordSource = PeriodRecordSource.MANUAL, val isConfirmed: Boolean = true, -) +) { + /** + * Deliberately dateless. + * + * This is the leak that needs no logging statement. `checkNoHealthLogging` + * stops anyone writing `Log.d(...)`, but a data class renders its own + * contents into any string that touches it — an exception message written as + * "bad record: $record", a crash payload, a coroutine's own diagnostics. The + * generated `toString()` would put a menstrual date in all three, and nobody + * would have typed a date anywhere. + * + * The id is kept because it identifies the row without describing the + * person, which is what a diagnostic actually needs. + */ + override fun toString(): String = + "PeriodRecord(id=$id, hasEnd=${endDate != null}, source=$source, confirmed=$isConfirmed)" +} /** * Spotting. Tracked separately and **never** treated as a period start. @@ -29,7 +45,10 @@ data class PeriodRecord( data class SpottingRecord( val id: Long, val date: LocalDate, -) +) { + /** Dateless for the reason [PeriodRecord.toString] gives. */ + override fun toString(): String = "SpottingRecord(id=$id)" +} /** * An interval between two confirmed period starts. @@ -42,7 +61,16 @@ data class CycleRecord( val currentPeriodStart: LocalDate, val cycleLengthDays: Int, val periodDurationDays: Int? = null, -) +) { + /** + * Lengths survive, dates do not. + * + * A cycle length on its own says nothing about when anything happened, and + * it is the field worth seeing in a diagnostic about the prediction engine. + */ + override fun toString(): String = + "CycleRecord(cycleLengthDays=$cycleLengthDays, periodDurationDays=$periodDurationDays)" +} /** * Derive the cycle intervals from confirmed period records. diff --git a/domain/prediction/src/main/kotlin/dev/privacyllc/period/domain/prediction/Prediction.kt b/domain/prediction/src/main/kotlin/dev/privacyllc/period/domain/prediction/Prediction.kt index e6334bc..096f75e 100644 --- a/domain/prediction/src/main/kotlin/dev/privacyllc/period/domain/prediction/Prediction.kt +++ b/domain/prediction/src/main/kotlin/dev/privacyllc/period/domain/prediction/Prediction.kt @@ -20,12 +20,33 @@ data class Prediction( val modelVersion: String, ) { init { - require(!windowStart.isAfter(windowEnd)) { "window start $windowStart is after window end $windowEnd" } + // These messages name the invariant and not the dates, deliberately. + // + // §45 forbids raw cycle dates in crash reports, and an `IllegalArgumentException` + // message is a crash report — it is the one string guaranteed to be + // collected, by any reporter, without anybody choosing to log it. These + // three requires previously interpolated five predicted dates between + // them, which is a health-data leak that needed no logging statement to + // exist. + // + // Nothing diagnostic is lost: which invariant broke is the useful half, + // and the values are in a debugger for anyone who can reproduce it. + // `confidenceScore` stays because a number between 0 and 1 says nothing + // about when anything happened. + require(!windowStart.isAfter(windowEnd)) { "window start is after window end" } require(mostLikelyStartDate in windowStart..windowEnd) { - "most likely $mostLikelyStartDate falls outside the window $windowStart..$windowEnd" + "most likely start falls outside the window" } require(confidenceScore in 0.0..1.0) { "confidence $confidenceScore is outside 0..1" } } + + /** + * Dateless, for the reason [dev.privacyllc.period.domain.cycle.PeriodRecord] + * gives: a data class renders itself into any string that touches it, and a + * forecast is four dates. + */ + override fun toString(): String = + "Prediction(confidence=$confidenceLabel, model=$modelVersion)" } /** @@ -37,7 +58,10 @@ data class Prediction( data class NotYetObservation( val date: LocalDate, val predictionId: Long? = null, -) +) { + /** Dateless. It records that the user answered, not when their body did. */ + override fun toString(): String = "NotYetObservation(predictionId=$predictionId)" +} /** * Everything an engine is allowed to know. diff --git a/domain/prediction/src/test/kotlin/dev/privacyllc/period/domain/prediction/NoDatesInDiagnosticsTest.kt b/domain/prediction/src/test/kotlin/dev/privacyllc/period/domain/prediction/NoDatesInDiagnosticsTest.kt new file mode 100644 index 0000000..ff29010 --- /dev/null +++ b/domain/prediction/src/test/kotlin/dev/privacyllc/period/domain/prediction/NoDatesInDiagnosticsTest.kt @@ -0,0 +1,120 @@ +package dev.privacyllc.period.domain.prediction + +import dev.privacyllc.period.domain.cycle.CycleRecord +import dev.privacyllc.period.domain.cycle.PeriodRecord +import dev.privacyllc.period.domain.cycle.SpottingRecord +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import java.time.LocalDate + +/** + * PRODUCT_PLAN.md §45, for the leak that needs no logging statement. + * + * `checkNoHealthLogging` in the root build script stops anyone writing a `Log.d` + * in a module that can see a cycle date. It cannot see two other routes to the + * same place, and both were open before this test existed: + * + * - **A data class renders its own contents.** `"bad record: $record"` in an + * exception message, a crash payload, or a coroutine's diagnostics prints + * every field. Nobody has to type a date for a date to be printed. + * - **`require` messages are crash reports.** `Prediction`'s init block + * interpolated five predicted dates across three messages — the one string + * guaranteed to be collected by any reporter, without anybody choosing to + * log it. + * + * The dates below are distinctive on purpose: 2026-08-18 renders as `2026-08-18` + * and its parts as `2026`, `08`, `18`, so a match is a real leak rather than a + * coincidence with a field name. + */ +class NoDatesInDiagnosticsTest { + + private val date = LocalDate.of(2026, 8, 18) + private val later = LocalDate.of(2026, 9, 15) + + /** The rendered ISO date, which is what interpolation produces. */ + private fun String.leaksA(d: LocalDate) = contains(d.toString()) + + @Test fun `PeriodRecord does not render its dates`() { + val s = PeriodRecord(id = 7, startDate = date, endDate = later).toString() + assertFalse("PeriodRecord.toString leaked a start date: $s", s.leaksA(date)) + assertFalse("PeriodRecord.toString leaked an end date: $s", s.leaksA(later)) + // The id survives, because it identifies the row without describing the person. + assertTrue("the id is what makes a diagnostic useful: $s", s.contains("id=7")) + } + + @Test fun `SpottingRecord does not render its date`() { + val s = SpottingRecord(id = 3, date = date).toString() + assertFalse(s, s.leaksA(date)) + } + + @Test fun `CycleRecord renders lengths but not dates`() { + val s = CycleRecord( + previousPeriodStart = date, + currentPeriodStart = later, + cycleLengthDays = 28, + ).toString() + assertFalse(s, s.leaksA(date)) + assertFalse(s, s.leaksA(later)) + assertTrue("a cycle length says nothing about when: $s", s.contains("28")) + } + + @Test fun `Prediction does not render its forecast`() { + val s = Prediction( + mostLikelyStartDate = date, + windowStart = date.minusDays(3), + windowEnd = date.plusDays(3), + confidenceScore = 0.5, + confidenceLabel = ConfidenceLabel.MEDIUM, + modelVersion = "test", + ).toString() + assertFalse(s, s.leaksA(date)) + assertFalse(s, s.leaksA(date.minusDays(3))) + assertFalse(s, s.leaksA(date.plusDays(3))) + } + + @Test fun `NotYetObservation does not render its date`() { + val s = NotYetObservation(date = date, predictionId = 4).toString() + assertFalse(s, s.leaksA(date)) + } + + /** + * The invariant failures, which are the ones that reach a crash reporter. + */ + @Test fun `a window in the wrong order fails without naming the dates`() { + val e = runCatching { + Prediction( + mostLikelyStartDate = date, + windowStart = later, + windowEnd = date, + confidenceScore = 0.5, + confidenceLabel = ConfidenceLabel.LOW, + modelVersion = "test", + ) + }.exceptionOrNull() + + val message = e?.message.orEmpty() + assertTrue("the invariant should still be named: $message", message.isNotBlank()) + assertFalse("require message leaked a date: $message", message.leaksA(date)) + assertFalse("require message leaked a date: $message", message.leaksA(later)) + } + + @Test fun `a most-likely date outside the window fails without naming the dates`() { + val e = runCatching { + Prediction( + mostLikelyStartDate = later, + windowStart = date.minusDays(1), + windowEnd = date.plusDays(1), + confidenceScore = 0.5, + confidenceLabel = ConfidenceLabel.LOW, + modelVersion = "test", + ) + }.exceptionOrNull() + + val message = e?.message.orEmpty() + assertTrue(message.isNotBlank()) + assertFalse("require message leaked a date: $message", message.leaksA(later)) + assertFalse("require message leaked a date: $message", message.leaksA(date.minusDays(1))) + assertFalse("require message leaked a date: $message", message.leaksA(date.plusDays(1))) + } +}