diff --git a/README.md b/README.md index 26b0db4..1258aa3 100644 --- a/README.md +++ b/README.md @@ -21,11 +21,13 @@ here. | script | what it does | |---|---| +| `next-version-code.sh` | print the next monotonic `versionCode` for an application id, read from a repo index -- called before the build, CI and a manual build alike (see "One versionCode rule" below) | | `sign-apk.sh` | check alignment, then sign a prebuilt unsigned release APK with a keystore the caller supplies | | `fdroid-update.sh` | run `fdroid update` over a `repo/` directory of already-signed APKs and produce the signed index | -| `publish.sh` | upload the resulting repo tree to an S3-compatible bucket | +| `publish.sh` | upload the resulting repo tree to an S3-compatible bucket -- files first, index last, and refuses to overwrite an already-published APK (see "Publish ordering and safety" below) | | `with-nixpkgs.sh` | run a command with one or more nixpkgs packages on PATH, pinned to the revision in `nixpkgs-pin` | | `check_zip_alignment.py` | the alignment check `sign-apk.sh` runs before signing (see "Align before sign" below) | +| `next_version_code.py` | the stdlib-only implementation `next-version-code.sh` wraps (index read, arithmetic) | | `action.yml` | the composite action a consumer includes to get all of the above | Deliberately **not** in scope: fetching the APK. That needs an upstream @@ -42,6 +44,13 @@ below for what that costs and why it is still the right call here. - uses: actions/checkout@v4 - id: ci uses: https://git.chaosbit.de/weblib/fdroid-repo-ci@main + +- run: | + version_code=$(bash "${{ steps.ci.outputs.path }}/next-version-code.sh" \ + "$FDROID_REPO_INDEX_URL" "$APPLICATION_ID") + echo "version_code=$version_code" >> "$GITHUB_ENV" + # ... feed $version_code into whatever builds app-release-unsigned.apk + - env: APK_KEYSTORE_PASSWORD: ${{ secrets.APK_KEYSTORE_PASSWORD }} run: | @@ -72,13 +81,104 @@ only one that works while a referenced repo is private -- irrelevant for consuming it, since that one holds the keys. `$APK_KEYSTORE`/`$INDEX_KEYSTORE`/`$FDROID_REPO_ROOT`/`$BUCKET`/ -`$S3_ENDPOINT_URL` above are the consumer's own paths and values -- nothing -in this repo defaults or hints at them. Env var *names* like -`APK_KEYSTORE_PASSWORD` are the consumer's choice too, not a convention this -repo imposes: `sign-apk.sh` and `fdroid-update.sh` each take the name of the -environment variable to read a password from as an argument, specifically so -two different keystores in the same job (the APK key and the separate repo -index key -- see below) never have to share one variable name. +`$S3_ENDPOINT_URL`/`$FDROID_REPO_INDEX_URL`/`$APPLICATION_ID` above are the +consumer's own paths and values -- nothing in this repo defaults or hints at +them. Env var *names* like `APK_KEYSTORE_PASSWORD` are the consumer's choice +too, not a convention this repo imposes: `sign-apk.sh` and `fdroid-update.sh` +each take the name of the environment variable to read a password from as an +argument, specifically so two different keystores in the same job (the APK +key and the separate repo index key -- see below) never have to share one +variable name. + +## One `versionCode` rule, shared by CI and a manual build (fdroid-repo-ci#6) + +Two implementations of "the next version code" is the exact failure mode +`next-version-code.sh` exists to avoid, so both CI and a manual build call +this one script rather than each computing their own: + +``` +candidate = whole minutes since 2026-01-01T00:00:00Z +published = highest versionCode already in the repo index for this applicationId +result = max(candidate, published + 1) +``` + +Minutes, not seconds or raw Unix time: `versionCode` is a signed 32-bit int, +and this scheme does not exhaust it for 4,082 years, against 67 years for +seconds and 11 for Unix time. The clamp against `published` exists because +minute granularity means two builds started in the same minute would +otherwise compute the same number -- and it also covers a clock stepping +backwards or an old commit being rebuilt, both of which would otherwise +produce a number lower than one a device has already seen. + +**The clamp is monotonic against *observed* state, not monotonic by +construction.** It does not close the race where two jobs start in the same +minute, both read the same `published` value, both compute the same next +versionCode, and both build and publish -- the window is the whole build, +not just the clock read, since the version code has to be baked into the APK +before it exists. Nothing in `next-version-code.sh` can prevent that. +`publish.sh`'s APK-existence check (below) is the other half of this: it +cannot stop the wasted build, but it turns the collision from a silent, +permanent wrong state into a loud failure at the last moment, when the +information is finally reliable. + +Takes an index-v2.json URL or local file path, and an application id; +prints the integer and nothing else, so a caller does +`versionCode=$(next-version-code.sh "$URL" "$APPLICATION_ID")`. The URL form +needs no credentials, since both this repo and the F-Droid repo it reads are +public -- the point of weblib-viewer#16's "buildable when CI is down". Fails +loudly (non-zero exit, nothing on stdout) if the index cannot be read; an +application id with no entry at all in the index is not an error -- that is +simply the first-ever publish for a channel, and yields the bare candidate. + +## Publish ordering and safety (fdroid-repo-ci#5, fdroid-repo-ci#6) + +An F-Droid repo is a signed index plus the files it names. `publish.sh` +uploads in two passes, unconditionally: every non-index file first, the +index (`index-v1.json(.asc)`, `index-v2.json`, `entry.json`, `entry.jar`, +matched only at the top level of the tree) last. A publish interrupted +anywhere in the first pass leaves the repo describing the old state a little +longer; it can never leave an index naming files that 404. Credentials are +verified with a real authenticated call (`head-bucket`) before either pass, +rather than merely checking that the environment variables are non-empty -- +an unusable-but-present credential failing halfway through is exactly how a +partial (and potentially index-first) upload happens. + +Before pass 1, `publish.sh` also checks every local `*.apk`'s destination +key with `head-object` and refuses the whole publish if any of them already +exists -- the safeguard `next-version-code.sh`'s clamp cannot provide on its +own (see above). This does not prevent the wasted build behind a same-minute +collision; it prevents the collision from silently corrupting the repo. +Skipped entirely for a plain `--index-only` invocation (below), since that +pass can never upload a `*.apk` in the first place. + +### `--files-only` and `--index-only` + +The two passes can also be run as two separate invocations: + +```bash +bash publish.sh --files-only "$FDROID_REPO_ROOT/repo" "$BUCKET" "$S3_ENDPOINT_URL" +# ... build finishes, or a later job resumes here ... +bash publish.sh --index-only "$FDROID_REPO_ROOT/repo" "$BUCKET" "$S3_ENDPOINT_URL" +``` + +With neither flag, both passes run in one call, files then index -- the +default, and the only way to reach "index before files" is to invoke +`--index-only` on its own, which is opt-in misuse, never the default. This +is real, independently useful behaviour, not just a testing seam: an +interrupted publish can resume at the index step without re-uploading every +APK, and a caller can stage files now and publish the index later. + +It is also what makes the ordering guarantee testable **by construction**: +run `--files-only`, assert the bucket has the file and not the index yet, +run `--index-only`, assert the index then appears -- two genuinely separate +invocations, with the bucket inspected for real in between. An earlier +version of this script proved the same property with a same-process +synchronization hook that `eval`'d an environment variable between the two +passes; that was removed in favour of these flags; a script that runs with +a release signing key and S3 credentials in its environment, in a public +repository, should not carry a documented path from env-var influence to +arbitrary code execution, however unlikely, when a real feature closes the +same gap for free. ## Align before sign, and how "no `zipalign`" was verified @@ -188,12 +288,22 @@ here needs, resolved through the same pin. bash run_tests.sh ``` -42 assertions across five suites, ~20s on a warm nix store: +91 assertions across seven suites offline (~20s on a warm nix store, 4 +deliberately skipped -- see below), plus 13 more that only run against a +real bucket: * `test_check_zip_alignment.py` -- pure standard library, no nix, no network at all; builds an aligned and a misaligned fixture zip by hand and checks both directions, including the 4096-byte boundary specific to `.so` entries. +* `test_next_version_code.py` -- pure standard library too, testing + `next_version_code.py` as a subprocess against fixture index-v2.json files + written to a temp dir: no prior build, a lower/equal/higher prior (the + clock-went-backwards case), several versions at once, a version entry with + no versionCode, an application id missing from the index entirely (not an + error), and an unreadable/malformed index (fails loudly, nothing on + stdout). "now" is pinned via an undocumented env var seam so the expected + candidate is a fixed number. * `test_with_nixpkgs.sh` -- offline; every way the pinning wrapper is supposed to refuse rather than silently degrade (missing pin, malformed pin, too few arguments). Does not exercise the happy path of actually @@ -205,14 +315,48 @@ bash run_tests.sh against a throwaway keystore generated fresh per run, real `apksigner` and `fdroid update`. Needs the pinned toolchain to be resolvable (locally cached, or fetched once over the network); nothing beyond that. -* `test_publish.sh` -- argument/credential validation and `--dry-run` - command construction, fully offline, plus one real invocation against a - closed local port (`127.0.0.1:1`, nothing listens there) to prove the - built command genuinely attempts a network call rather than failing on its - own arguments. The actual upload path is out of scope for an offline - suite, matching the constraint that only `publish.sh` needs network in the +* `test_next_version_code.sh` -- the real `next-version-code.sh` CLI, through + `with-nixpkgs.sh`: argument count, a local index file end to end, a + missing application id, a missing/malformed local file, and an + unreachable URL (`http://127.0.0.1:1`, the same closed-port trick + `test_publish.sh` uses) -- proving the wrapper's argv/exit-code/stdout + contract, not just the Python module's arithmetic. +* `test_publish.sh` -- argument/credential validation, `--dry-run` command + construction (both the default two-command form, files then index, and + each of `--files-only`/`--index-only` printing exactly one command, plus + their mutual exclusivity), and a real invocation against a closed local + port (`127.0.0.1:1`), fully offline. Also runs `test_s3_stub.py`, a + same-repo, HEAD-only local HTTP stub, to prove the credential check is a + real authenticated call (a wrong bucket is rejected), that an + already-published APK is refused before any upload, that `--index-only` + alone never runs that check at all (it cannot upload an APK), and a + negative control proving a non-colliding APK does not trip the check. The + actual upload path -- and the two-pass *ordering* of real uploads, which + nothing offline can see -- is exercised only by the opt-in live round trip + below, matching the constraint that only `publish.sh` needs network in the first place. +`test_publish.sh`'s live section (skipped by default; needs +`FDROID_CI_LIVE_BUCKET`, `FDROID_CI_LIVE_ENDPOINT` and real credentials) adds +13 more assertions against a real bucket: + +* the round trip itself, both objects reading back byte-identical; +* upload ordering proven **by construction**: `--files-only` run alone, + then the bucket inspected for real (file present, index absent), then + `--index-only` run alone, then the bucket inspected again (index now + present) -- two genuinely separate invocations, nothing racing, no + synchronization hook, no `eval` (see "`--files-only` and `--index-only`" + above for why that matters in a public repo); +* a fault-injection contrast proving the flags have real teeth: invoking + `--index-only` **alone**, against a prefix `--files-only` never touched, + really does publish an index naming a file that is not there -- the exact + state the whole ordering guarantee exists to prevent -- while the + *default* path (neither flag, one call) never leaves that state, which + holds by construction (the default's first phase is the identical command + array `--files-only` runs) and is checked here by its final state; +* a second publish refused when it would overwrite a real, already-published + APK, and that APK's bytes unchanged afterwards. + No `.gitea/workflows` runs this, on the same reasoning weblib-ci#9 recorded: run it by hand before pushing. If that changes for weblib-ci it should probably change here too, at the same time. diff --git a/action.yml b/action.yml index c37f173..208bce0 100644 --- a/action.yml +++ b/action.yml @@ -1,5 +1,5 @@ -# Makes sign-apk.sh, fdroid-update.sh, publish.sh and with-nixpkgs.sh -# available to a workflow and reports where they are. +# Makes sign-apk.sh, fdroid-update.sh, publish.sh, next-version-code.sh and +# with-nixpkgs.sh available to a workflow and reports where they are. # # Consume it with the **full URL**, the same reason recorded in weblib-ci's # README: `uses: owner/repo@main` resolves against the instance's default @@ -17,15 +17,17 @@ name: F-Droid repo publishing tools description: >- Shared, key-free tooling to align-check and sign a prebuilt APK, run - `fdroid update` over a binary repo, and publish the result to an - S3-compatible bucket. The caller supplies the keys, the bucket and the - APK; this action supplies the pinned toolchain and the scripts. + `fdroid update` over a binary repo, compute the next monotonic versionCode, + and publish the result to an S3-compatible bucket. The caller supplies the + keys, the bucket and the APK; this action supplies the pinned toolchain and + the scripts. outputs: path: description: >- Directory holding with-nixpkgs.sh, nixpkgs-pin, sign-apk.sh, - fdroid-update.sh and publish.sh. + fdroid-update.sh, next-version-code.sh, next_version_code.py and + publish.sh. value: ${{ github.action_path }} runs: @@ -34,6 +36,7 @@ runs: - shell: bash run: | echo "fdroid-repo-ci tools at ${{ github.action_path }}" - for f in with-nixpkgs.sh nixpkgs-pin sign-apk.sh fdroid-update.sh publish.sh; do + for f in with-nixpkgs.sh nixpkgs-pin sign-apk.sh fdroid-update.sh \ + next-version-code.sh next_version_code.py publish.sh; do test -f "${{ github.action_path }}/$f" done diff --git a/next-version-code.sh b/next-version-code.sh new file mode 100755 index 0000000..9d2df65 --- /dev/null +++ b/next-version-code.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash +# +# One monotonic versionCode rule, shared by CI and manual builds +# (fdroid-repo-ci#6). Two implementations of "the next version code" is the +# whole failure mode this avoids, so both paths call this one script rather +# than each computing their own. +# +# next-version-code.sh +# +# Prints the integer to stdout and nothing else: +# +# versionCode=$(bash next-version-code.sh "$INDEX_URL" "$APPLICATION_ID") +# +# The index source may be an http(s) URL -- what both CI and a manual build +# use in practice, and needs no credentials, since this repo and the F-Droid +# repo it reads are both public (weblib-viewer#16: buildable when CI is +# down) -- or a local file path, which is mainly what the offline test suite +# uses but is equally valid for a build that already has the index checked +# out. +# +# Fails loudly (non-zero exit, message on stderr, nothing on stdout) if the +# index cannot be fetched or parsed -- see next_version_code.py for why that +# is never allowed to fall back to a guess. An index with no entry at all for +# this application id is NOT an error: that is a normal first-ever publish +# for a channel, and yields the bare candidate. +# +# The rule itself, and why each part of it is shaped this way, lives in +# next_version_code.py's own docstring -- read that, not this file, for the +# reasoning. This script only resolves the pinned interpreter and forwards +# arguments; everything that can be unit-tested without nix lives in the +# stdlib-only Python module next to it. +# +# Pins python3 (stdlib json/urllib/datetime/argparse does the rest -- no +# other package) via with-nixpkgs.sh/nixpkgs-pin, next to this script. +set -euo pipefail + +here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) + +if [ "$#" -ne 2 ]; then + cat >&2 <<'USAGE' +usage: next-version-code.sh +USAGE + exit 2 +fi + +exec bash "$here/with-nixpkgs.sh" python3 python3 "$here/next_version_code.py" "$@" diff --git a/next_version_code.py b/next_version_code.py new file mode 100755 index 0000000..07b5656 --- /dev/null +++ b/next_version_code.py @@ -0,0 +1,124 @@ +#!/usr/bin/env python3 +"""Compute the next monotonic Android versionCode for an application id. + +Implements the rule from fdroid-repo-ci#6 -- read that issue for the reasoning +behind each part; this is only the arithmetic and the index read. + + candidate = whole minutes since 2026-01-01T00:00:00Z + published = highest versionCode already in the index for this app id + result = max(candidate, published + 1) + +Minutes, not seconds or raw Unix time: versionCode is a signed 32-bit int, +and minutes-since-2026 does not exhaust it for 4,082 years. The clamp against +`published` is not optional -- two builds inside the same minute would +otherwise compute the same candidate, and it also covers a clock stepping +backwards or an old commit being rebuilt, both of which would otherwise emit +a number lower than one a device has already seen. + +Reads `packages..versions.*.manifest.versionCode`, the shape +fdroidserver's own index.py actually writes (`packages` keyed by +applicationId/packageName; `versions` a dict keyed by each APK's sha256, not +a list; the versionCode nested under that version's `manifest`) -- verified +against fdroidserver's source, not assumed from an older v1-shaped example. + +An application id absent from the index entirely is not an error: it is the +first-ever publish for a channel, and "published" is then simply nobody, so +the bare candidate is correct. An unreadable or unparseable index IS an +error -- guessing a number when the published state is unknown is exactly +how a duplicate or a decrease gets published. + +Standard library only, deliberately -- no third-party dependency for a script +that reads a signed index. `urllib.request` handles the http(s) case; a local +path is read directly, which is what makes the offline test suite possible +(fixture index-v2.json files, no server, no network). +""" +import argparse +import datetime +import json +import os +import sys +import urllib.request + +EPOCH = datetime.datetime(2026, 1, 1, 0, 0, 0, tzinfo=datetime.timezone.utc) + + +def minutes_since_epoch(now=None): + now = now if now is not None else datetime.datetime.now(datetime.timezone.utc) + return int((now - EPOCH).total_seconds() // 60) + + +def load_index(source, timeout=30): + """source is an http(s) URL or a local file path to index-v2.json.""" + if source.startswith("http://") or source.startswith("https://"): + with urllib.request.urlopen(source, timeout=timeout) as resp: + data = resp.read() + else: + with open(source, "rb") as f: + data = f.read() + return json.loads(data) + + +def highest_published_version_code(index, application_id): + """The highest versionCode already published for application_id, or None + if the index has no entry for it at all (a normal state: the first + publish for this channel).""" + packages = index.get("packages") or {} + package = packages.get(application_id) + if not package: + return None + versions = package.get("versions") or {} + codes = [ + manifest["versionCode"] + for version in versions.values() + if isinstance((manifest := (version or {}).get("manifest")), dict) + and isinstance(manifest.get("versionCode"), int) + ] + return max(codes) if codes else None + + +def next_version_code(source, application_id, now=None): + index = load_index(source) + candidate = minutes_since_epoch(now) + published = highest_published_version_code(index, application_id) + if published is None: + return candidate + return max(candidate, published + 1) + + +def main(argv): + parser = argparse.ArgumentParser( + description="Print the next monotonic versionCode for an application id." + ) + parser.add_argument("index_source", help="index-v2.json URL or local path") + parser.add_argument("application_id") + args = parser.parse_args(argv) + + # Testing seam only, deliberately undocumented in the usage banner: lets + # the offline suite pin "now" so `candidate` is a fixed, known number + # instead of whatever minute the test happens to run in. Unset in every + # real invocation, so production behaviour is untouched. + now = None + now_override = os.environ.get("NEXT_VERSION_CODE_NOW") + if now_override: + now = datetime.datetime.fromisoformat(now_override) + if now.tzinfo is None: + now = now.replace(tzinfo=datetime.timezone.utc) + + try: + result = next_version_code(args.index_source, args.application_id, now=now) + except (OSError, ValueError) as e: + # OSError covers both a local file that can't be read and a network + # failure (urllib.error.URLError is a subclass); ValueError covers + # malformed JSON. Fail loudly on stderr, print nothing on stdout -- + # a caller doing `versionCode=$(next-version-code.sh ...)` must never + # capture a number that was actually an error message. + print(f"next-version-code.py: could not read the index at " + f"{args.index_source!r}: {e}", file=sys.stderr) + return 1 + + print(result) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/publish.sh b/publish.sh index 16e3be6..e4d866e 100755 --- a/publish.sh +++ b/publish.sh @@ -17,45 +17,158 @@ # backend that ignores it; set AWS_DEFAULT_REGION if the endpoint cares, # otherwise the placeholder below is never actually consulted. # +# ## Ordering (fdroid-repo-ci#5) +# +# An F-Droid repo is a signed index plus the files it names. A partial +# upload can leave them disagreeing in two ways: the index lands first and +# names files that still 404 (breaks the repo for every client until the +# upload finishes or is rolled back), or the files land first and the index +# is not yet updated (harmless -- the new build is merely not visible yet). +# Only the first is dangerous, and `aws s3 sync` walking a tree in its own +# order gives no protection against it. So this script uploads in two +# passes, unconditionally: every non-index file first, the index itself +# last. The repo is therefore always internally consistent -- before a +# publish it still describes the old state, after it describes the new one, +# and an interruption anywhere in pass 1 simply leaves it describing the old +# state a little longer, never a mixture. +# +# "The index" here is index-v1.json(.asc), index-v2.json, entry.json and +# entry.jar -- fdroidserver's actual top-level index/entry file names, +# matched only at the top level of the tree (a nested path like +# diff/.json is not "the index" and travels with the ordinary files; +# it is only ever reached by a client that already has last publish's index +# and is willing to look for it). +# +# The two passes are also separately invokable, with `--files-only` and +# `--index-only`. This is not a test seam -- it is real, useful on its own: +# an interrupted publish can resume at the index step without re-uploading +# every APK, and a caller can stage files now and publish the index later. +# It is also what makes the ordering guarantee testable *by construction*: +# run `--files-only`, assert the bucket has the files and not the index, run +# `--index-only`, assert the index appears -- no synchronization hook and no +# `eval` of anything env-supplied needed in a script that runs with a +# signing key and S3 credentials in its environment, in a public repo. (An +# earlier version of this script had exactly such a hook; it was removed +# once these flags made it unnecessary.) With neither flag, both passes run, +# files then index, which is the safe default and is unconditional -- there +# is no way to select "index then files" short of invoking `--index-only` +# by hand before ever running `--files-only`, which is the deliberate +# misuse the two flags exist to make possible for cases that want it +# (resuming, staging) while never being the default. +# +# ## Credentials are verified before anything is uploaded (fdroid-repo-ci#5) +# +# Checking that AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY are merely *set* does +# not mean they are *usable* -- an unusable-but-present credential failing +# halfway through is exactly how a partial (and, per the ordering point +# above, potentially index-first) upload happens. So before pass 1 this +# script makes one real authenticated call (`head-bucket`) and aborts if it +# fails, rather than discovering that the credentials do not work partway +# through a sync. +# +# ## Refusing to overwrite an existing APK (fdroid-repo-ci#6) +# +# next-version-code.sh's clamp is monotonic against *observed* state, not +# monotonic by construction -- it does not close the race where two jobs +# start in the same minute, both read the same `published` value, both +# compute the same next versionCode, and both build and publish. The window +# is the whole build, not just the clock read, because the version code is +# baked into the APK before it exists. Nothing in this script can prevent +# that wasted build. What it CAN do -- and is required to do, fdroid-repo-ci#6 +# -- is turn the collision from a silent, permanent wrong state (two +# different APKs claiming one versionCode, with clients pinned to whichever +# they happened to fetch first) into a loud failure of the second job, at +# the last moment when the information is finally reliable: immediately +# before that job would otherwise upload over the first job's APK. An +# F-Droid repo names an APK `_.apk`, so the +# collision this guards against is exactly a name collision -- every local +# *.apk file's destination key is checked with `head-object` before pass 1, +# and the whole publish aborts if any of them is already there. This runs +# for every *.apk regardless of whether it actually collides, because there +# is no cheaper way to know in advance which ones might. +# +# Note this also means retrying a publish that already got partway through +# uploading its own APKs (e.g. pass 1 died mid-sync from a network drop) +# will now itself be refused on retry, indistinguishable at the object level +# from a genuine concurrent collision. That is an accepted cost of the same +# trade: this check cannot tell "the same job resuming" from "a different +# job that computed the same versionCode" without hashing every object, +# which was judged not worth the added complexity here. Recovering from that +# case is a fresh publish with a new versionCode, or deleting the partial +# upload by hand. +# # Uses `aws s3 sync`, not `cp --recursive`: re-publishing re-uploads only # what changed. Does **not** pass `--delete` by default -- an F-Droid repo # normally keeps every APK it has ever served (older index entries can still # point at them), so silently pruning the remote on every publish is the # wrong default. Pass `--delete` as an extra argument if a caller genuinely -# wants the remote pruned to match local. +# wants the remote pruned to match local; it is forwarded to *both* passes, +# which is safe because `aws s3 sync --delete` never deletes anything that +# was excluded by that pass's own --exclude/--include filters -- so the +# files-pass never touches an index object and the index-pass never touches +# a file object, regardless of --delete. # # `fdroid update` needs no network; this is the one script in this repo that # does (fdroid-repo-ci#1), and is accordingly the one script whose happy path # the test suite cannot exercise against a real bucket. `--dry-run` prints -# the exact command this script would run -- including the `with-nixpkgs.sh` -# pin -- and exits without touching the network, which is what the test -# suite asserts against. (A PATH-stubbed `aws` cannot do this job: `nix +# the command(s) this script would run for whichever pass(es) are selected, +# in order -- including the `with-nixpkgs.sh` pin -- and exits without +# touching the network at all: no credential check, no APK-existence check, +# no sync. That is what the test suite asserts against for command +# construction and ordering. (A PATH-stubbed `aws` cannot do this job: `nix # shell` puts the packages it was asked for at the *front* of PATH, ahead of -# anything already there, so a stub earlier on the caller's PATH would simply -# be shadowed by the real `aws` from awscli2.) Separately, the test suite -# also runs this for real against a closed local port, to confirm the -# constructed command is well-formed and genuinely attempts a network call -# rather than failing on argument parsing. +# anything already there, so a stub earlier on the caller's PATH would +# simply be shadowed by the real `aws` from awscli2.) The credential check, +# the APK-existence check and the two-pass ordering are each proven +# separately: a real invocation against a closed local port for "fails on +# the network, and fails during the credential check specifically, before +# any upload is attempted"; a tiny local HEAD-only HTTP stub for "an +# existing APK is refused, and only an existing APK", offline; and the +# opt-in live round trip against a real bucket, run once as `--files-only` +# then `--index-only` to observe the ordering by construction, and once +# more as a real head-to-head APK collision. # # Pins awscli2 via with-nixpkgs.sh/nixpkgs-pin, next to this script. set -euo pipefail here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) -if [ "$#" -lt 3 ]; then +usage() { cat >&2 <<'USAGE' -usage: publish.sh [--dry-run] [] [extra aws s3 sync args...] +usage: publish.sh [--dry-run] [--files-only | --index-only] \ + [] [extra aws s3 sync args...] + +With neither --files-only nor --index-only, both passes run: every +non-index file first, then the index -- the safe default, and the only way +to select "index before files" is to invoke --index-only by hand ahead of +--files-only, never the default. --files-only and --index-only are mutually +exclusive. Credentials: AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY (and optionally AWS_SESSION_TOKEN) must already be exported -- never passed as arguments. USAGE +} + +dry_run=false +files_only=false +index_only=false +while [ "$#" -gt 0 ]; do + case "$1" in + --dry-run) dry_run=true; shift ;; + --files-only) files_only=true; shift ;; + --index-only) index_only=true; shift ;; + *) break ;; + esac +done + +if [ "$files_only" = true ] && [ "$index_only" = true ]; then + echo "publish.sh: --files-only and --index-only are mutually exclusive" >&2 exit 2 fi -dry_run=false -if [ "$1" = "--dry-run" ]; then - dry_run=true - shift +if [ "$#" -lt 3 ]; then + usage + exit 2 fi local_dir=$1 @@ -79,6 +192,7 @@ dest="s3://$bucket" if [ -n "$prefix" ]; then dest="$dest/${prefix#/}" fi +region="${AWS_DEFAULT_REGION:-us-east-1}" # awscli2 2.23+ attaches a checksum algorithm to every PutObject by default. # Garage rejects it outright -- measured against a real bucket: @@ -98,16 +212,78 @@ fi export AWS_REQUEST_CHECKSUM_CALCULATION="${AWS_REQUEST_CHECKSUM_CALCULATION:-when_required}" export AWS_RESPONSE_CHECKSUM_VALIDATION="${AWS_RESPONSE_CHECKSUM_VALIDATION:-when_required}" -cmd=(bash "$here/with-nixpkgs.sh" awscli2 aws s3 sync "$local_dir" "$dest" - --endpoint-url "$endpoint" - --region "${AWS_DEFAULT_REGION:-us-east-1}" - "${extra_args[@]}") +aws_base=(bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$endpoint" --region "$region") + +# Top-level index/entry file names only -- see the "Ordering" comment above +# for why a nested path such as diff/.json is deliberately not matched. +index_exclude_filters=(--exclude "index*" --exclude "entry.*") +index_only_filters=(--exclude "*" --include "index*" --include "entry.*") + +cmd_files=("${aws_base[@]}" s3 sync "$local_dir" "$dest" + "${index_exclude_filters[@]}" "${extra_args[@]}") +cmd_index=("${aws_base[@]}" s3 sync "$local_dir" "$dest" + "${index_only_filters[@]}" "${extra_args[@]}") + +# Which pass(es) this invocation runs. Unconditional and exactly one of +# three shapes: both (the default), files alone, or index alone -- the +# mutual-exclusivity check above already ruled out both flags together. +run_files=true +run_index=true +[ "$index_only" = true ] && run_files=false +[ "$files_only" = true ] && run_index=false if [ "$dry_run" = true ]; then - printf '%q ' "${cmd[@]}" - printf '\n' + if [ "$run_files" = true ]; then + printf '%q ' "${cmd_files[@]}" + printf '\n' + fi + if [ "$run_index" = true ]; then + printf '%q ' "${cmd_index[@]}" + printf '\n' + fi exit 0 fi -echo "publish.sh: syncing $local_dir -> $dest (endpoint $endpoint)" -exec "${cmd[@]}" +echo "publish.sh: verifying credentials against $endpoint" +if ! verify_out=$("${aws_base[@]}" s3api head-bucket --bucket "$bucket" 2>&1); then + echo "publish.sh: credentials rejected (or bucket unreachable) -- nothing was uploaded:" >&2 + echo "$verify_out" >&2 + exit 1 +fi + +if [ "$run_files" = true ]; then + # Refuse to overwrite an existing APK -- see the comment block above for + # why this exists and what it does and does not close. Every local *.apk + # is checked, regardless of nesting, since fdroidserver's binary-repo + # layout keeps them under repo/ directly but nothing here should assume + # that. Skipped entirely for a plain --index-only invocation: the index + # pass never uploads a *.apk (index_only_filters excludes everything but + # the index/entry names), so there is nothing this check could catch. + while IFS= read -r -d '' apk; do + rel=${apk#"$local_dir"/} + key="$rel" + if [ -n "$prefix" ]; then + key="${prefix#/}/$rel" + fi + if head_out=$("${aws_base[@]}" s3api head-object --bucket "$bucket" --key "$key" 2>&1); then + echo "publish.sh: refusing to publish -- $key already exists in s3://$bucket (would overwrite an existing APK, fdroid-repo-ci#6)" >&2 + exit 1 + elif echo "$head_out" | grep -qi '404\|not found\|NoSuchKey'; then + : # does not exist yet -- fine, this is the common case + else + echo "publish.sh: could not check whether $key already exists -- refusing to guess:" >&2 + echo "$head_out" >&2 + exit 1 + fi + done < <(find "$local_dir" -type f -name '*.apk' -print0) + + echo "publish.sh: pass 1/2: uploading files (index deferred) $local_dir -> $dest" + "${cmd_files[@]}" +fi + +if [ "$run_index" = true ]; then + echo "publish.sh: pass 2/2: uploading the index $local_dir -> $dest" + "${cmd_index[@]}" +fi + +echo "publish.sh: done" diff --git a/run_tests.sh b/run_tests.sh index 56a8484..aed150b 100755 --- a/run_tests.sh +++ b/run_tests.sh @@ -7,11 +7,13 @@ # # bash run_tests.sh # -# check_zip_alignment.py's own suite needs only python3 (via with-nixpkgs.sh, -# since none is assumed to be on PATH); everything else additionally needs -# apksigner/fdroidserver/jdk21/awscli2, resolved through the same pin. All of -# it stays on loopback/local-store -- nothing here reaches a real host, -# bucket or key. +# check_zip_alignment.py's and next_version_code.py's own suites need only +# python3 (via with-nixpkgs.sh, since none is assumed to be on PATH); +# everything else additionally needs apksigner/fdroidserver/jdk21/awscli2, +# resolved through the same pin. All of it stays on loopback/local-store -- +# nothing here reaches a real host, bucket or key, except the opt-in live +# section inside test_publish.sh, which stays skipped unless +# FDROID_CI_LIVE_BUCKET/FDROID_CI_LIVE_ENDPOINT and real credentials are set. set -uo pipefail here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) @@ -21,9 +23,10 @@ total_fail=0 any_hard_fail=0 run_python_suite() { - echo "=== test_check_zip_alignment.py ===" + local script=$1 + echo "=== $script ===" local out - if out=$(bash "$here/with-nixpkgs.sh" python3 python3 "$here/test_check_zip_alignment.py" -v 2>&1); then + if out=$(bash "$here/with-nixpkgs.sh" python3 python3 "$here/$script" -v 2>&1); then local n n=$(echo "$out" | grep -oE '^Ran [0-9]+ test' | grep -oE '[0-9]+') total_pass=$((total_pass + ${n:-0})) @@ -59,10 +62,12 @@ run_shell_suite() { echo } -run_python_suite +run_python_suite test_check_zip_alignment.py +run_python_suite test_next_version_code.py run_shell_suite test_with_nixpkgs.sh run_shell_suite test_sign_apk.sh run_shell_suite test_fdroid_update.sh +run_shell_suite test_next_version_code.sh run_shell_suite test_publish.sh echo "====================================" diff --git a/test_next_version_code.py b/test_next_version_code.py new file mode 100755 index 0000000..1aab1d6 --- /dev/null +++ b/test_next_version_code.py @@ -0,0 +1,174 @@ +#!/usr/bin/env python3 +"""Offline tests for next_version_code.py (fdroid-repo-ci#6). + +Standard library only, no network, no nix -- like test_check_zip_alignment.py, +this is the piece of the change that needs none of the pinned toolchain. + +Runs next_version_code.py as a *subprocess* against fixture index-v2.json +files written to a temp dir, the same reasoning test_check_zip_alignment.py +gives: a test that only imports the function would miss an argv/exit-code/ +stdout-pollution regression, and "prints the result; do nothing else" is +itself part of the contract (README/issue #6: a caller does +`versionCode=$(next-version-code.sh ...)`, so stray stdout output would be +silently captured as part of the number). + +"now" is pinned via the NEXT_VERSION_CODE_NOW testing seam (see the script's +main()) so `candidate` is a fixed, known integer instead of whatever minute +the suite happens to run in. + +The property every case after the first two actually defends -- and the one +a future refactor could quietly break while still "looking" correct -- is +that the result is *strictly greater than every versionCode already in the +fixture*, not merely equal to some formula. Each clamp case asserts that +directly, not just the arithmetic. +""" +import datetime +import json +import os +import subprocess +import sys +import tempfile +import unittest + +HERE = os.path.dirname(os.path.abspath(__file__)) +SCRIPT = os.path.join(HERE, "next_version_code.py") +APP_ID = "de.example.app" + +EPOCH = datetime.datetime(2026, 1, 1, tzinfo=datetime.timezone.utc) +# An arbitrary, fixed "now": 100 minutes after the epoch this script uses, +# so the expected candidate (100) is known without re-deriving the formula +# under test. +FIXED_NOW = (EPOCH + datetime.timedelta(minutes=100)).isoformat() +CANDIDATE_AT_FIXED_NOW = 100 + + +def index_with_versions(application_id, version_codes): + """Build an index-v2.json fixture with one package with each versionCode + in a separate version entry, matching fdroidserver's own shape: + packages..versions..manifest.versionCode.""" + versions = {} + for i, vc in enumerate(version_codes): + versions[f"fakehash{i}"] = {"manifest": {"versionCode": vc}} + return {"packages": {application_id: {"versions": versions}}} + + +class NextVersionCodeTest(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + + def _write_index(self, name, obj_or_text): + path = os.path.join(self.tmp.name, name) + with open(path, "w") as f: + if isinstance(obj_or_text, str): + f.write(obj_or_text) + else: + json.dump(obj_or_text, f) + return path + + def _run(self, index_path, application_id=APP_ID, now=FIXED_NOW): + env = dict(os.environ) + if now is not None: + env["NEXT_VERSION_CODE_NOW"] = now + else: + env.pop("NEXT_VERSION_CODE_NOW", None) + return subprocess.run( + [sys.executable, SCRIPT, index_path, application_id], + capture_output=True, text=True, env=env, + ) + + # -- no prior build: the first-ever publish for a channel ------------- + + def test_no_prior_build_yields_bare_candidate(self): + path = self._write_index("empty.json", {"packages": {}}) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, f"{CANDIDATE_AT_FIXED_NOW}\n") + + def test_application_id_missing_entirely_is_not_an_error(self): + path = self._write_index( + "other-app.json", index_with_versions("some.other.app", [5])) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, f"{CANDIDATE_AT_FIXED_NOW}\n") + + # -- the clamp: result must exceed every published code ---------------- + + def test_lower_prior_is_ignored_in_favour_of_the_clock(self): + path = self._write_index("lower.json", index_with_versions(APP_ID, [1, 2, 3])) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + self.assertEqual(got, CANDIDATE_AT_FIXED_NOW) + self.assertGreater(got, 3) + + def test_equal_prior_is_bumped_by_one(self): + # published == candidate: the same-minute collision case. The clamp + # must still move strictly past it. + path = self._write_index( + "equal.json", index_with_versions(APP_ID, [CANDIDATE_AT_FIXED_NOW])) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + self.assertGreater(got, CANDIDATE_AT_FIXED_NOW) + self.assertEqual(got, CANDIDATE_AT_FIXED_NOW + 1) + + def test_higher_prior_wins_the_clamp_clock_went_backwards(self): + higher = CANDIDATE_AT_FIXED_NOW + 500 + path = self._write_index("higher.json", index_with_versions(APP_ID, [higher])) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + self.assertGreater(got, higher) + self.assertEqual(got, higher + 1) + + def test_highest_of_several_versions_is_the_one_that_matters(self): + path = self._write_index( + "several.json", index_with_versions(APP_ID, [10, 999, 50])) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + self.assertGreater(got, 999) + + def test_version_entry_without_a_versionCode_is_ignored_not_fatal(self): + # A version whose manifest lacks versionCode (or has no manifest at + # all) must not crash the read -- it is simply not a candidate for + # "published". + obj = index_with_versions(APP_ID, [7]) + obj["packages"][APP_ID]["versions"]["nomanifest"] = {} + obj["packages"][APP_ID]["versions"]["emptymanifest"] = {"manifest": {}} + path = self._write_index("partial.json", obj) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + self.assertGreater(got, 7) + + # -- failure modes: fail loudly, never guess --------------------------- + + def test_unreadable_index_fails_loudly(self): + missing = os.path.join(self.tmp.name, "does-not-exist.json") + result = self._run(missing) + self.assertNotEqual(result.returncode, 0) + self.assertEqual(result.stdout, "") # never a guessed number on stdout + self.assertIn("could not read the index", result.stderr) + + def test_malformed_index_fails_loudly(self): + path = self._write_index("malformed.json", "{not valid json") + result = self._run(path) + self.assertNotEqual(result.returncode, 0) + self.assertEqual(result.stdout, "") + self.assertIn("could not read the index", result.stderr) + + # -- output contract: nothing but the integer on stdout ----------------- + + def test_stdout_is_only_the_integer(self): + path = self._write_index("clean.json", {"packages": {}}) + result = self._run(path) + self.assertEqual(result.returncode, 0, result.stderr) + # Exactly one line, and it parses as an int with no surrounding text. + self.assertEqual(int(result.stdout.strip()), CANDIDATE_AT_FIXED_NOW) + self.assertEqual(result.stdout.count("\n"), 1) + + +if __name__ == "__main__": + unittest.main() diff --git a/test_next_version_code.sh b/test_next_version_code.sh new file mode 100755 index 0000000..1a2d07e --- /dev/null +++ b/test_next_version_code.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# +# Shell-level integration test for next-version-code.sh (fdroid-repo-ci#6) -- +# the actual CLI entry point, run the same way sign-apk.sh/fdroid-update.sh +# are tested: through with-nixpkgs.sh, against the pinned interpreter, +# exercising real argv/exit-code/stdout behaviour rather than importing a +# function. The arithmetic itself, and every clamp case, is covered offline +# and without nix by test_next_version_code.py; this file only has to prove +# the wrapper script wires that up correctly, plus the two things only the +# wrapper (not the Python module directly) can be asked about: a local file +# path as the index source, and an unreachable URL as one. +# +# bash test_next_version_code.sh +set -euo pipefail +here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +source "$here/test_lib.sh" + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +APP_ID="de.example.app" + +echo "--- wrong argument count is refused ---" +assert_failure "no arguments" bash "$here/next-version-code.sh" +assert_failure "one argument" bash "$here/next-version-code.sh" "$work/index.json" + +echo "--- a local file path works end to end, through the real CLI ---" +cat > "$work/index.json" < "$work/other-app.json" <<'JSON' +{"packages": {"some.other.app": {"versions": {"h": {"manifest": {"versionCode": 5}}}}}} +JSON +out2=$(bash "$here/next-version-code.sh" "$work/other-app.json" "$APP_ID") +if [[ "$out2" =~ ^[0-9]+$ ]]; then + pass "an application id absent from the index is not an error" +else + fail "an application id absent from the index is not an error (got '$out2')" +fi + +echo "--- a missing local file fails loudly, prints nothing on stdout ---" +out3=$(bash "$here/next-version-code.sh" "$work/does-not-exist.json" "$APP_ID" 2>/tmp/nvc_stderr.$$) && rc=0 || rc=$? +err3=$(cat /tmp/nvc_stderr.$$); rm -f /tmp/nvc_stderr.$$ +if [ "${rc:-0}" -ne 0 ]; then pass "a missing index file exits non-zero"; else + fail "a missing index file exits non-zero (got 0)" +fi +if [ -z "$out3" ]; then pass "a missing index file prints nothing on stdout"; else + fail "a missing index file prints nothing on stdout (got '$out3')" +fi +assert_contains "the error explains the index could not be read" "$err3" "could not read the index" + +echo "--- an unreachable URL fails loudly too, the same way, through the real network path ---" +# port 1 is a reserved, unassigned TCP port -- nothing listens there, so +# this never leaves the loopback interface and needs no real network access, +# the same trick test_publish.sh uses. +out4=$(bash "$here/next-version-code.sh" "http://127.0.0.1:1/index-v2.json" "$APP_ID" 2>/tmp/nvc_stderr2.$$) && rc4=0 || rc4=$? +err4=$(cat /tmp/nvc_stderr2.$$); rm -f /tmp/nvc_stderr2.$$ +if [ "${rc4:-0}" -ne 0 ]; then pass "an unreachable index URL exits non-zero"; else + fail "an unreachable index URL exits non-zero (got 0)" +fi +if [ -z "$out4" ]; then pass "an unreachable index URL prints nothing on stdout"; else + fail "an unreachable index URL prints nothing on stdout (got '$out4')" +fi +assert_contains "the error explains the index could not be read (URL case)" "$err4" "could not read the index" + +echo "--- malformed JSON fails loudly ---" +echo "{not valid json" > "$work/malformed.json" +out5=$(bash "$here/next-version-code.sh" "$work/malformed.json" "$APP_ID" 2>/tmp/nvc_stderr3.$$) && rc5=0 || rc5=$? +err5=$(cat /tmp/nvc_stderr3.$$); rm -f /tmp/nvc_stderr3.$$ +if [ "${rc5:-0}" -ne 0 ]; then pass "malformed JSON exits non-zero"; else + fail "malformed JSON exits non-zero (got 0)" +fi +assert_contains "the error explains the index could not be read (malformed JSON)" "$err5" "could not read the index" + +report_and_exit diff --git a/test_publish.sh b/test_publish.sh index eb3d088..a2b7d6f 100755 --- a/test_publish.sh +++ b/test_publish.sh @@ -2,13 +2,32 @@ # # Tests for publish.sh -- the one script in this repo that needs real # network to do its actual job (fdroid-repo-ci#1), so its happy path against -# a genuine bucket is out of scope here. What this asserts instead: +# a genuine bucket is out of scope for most of this file. What this asserts: # # * argument/credential validation, offline; -# * `--dry-run` builds the exact command a real run would use, offline; +# * `--dry-run` builds the exact command(s) a real run would use for +# whichever of --files-only/--index-only (or neither -> both, files then +# index) is selected, offline; # * a real invocation against a closed local port fails on the network call -# itself, not on argument parsing -- proving the command it built is -# well-formed, without needing a real bucket or reaching beyond loopback. +# itself -- during the credential check specifically, before any upload is +# attempted -- proving both that the constructed command is well-formed +# and that verification genuinely comes first (fdroid-repo-ci#5); +# * a real invocation against a tiny local HEAD-only S3 stub (test_s3_stub.py) +# proves the credential check is a genuine authenticated call (a wrong +# bucket is rejected, not waved through because a variable was merely +# non-empty), that an already-published APK is refused before any upload +# (and that --index-only alone never runs that check at all, since it +# cannot upload an APK) -- and, as a negative control, that a +# *non*-colliding APK does NOT trip the same check (fdroid-repo-ci#6); +# * the opt-in live round trip against a real bucket, extended to prove the +# two-pass ordering **by construction**: `--files-only` then `--index-only` +# as two genuinely separate invocations, with the bucket state asserted in +# between -- no synchronization hook, no `eval` of anything in this public +# repo's production script -- plus the APK-collision refusal against a +# real object, and a deliberate-misuse case proving `--index-only` alone +# really does produce the exact state the ordering guarantee exists to +# prevent (an index naming a file that is not there), which the *default* +# path never reaches. set -euo pipefail here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) @@ -23,6 +42,14 @@ LIVE_SECRET="${AWS_SECRET_ACCESS_KEY:-}" LIVE_REGION="${AWS_DEFAULT_REGION:-}" source "$here/test_lib.sh" +PYTHON="python3" +have_python=false +if command -v python3 >/dev/null 2>&1; then + have_python=true +elif py=$(bash "$here/with-nixpkgs.sh" python3 sh -c 'command -v python3' 2>/dev/null); then + have_python=true +fi + work=$(mktemp -d) trap 'rm -rf "$work"' EXIT mkdir -p "$work/repo" @@ -40,7 +67,7 @@ echo "--- missing local directory is refused ---" assert_failure "publish.sh refuses a non-existent local directory" \ bash "$here/publish.sh" "$work/does-not-exist" mybucket https://s3.example.invalid -echo "--- --dry-run builds the right command, touches no network ---" +echo "--- --dry-run builds two commands, files then index, touches no network ---" out=$(bash "$here/publish.sh" --dry-run "$work/repo" mybucket https://s3.example.invalid myprefix) assert_contains "dry-run names the destination bucket and prefix" "$out" "s3://mybucket/myprefix" assert_contains "dry-run names the endpoint" "$out" "--endpoint-url https://s3.example.invalid" @@ -51,11 +78,66 @@ else pass "dry-run does not silently add --delete" fi +nlines=$(echo "$out" | grep -c 's3 sync' || true) +if [ "$nlines" -eq 2 ]; then + pass "dry-run prints exactly two sync commands (files pass, index pass)" +else + fail "dry-run prints exactly two sync commands (files pass, index pass) (got $nlines)" +fi + +files_line=$(echo "$out" | grep -n 's3 sync' | sed -n '1p' | cut -d: -f1) +index_line=$(echo "$out" | grep -n 's3 sync' | sed -n '2p' | cut -d: -f1) +files_cmd=$(echo "$out" | sed -n "${files_line}p") +index_cmd=$(echo "$out" | sed -n "${index_line}p") + +assert_contains "the first (files) pass excludes index-shaped names" "$files_cmd" "--exclude index" +assert_contains "the first (files) pass excludes entry-shaped names" "$files_cmd" "--exclude entry." +if echo "$files_cmd" | grep -q -- "--include"; then + fail "the first (files) pass has no --include (it did!)" +else + pass "the first (files) pass has no --include" +fi +assert_contains "the second (index) pass includes index-shaped names" "$index_cmd" "--include index" +assert_contains "the second (index) pass includes entry-shaped names" "$index_cmd" "--include entry." +assert_contains "the second (index) pass excludes everything else first" "$index_cmd" "--exclude" +if [ "$files_line" -lt "$index_line" ]; then + pass "the files pass is printed before the index pass -- index really does go last" +else + fail "the files pass is printed before the index pass (files at line $files_line, index at $index_line)" +fi + echo "--- --dry-run with no prefix omits the trailing slash entirely ---" out_noprefix=$(bash "$here/publish.sh" --dry-run "$work/repo" mybucket https://s3.example.invalid) assert_contains "no-prefix dry-run targets the bucket root" "$out_noprefix" "s3://mybucket " -echo "--- a real invocation fails on the network call, not on arguments ---" +echo "--- --files-only and --index-only each print exactly one command, and are mutually exclusive ---" +out_files_only=$(bash "$here/publish.sh" --dry-run --files-only "$work/repo" mybucket https://s3.example.invalid myprefix) +n_files_only=$(echo "$out_files_only" | grep -c 's3 sync' || true) +if [ "$n_files_only" -eq 1 ]; then + pass "--files-only prints exactly one sync command" +else + fail "--files-only prints exactly one sync command (got $n_files_only)" +fi +assert_contains "the --files-only command still excludes index-shaped names" "$out_files_only" "--exclude index" +if echo "$out_files_only" | grep -q -- "--include"; then + fail "--files-only has no --include (it did!)" +else + pass "--files-only has no --include" +fi + +out_index_only=$(bash "$here/publish.sh" --dry-run --index-only "$work/repo" mybucket https://s3.example.invalid myprefix) +n_index_only=$(echo "$out_index_only" | grep -c 's3 sync' || true) +if [ "$n_index_only" -eq 1 ]; then + pass "--index-only prints exactly one sync command" +else + fail "--index-only prints exactly one sync command (got $n_index_only)" +fi +assert_contains "the --index-only command still includes index-shaped names" "$out_index_only" "--include index" + +assert_failure "publish.sh refuses --files-only together with --index-only" \ + bash "$here/publish.sh" --dry-run --files-only --index-only "$work/repo" mybucket https://s3.example.invalid + +echo "--- a real invocation fails on the network call, not on arguments -- and fails during credential verification, before any upload ---" # port 1 is a reserved, unassigned TCP port -- nothing listens there, so this # never leaves the loopback interface and needs no real network access. real_out=$(bash "$here/publish.sh" "$work/repo" mybucket "http://127.0.0.1:1" 2>&1) && rc=0 || rc=$? @@ -63,43 +145,316 @@ if [ "${rc:-0}" -ne 0 ]; then pass "publish.sh against a closed port exits non-z fail "publish.sh against a closed port exits non-zero (got 0)" fi assert_contains "the failure is a connection error, not an argument error" "$real_out" "Could not connect" +assert_contains "publish.sh announces credential verification" "$real_out" "verifying credentials" +if echo "$real_out" | grep -q "pass 1/2"; then + fail "publish.sh must not start pass 1 when credential verification never completed (it did!)" +else + pass "publish.sh never starts pass 1 when credential verification fails" +fi + +# --- offline, against a local HEAD-only S3 stub ----------------------------- +# Real S3 semantics (200/404 over HTTP, path-style addressing since the +# endpoint host here is a literal IP) without a real bucket or real +# credentials -- see test_s3_stub.py's own docstring for exactly what it +# does and does not implement, and why that is enough for these two checks +# without ever letting publish.sh reach `aws s3 sync`. +if [ "$have_python" = true ]; then + start_stub() { + # start_stub [ ...] -- prints the chosen port on + # stdout, leaves the server running in the background; caller kills $stub_pid. + local logf + logf=$(mktemp) + bash "$here/with-nixpkgs.sh" python3 python3 "$here/test_s3_stub.py" "$@" \ + >"$logf" 2>&1 & + stub_pid=$! + for _ in $(seq 1 50); do + [ -s "$logf" ] && break + sleep 0.1 + done + stub_port=$(head -1 "$logf") + stub_log=$logf + } + + echo "--- credential verification is a real authenticated call, not just 'is it set' ---" + start_stub realbucket + out=$(bash "$here/publish.sh" "$work/repo" wrongbucket "http://127.0.0.1:$stub_port" 2>&1) && rc=0 || rc=$? + kill "$stub_pid" 2>/dev/null || true + wait "$stub_pid" 2>/dev/null || true + if [ "${rc:-0}" -ne 0 ]; then + pass "publish.sh refuses a bucket the stub does not recognise (bad/unusable credentials)" + else + fail "publish.sh refuses a bucket the stub does not recognise (bad/unusable credentials) (got exit 0)" + fi + assert_contains "the rejection is reported before any upload" "$out" "credentials rejected" + if echo "$out" | grep -q "pass 1/2"; then + fail "publish.sh must not start pass 1 after a rejected credential check (it did!)" + else + pass "publish.sh never starts pass 1 after a rejected credential check" + fi + + echo "--- an already-published APK is refused, before any upload (fdroid-repo-ci#6) ---" + collide_dir="$work/collide" + mkdir -p "$collide_dir" + echo '{}' > "$collide_dir/index-v2.json" + echo 'unsigned bytes racing the real one' > "$collide_dir/de.example.app_1.apk" + start_stub mybucket "myprefix/de.example.app_1.apk" + out=$(bash "$here/publish.sh" "$collide_dir" mybucket "http://127.0.0.1:$stub_port" myprefix 2>&1) && rc=0 || rc=$? + kill "$stub_pid" 2>/dev/null || true + wait "$stub_pid" 2>/dev/null || true + if [ "${rc:-0}" -ne 0 ]; then + pass "publish.sh refuses to publish over an already-existing APK" + else + fail "publish.sh refuses to publish over an already-existing APK (got exit 0)" + fi + assert_contains "the refusal names the colliding key" "$out" "myprefix/de.example.app_1.apk" + assert_contains "the refusal explains why" "$out" "already exists" + if echo "$out" | grep -q "pass 1/2"; then + fail "publish.sh must not start pass 1 after refusing a colliding APK (it did!)" + else + pass "publish.sh never starts pass 1 after refusing a colliding APK" + fi + + echo "--- negative control: a non-colliding APK does not trip the same check ---" + start_stub mybucket # nothing pre-exists + out=$(bash "$here/publish.sh" "$collide_dir" mybucket "http://127.0.0.1:$stub_port" myprefix 2>&1) && rc=0 || rc=$? + kill "$stub_pid" 2>/dev/null || true + wait "$stub_pid" 2>/dev/null || true + # The stub has no sync/PUT support at all, so pass 1 is expected to fail + # once it gets there -- the only thing this proves is that the collision + # check itself did not false-positive and let the script proceed past it. + assert_contains "with nothing pre-existing, publish.sh proceeds to pass 1" "$out" "pass 1/2" + + echo "--- --index-only alone never runs the APK-collision check at all (it cannot upload one) ---" + # Same colliding key pre-seeded as the refusal test above, but this time + # asking only for the index pass: the check must not even run, since + # index_only_filters can never select a *.apk, so there is nothing it + # could refuse. Proceeding to "pass 2/2" (and failing there, since this + # stub has no sync support) is exactly the proof: a version of publish.sh + # that ran the check regardless of the flags would instead refuse here, + # before ever printing "pass 2/2". + start_stub mybucket "myprefix/de.example.app_1.apk" + out=$(bash "$here/publish.sh" --index-only "$collide_dir" mybucket "http://127.0.0.1:$stub_port" myprefix 2>&1) && rc=0 || rc=$? + kill "$stub_pid" 2>/dev/null || true + wait "$stub_pid" 2>/dev/null || true + assert_contains "--index-only proceeds straight to pass 2/2, skipping the APK check" "$out" "pass 2/2" + if echo "$out" | grep -q "already exists"; then + fail "--index-only must not run the APK-collision check (it refused anyway!)" + else + pass "--index-only does not run the APK-collision check" + fi +else + skip "credential-check-is-real / APK-collision-refusal (no python3 available, even via with-nixpkgs.sh)" + skip "negative control: non-colliding APK does not trip the check" + skip "--index-only never runs the APK-collision check" +fi # --- live round trip, opt-in ------------------------------------------------ # Skipped unless a scratch bucket and credentials are present, so the default -# offline run is unchanged. This is the only test here that talks to a real -# server, and the only one that could have caught the Garage checksum -# rejection (#3): everything above proves the command is well formed, which is -# a different claim from a server accepting it. +# offline run is unchanged. This is the only part of the suite that talks to +# a real server, and the only part that can prove the two-pass *ordering* +# with real uploads rather than command construction, and the APK-collision +# refusal against a real, already-published object. if [ -n "${FDROID_CI_LIVE_BUCKET:-}" ] && [ -n "${FDROID_CI_LIVE_ENDPOINT:-}" ] \ && [ -n "$LIVE_KEY_ID" ] && [ -n "$LIVE_SECRET" ]; then echo "--- a real round trip against a real endpoint ---" export AWS_ACCESS_KEY_ID="$LIVE_KEY_ID" export AWS_SECRET_ACCESS_KEY="$LIVE_SECRET" [ -n "$LIVE_REGION" ] && export AWS_DEFAULT_REGION="$LIVE_REGION" + # publish.sh sets these itself, but only inside its own (child) process -- + # it does not propagate back here, and every direct `aws s3 cp -` *write* + # this file does below needs it too, or it fails exactly like the Garage + # checksum bug documented in publish.sh (InvalidRequest: invalid checksum + # algorithm), which reads like a permissions problem. + export AWS_REQUEST_CHECKSUM_CALCULATION="${AWS_REQUEST_CHECKSUM_CALCULATION:-when_required}" + export AWS_RESPONSE_CHECKSUM_VALIDATION="${AWS_RESPONSE_CHECKSUM_VALIDATION:-when_required}" live_dir=$(mktemp -d) stamp="live-$(date -u +%s)-$$" printf 'publish.sh live probe %s\n' "$stamp" > "$live_dir/probe.txt" + printf '{"probe":"%s"}\n' "$stamp" > "$live_dir/index-v2.json" - if bash "$here/publish.sh" "$live_dir" "$FDROID_CI_LIVE_BUCKET" \ - "$FDROID_CI_LIVE_ENDPOINT" "$stamp" >/dev/null 2>&1; then - pass "publish.sh uploaded to a real bucket" + cleanup_live() { + bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ + s3 rm --recursive "s3://$FDROID_CI_LIVE_BUCKET/$stamp/" >/dev/null 2>&1 || true + rm -rf "$live_dir" + } + + object_exists() { + # object_exists -- a real head-object against the live bucket. + bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ + --region "${LIVE_REGION:-us-east-1}" \ + s3api head-object --bucket "$FDROID_CI_LIVE_BUCKET" --key "$1" >/dev/null 2>&1 + } + + echo "--- upload ordering by construction: --files-only, assert, then --index-only, assert (the hard part -- see below) ---" + # This is the one property the offline suite structurally cannot see: it + # is about the order two *real* uploads land in, which only a real bucket + # can answer. Racing a check in from outside against one real invocation + # doing both passes is exactly the flakiness fdroid-repo-ci#5 warned off + # ("worth building the check into the script's own output rather than + # trying to catch a race from outside") -- and, per review on #7, doing + # that with an env-var-triggered `eval` synchronization hook inside + # publish.sh was its own problem in a public script that runs with a + # signing key and S3 credentials in its environment. --files-only and + # --index-only remove the need for either: they are two genuinely separate + # invocations, so the bucket can be inspected for real, in between, with + # nothing racing and nothing evaluated. + bash "$here/publish.sh" --files-only "$live_dir" "$FDROID_CI_LIVE_BUCKET" \ + "$FDROID_CI_LIVE_ENDPOINT" "$stamp" >/tmp/publish_live_out.$$ 2>&1 + files_only_rc=$? + if [ "$files_only_rc" -eq 0 ]; then + pass "--files-only uploads to a real bucket" else - fail "publish.sh uploaded to a real bucket" + fail "--files-only uploads to a real bucket (exit $files_only_rc)" + cat /tmp/publish_live_out.$$ >&2 + fi + rm -f /tmp/publish_live_out.$$ + + if object_exists "$stamp/probe.txt"; then + pass "after --files-only alone: the file object exists" + else + fail "after --files-only alone: the file object exists" + fi + if object_exists "$stamp/index-v2.json"; then + fail "after --files-only alone: the index object must not exist yet (it does!)" + else + pass "after --files-only alone: the index object does not exist yet" + fi + + bash "$here/publish.sh" --index-only "$live_dir" "$FDROID_CI_LIVE_BUCKET" \ + "$FDROID_CI_LIVE_ENDPOINT" "$stamp" >/tmp/publish_live_out2.$$ 2>&1 + index_only_rc=$? + if [ "$index_only_rc" -eq 0 ]; then + pass "--index-only uploads to a real bucket" + else + fail "--index-only uploads to a real bucket (exit $index_only_rc)" + cat /tmp/publish_live_out2.$$ >&2 + fi + rm -f /tmp/publish_live_out2.$$ + + if object_exists "$stamp/index-v2.json"; then + pass "after --index-only resumes: the index object now exists too" + else + fail "after --index-only resumes: the index object now exists too" fi got=$(bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ s3 cp "s3://$FDROID_CI_LIVE_BUCKET/$stamp/probe.txt" - 2>/dev/null || true) if [ "$got" = "publish.sh live probe $stamp" ]; then - pass "the uploaded object reads back byte-identical" + pass "the uploaded file object reads back byte-identical" else - fail "the uploaded object reads back byte-identical" + fail "the uploaded file object reads back byte-identical" + fi + got_index=$(bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ + s3 cp "s3://$FDROID_CI_LIVE_BUCKET/$stamp/index-v2.json" - 2>/dev/null || true) + if [ "$got_index" = "{\"probe\":\"$stamp\"}" ]; then + pass "the uploaded index object (pass 2) reads back byte-identical" + else + fail "the uploaded index object (pass 2) reads back byte-identical" + fi + + echo "--- fault injection: --index-only alone (misuse) really does produce the dangerous state; the default never does ---" + # The property that actually matters: --index-only in isolation, against a + # prefix that has never had --files-only run against it, must produce + # exactly the state the whole ordering guarantee exists to prevent -- an + # index naming a file that 404s. If it did not, the two flags would not be + # a faithful decomposition of the default and the "by construction" claim + # above would be empty. + danger_dir=$(mktemp -d) + danger_stamp="live-danger-$(date -u +%s)-$$" + printf 'publish.sh live probe %s\n' "$danger_stamp" > "$danger_dir/probe.txt" + printf '{"probe":"%s"}\n' "$danger_stamp" > "$danger_dir/index-v2.json" + + bash "$here/publish.sh" --index-only "$danger_dir" "$FDROID_CI_LIVE_BUCKET" \ + "$FDROID_CI_LIVE_ENDPOINT" "$danger_stamp" >/tmp/publish_danger.$$ 2>&1 + danger_rc=$? + if [ "$danger_rc" -ne 0 ]; then + fail "misuse setup: --index-only alone uploads to a real bucket (exit $danger_rc)" + cat /tmp/publish_danger.$$ >&2 + fi + rm -f /tmp/publish_danger.$$ + + if object_exists "$danger_stamp/index-v2.json"; then + pass "misuse (--index-only alone) really does publish an index" + else + fail "misuse (--index-only alone) really does publish an index" + fi + if object_exists "$danger_stamp/probe.txt"; then + fail "misuse (--index-only alone) must leave the file it names absent (it does not!)" + else + pass "misuse (--index-only alone) leaves an index naming a file that 404s -- exactly the state the ordering guarantee exists to prevent" fi bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ - s3 rm --recursive "s3://$FDROID_CI_LIVE_BUCKET/$stamp/" >/dev/null 2>&1 || true - rm -rf "$live_dir" + s3 rm --recursive "s3://$FDROID_CI_LIVE_BUCKET/$danger_stamp/" >/dev/null 2>&1 || true + rm -rf "$danger_dir" + + # Contrast: the default (neither flag, one invocation) must never reach + # that state. There is deliberately no mid-flight observation point for a + # single default invocation any more (that was the hook this PR removes), + # so this is not re-proven by racing a check against it -- it holds by + # construction instead: the default's first phase runs the *same command + # array*, built the same way, as --files-only above, so its effect on the + # bucket is identical -- files land, and only then does the index command + # run. There is no code path through publish.sh that reaches "index + # present, file absent" without --index-only having been invoked alone, + # which is opt-in and never the default. What is left to check here is + # only the default's own final state. + safe_dir=$(mktemp -d) + safe_stamp="live-safe-$(date -u +%s)-$$" + printf 'publish.sh live probe %s\n' "$safe_stamp" > "$safe_dir/probe.txt" + printf '{"probe":"%s"}\n' "$safe_stamp" > "$safe_dir/index-v2.json" + bash "$here/publish.sh" "$safe_dir" "$FDROID_CI_LIVE_BUCKET" \ + "$FDROID_CI_LIVE_ENDPOINT" "$safe_stamp" >/tmp/publish_safe.$$ 2>&1 + safe_rc=$? + rm -f /tmp/publish_safe.$$ + if [ "$safe_rc" -eq 0 ] && object_exists "$safe_stamp/probe.txt" && object_exists "$safe_stamp/index-v2.json"; then + pass "the default path (neither flag) never leaves an index without its file: both are present after one call" + else + fail "the default path (neither flag) never leaves an index without its file" + fi + bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ + s3 rm --recursive "s3://$FDROID_CI_LIVE_BUCKET/$safe_stamp/" >/dev/null 2>&1 || true + rm -rf "$safe_dir" + + echo "--- APK-collision refusal against a real, already-published object ---" + apk_stamp="live-apk-$(date -u +%s)-$$" + apk_key="$apk_stamp/de.example.app_1.apk" + original_bytes="original apk bytes $apk_stamp" + printf '%s' "$original_bytes" | bash "$here/with-nixpkgs.sh" awscli2 aws \ + --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" --region "${LIVE_REGION:-us-east-1}" \ + s3 cp - "s3://$FDROID_CI_LIVE_BUCKET/$apk_key" >/dev/null 2>&1 + + race_dir=$(mktemp -d) + echo '{}' > "$race_dir/index-v2.json" + printf 'a different build racing the same versionCode' > "$race_dir/de.example.app_1.apk" + + race_out=$(bash "$here/publish.sh" "$race_dir" "$FDROID_CI_LIVE_BUCKET" \ + "$FDROID_CI_LIVE_ENDPOINT" "$apk_stamp" 2>&1) && race_rc=0 || race_rc=$? + if [ "${race_rc:-0}" -ne 0 ]; then + pass "publish.sh refuses a second publish that would overwrite a real, already-published APK" + else + fail "publish.sh refuses a second publish that would overwrite a real, already-published APK (got exit 0)" + fi + assert_contains "the real-bucket refusal names the colliding key" "$race_out" "$apk_key" + + after_bytes=$(bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ + s3 cp "s3://$FDROID_CI_LIVE_BUCKET/$apk_key" - 2>/dev/null || true) + if [ "$after_bytes" = "$original_bytes" ]; then + pass "the original APK's bytes are unchanged after the refused publish" + else + fail "the original APK's bytes are unchanged after the refused publish" + fi + + bash "$here/with-nixpkgs.sh" awscli2 aws --endpoint-url "$FDROID_CI_LIVE_ENDPOINT" \ + s3 rm --recursive "s3://$FDROID_CI_LIVE_BUCKET/$apk_stamp/" >/dev/null 2>&1 || true + rm -rf "$race_dir" + + cleanup_live else skip "live round trip (set FDROID_CI_LIVE_BUCKET, FDROID_CI_LIVE_ENDPOINT and AWS credentials)" + skip "live upload ordering by construction (--files-only then --index-only)" + skip "live fault injection: --index-only misuse vs. the default" + skip "live APK-collision refusal against a real object" fi report_and_exit diff --git a/test_s3_stub.py b/test_s3_stub.py new file mode 100755 index 0000000..a654ca4 --- /dev/null +++ b/test_s3_stub.py @@ -0,0 +1,70 @@ +#!/usr/bin/env python3 +"""Minimal path-style S3 HEAD-only stub server, used only by test_publish.sh +to test publish.sh's credential check and APK-existence check entirely +offline -- no real bucket, no real credentials, nothing beyond loopback. + +Only implements what those two checks actually call: + + HEAD / -- head-bucket, used to verify credentials + HEAD // -- head-object, used to check for an existing APK + +`aws`'s S3 client only uses path-style addressing (rather than +`.` virtual-hosted style) when the endpoint host is a literal +IP address, which `--endpoint-url http://127.0.0.1:` is -- so no +`--endpoint-url` or config trickery is needed to get requests shaped this +way. + +Existing objects are given on the command line; every other key 404s, as +does any bucket name other than the one given -- so a test that passes the +wrong bucket name notices, rather than getting a silent "yes" back. + +Deliberately never handles PUT/GET/LIST: publish.sh must never reach the +`aws s3 sync` calls in a test that uses this stub, since both a rejected +credential and an existing APK are supposed to abort before any upload is +attempted. If a future test needs sync to succeed against this stub, that is +new scope for this file, not an oversight here. + + test_s3_stub.py [ ...] + +Prints the bound port as the one line on stdout, then serves until killed. +""" +import http.server +import socketserver +import sys + + +def main(): + bucket = sys.argv[1] + existing = set(sys.argv[2:]) + + class Handler(http.server.BaseHTTPRequestHandler): + def do_HEAD(self): + path = self.path.lstrip("/") + parts = path.split("/", 1) + if not parts or parts[0] != bucket: + self.send_response(404) + self.end_headers() + return + if len(parts) == 1 or parts[1] == "": + # head-bucket: the one bucket named on the command line + # always "exists" and is reachable with these "credentials". + self.send_response(200) + self.end_headers() + return + key = parts[1] + self.send_response(200 if key in existing else 404) + self.end_headers() + + def log_message(self, fmt, *args): + pass # keep test output quiet; nothing here is diagnostic + + class Server(socketserver.TCPServer): + allow_reuse_address = True + + with Server(("127.0.0.1", 0), Handler) as httpd: + print(httpd.server_address[1], flush=True) + httpd.serve_forever() + + +if __name__ == "__main__": + main()