diff --git a/.github/scripts/test_zenodo_release.py b/.github/scripts/test_zenodo_release.py index c4248a3..7c9d509 100644 --- a/.github/scripts/test_zenodo_release.py +++ b/.github/scripts/test_zenodo_release.py @@ -34,19 +34,29 @@ def json(self) -> dict: return self._payload -def draft_payload(deposit_id: int = 111, files: list | None = None) -> dict: +def draft_payload( + deposit_id: int = 111, + files: list | None = None, + bucket_base: str = "https://zenodo.org/api", +) -> dict: + # Zenodo returns the bucket on its own host. `bucket_base` lets a test hand + # back a foreign host instead, which the script must refuse to upload to. return { "id": deposit_id, "files": files or [], "links": { - "bucket": f"https://example.invalid/api/files/bucket-{deposit_id}", + "bucket": f"{bucket_base}/files/bucket-{deposit_id}", "html": f"https://example.invalid/deposit/{deposit_id}", "latest_draft": f"https://example.invalid/api/deposit/depositions/{deposit_id}", }, } -def install_fake_requests(concept_exists: bool, inherited: list | None = None) -> None: +def install_fake_requests( + concept_exists: bool, + inherited: list | None = None, + bucket_base: str = "https://zenodo.org/api", +) -> None: """Swap in a `requests` module that records calls and returns canned data.""" def record(method): @@ -55,9 +65,9 @@ def call(url, **kwargs): if method == "GET" and "/records/" in url: return FakeResponse({"id": 999}) if method == "GET" and "/deposit/depositions/" in url: - return FakeResponse(draft_payload(111, inherited)) + return FakeResponse(draft_payload(111, inherited, bucket_base)) if method == "POST" and url.endswith("/actions/newversion"): - return FakeResponse(draft_payload(111, inherited)) + return FakeResponse(draft_payload(111, inherited, bucket_base)) if method == "POST" and url.endswith("/actions/publish"): return FakeResponse( { @@ -67,10 +77,10 @@ def call(url, **kwargs): } ) if method == "POST" and url.endswith("/deposit/depositions"): - return FakeResponse(draft_payload(111)) + return FakeResponse(draft_payload(111, None, bucket_base)) if method == "DELETE": return FakeResponse({}) - return FakeResponse(draft_payload(111, inherited)) + return FakeResponse(draft_payload(111, inherited, bucket_base)) return call @@ -92,10 +102,39 @@ def sequence() -> list[tuple[str, str]]: return [(m, u.split("/api", 1)[-1] if "/api" in u else u) for m, u, _ in CALLS] -def run(name: str, *, concept: str | None, publish: bool, sandbox: bool, inherited=None): - global CALLS - CALLS = [] - install_fake_requests(bool(concept), inherited) +def check_token(label: str) -> None: + """Assert the token reached every call site of one run as a header. + + `CALLS` resets at the top of `run()`, so this has to be called after each + run. The concept-record path carries four call sites the first run never + reaches, and they went unasserted while this lived inline. + """ + # The token belongs in a header. Query strings reach access logs, egress + # proxies and Referer headers, all of which outlive the request. + check( + f"token sent as an Authorization Bearer header ({label})", + all((k.get("headers") or {}).get("Authorization") == "Bearer fake-token" for _, _, k in CALLS), + ) + check( + f"token never reaches a query string ({label})", + not any( + "access_token" in u or "access_token" in (k.get("params") or {}) + for _, u, k in CALLS + ), + ) + + +def build_inputs(): + """Bundle and metadata, rebuilt from the repository each time.""" + config = z.load_config() + cff = z.load_citation() + spec = config["deposits"]["top10"] + files = z.collect_files(spec, "2026") + bundle = z.write_bundle(files, Path(sys.argv[1] if len(sys.argv) > 1 else "/tmp") / "t.zip") + return bundle, z.build_metadata(cff, spec, "2026", "2026-06-15") + + +def set_env(concept: str | None) -> None: import os os.environ["ZENODO_TOKEN"] = "fake-token" @@ -104,12 +143,23 @@ def run(name: str, *, concept: str | None, publish: bool, sandbox: bool, inherit else: os.environ.pop("TEST_CONCEPT", None) - config = z.load_config() - cff = z.load_citation() - spec = config["deposits"]["top10"] - files = z.collect_files(spec, "2026") - bundle = z.write_bundle(files, Path(sys.argv[1] if len(sys.argv) > 1 else "/tmp") / "t.zip") - metadata = z.build_metadata(cff, spec, "2026", "2026-06-15") + +def run( + name: str, + *, + concept: str | None, + publish: bool, + sandbox: bool, + inherited=None, + bucket_base: str | None = None, +): + global CALLS + CALLS = [] + base = "https://sandbox.zenodo.org/api" if sandbox else "https://zenodo.org/api" + install_fake_requests(bool(concept), inherited, bucket_base or base) + set_env(concept) + + bundle, metadata = build_inputs() print(f"\n{name}") z.deposit(bundle, metadata, concept_env="TEST_CONCEPT", sandbox=sandbox, publish=publish) return metadata @@ -123,9 +173,10 @@ def run(name: str, *, concept: str | None, publish: bool, sandbox: bool, inherit check("does not call newversion", not any("newversion" in u for _, u in seq)) check("uploads to the bucket URL", any(m == "PUT" and "files/bucket" in u for m, u in seq)) check("publishes", any(u.endswith("/actions/publish") for _, u in seq)) -# The bucket URL comes from Zenodo's response, so only check URLs we build. -ours = [u for _, u, _ in CALLS if "zenodo.org" in u] -check("uses the production API base", ours and all(u.startswith("https://zenodo.org/api") for u in ours)) +# Every request now goes to the API host, the bucket upload included, so this +# covers the whole run. A foreign URL fails here rather than being filtered out. +urls = [u for _, u, _ in CALLS] +check("uses the production API base", urls and all(u.startswith("https://zenodo.org/api") for u in urls)) put_meta = [k for m, u, k in CALLS if m == "PUT" and "depositions" in u] check("sends metadata under a `metadata` key", bool(put_meta) and "metadata" in put_meta[0]["json"]) sent = put_meta[0]["json"]["metadata"] @@ -140,7 +191,7 @@ def run(name: str, *, concept: str | None, publish: bool, sandbox: bool, inherit check("every author is deposited", len(sent["creators"]) == len(people), str(len(sent["creators"]))) check("author order survives", sent["creators"][-1]["name"] == "Klondike, Gavin") check("contributor type is valid", sent["contributors"][0]["type"] == "HostingInstitution") -check("token passed as access_token", all(k.get("params", {}).get("access_token") == "fake-token" for _, _, k in CALLS)) +check_token("first deposit") run( "new version with inherited files, sandbox, draft only", @@ -155,9 +206,45 @@ def run(name: str, *, concept: str | None, publish: bool, sandbox: bool, inherit check("deletes the inherited file", any(m == "DELETE" and u.endswith("/files/f1") for m, u in seq)) check("uploads the new bundle", any(m == "PUT" and "files/bucket" in u for m, u in seq)) check("does NOT publish in draft mode", not any(u.endswith("/actions/publish") for _, u in seq)) -ours = [u for _, u, _ in CALLS if "zenodo.org" in u] -check("uses the sandbox API base", ours and all(u.startswith("https://sandbox.zenodo.org/api") for u in ours)) -check("never touches production", not any(u.startswith("https://zenodo.org/api") for u in ours)) +urls = [u for _, u, _ in CALLS] +check("uses the sandbox API base", urls and all(u.startswith("https://sandbox.zenodo.org/api") for u in urls)) +check("never touches production", not any(u.startswith("https://zenodo.org/api") for u in urls)) +# The concept path adds four call sites the first run never reaches: the record +# GET, the newversion POST, the draft GET and the inherited-file DELETE. +check_token("new version") + +# The bucket URL arrives in Zenodo's response. Attaching the token to whatever +# host it names would hand the credential to anyone who can shape that response. +def refuse(name: str, bucket_base: str) -> str: + """Run a deposit that must fail closed, and return why it exited. + + The reason matters: a bare `except SystemExit` also catches the missing + bucket link and every API error, so it would pass with the host check gone. + """ + global CALLS + CALLS = [] + install_fake_requests(False, None, bucket_base) + set_env(None) + bundle, metadata = build_inputs() + print(f"\n{name}") + try: + z.deposit(bundle, metadata, concept_env="TEST_CONCEPT", sandbox=False, publish=True) + return "" + except SystemExit as exc: + return str(exc) + + +why = refuse("bucket URL on a host the script did not verify", "https://evil.invalid/api") +check("refuses on a host mismatch", "does not match the API host" in why, why or "no SystemExit") +check("sends nothing to the foreign host", not any("evil.invalid" in u for _, u, _ in CALLS)) +check("does not publish", not any(u.endswith("/actions/publish") for _, u, _ in CALLS)) + +# Right host, wrong scheme. Comparing netloc alone lets this through, and the +# token then crosses the network in cleartext. +why = refuse("bucket URL on the API host over http", "http://zenodo.org/api") +check("refuses on a plaintext bucket URL", "is not https" in why, why or "no SystemExit") +check("sends the token over no cleartext URL", not any(u.startswith("http://") for _, u, _ in CALLS)) +check("does not publish over http", not any(u.endswith("/actions/publish") for _, u, _ in CALLS)) print() if FAILURES: diff --git a/.github/scripts/zenodo_release.py b/.github/scripts/zenodo_release.py index ffddd45..194f8f4 100644 --- a/.github/scripts/zenodo_release.py +++ b/.github/scripts/zenodo_release.py @@ -258,6 +258,30 @@ def api_call(response, what: str) -> dict: return response.json() if response.content else {} +def upload_url(draft: dict, base: str, filename: str) -> str: + """The upload target for the bundle, once its host has been checked. + + Zenodo names the bucket in its own response. The token is about to be + attached to that URL, so anything able to shape the response could otherwise + redirect a `deposit:write` credential to a host of its choosing, or downgrade + it to cleartext. Fail closed unless the bucket sits on the API host the + script chose and arrives over https. + + Scheme and host exit separately. ZENODO.md documents the error text an + operator has to act on, and the two failure modes call for different action. + """ + bucket = (draft.get("links") or {}).get("bucket") + if not bucket: + sys.exit("error: Zenodo returned no bucket link for the draft") + if urlparse(bucket).scheme != "https": + sys.exit(f"error: bucket URL is not https: {bucket!r}") + expected = urlparse(base).netloc + found = urlparse(bucket).netloc + if found != expected: + sys.exit(f"error: bucket host {found!r} does not match the API host {expected!r}") + return f"{bucket}/{filename}" + + def deposit( bundle: Path, metadata: dict, @@ -271,19 +295,21 @@ def deposit( if not token: sys.exit("error: ZENODO_TOKEN is not set") base = "https://sandbox.zenodo.org/api" if sandbox else "https://zenodo.org/api" - params = {"access_token": token} + # A header rather than an `access_token` query parameter: query strings are + # recorded by access logs, egress proxies and Referer headers. + headers = {"Authorization": f"Bearer {token}"} concept = os.environ.get(concept_env) if concept: print(f"creating a new version of concept record {concept}") latest = api_call( - requests.get(f"{base}/records/{concept}", params=params, timeout=TIMEOUT), + requests.get(f"{base}/records/{concept}", headers=headers, timeout=TIMEOUT), "resolving concept record", ) versioned = api_call( requests.post( f"{base}/deposit/depositions/{latest['id']}/actions/newversion", - params=params, + headers=headers, timeout=TIMEOUT, ), "creating new version", @@ -291,7 +317,7 @@ def deposit( draft_id = urlparse(versioned["links"]["latest_draft"]).path.rstrip("/").rsplit("/", 1)[-1] draft = api_call( requests.get( - f"{base}/deposit/depositions/{draft_id}", params=params, timeout=TIMEOUT + f"{base}/deposit/depositions/{draft_id}", headers=headers, timeout=TIMEOUT ), "fetching draft", ) @@ -300,7 +326,7 @@ def deposit( api_call( requests.delete( f"{base}/deposit/depositions/{draft['id']}/files/{stale['id']}", - params=params, + headers=headers, timeout=TIMEOUT, ), f"removing inherited file {stale.get('filename')}", @@ -309,19 +335,17 @@ def deposit( print(f"{concept_env} is unset, creating the first deposition") draft = api_call( requests.post( - f"{base}/deposit/depositions", params=params, json={}, timeout=TIMEOUT + f"{base}/deposit/depositions", headers=headers, json={}, timeout=TIMEOUT ), "creating deposition", ) + # Resolved before the file is opened, so a rejected host stops the run + # without the token ever leaving the process. + target = upload_url(draft, base, bundle.name) with bundle.open("rb") as handle: api_call( - requests.put( - f"{draft['links']['bucket']}/{bundle.name}", - data=handle, - params=params, - timeout=TIMEOUT, - ), + requests.put(target, data=handle, headers=headers, timeout=TIMEOUT), f"uploading {bundle.name}", ) print(f"uploaded {bundle.name} ({bundle.stat().st_size} bytes)") @@ -329,7 +353,7 @@ def deposit( draft = api_call( requests.put( f"{base}/deposit/depositions/{draft['id']}", - params=params, + headers=headers, json={"metadata": metadata}, timeout=TIMEOUT, ), @@ -343,7 +367,7 @@ def deposit( published = api_call( requests.post( f"{base}/deposit/depositions/{draft['id']}/actions/publish", - params=params, + headers=headers, timeout=TIMEOUT, ), "publishing", diff --git a/documentation/ZENODO.md b/documentation/ZENODO.md index e742006..a1437c0 100644 --- a/documentation/ZENODO.md +++ b/documentation/ZENODO.md @@ -96,6 +96,37 @@ use it. Decide who owns the record before wiring that up. the existing record. Without it the script creates an unrelated record and splits the citation history. +## How the token is handled + +The token goes out as an `Authorization: Bearer` header, never as a query +parameter. URLs carrying a credential get copied into access logs, egress +proxies and `Referer` headers, all of which outlive the request. + +Zenodo names the upload bucket in its own response, and the script checks that +bucket before attaching the token. Two things have to hold: the bucket sits on +the same host as the API, and it arrives over https. + +If Zenodo ever moves file storage to a different host, the deposit stops with: + +```text +error: bucket host 'files.example.org' does not match the API host 'zenodo.org' +``` + +That is the check doing its job, not a broken bundle. Confirm the new host is +really Zenodo's, then widen the comparison in `upload_url()`. Failing closed is +deliberate: the alternative sends a `deposit:write` token wherever the response +points. + +A bucket URL on the right host but the wrong scheme stops with: + +```text +error: bucket URL is not https: 'http://zenodo.org/api/files/bucket-1' +``` + +Treat that one as a problem to report, not a comparison to widen. Uploading +would put the token on the wire in cleartext, so there is no version of this +worth allowing. + ## Releasing Add `/final/`, update `CITATION.cff` with that edition's authors, then: