From f0b5b8cf6c307c299d8ac8feb490e25d3f06f2df Mon Sep 17 00:00:00 2001 From: rocklambros Date: Wed, 26 Aug 2026 16:07:57 -0600 Subject: [PATCH 1/2] zenodo: send the API token as a Bearer header and verify the bucket host The deposit script passed the token as an `access_token` query parameter on every request. Query strings are recorded by access logs, egress proxies and Referer headers, so the credential outlived the request in places nobody audits. Zenodo accepts `Authorization: Bearer`, which keeps it in a header. The bundle upload also targeted `draft["links"]["bucket"]`, a URL taken straight from the API response, and attached the token to it without checking where it pointed. The API base is hardcoded, so this rested on trusting Zenodo to name its own host rather than on an open redirect, but a `deposit:write` credential should not follow an unverified URL. `upload_url()` now compares the bucket host against the API host and fails closed, resolving the target before the bundle is opened so a rejected host stops the run with nothing sent. Both matter more once the deposit runs from CI against a repository secret, which is the direction documentation/ZENODO.md lays out. Tests cover the Bearer header, the absence of the token from any query string, and a bucket URL on a foreign host. The stub now returns a bucket on the same host as the API base, matching what Zenodo does. Closes #138 --- .github/scripts/test_zenodo_release.py | 92 +++++++++++++++++++++----- .github/scripts/zenodo_release.py | 46 +++++++++---- documentation/ZENODO.md | 19 ++++++ 3 files changed, 125 insertions(+), 32 deletions(-) diff --git a/.github/scripts/test_zenodo_release.py b/.github/scripts/test_zenodo_release.py index c4248a3..77f1aef 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,17 @@ 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 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 +121,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 @@ -140,7 +168,19 @@ 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)) +# The token belongs in a header. Query strings reach access logs, egress proxies +# and Referer headers, all of which outlive the request. +check( + "token sent as an Authorization Bearer header", + all((k.get("headers") or {}).get("Authorization") == "Bearer fake-token" for _, _, k in CALLS), +) +check( + "token never reaches a query string", + not any( + "access_token" in u or "access_token" in (k.get("params") or {}) + for _, u, k in CALLS + ), +) run( "new version with inherited files, sandbox, draft only", @@ -159,6 +199,22 @@ def run(name: str, *, concept: str | None, publish: bool, sandbox: bool, inherit 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)) +# 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. +CALLS = [] +install_fake_requests(False, None, "https://evil.invalid/api") +set_env(None) +bundle, metadata = build_inputs() +print("\nbucket URL on a host the script did not verify") +try: + z.deposit(bundle, metadata, concept_env="TEST_CONCEPT", sandbox=False, publish=True) + refused = False +except SystemExit: + refused = True +check("refuses to upload", refused) +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)) + print() if FAILURES: print(f"{len(FAILURES)} failed: {', '.join(FAILURES)}") diff --git a/.github/scripts/zenodo_release.py b/.github/scripts/zenodo_release.py index ffddd45..b7ea320 100644 --- a/.github/scripts/zenodo_release.py +++ b/.github/scripts/zenodo_release.py @@ -258,6 +258,24 @@ 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. Fail closed + when the bucket does not sit on the API host the script chose. + """ + bucket = (draft.get("links") or {}).get("bucket") + if not bucket: + sys.exit("error: Zenodo returned no bucket link for the draft") + 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 +289,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 +311,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 +320,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 +329,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 +347,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 +361,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..a143ca2 100644 --- a/documentation/ZENODO.md +++ b/documentation/ZENODO.md @@ -96,6 +96,25 @@ 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 +the bucket sits on the same host as the API before attaching the token. 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. + ## Releasing Add `/final/`, update `CITATION.cff` with that edition's authors, then: From 69f29372285604dd25b18b85dee8bb60ddc49ec7 Mon Sep 17 00:00:00 2001 From: rocklambros Date: Thu, 27 Aug 2026 09:24:30 -0600 Subject: [PATCH 2/2] zenodo: reject a plaintext bucket URL and assert the token on every call Review on #139 found two assertions that looked like coverage and were not. upload_url() compared netloc alone, so a bucket on the API host over http passed the check and the Bearer token went out in cleartext. The script builds that URL itself, so redirect stripping never applied. Scheme and host now exit separately, because widening the host comparison is the right response to one and never the right response to the other. The two token assertions ran after the first deposit only, and CALLS resets at the top of every run(). The concept path contributes four of the eight call sites, and none of them were asserted: reverting the concept record GET to params= left the suite green. Hoisted into check_token(), called after both runs. Also: the foreign-host case accepted any SystemExit, so it passed when the run died for an unrelated reason, and the comment above the API base check no longer described what the check did once the stub moved the bucket onto the API host. Each assertion was confirmed failing against the code it guards. 31 checks, --check passes, semgrep p/python, p/security-audit and p/secrets report 0 findings on .github/scripts/. --- .github/scripts/test_zenodo_release.py | 91 +++++++++++++++++--------- .github/scripts/zenodo_release.py | 10 ++- documentation/ZENODO.md | 16 ++++- 3 files changed, 83 insertions(+), 34 deletions(-) diff --git a/.github/scripts/test_zenodo_release.py b/.github/scripts/test_zenodo_release.py index 77f1aef..7c9d509 100644 --- a/.github/scripts/test_zenodo_release.py +++ b/.github/scripts/test_zenodo_release.py @@ -102,6 +102,28 @@ 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 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() @@ -151,9 +173,10 @@ def run( 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"] @@ -168,19 +191,7 @@ def run( 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") -# The token belongs in a header. Query strings reach access logs, egress proxies -# and Referer headers, all of which outlive the request. -check( - "token sent as an Authorization Bearer header", - all((k.get("headers") or {}).get("Authorization") == "Bearer fake-token" for _, _, k in CALLS), -) -check( - "token never reaches a query string", - not any( - "access_token" in u or "access_token" in (k.get("params") or {}) - for _, u, k in CALLS - ), -) +check_token("first deposit") run( "new version with inherited files, sandbox, draft only", @@ -195,26 +206,46 @@ def run( 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. -CALLS = [] -install_fake_requests(False, None, "https://evil.invalid/api") -set_env(None) -bundle, metadata = build_inputs() -print("\nbucket URL on a host the script did not verify") -try: - z.deposit(bundle, metadata, concept_env="TEST_CONCEPT", sandbox=False, publish=True) - refused = False -except SystemExit: - refused = True -check("refuses to upload", refused) +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: print(f"{len(FAILURES)} failed: {', '.join(FAILURES)}") diff --git a/.github/scripts/zenodo_release.py b/.github/scripts/zenodo_release.py index b7ea320..194f8f4 100644 --- a/.github/scripts/zenodo_release.py +++ b/.github/scripts/zenodo_release.py @@ -263,12 +263,18 @@ def upload_url(draft: dict, base: str, filename: str) -> str: 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. Fail closed - when the bucket does not sit on the API host the script chose. + 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: diff --git a/documentation/ZENODO.md b/documentation/ZENODO.md index a143ca2..a1437c0 100644 --- a/documentation/ZENODO.md +++ b/documentation/ZENODO.md @@ -103,8 +103,10 @@ 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 -the bucket sits on the same host as the API before attaching the token. If -Zenodo ever moves file storage to a different host, the deposit stops with: +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' @@ -115,6 +117,16 @@ 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: