publish.sh: replace the test-only eval hook with --files-only/--index-only
bit's review on PR #7 (comment on the PR, appended to this reasoning): run_test_hook() eval'd an environment variable in publish.sh, which runs with the release signing key and S3 credentials in its environment, in a public repo. "Never set by a real caller" was true, but it was still a documented, discoverable path from any env-var influence to arbitrary code execution at the worst possible moment -- unnecessary, which is the stronger reason to remove it than any claim about likelihood. The replacement is a feature rather than a seam: --files-only and --index-only split the two passes into separately invokable steps. With neither flag the default is unchanged (both passes, files first). This makes the ordering guarantee testable by construction -- run --files-only, assert the bucket state, run --index-only, assert again -- with no hook, no eval, and no test-only branch in production code. It is also independently useful: 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. The APK-existence check is skipped entirely for a plain --index-only invocation, since that pass can never upload a *.apk. Re-tested end to end: offline 91 passed / 4 skipped / 0 failed (up from 83/ 3/0, mostly new flag-construction and stub assertions); live against fdroid-weblib-dev 104 passed / 0 skipped / 0 failed, bucket verified empty before and after. Added a live fault-injection test that --index-only alone against a prefix --files-only never touched really does publish an index naming an absent file -- the exact state the ordering guarantee exists to prevent -- contrasted with the default path, which never reaches it. Offline, removing the run_files/run_index gating (simulating the flags being ignored) was confirmed to fail 5 of the new assertions before being reverted. Left the versionCode granularity as minutes, per instruction: that is being settled separately on weblib-viewer#15 and is a one-constant change for whenever it lands, not something to pre-empt here. Co-authored-by: bit <bit@das-labor.org>
This commit is contained in:
@@ -148,6 +148,37 @@ 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
|
||||
|
||||
@@ -257,8 +288,9 @@ here needs, resolved through the same pin.
|
||||
bash run_tests.sh
|
||||
```
|
||||
|
||||
83 assertions across seven suites offline (~20s on a warm nix store), plus 5
|
||||
more that only run against a real bucket (see below):
|
||||
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
|
||||
@@ -290,13 +322,15 @@ more that only run against a real bucket (see below):
|
||||
`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 (now two commands, files then index, and asserted in that
|
||||
order), and a real invocation against a closed local port
|
||||
(`127.0.0.1:1`), fully offline. Also runs `test_s3_stub.py`, a
|
||||
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) and that an
|
||||
already-published APK is refused before any upload, with a negative
|
||||
control proving a non-colliding APK does not trip the same check. The
|
||||
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
|
||||
@@ -304,15 +338,24 @@ more that only run against a real bucket (see below):
|
||||
|
||||
`test_publish.sh`'s live section (skipped by default; needs
|
||||
`FDROID_CI_LIVE_BUCKET`, `FDROID_CI_LIVE_ENDPOINT` and real credentials) adds
|
||||
5 more assertions against a real bucket: the round trip itself; the index
|
||||
object provably absent while the file object is already present at the one
|
||||
point in the middle of a real publish that the ordering guarantee depends on
|
||||
(observed via a same-process synchronization hook `publish.sh` exposes only
|
||||
for this, `PUBLISH_SH_TEST_HOOK_BETWEEN_PASSES` -- deliberately not a poll
|
||||
racing a background upload from outside, since that is exactly the
|
||||
flakiness fdroid-repo-ci#5 warned against); both objects reading back
|
||||
byte-identical; a second publish refused when it would overwrite a
|
||||
real, already-published APK; and that APK's bytes unchanged afterwards.
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user