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.