From 97b04f9f92584ec9c0d0bddee5095ddd4455189b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 8 Oct 2026 09:21:40 -0400 Subject: [PATCH] Close the gaps family idea #5103 found in how Inkwell distributes its app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the idea's practices this project still owed (Scribe #5118): Practice 3, CI fails on the wrong signer. The signing step printed the certificate and went on. It now fails unless the APK has exactly one signer and that signer is the release certificate (SHA-256 408a5835…, pinned from run 8753). The steps that publish come after it in the same job, so a wrongly signed build is never staged or published. Practice 6, app downloads are throttled and carry a sha256 ETag. - The download route counts per account and answers 429 with Retry-After past the limit. The limit is a new Settings → Security value, "App downloads per account per hour" (default 30), live like the sign-in limits. - The ETag is the sidecar's sha256, not Quart's mtime-and-path, so a phone resuming a download across a redeploy is not told its partial copy is stale. Quart's own ETag and conditional handling are off, and the route runs the conditional pass after setting the ETag, so Range and If-Range are judged against the content. Practice 9, the update offer and debug builds. - The install-permission notice re-reads the grant each time the app comes back, as ReminderNotice does. Read once, it stayed up after someone granted the permission in Settings and came back. - A debuggable build says it can't update itself and checks for nothing. Android would refuse the release-signed APK over a debug signature anyway. Co-Authored-By: Claude Opus 5.5 --- .forgejo/workflows/android.yml | 24 +++++++- .../java/com/fabledsword/inkwell/AppUpdate.kt | 13 ++++ .../com/fabledsword/inkwell/ui/UpdateCard.kt | 22 ++++++- .../fabledsword/inkwell/ui/UpdateViewModel.kt | 12 +++- android/app/src/main/res/values/strings.xml | 1 + src/inkwell/client_dist.py | 44 ++++++++++++- src/inkwell/ratelimit.py | 21 ++++++- src/inkwell/settings.py | 12 ++++ tests/test_client_dist.py | 61 +++++++++++++++++++ tests/test_integration.py | 1 + 10 files changed, 202 insertions(+), 9 deletions(-) diff --git a/.forgejo/workflows/android.yml b/.forgejo/workflows/android.yml index ca1433f..5449d4a 100644 --- a/.forgejo/workflows/android.yml +++ b/.forgejo/workflows/android.yml @@ -232,11 +232,31 @@ jobs: # generated. Signing with the WRONG key produces a perfectly valid APK that # simply refuses to install over the app already on the phone — a failure # that otherwise only shows up on the device, after the run is green. - - name: Show the signing certificate + # + # And FAILS unless there is exactly one signer and it is the release key + # (family idea #5103, practice 3). Printing alone was not a check: a build + # signed with any other key would have gone on to be staged and published, + # and every phone would have refused it. The steps that publish come after + # this one in the same job, so a failure here stops them. + - name: Check the signing certificate if: steps.build.outputs.keystore != '' + env: + # The release certificate's SHA-256, as apksigner printed it for the + # signed APK in run 8753 (CN=Bryan Van Deusen, O=fabledsword). Every + # installed copy carries this certificate. It only changes if the key + # does, and then every install has to be replaced by hand anyway. + RELEASE_CERT_SHA256: 408a5835ea1627f329d9fba4f00712b096faf2d30624ec6f3a8b2b5f8bb15caa run: | apksigner="$(ls /opt/android-sdk/build-tools/*/apksigner | head -1)" - "$apksigner" verify --print-certs "app/build/outputs/apk/release/app-release.apk" + certs="$("$apksigner" verify --print-certs "app/build/outputs/apk/release/app-release.apk")" + printf '%s\n' "$certs" + signers="$(printf '%s\n' "$certs" | grep -c '^Signer #[0-9]* certificate SHA-256 digest:' || true)" + digest="$(printf '%s\n' "$certs" | sed -n 's/^Signer #1 certificate SHA-256 digest: //p')" + if [ "$signers" != 1 ] || [ "$digest" != "$RELEASE_CERT_SHA256" ]; then + echo "::error::The APK is not signed by the release key alone ($signers signer(s), first $digest; expected $RELEASE_CERT_SHA256)." + exit 1 + fi + echo "Signed by the release key." # Staged with a STABLE name plus the sidecar the server reads its version # out of — an APK keeps that in a binary manifest Python cannot parse, and diff --git a/android/app/src/main/java/com/fabledsword/inkwell/AppUpdate.kt b/android/app/src/main/java/com/fabledsword/inkwell/AppUpdate.kt index 7a351bc..e89ee5b 100644 --- a/android/app/src/main/java/com/fabledsword/inkwell/AppUpdate.kt +++ b/android/app/src/main/java/com/fabledsword/inkwell/AppUpdate.kt @@ -3,6 +3,7 @@ package com.fabledsword.inkwell import android.content.Context import android.content.Intent import android.content.IntentSender +import android.content.pm.ApplicationInfo import android.content.pm.PackageInstaller import android.net.ConnectivityManager import android.net.NetworkCapabilities @@ -50,6 +51,18 @@ object AppUpdate { context.packageManager.getPackageInfo(context.packageName, 0).longVersionCode }.getOrDefault(0L) + /** + * Whether this build can update itself at all. + * + * A debuggable build is signed with a debug key (CI's unsigned path, or a local + * build), and Android refuses the release-signed APK over it with + * INSTALL_FAILED_UPDATE_INCOMPATIBLE. Offering that update would be a download + * that can only fail, so a debug build says so instead (family idea #5103, + * practice 9). + */ + fun selfUpdates(context: Context): Boolean = + (context.applicationInfo.flags and ApplicationInfo.FLAG_DEBUGGABLE) == 0 + /** * Whether this app may install packages at all. * diff --git a/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateCard.kt b/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateCard.kt index 7c9c95a..c675c29 100644 --- a/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateCard.kt +++ b/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateCard.kt @@ -19,6 +19,10 @@ import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip @@ -50,6 +54,13 @@ fun UpdateCard( ) { val context = LocalContext.current + // Re-read on every return to the app, as ReminderNotice does: the grant is given + // in a system screen this app cannot observe. Read once, the warning would still + // be up after someone granted it and came back, and the offer below it would + // look blocked when it is not (family idea #5103, practice 9). + var canInstall by remember { mutableStateOf(AppUpdate.canInstall(context)) } + ForegroundTransitions(onForeground = { canInstall = AppUpdate.canInstall(context) }, onBackground = {}) + // The system answers an install through a BroadcastReceiver, which has no way // back into a view model. This is the seam. UpdateOutcome.latest?.let { result -> @@ -63,6 +74,15 @@ fun UpdateCard( color = MaterialTheme.colorScheme.onSurfaceVariant, ) + if (!state.selfUpdates) { + Text( + text = stringResource(R.string.update_debug_build), + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + return@Column + } + val available = state.available if (available != null) { Text( @@ -101,7 +121,7 @@ fun UpdateCard( // Android's "install unknown apps" grant is separate from anything in the // manifest and only the person can give it. Said BEFORE a download rather // than after, so nobody spends 55 MiB to be told no. - if (available != null && !AppUpdate.canInstall(context)) { + if (available != null && !canInstall) { Notice( tone = Tone.WARN, title = stringResource(R.string.update_permission_title), diff --git a/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateViewModel.kt b/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateViewModel.kt index f1190a5..3e27390 100644 --- a/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/inkwell/ui/UpdateViewModel.kt @@ -31,6 +31,8 @@ data class UpdateState( val error: String? = null, /** The banner has been waved away — until the app next comes forward. */ val nagDismissed: Boolean = false, + /** False on a debug build, which can never install a release update over itself. */ + val selfUpdates: Boolean = true, ) { val busy: Boolean get() = checking || downloading || working @@ -68,7 +70,12 @@ class UpdateViewModel( */ private val context: Context, ) : ViewModel() { - var state by mutableStateOf(UpdateState(installedVersion = AppUpdate.installedVersionCode(context))) + var state by mutableStateOf( + UpdateState( + installedVersion = AppUpdate.installedVersionCode(context), + selfUpdates = AppUpdate.selfUpdates(context), + ), + ) private set /** When the last check ran, so coming back to the app twice in a minute is one. */ @@ -90,6 +97,9 @@ class UpdateViewModel( fun checkInBackground() { val now = System.currentTimeMillis() when { + // Nothing a debug build could do with what it found. + !state.selfUpdates -> Unit + state.busy -> Unit // Already fetched and waved away — say so again. "Later" is for that diff --git a/android/app/src/main/res/values/strings.xml b/android/app/src/main/res/values/strings.xml index fadb2b7..72f2529 100644 --- a/android/app/src/main/res/values/strings.xml +++ b/android/app/src/main/res/values/strings.xml @@ -228,6 +228,7 @@ Android needs your permission Inkwell has to be allowed to install apps before it can update itself. This is a one-time setting. Allow installing + This is a debug build, so it can\'t update itself. Install new builds by hand. App updates come from a server you connect. Until then, install new builds yourself. Your notes live on this device either way — syncing just keeps a server copy in step, so your other devices can catch up. Sync failed diff --git a/src/inkwell/client_dist.py b/src/inkwell/client_dist.py index c99e3ee..465f9cc 100644 --- a/src/inkwell/client_dist.py +++ b/src/inkwell/client_dist.py @@ -76,11 +76,12 @@ import json from dataclasses import dataclass from pathlib import Path -from quart import Blueprint, jsonify, request, send_from_directory +from quart import Blueprint, Response, g, jsonify, request, send_from_directory from .auth import login_required from .config import Config from .proxy import is_https +from .ratelimit import downloads_by_account from .responses import json_error @@ -378,6 +379,10 @@ async def client_download(platform_id: str): Authenticated — by session cookie from a browser, or by device bearer token from a client updating itself; `login_required` accepts either. The metadata above is public; the bytes are not for anyone who can reach the port. + + Throttled per account (Settings → Security), because each answer is tens of + megabytes: one account or a leaked device token looping here could fill the + server's uplink (family idea #5103, practice 6). """ platform = BY_ID.get(platform_id) if platform is None: @@ -385,11 +390,44 @@ async def client_download(platform_id: str): resolved = _resolve(platform) if resolved is None: return json_error(f"this server has no {platform_id} client", 404) + + account = str(g.user_id) + wait = downloads_by_account.retry_after(account) + if wait is not None: + response = jsonify({"error": "too many downloads from this account; try again later"}) + response.status_code = 429 + response.headers["Retry-After"] = str(wait) + return response + downloads_by_account.record(account) + # From the SAME directory the metadata came from, or a drop-in appearing between # the two calls would serve bytes the metadata does not describe. - root, _ = resolved - response = await send_from_directory(root, platform.artifact, mimetype=platform.mimetype) + root, found = resolved + return await send_artifact(root, platform, found) + + +async def send_artifact(root: Path, platform: Platform, found: dict) -> Response: + """The artifact, with its sha256 as the ETag and Range honoured against it. + + Quart's own ETag is the file's mtime, size and path. That changes when a + container restarts on the same bytes, so a phone resuming a download across a + redeploy would be told its partial copy is stale. It also says nothing about + content. The sidecar's sha256 is the identity the client already checks, so + `If-Range` is answered against the bytes themselves (family idea #5103, + practice 6). + + So Quart's ETag and conditional handling are both turned off, and the + conditional pass runs here after the right ETag is set. Run before it, Range + and If-None-Match would be judged against the wrong one. + """ + response = await send_from_directory( + root, platform.artifact, mimetype=platform.mimetype, add_etags=False, conditional=False + ) + response.set_etag(found["sha256"]) # Without this some browsers try to render it, and Android's download handler # wants a filename to hand to the package installer. response.headers["Content-Disposition"] = f'attachment; filename="{platform.artifact}"' + # `size` is the file's real length: `_read` refuses a sidecar whose size does + # not match the file beside it. + await response.make_conditional(request, accept_ranges=True, complete_length=found["size"]) return response diff --git a/src/inkwell/ratelimit.py b/src/inkwell/ratelimit.py index 336efd8..d27f061 100644 --- a/src/inkwell/ratelimit.py +++ b/src/inkwell/ratelimit.py @@ -1,9 +1,13 @@ """Throttling for the endpoints that a public deployment leaves exposed. -Only the credential endpoints are rate-limited: login, register, and the native +The credential endpoints are rate-limited: login, register, and the native device-link exchange. Everything else already needs a session or a device token to reach, so an attacker has to get through one of these three first. +The one exception is the app downloads (`client_dist`): signed in, but each one is +tens of megabytes, so one account or a leaked device token looping on it could fill +the server's uplink (family idea #5103, practice 6). + ## Why in-process is enough here, and where that stops being true State lives in module-level dicts, so it is per-process. That is correct for how @@ -133,7 +137,20 @@ reset_mail_by_account = SlidingWindow( ) +# App downloads, per account. Counted per REQUEST, so resuming a broken download +# counts each piece; the setting's default leaves room for that. The window is fixed +# at an hour because the setting's label says "per hour", and one number is easier +# to judge than two. +downloads_by_account = SlidingWindow(lambda: live("client_downloads_per_hour"), lambda: 3600.0) + + def reset_all() -> None: """Drop every counter. For tests — nothing in the app calls this.""" - for window in (sign_in_by_account, sign_in_by_address, register_by_address, reset_mail_by_account): + for window in ( + sign_in_by_account, + sign_in_by_address, + register_by_address, + reset_mail_by_account, + downloads_by_account, + ): window.clear() diff --git a/src/inkwell/settings.py b/src/inkwell/settings.py index 44f9de0..202886d 100644 --- a/src/inkwell/settings.py +++ b/src/inkwell/settings.py @@ -207,6 +207,17 @@ REGISTRY: list[SettingDef] = [ minimum=1, maximum=100, ), + SettingDef( + "client_downloads_per_hour", + "int", + 30, + "App downloads per account per hour", + "How many times one account or linked device may download an app in an hour. " + "Each piece of a resumed download counts, so leave room for a bad connection.", + "Security", + minimum=1, + maximum=1000, + ), ] _BY_KEY: dict[str, SettingDef] = {d.key: d for d in REGISTRY} @@ -326,6 +337,7 @@ _LIVE_KEYS = ( "register_limit_per_address", "register_window_minutes", "reset_emails_per_account", + "client_downloads_per_hour", ) _live: dict[str, Any] = {k: _BY_KEY[k].default for k in _LIVE_KEYS} diff --git a/tests/test_client_dist.py b/tests/test_client_dist.py index e01c8bf..8dc3095 100644 --- a/tests/test_client_dist.py +++ b/tests/test_client_dist.py @@ -422,3 +422,64 @@ async def test_a_server_without_the_appimage_404s_its_updater_manifest(app): "up to date", so the 404 has to be there to turn.""" resp = await app.test_client().get("/api/client/linux-appimage/update.json") assert resp.status_code == 404 + + +# --- serving the bytes (#5118, family idea #5103 practice 6) ----------------- +# +# `send_artifact` is called directly in a request context: the route in front of it +# needs a signed-in account, and this suite has no database to sign one in with. + + +async def _send(app, headers: dict): + place("android") + found = release("android") + root = Path(Config.client_root()) + async with app.test_request_context("/api/client/android/download", headers=headers): + resp = await client_dist.send_artifact(root, BY_ID["android"], found) + return resp, await resp.get_data() + + +async def test_the_etag_is_the_sha256_the_sidecar_records(app): + """Content identity, not Quart's mtime-and-path: the same bytes after a redeploy + must still match a partial download a phone is resuming.""" + resp, body = await _send(app, {}) + assert resp.status_code == 200 + assert resp.headers["ETag"] == f'"{"ab" * 32}"' + assert body == PAYLOAD + + +async def test_a_range_request_gets_just_that_range(app): + resp, body = await _send(app, {"Range": "bytes=4-9"}) + assert resp.status_code == 206 + assert body == PAYLOAD[4:10] + assert resp.headers["Content-Range"] == f"bytes 4-9/{len(PAYLOAD)}" + assert resp.headers["Accept-Ranges"] == "bytes" + + +async def test_resuming_a_different_build_starts_again_from_the_top(app): + """If-Range names the build the partial copy came from. A different one must + not be stitched onto it: the whole file comes back instead.""" + resp, body = await _send(app, {"Range": "bytes=4-9", "If-Range": f'"{"cd" * 32}"'}) + assert resp.status_code == 200 + assert body == PAYLOAD + + +async def test_resuming_the_same_build_continues_it(app): + resp, body = await _send(app, {"Range": "bytes=4-9", "If-Range": f'"{"ab" * 32}"'}) + assert resp.status_code == 206 + assert body == PAYLOAD[4:10] + + +async def test_a_client_holding_this_build_is_told_nothing_changed(app): + resp, body = await _send(app, {"If-None-Match": f'"{"ab" * 32}"'}) + assert resp.status_code == 304 + assert body == b"" + + +def test_the_download_throttle_reads_its_limit_from_settings(): + """The limit an admin saves is the one that applies, without a restart.""" + from inkwell import ratelimit + from inkwell.settings import live + + assert ratelimit.downloads_by_account.limit == live("client_downloads_per_hour") + assert ratelimit.downloads_by_account.window_s == 3600.0 diff --git a/tests/test_integration.py b/tests/test_integration.py index 6fbabf1..b7063a5 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -480,6 +480,7 @@ async def test_the_security_group_reaches_the_admin_ui(app_client, db): "register_limit_per_address", "register_window_minutes", "reset_emails_per_account", + "client_downloads_per_hour", } # The UI renders a number input from these, and it cannot offer a safe range it # was never told about.