Close the gaps family idea #5103 found in how Inkwell distributes its app

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 <noreply@anthropic.com>

.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

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.
      *

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),

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

android/app/src/main/res/values/strings.xml

@@ -228,6 +228,7 @@
     <string name="update_permission_title">Android needs your permission</string>
     <string name="update_permission_body">Inkwell has to be allowed to install apps before it can update itself. This is a one-time setting.</string>
     <string name="update_permission_action">Allow installing</string>
+    <string name="update_debug_build">This is a debug build, so it can\'t update itself. Install new builds by hand.</string>
     <string name="update_needs_server">App updates come from a server you connect. Until then, install new builds yourself.</string>
     <string name="sync_footer">Your notes live on this device either way — syncing just keeps a server copy in step, so your other devices can catch up.</string>
     <string name="sync_failed_title">Sync failed</string>

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
+    root, found = resolved
-    response = await send_from_directory(root, platform.artifact, mimetype=platform.mimetype)
+    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

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()

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}

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

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.