diff --git a/README.md b/README.md index 1258aa3..768925b 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ 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) | +| `next-version-code.sh` | print the next monotonic `versionCode` for an application id and a commit's committer date, 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 -- files first, index last, and refuses to overwrite an already-published APK (see "Publish ordering and safety" below) | @@ -47,7 +47,7 @@ below for what that costs and why it is still the right call here. - run: | version_code=$(bash "${{ steps.ci.outputs.path }}/next-version-code.sh" \ - "$FDROID_REPO_INDEX_URL" "$APPLICATION_ID") + "$FDROID_REPO_INDEX_URL" "$APPLICATION_ID" "$(git log -1 --format=%ct)") echo "version_code=$version_code" >> "$GITHUB_ENV" # ... feed $version_code into whatever builds app-release-unsigned.apk @@ -94,21 +94,49 @@ variable name. 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: +this one script rather than each computing their own. The candidate is +derived from the **committer date of the commit being built**, not the +build clock -- deterministic per commit, rather than depending on when a +runner happened to pick the job up -- and clamped on **both** sides: ``` -candidate = whole minutes since 2026-01-01T00:00:00Z +candidate = whole minutes since 2026-01-01T00:00:00Z, of the commit's committer date +candidate = min(candidate, minutes(now) + TOLERANCE) # forwards published = highest versionCode already in the repo index for this applicationId -result = max(candidate, published + 1) +result = max(candidate, published + 1) # backwards ``` 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. +seconds and 11 for Unix time. Granularity is no longer what keeps +same-commit rebuilds from colliding, though -- that is a concurrency-group +plus refuse-on-existing-APK problem now (see below) -- so minutes stand +purely as a wide, cheap headroom choice, not a load-bearing part of the +concurrency story. + +**Backwards.** Rebase, amend and cherry-pick can all produce a commit older +than one already published. The rule uses the commit's **committer** date, +not its author date, specifically because of this: author date survives a +rebase unchanged, so a rebased branch would keep re-emitting its original +(now stale) timestamp and the backwards clamp would never see it move. +Committer date changes on every rewrite, which is what lets `published + 1` +actually catch it. + +**Forwards.** A commit's timestamp is attacker- and typo-controlled, and the +number this rule produces only ever rises -- so a bad one can never be +walked back down. A commit dated a century ahead burns decades of headroom +permanently; worse, a value placed just *below* the int32 ceiling is still +accepted by Android, still publishes, and then **no later build can ever +exceed it** -- the app becomes permanently un-updatable, recoverable only by +shipping a new `applicationId` and asking every user to reinstall by hand. +`TOLERANCE` only has to absorb honest clock skew between the committer's +machine and the runner -- a few hours is generous, a day would still be +harmless, since its only job is keeping the number near the present. A +commit that lands beyond it is **clamped and a warning is printed, never +rejected outright**: rejecting would block a legitimate build over a few +minutes of innocent skew, and the clamp already removes the actual harm (a +permanently reserved version-code range), so a hard failure would buy +nothing further. **The clamp is monotonic against *observed* state, not monotonic by construction.** It does not close the race where two jobs start in the same @@ -119,16 +147,24 @@ 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. +information is finally reliable. Concurrent same-commit builds are now also +addressed at the workflow level with a concurrency group +(cancel-in-progress) -- measured working on this Gitea instance -- which is +an optimization on top of the refusal, not a replacement for it. -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. +Takes an index-v2.json URL or local file path, an application id, and the +committer timestamp of the commit being built as Unix seconds (exactly what +`git log -1 --format=%ct` prints); prints the integer and nothing else, so a +caller does +`versionCode=$(next-version-code.sh "$URL" "$APPLICATION_ID" "$(git log -1 --format=%ct)")`. +The script takes the timestamp as an argument rather than shelling out to +git itself, which keeps it pure and testable -- the caller already knows +its own commit. 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 (clamped) candidate. ## Publish ordering and safety (fdroid-repo-ci#5, fdroid-repo-ci#6) @@ -288,7 +324,7 @@ here needs, resolved through the same pin. bash run_tests.sh ``` -91 assertions across seven suites offline (~20s on a warm nix store, 4 +98 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: @@ -298,12 +334,23 @@ real bucket: 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 + written to a temp dir: a normal commit yields its own minute, a commit + older than the published maximum still yields published+1 (backwards -- + rebase/amend/cherry-pick), a future-dated commit within tolerance passes + through untouched, one beyond tolerance is clamped to roughly now *and + warns* (forwards -- a bad clock or a malicious/typo'd date), a commit + dated near the int32 ceiling (year 2999) does not produce a near-int32 + result, several published 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. + stdout). A dedicated property test also checks, across every one of those + scenarios in one place, that the result always strictly exceeds every + versionCode already in the fixture -- the property that actually matters, + which a future refactor could break while each individual case still + looks right. Both the commit's committer date and "now" (used only for + the forwards clamp) are pinned independently, the latter via an + undocumented env var seam, so every expected number is fixed rather than + depending on whatever minute the suite happens to run in. * `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 @@ -316,11 +363,14 @@ real bucket: and `fdroid update`. Needs the pinned toolchain to be resolvable (locally cached, or fetched once over the network); nothing beyond that. * `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. + `with-nixpkgs.sh`: argument count (now three: index, application id, + committer timestamp), a local index file end to end, a first-ever publish + for an application id, a far-future committer date clamped with a warning + on stderr, a non-numeric committer timestamp refused rather than coerced, + 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 diff --git a/action.yml b/action.yml index 208bce0..bee4ce6 100644 --- a/action.yml +++ b/action.yml @@ -17,10 +17,10 @@ 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, 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. + `fdroid update` over a binary repo, compute the next monotonic versionCode + for a commit's committer date, and publish the result to an S3-compatible + bucket. The caller supplies the keys, the bucket, the APK and the commit + being built; this action supplies the pinned toolchain and the scripts. outputs: path: diff --git a/next-version-code.sh b/next-version-code.sh index 9d2df65..8cde09d 100755 --- a/next-version-code.sh +++ b/next-version-code.sh @@ -5,11 +5,20 @@ # whole failure mode this avoids, so both paths call this one script rather # than each computing their own. # -# next-version-code.sh +# next-version-code.sh \ +# +# +# is the COMMITTER date (not author date -- a rebase +# leaves author date unchanged, which would defeat the backwards clamp) of +# the commit being built, as Unix seconds: `git log -1 --format=%ct`. This +# script takes it as an argument rather than reading git itself, on purpose +# -- the caller already knows its own commit, and it keeps the underlying +# Python pure and testable without a git checkout. # # Prints the integer to stdout and nothing else: # -# versionCode=$(bash next-version-code.sh "$INDEX_URL" "$APPLICATION_ID") +# versionCode=$(bash next-version-code.sh "$INDEX_URL" "$APPLICATION_ID" \ +# "$(git log -1 --format=%ct)") # # 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 @@ -22,7 +31,10 @@ # 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. +# for a channel, and yields the bare (clamped) candidate. A commit dated +# implausibly far in the future is also NOT a hard failure -- it is clamped +# to roughly now and a warning is printed on stderr; see next_version_code.py +# for why rejecting it outright would be worse. # # 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 @@ -36,9 +48,9 @@ set -euo pipefail here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) -if [ "$#" -ne 2 ]; then +if [ "$#" -ne 3 ]; then cat >&2 <<'USAGE' -usage: next-version-code.sh +usage: next-version-code.sh USAGE exit 2 fi diff --git a/next_version_code.py b/next_version_code.py index 07b5656..2ad6f70 100755 --- a/next_version_code.py +++ b/next_version_code.py @@ -1,19 +1,58 @@ #!/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. +Implements the rule from fdroid-repo-ci#6 -- read that issue (including the +"Superseded" sections, which change the original rule) for the full +reasoning; this is only the arithmetic and the index read. - candidate = whole minutes since 2026-01-01T00:00:00Z + candidate = whole minutes since 2026-01-01T00:00:00Z, of the COMMITTER + date of the commit being built + candidate = min(candidate, minutes(now) + TOLERANCE) -- forwards published = highest versionCode already in the index for this app id - result = max(candidate, published + 1) + result = max(candidate, published + 1) -- backwards 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. +and minutes-since-2026 does not exhaust it for 4,082 years. Granularity is +not what keeps two builds of the same commit from colliding any more -- +that is now a concurrency-group-plus-refuse-on-existing problem, handled +elsewhere (see the README) -- so minutes stand purely as a wide, cheap +headroom choice. + +The candidate is derived from the COMMIT being built, not the build clock, +so the result is a function of the commit rather than of when a runner +happened to pick the job up: rebuilding the identical commit twice (e.g. a +retry) yields the identical number. That determinism is what makes the two +clamps below necessary, one per direction: + +BACKWARDS -- rebase, amend and cherry-pick can all produce a commit that is +older than one already published, and it must not be allowed to re-emit a +number a device has already seen a higher one for. This is why the +COMMITTER date is used and not the author date: author date is preserved +across a rebase, so a rebased branch would keep replaying its original +(now stale) timestamp and defeat this exact clamp. Committer date changes +every time the commit is rewritten, which is what makes "published + 1" +actually catch it. + +FORWARDS -- a commit's timestamp is attacker- and typo-controlled (it is +just a field in a commit object anyone with push access can set), and the +version code this produces only ever goes up, so a bad one can never be +walked back down. A commit dated a century in the future would burn decades +of headroom permanently; worse, a value placed just *below* the int32 +ceiling would still be accepted by Android, would still publish, and then +NO LATER BUILD COULD EVER EXCEED IT -- the application becomes permanently +un-updatable, recoverable only by shipping a new applicationId and asking +every user to reinstall by hand. Do not remove this clamp to "simplify" the +arithmetic; it is the one guarding against unrecoverable, not merely +annoying, breakage. + +TOLERANCE exists only to absorb honest clock skew between the committer's +machine and the runner that builds it -- a few hours is generous for that, +a full day would still be harmless, since its only job is keeping the +number near the present, not pinning it exactly. A future-dated commit is +therefore CLAMPED AND A WARNING IS PRINTED, never rejected outright: +rejecting would block a legitimate build over a few minutes of innocent +skew, and the clamp already removes the actual harm (a permanently reserved +version code range), so there is nothing left for a hard failure to buy. Reads `packages..versions.*.manifest.versionCode`, the shape fdroidserver's own index.py actually writes (`packages` keyed by @@ -23,14 +62,14 @@ 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. +the bare (clamped) 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). +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 @@ -41,10 +80,16 @@ import urllib.request EPOCH = datetime.datetime(2026, 1, 1, 0, 0, 0, tzinfo=datetime.timezone.utc) +# Clock skew between the committer's machine and the runner, absorbed +# without complaint. Not tight by design -- see the FORWARDS section of the +# module docstring for why a generous value here is fine: this only has to +# keep a well-formed commit's number near the present, not pin it exactly, +# and everything past it is clamped (and warned about) regardless of size. +TOLERANCE_MINUTES = 6 * 60 # 6 hours -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 minutes_since_epoch(dt): + return int((dt - EPOCH).total_seconds() // 60) def load_index(source, timeout=30): @@ -76,27 +121,70 @@ def highest_published_version_code(index, application_id): return max(codes) if codes else None -def next_version_code(source, application_id, now=None): +def clamped_candidate(committer_time, now, tolerance_minutes=TOLERANCE_MINUTES): + """The forwards clamp: minutes-since-epoch of the commit's committer + date, capped at minutes-since-epoch of "now" plus TOLERANCE. Warns on + stderr (never fails) when the cap actually bites, since that is either a + broken clock or someone probing, and both are worth seeing in the log -- + see the FORWARDS section of the module docstring for why this is a + clamp-and-warn rather than a rejection.""" + candidate = minutes_since_epoch(committer_time) + ceiling = minutes_since_epoch(now) + tolerance_minutes + if candidate > ceiling: + print( + f"next_version_code.py: warning: committer date " + f"{committer_time.isoformat()} is {candidate - ceiling} minute(s) " + f"beyond now+tolerance; clamping the version code candidate to " + f"{ceiling} instead of {candidate}", + file=sys.stderr, + ) + return ceiling + return candidate + + +def next_version_code(source, application_id, committer_time, now=None): + now = now if now is not None else datetime.datetime.now(datetime.timezone.utc) index = load_index(source) - candidate = minutes_since_epoch(now) + candidate = clamped_candidate(committer_time, now) published = highest_published_version_code(index, application_id) if published is None: return candidate return max(candidate, published + 1) +def _parse_committer_timestamp(value): + # Unix seconds, exactly what `git log -1 --format=%ct` prints -- the + # caller already has the commit, so this script takes it as an argument + # rather than shelling out to git itself (fdroid-repo-ci#6: keeps this + # script pure and testable). + try: + seconds = int(value) + except ValueError: + raise argparse.ArgumentTypeError( + f"expected Unix seconds (e.g. `git log -1 --format=%ct`), got {value!r}" + ) + return datetime.datetime.fromtimestamp(seconds, tz=datetime.timezone.utc) + + 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") + parser.add_argument( + "committer_timestamp", + type=_parse_committer_timestamp, + help="committer date of the commit being built, as Unix seconds " + "(`git log -1 --format=%%ct`)", + ) 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. + # the offline suite pin "now" -- the runner's clock, used only for the + # forwards clamp -- to a fixed, known value 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: @@ -105,7 +193,9 @@ def main(argv): now = now.replace(tzinfo=datetime.timezone.utc) try: - result = next_version_code(args.index_source, args.application_id, now=now) + result = next_version_code( + args.index_source, args.application_id, args.committer_timestamp, 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 diff --git a/test_next_version_code.py b/test_next_version_code.py index 1aab1d6..2aa4ffd 100755 --- a/test_next_version_code.py +++ b/test_next_version_code.py @@ -1,5 +1,7 @@ #!/usr/bin/env python3 -"""Offline tests for next_version_code.py (fdroid-repo-ci#6). +"""Offline tests for next_version_code.py (fdroid-repo-ci#6, including its +"Superseded" sections: the candidate is derived from the COMMITTER date of +the commit being built, clamped on both sides, not from the build clock). 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. @@ -12,15 +14,20 @@ 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. +Two clocks are pinned independently, deliberately kept distinct so a test +can move either one without the other: -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. +* the COMMIT's committer date -- the third CLI argument, exactly as a real + caller passes `git log -1 --format=%ct`; +* "now" -- the runner's clock, used only for the forwards clamp, pinned via + the NEXT_VERSION_CODE_NOW testing seam (see the script's main()). + +The property that actually matters -- 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. Every clamp case below asserts that directly, +not just the arithmetic, and test_property_result_exceeds_every_published_code +checks it again across every scenario in one place. """ import datetime import json @@ -35,11 +42,21 @@ 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 +TOLERANCE_MINUTES = 6 * 60 # must track next_version_code.py's TOLERANCE_MINUTES +INT32_MAX = 2**31 - 1 + +# "Now" is pinned at minute 100 after the epoch throughout, unless a test +# overrides it -- an arbitrary, fixed instant so expected candidates are +# known integers instead of whatever minute the suite happens to run in. +NOW_MINUTES = 100 +NOW_ISO = (EPOCH + datetime.timedelta(minutes=NOW_MINUTES)).isoformat() + + +def minutes_to_unix_seconds(minutes): + """Convert "minutes since the 2026 epoch" (what the script's candidate + is expressed in) to Unix seconds (what its CLI argument, and real + `git log -1 --format=%ct`, actually take).""" + return int((EPOCH + datetime.timedelta(minutes=minutes)).timestamp()) def index_with_versions(application_id, version_codes): @@ -66,66 +83,65 @@ class NextVersionCodeTest(unittest.TestCase): json.dump(obj_or_text, f) return path - def _run(self, index_path, application_id=APP_ID, now=FIXED_NOW): + def _run(self, index_path, committer_minutes, application_id=APP_ID, + now_iso=NOW_ISO): env = dict(os.environ) - if now is not None: - env["NEXT_VERSION_CODE_NOW"] = now + if now_iso is not None: + env["NEXT_VERSION_CODE_NOW"] = now_iso else: env.pop("NEXT_VERSION_CODE_NOW", None) + committer_ts = str(minutes_to_unix_seconds(committer_minutes)) return subprocess.run( - [sys.executable, SCRIPT, index_path, application_id], + [sys.executable, SCRIPT, index_path, application_id, committer_ts], capture_output=True, text=True, env=env, ) - # -- no prior build: the first-ever publish for a channel ------------- + # -- a normal commit yields its own minute ----------------------------- - def test_no_prior_build_yields_bare_candidate(self): + def test_normal_commit_yields_its_own_minute(self): + # Committer date deliberately different from "now" (250 vs 100), and + # well inside tolerance, to prove the result tracks the COMMIT, not + # the clock the script happens to run under. path = self._write_index("empty.json", {"packages": {}}) - result = self._run(path) + result = self._run(path, committer_minutes=250) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(result.stdout, f"{CANDIDATE_AT_FIXED_NOW}\n") + self.assertEqual(result.stdout, "250\n") + self.assertEqual(result.stderr, "") # no warning: well within tolerance 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) + result = self._run(path, committer_minutes=100) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(result.stdout, f"{CANDIDATE_AT_FIXED_NOW}\n") + self.assertEqual(result.stdout, "100\n") - # -- the clamp: result must exceed every published code ---------------- + # -- backwards: a commit older than published still exceeds it --------- - 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) + def test_commit_older_than_published_max_still_yields_published_plus_one(self): + # The rebase/amend/cherry-pick case: committer date (10) is far + # older than what is already published (500). Fault this catches if + # removed: the script would print 10, a number every device that + # saw 500 has already rejected as a downgrade. + path = self._write_index("higher.json", index_with_versions(APP_ID, [500])) + result = self._run(path, committer_minutes=10) self.assertEqual(result.returncode, 0, result.stderr) got = int(result.stdout) - self.assertEqual(got, CANDIDATE_AT_FIXED_NOW) - self.assertGreater(got, 3) + self.assertEqual(got, 501) + self.assertGreater(got, 500) 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) + # committer minute == published: the same-minute collision case. + # The clamp must still move strictly past it. + path = self._write_index("equal.json", index_with_versions(APP_ID, [100])) + result = self._run(path, committer_minutes=100) 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) + self.assertEqual(got, 101) 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) + result = self._run(path, committer_minutes=100) self.assertEqual(result.returncode, 0, result.stderr) got = int(result.stdout) self.assertGreater(got, 999) @@ -138,23 +154,105 @@ class NextVersionCodeTest(unittest.TestCase): obj["packages"][APP_ID]["versions"]["nomanifest"] = {} obj["packages"][APP_ID]["versions"]["emptymanifest"] = {"manifest": {}} path = self._write_index("partial.json", obj) - result = self._run(path) + result = self._run(path, committer_minutes=100) self.assertEqual(result.returncode, 0, result.stderr) got = int(result.stdout) self.assertGreater(got, 7) + # -- forwards: a future-dated commit is clamped, and warns ------------- + + def test_future_dated_commit_within_tolerance_is_not_clamped(self): + # Negative control: honest clock skew, well inside TOLERANCE, must + # pass through untouched and silently -- proves the forwards clamp + # is not simply "always clamp to now". + just_ahead = NOW_MINUTES + TOLERANCE_MINUTES - 1 + path = self._write_index("empty.json", {"packages": {}}) + result = self._run(path, committer_minutes=just_ahead) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout, f"{just_ahead}\n") + self.assertEqual(result.stderr, "") + + def test_future_dated_commit_beyond_tolerance_is_clamped_and_warns(self): + # A commit dated far ahead of "now" (attacker- or typo-controlled, + # per the module docstring) must be clamped to roughly now, not + # trusted outright -- and the clamp must be visible, not silent. + far_ahead = NOW_MINUTES + TOLERANCE_MINUTES + 100_000 + path = self._write_index("empty.json", {"packages": {}}) + result = self._run(path, committer_minutes=far_ahead) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + expected_ceiling = NOW_MINUTES + TOLERANCE_MINUTES + self.assertEqual(got, expected_ceiling) + self.assertLess(got, far_ahead) # actually clamped, not passed through + self.assertIn("warning", result.stderr.lower()) + self.assertIn("clamping", result.stderr.lower()) + + def test_commit_dated_near_int32_does_not_produce_a_near_int32_result(self): + # The unrecoverable failure mode from issue #6: a commit dated to + # land just below the int32 ceiling must NOT be allowed through, + # since that would publish a versionCode no later build could ever + # exceed. This uses a real distant year (2999) via a raw Unix + # timestamp, not a minutes-since-epoch shortcut, to exercise the + # actual conversion path a malicious or badly-typo'd commit date + # would take. + year_2999 = datetime.datetime(2999, 1, 1, tzinfo=datetime.timezone.utc) + ts = str(int(year_2999.timestamp())) + path = self._write_index("empty.json", {"packages": {}}) + env = dict(os.environ) + env["NEXT_VERSION_CODE_NOW"] = NOW_ISO + result = subprocess.run( + [sys.executable, SCRIPT, path, APP_ID, ts], + capture_output=True, text=True, env=env, + ) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + self.assertLess(got, INT32_MAX - 1_000_000) + # Tighter than "not overflowing": it must be near the clamp ceiling, + # i.e. near *now*, not merely somewhere short of int32. + self.assertEqual(got, NOW_MINUTES + TOLERANCE_MINUTES) + self.assertIn("warning", result.stderr.lower()) + + # -- the property that actually matters --------------------------------- + + def test_property_result_exceeds_every_published_code(self): + """Across a spread of scenarios -- ordinary, backwards, forwards, + far-forwards -- the result must always exceed every versionCode + already in the fixture. This is the property a future refactor + could quietly break while every individual arithmetic case still + happens to look right.""" + scenarios = [ + ([], 5), + ([1, 2, 3], 5), + ([100], 100), + ([500], 10), # backwards + ([1], NOW_MINUTES + TOLERANCE_MINUTES - 1), # forwards, in tolerance + ([1], NOW_MINUTES + TOLERANCE_MINUTES + 100_000), # forwards, clamped + ] + for published, committer_minutes in scenarios: + with self.subTest(published=published, committer_minutes=committer_minutes): + path = self._write_index( + f"scenario-{committer_minutes}.json", + index_with_versions(APP_ID, published) if published + else {"packages": {}}, + ) + result = self._run(path, committer_minutes=committer_minutes) + self.assertEqual(result.returncode, 0, result.stderr) + got = int(result.stdout) + ceiling = max(published) if published else float("-inf") + self.assertGreater(got, ceiling) + # -- 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) + result = self._run(missing, committer_minutes=100) 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) + result = self._run(path, committer_minutes=100) self.assertNotEqual(result.returncode, 0) self.assertEqual(result.stdout, "") self.assertIn("could not read the index", result.stderr) @@ -163,10 +261,10 @@ class NextVersionCodeTest(unittest.TestCase): def test_stdout_is_only_the_integer(self): path = self._write_index("clean.json", {"packages": {}}) - result = self._run(path) + result = self._run(path, committer_minutes=100) 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(int(result.stdout.strip()), 100) self.assertEqual(result.stdout.count("\n"), 1) diff --git a/test_next_version_code.sh b/test_next_version_code.sh index 1a2d07e..98f69fc 100755 --- a/test_next_version_code.sh +++ b/test_next_version_code.sh @@ -20,15 +20,30 @@ trap 'rm -rf "$work"' EXIT APP_ID="de.example.app" +# "now" is pinned for the whole script, the same reasoning +# test_next_version_code.py uses NEXT_VERSION_CODE_NOW for: every expected +# result below is then a fixed number, not one that depends on the real +# wall clock (and, unlike the Python suite, this file drives the real CLI +# through with-nixpkgs.sh, so it is also slower -- one more reason not to +# let it depend on when it happens to run). +export NEXT_VERSION_CODE_NOW="2026-06-01T00:00:00+00:00" +# The committer timestamp for most cases: the *same* instant as "now" above, +# as Unix seconds (`git log -1 --format=%ct`'s own unit) -- comfortably +# inside tolerance, so a normal invocation is not accidentally +# forward-clamped. Verified equal to NEXT_VERSION_CODE_NOW by construction: +# both come from 2026-06-01T00:00:00Z. +COMMITTER_TS=1780272000 + 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" +assert_failure "two arguments" bash "$here/next-version-code.sh" "$work/index.json" "$APP_ID" 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") +out2=$(bash "$here/next-version-code.sh" "$work/other-app.json" "$APP_ID" "$COMMITTER_TS") 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 commit dated far in the future is clamped through the real CLI, and warns ---" +# "now" is still pinned at 2026-06-01T00:00:00Z (exported above), so a +# committer date one century later (2126-01-01T00:00:00Z, hardcoded rather +# than computed with `date`, whose -d/-j flags differ between GNU and BSD) +# is unambiguously beyond tolerance. +future_ts=4922899200 +out6=$(bash "$here/next-version-code.sh" "$work/other-app.json" "$APP_ID" "$future_ts" \ + 2>/tmp/nvc_stderr4.$$) || true +err6=$(cat /tmp/nvc_stderr4.$$); rm -f /tmp/nvc_stderr4.$$ +if [[ "$out6" =~ ^[0-9]+$ ]]; then + pass "a far-future commit date still exits 0 (clamp, not rejection)" +else + fail "a far-future commit date still exits 0 (clamp, not rejection) (got '$out6')" +fi +assert_contains "a far-future commit date warns on stderr" "$err6" "warning" + 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=$? +out3=$(bash "$here/next-version-code.sh" "$work/does-not-exist.json" "$APP_ID" "$COMMITTER_TS" 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)" @@ -69,7 +100,7 @@ echo "--- an unreachable URL fails loudly too, the same way, through the real ne # 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=$? +out4=$(bash "$here/next-version-code.sh" "http://127.0.0.1:1/index-v2.json" "$APP_ID" "$COMMITTER_TS" 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)" @@ -81,11 +112,15 @@ assert_contains "the error explains the index could not be read (URL case)" "$er 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=$? +out5=$(bash "$here/next-version-code.sh" "$work/malformed.json" "$APP_ID" "$COMMITTER_TS" 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" +echo "--- a non-numeric committer timestamp is refused, not silently coerced ---" +assert_failure "non-numeric committer timestamp" \ + bash "$here/next-version-code.sh" "$work/index.json" "$APP_ID" "not-a-timestamp" + report_and_exit