From 74bd6dd6daf5a77eeb8da27de393ebf7c7794e21 Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Sun, 9 Aug 2026 10:55:27 -0500 Subject: [PATCH 1/4] fix(pipeline): correct dispatch correlation and log rendering --- src/odoo_forge_cli/commands/manifest.py | 12 +- src/odoo_forge_pipeline_github/provider.py | 3 +- src/odoo_forge_pipeline_github/transport.py | 59 +++++---- tests/cli/test_configure.py | 11 +- tests/pipeline_github/fakes.py | 12 +- tests/pipeline_github/test_hermetic.py | 2 +- tests/pipeline_github/test_neutrality.py | 2 +- tests/pipeline_github/test_rest_transport.py | 112 ++++++++++++++++++ .../test_transport_protocol.py | 5 +- tests/pipeline_github/test_trigger.py | 9 +- 10 files changed, 166 insertions(+), 61 deletions(-) create mode 100644 tests/pipeline_github/test_rest_transport.py diff --git a/src/odoo_forge_cli/commands/manifest.py b/src/odoo_forge_cli/commands/manifest.py index 13ae6a7..ece3e5f 100644 --- a/src/odoo_forge_cli/commands/manifest.py +++ b/src/odoo_forge_cli/commands/manifest.py @@ -94,11 +94,7 @@ def _collect_published_layer() -> dict[str, object]: def _collect_layers() -> list[dict[str, object]]: layers: list[dict[str, object]] = [] while typer.confirm("Add a layer?" if not layers else "Add another layer?", default=False): - layer_type = _prompt_text("Layer type (git or published)").lower() - while layer_type not in {"git", "published"}: - typer.echo("error: layer type must be 'git' or 'published'", err=True) - layer_type = _prompt_text("Layer type (git or published)").lower() - layers.append(_collect_git_layer() if layer_type == "git" else _collect_published_layer()) + layers.append(_collect_git_layer()) return layers @@ -162,11 +158,7 @@ def _collect_draft() -> dict[str, object]: layers = _collect_layers() draft["layers"] = layers addons_path = _prompt_text("Client addons path") - requirements = _prompt_text("Python requirements path", "") - client: dict[str, object] = {"addons_path": addons_path} - if requirements: - client["python_requirements"] = requirements - draft["client"] = client + draft["client"] = {"addons_path": addons_path} draft["overrides"] = _collect_overrides() if typer.confirm("Configure workspace", default=False): diff --git a/src/odoo_forge_pipeline_github/provider.py b/src/odoo_forge_pipeline_github/provider.py index 461b51b..8afa46e 100644 --- a/src/odoo_forge_pipeline_github/provider.py +++ b/src/odoo_forge_pipeline_github/provider.py @@ -60,8 +60,7 @@ def __init__( self._ref = ref def trigger(self, spec: PipelineRunSpec) -> PipelineRunRef: - self._transport.dispatch_workflow(spec.definition, self._ref, spec.parameters) - run_id = self._transport.latest_run_id(spec.definition, self._ref) + run_id = self._transport.dispatch_workflow(spec.definition, self._ref, spec.parameters) return PipelineRunRef(run_id=run_id) def status(self, ref: PipelineRunRef) -> PipelineRunStatus: diff --git a/src/odoo_forge_pipeline_github/transport.py b/src/odoo_forge_pipeline_github/transport.py index bab4e97..f59b25e 100644 --- a/src/odoo_forge_pipeline_github/transport.py +++ b/src/odoo_forge_pipeline_github/transport.py @@ -9,22 +9,24 @@ from __future__ import annotations +import io import json import urllib.request +import zipfile +from pathlib import PurePosixPath from typing import Protocol, runtime_checkable GITHUB_API_BASE_URL = "https://api.github.com" DEFAULT_TIMEOUT_SECONDS = 30.0 +MAX_LOG_ARCHIVE_BYTES = 20 * 1024 * 1024 +MAX_LOG_BYTES = 100 * 1024 * 1024 +MAX_LOG_ENTRIES = 1000 @runtime_checkable class GitHubActionsTransport(Protocol): - def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> None: - """Trigger a `workflow_dispatch` event for `workflow` on `ref`.""" - ... - - def latest_run_id(self, workflow: str, ref: str) -> str: - """Return the id of the newest run for `workflow` on `ref`.""" + def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> str: + """Trigger `workflow` on `ref` and return the dispatched run id.""" ... def get_run_state(self, run_id: str) -> tuple[str, str | None]: @@ -54,24 +56,20 @@ def __init__( self._base_url = base_url.rstrip("/") self._timeout = timeout - def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> None: + def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> str: url = ( f"{self._base_url}/repos/{self._owner}/{self._repo}/actions/" f"workflows/{workflow}/dispatches" ) body = json.dumps({"ref": ref, "inputs": inputs}).encode("utf-8") - self._request(url, method="POST", body=body) - - def latest_run_id(self, workflow: str, ref: str) -> str: - url = ( - f"{self._base_url}/repos/{self._owner}/{self._repo}/actions/workflows/" - f"{workflow}/runs?branch={ref}&per_page=1" - ) - payload = json.loads(self._request(url, method="GET")) - runs = payload.get("workflow_runs", []) - if not runs: - raise RuntimeError(f"no runs found for workflow {workflow!r} on ref {ref!r}") - return str(runs[0]["id"]) + try: + payload = json.loads(self._request(url, method="POST", body=body)) + run_id = payload["workflow_run_id"] + except (json.JSONDecodeError, KeyError, TypeError) as exc: + raise RuntimeError("workflow dispatch response has no workflow run id") from exc + if isinstance(run_id, bool) or not isinstance(run_id, (int, str)) or not str(run_id): + raise RuntimeError("workflow dispatch response has no workflow run id") + return str(run_id) def get_run_state(self, run_id: str) -> tuple[str, str | None]: url = f"{self._base_url}/repos/{self._owner}/{self._repo}/actions/runs/{run_id}" @@ -80,7 +78,27 @@ def get_run_state(self, run_id: str) -> tuple[str, str | None]: def get_run_logs(self, run_id: str) -> str: url = f"{self._base_url}/repos/{self._owner}/{self._repo}/actions/runs/{run_id}/logs" - return self._request(url, method="GET").decode("utf-8", errors="replace") + archive_bytes = self._request(url, method="GET") + if len(archive_bytes) > MAX_LOG_ARCHIVE_BYTES: + raise RuntimeError("log archive exceeds the compressed size limit") + try: + with zipfile.ZipFile(io.BytesIO(archive_bytes)) as archive: + entries = [entry for entry in archive.infolist() if not entry.is_dir()] + if len(entries) > MAX_LOG_ENTRIES: + raise RuntimeError("log archive contains too many entries") + if sum(entry.file_size for entry in entries) > MAX_LOG_BYTES: + raise RuntimeError("log archive exceeds the uncompressed size limit") + + output: list[str] = [] + for entry in sorted(entries, key=lambda item: item.filename): + normalized = entry.filename.replace("\\", "/") + path = PurePosixPath(normalized) + if path.is_absolute() or ".." in path.parts or entry.flag_bits & 0x1: + raise RuntimeError("log archive contains an unsafe entry") + output.append(archive.read(entry).decode("utf-8", errors="replace")) + return "".join(output) + except zipfile.BadZipFile as exc: + raise RuntimeError("invalid log archive") from exc def _request(self, url: str, *, method: str, body: bytes | None = None) -> bytes: request = urllib.request.Request( @@ -90,6 +108,7 @@ def _request(self, url: str, *, method: str, body: bytes | None = None) -> bytes headers={ "Authorization": f"Bearer {self._token}", "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2026-03-10", }, ) with urllib.request.urlopen(request, timeout=self._timeout) as response: # noqa: S310 diff --git a/tests/cli/test_configure.py b/tests/cli/test_configure.py index 185335b..af5037f 100644 --- a/tests/cli/test_configure.py +++ b/tests/cli/test_configure.py @@ -17,7 +17,7 @@ def _invoke_configure(target: Path, *, name: str = "demo", edition: str = "community") -> Any: - scripted_input = f"{name}\n19.0\n{edition}\n\n\nn\nclient/addons\n\nn\nn\nn\nn\ny\n" + scripted_input = f"{name}\n19.0\n{edition}\n\n\nn\nclient/addons\nn\nn\nn\nn\ny\n" return runner.invoke(app, ["configure", "--manifest", str(target)], input=scripted_input) @@ -70,7 +70,6 @@ def test_configure_community_yaml(tmp_path: Path) -> None: def test_configure_enterprise_collects_all_optional_branches( monkeypatch: pytest.MonkeyPatch, - capsys: pytest.CaptureFixture[str], ) -> None: prompts = { "Project name": "enterprise-demo", @@ -80,17 +79,11 @@ def test_configure_enterprise_collects_all_optional_branches( "Core ref override": "stable", "Enterprise URL override": "https://example.test/enterprise.git", "Enterprise ref override": "stable", - "Layer type": ["unsupported", "git", "published"], "Layer name": "custom", "Layer category": "localization", "Repository URL": "https://example.test/custom.git", "Repository ref": "main", - "Published layer name": "published", - "Published layer source": "registry://example/addons", - "Published layer version": "1.0", - "Published layer category": "enterprise-addons", "Client addons path": "client/addons", - "Python requirements path": "client/requirements.txt", "Override layer": "custom", "Override repository": "https://example.test/custom.git", "Override fork": "https://example.test/fork.git", @@ -104,7 +97,6 @@ def test_configure_enterprise_collects_all_optional_branches( "Add a layer": True, "Add another repository": False, "Add another layer": [True, False], - "Published layer requires enterprise": True, "Add another override": False, "Add an override": True, "Configure workspace": True, @@ -119,7 +111,6 @@ def test_configure_enterprise_collects_all_optional_branches( manifest.configure(Path("project.yaml")) assert raised.value.exit_code == 0 - assert "layer type must be 'git' or 'published'" in capsys.readouterr().err def test_configure_rejects_existing_target_before_prompt(tmp_path: Path) -> None: diff --git a/tests/pipeline_github/fakes.py b/tests/pipeline_github/fakes.py index 4a0646e..37dd7ac 100644 --- a/tests/pipeline_github/fakes.py +++ b/tests/pipeline_github/fakes.py @@ -13,24 +13,20 @@ class FakeGitHubActionsTransport: def __init__( self, *, - run_ids: list[str] | None = None, + run_id: str = "1", run_state: tuple[str, str | None] = ("queued", None), run_logs: str = "", ) -> None: self.dispatch_calls: list[tuple[str, str, dict[str, str]]] = [] - self.latest_run_id_calls: list[tuple[str, str]] = [] self.get_run_state_calls: list[str] = [] self.get_run_logs_calls: list[str] = [] - self._run_ids = run_ids if run_ids is not None else ["1"] + self._run_id = run_id self._run_state = run_state self._run_logs = run_logs - def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> None: + def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> str: self.dispatch_calls.append((workflow, ref, inputs)) - - def latest_run_id(self, workflow: str, ref: str) -> str: - self.latest_run_id_calls.append((workflow, ref)) - return self._run_ids[-1] + return self._run_id def get_run_state(self, run_id: str) -> tuple[str, str | None]: self.get_run_state_calls.append(run_id) diff --git a/tests/pipeline_github/test_hermetic.py b/tests/pipeline_github/test_hermetic.py index 568cd00..3a02183 100644 --- a/tests/pipeline_github/test_hermetic.py +++ b/tests/pipeline_github/test_hermetic.py @@ -18,7 +18,7 @@ def _raise(*args: object, **kwargs: object) -> None: def test_trigger_status_logs_never_touch_the_network(block_network: None) -> None: fake = FakeGitHubActionsTransport( - run_ids=["1"], run_state=("in_progress", None), run_logs="hermetic log" + run_id="1", run_state=("in_progress", None), run_logs="hermetic log" ) provider = GitHubActionsPipelineProvider( transport=fake, owner="acme", repo="widgets", ref="main" diff --git a/tests/pipeline_github/test_neutrality.py b/tests/pipeline_github/test_neutrality.py index 183ebf1..5da9ee3 100644 --- a/tests/pipeline_github/test_neutrality.py +++ b/tests/pipeline_github/test_neutrality.py @@ -5,7 +5,7 @@ def test_trigger_status_logs_return_exactly_the_neutral_types() -> None: fake = FakeGitHubActionsTransport( - run_ids=["55"], run_state=("completed", "success"), run_logs="log text" + run_id="55", run_state=("completed", "success"), run_logs="log text" ) provider = GitHubActionsPipelineProvider( transport=fake, owner="acme", repo="widgets", ref="main" diff --git a/tests/pipeline_github/test_rest_transport.py b/tests/pipeline_github/test_rest_transport.py new file mode 100644 index 0000000..4d23f4a --- /dev/null +++ b/tests/pipeline_github/test_rest_transport.py @@ -0,0 +1,112 @@ +import io +import json +import urllib.request +import zipfile + +import pytest + +from odoo_forge_pipeline_github.transport import GitHubActionsRestTransport + + +def _transport() -> GitHubActionsRestTransport: + return GitHubActionsRestTransport(token="token", owner="acme", repo="widgets") + + +def _zip_bytes(entries: dict[str, bytes]) -> bytes: + output = io.BytesIO() + with zipfile.ZipFile(output, "w") as archive: + for name, content in entries.items(): + archive.writestr(name, content) + return output.getvalue() + + +def test_requests_pin_the_github_api_version(monkeypatch: pytest.MonkeyPatch) -> None: + captured_request: urllib.request.Request | None = None + + class Response: + def __enter__(self) -> "Response": + return self + + def __exit__(self, *args: object) -> None: + return None + + def read(self) -> bytes: + return b"{}" + + def urlopen(request: urllib.request.Request, *, timeout: float) -> Response: + nonlocal captured_request + captured_request = request + return Response() + + monkeypatch.setattr(urllib.request, "urlopen", urlopen) + + _transport()._request("https://example.test", method="GET") + + assert captured_request is not None + assert captured_request.get_header("X-github-api-version") == "2026-03-10" + + +def test_dispatch_returns_the_exact_workflow_run_id( + monkeypatch: pytest.MonkeyPatch, +) -> None: + transport = _transport() + request_calls: list[tuple[str, str, bytes | None]] = [] + + def request(url: str, *, method: str, body: bytes | None = None) -> bytes: + request_calls.append((url, method, body)) + return json.dumps({"workflow_run_id": 314}).encode() + + monkeypatch.setattr(transport, "_request", request) + + assert transport.dispatch_workflow("ci.yml", "main", {"env": "qa"}) == "314" + assert request_calls[0][1:] == ( + "POST", + json.dumps({"ref": "main", "inputs": {"env": "qa"}}).encode(), + ) + + +@pytest.mark.parametrize("response", [b"{}", b"not-json"]) +def test_dispatch_fails_when_the_run_id_is_missing_or_malformed( + response: bytes, monkeypatch: pytest.MonkeyPatch +) -> None: + transport = _transport() + monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: response) + + with pytest.raises(RuntimeError, match="workflow run id"): + transport.dispatch_workflow("ci.yml", "main", {}) + + +def test_logs_render_real_zip_entries_in_deterministic_name_order( + monkeypatch: pytest.MonkeyPatch, +) -> None: + transport = _transport() + archive = _zip_bytes({"job/z.txt": b"last\n", "job/a.txt": b"first\n"}) + monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: archive) + + assert transport.get_run_logs("42") == "first\nlast\n" + + +@pytest.mark.parametrize( + "archive", + [b"not-a-zip", _zip_bytes({"../secret.txt": b"secret"}), _zip_bytes({"/abs.txt": b"x"})], +) +def test_logs_reject_malformed_or_unsafe_archives( + archive: bytes, monkeypatch: pytest.MonkeyPatch +) -> None: + transport = _transport() + monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: archive) + + with pytest.raises(RuntimeError, match="log archive"): + transport.get_run_logs("42") + + +def test_logs_reject_archives_over_the_uncompressed_limit( + monkeypatch: pytest.MonkeyPatch, +) -> None: + transport = _transport() + archive = _zip_bytes({"large.txt": b"x" * 101}) + monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: archive) + monkeypatch.setattr("odoo_forge_pipeline_github.transport.MAX_LOG_BYTES", 100) + + with pytest.raises(RuntimeError, match="log archive"): + transport.get_run_logs("42") diff --git a/tests/pipeline_github/test_transport_protocol.py b/tests/pipeline_github/test_transport_protocol.py index bcf6902..22a0b3e 100644 --- a/tests/pipeline_github/test_transport_protocol.py +++ b/tests/pipeline_github/test_transport_protocol.py @@ -2,10 +2,7 @@ class _FakeTransportForProtocolCheck: - def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> None: - return None - - def latest_run_id(self, workflow: str, ref: str) -> str: + def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> str: return "1" def get_run_state(self, run_id: str) -> tuple[str, str | None]: diff --git a/tests/pipeline_github/test_trigger.py b/tests/pipeline_github/test_trigger.py index d8ddb07..aea75e0 100644 --- a/tests/pipeline_github/test_trigger.py +++ b/tests/pipeline_github/test_trigger.py @@ -3,8 +3,8 @@ from tests.pipeline_github.fakes import FakeGitHubActionsTransport -def test_trigger_dispatches_workflow_and_returns_newest_run_ref() -> None: - fake = FakeGitHubActionsTransport(run_ids=["101", "202"]) +def test_trigger_returns_the_dispatched_run_without_a_newest_run_lookup() -> None: + fake = FakeGitHubActionsTransport(run_id="101") provider = GitHubActionsPipelineProvider( transport=fake, owner="acme", repo="widgets", ref="main" ) @@ -13,12 +13,11 @@ def test_trigger_dispatches_workflow_and_returns_newest_run_ref() -> None: result = provider.trigger(spec) assert fake.dispatch_calls == [("ci.yml", "main", {"env": "staging"})] - assert fake.latest_run_id_calls == [("ci.yml", "main")] - assert result == PipelineRunRef(run_id="202") + assert result == PipelineRunRef(run_id="101") def test_trigger_uses_the_configured_ref_for_a_different_provider() -> None: - fake = FakeGitHubActionsTransport(run_ids=["7"]) + fake = FakeGitHubActionsTransport(run_id="7") provider = GitHubActionsPipelineProvider( transport=fake, owner="acme", repo="widgets", ref="release/1.0" ) From 2a91f64bfb70f964e689424e31743a1a964931da Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Sun, 9 Aug 2026 11:02:46 -0500 Subject: [PATCH 2/4] fix(pipeline): handle malformed dispatch encoding --- src/odoo_forge_pipeline_github/transport.py | 2 +- tests/pipeline_github/test_rest_transport.py | 43 +++++++++----------- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/odoo_forge_pipeline_github/transport.py b/src/odoo_forge_pipeline_github/transport.py index f59b25e..fd3da51 100644 --- a/src/odoo_forge_pipeline_github/transport.py +++ b/src/odoo_forge_pipeline_github/transport.py @@ -65,7 +65,7 @@ def dispatch_workflow(self, workflow: str, ref: str, inputs: dict[str, str]) -> try: payload = json.loads(self._request(url, method="POST", body=body)) run_id = payload["workflow_run_id"] - except (json.JSONDecodeError, KeyError, TypeError) as exc: + except (UnicodeDecodeError, json.JSONDecodeError, KeyError, TypeError) as exc: raise RuntimeError("workflow dispatch response has no workflow run id") from exc if isinstance(run_id, bool) or not isinstance(run_id, (int, str)) or not str(run_id): raise RuntimeError("workflow dispatch response has no workflow run id") diff --git a/tests/pipeline_github/test_rest_transport.py b/tests/pipeline_github/test_rest_transport.py index 4d23f4a..c283512 100644 --- a/tests/pipeline_github/test_rest_transport.py +++ b/tests/pipeline_github/test_rest_transport.py @@ -20,8 +20,8 @@ def _zip_bytes(entries: dict[str, bytes]) -> bytes: return output.getvalue() -def test_requests_pin_the_github_api_version(monkeypatch: pytest.MonkeyPatch) -> None: - captured_request: urllib.request.Request | None = None +def _mock_urlopen(monkeypatch: pytest.MonkeyPatch, response: bytes) -> list[urllib.request.Request]: + requests: list[urllib.request.Request] = [] class Response: def __enter__(self) -> "Response": @@ -31,46 +31,41 @@ def __exit__(self, *args: object) -> None: return None def read(self) -> bytes: - return b"{}" + return response def urlopen(request: urllib.request.Request, *, timeout: float) -> Response: - nonlocal captured_request - captured_request = request + requests.append(request) return Response() monkeypatch.setattr(urllib.request, "urlopen", urlopen) + return requests + + +def test_requests_pin_the_github_api_version(monkeypatch: pytest.MonkeyPatch) -> None: + requests = _mock_urlopen(monkeypatch, b'{"workflow_run_id": 314}') - _transport()._request("https://example.test", method="GET") + _transport().dispatch_workflow("ci.yml", "main", {}) - assert captured_request is not None - assert captured_request.get_header("X-github-api-version") == "2026-03-10" + assert requests[0].get_header("X-github-api-version") == "2026-03-10" def test_dispatch_returns_the_exact_workflow_run_id( monkeypatch: pytest.MonkeyPatch, ) -> None: transport = _transport() - request_calls: list[tuple[str, str, bytes | None]] = [] - - def request(url: str, *, method: str, body: bytes | None = None) -> bytes: - request_calls.append((url, method, body)) - return json.dumps({"workflow_run_id": 314}).encode() - - monkeypatch.setattr(transport, "_request", request) + requests = _mock_urlopen(monkeypatch, json.dumps({"workflow_run_id": 314}).encode()) assert transport.dispatch_workflow("ci.yml", "main", {"env": "qa"}) == "314" - assert request_calls[0][1:] == ( - "POST", - json.dumps({"ref": "main", "inputs": {"env": "qa"}}).encode(), - ) + assert requests[0].method == "POST" + assert requests[0].data == json.dumps({"ref": "main", "inputs": {"env": "qa"}}).encode() -@pytest.mark.parametrize("response", [b"{}", b"not-json"]) +@pytest.mark.parametrize("response", [b"{}", b"not-json", b"\xff"]) def test_dispatch_fails_when_the_run_id_is_missing_or_malformed( response: bytes, monkeypatch: pytest.MonkeyPatch ) -> None: transport = _transport() - monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: response) + _mock_urlopen(monkeypatch, response) with pytest.raises(RuntimeError, match="workflow run id"): transport.dispatch_workflow("ci.yml", "main", {}) @@ -81,7 +76,7 @@ def test_logs_render_real_zip_entries_in_deterministic_name_order( ) -> None: transport = _transport() archive = _zip_bytes({"job/z.txt": b"last\n", "job/a.txt": b"first\n"}) - monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: archive) + _mock_urlopen(monkeypatch, archive) assert transport.get_run_logs("42") == "first\nlast\n" @@ -94,7 +89,7 @@ def test_logs_reject_malformed_or_unsafe_archives( archive: bytes, monkeypatch: pytest.MonkeyPatch ) -> None: transport = _transport() - monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: archive) + _mock_urlopen(monkeypatch, archive) with pytest.raises(RuntimeError, match="log archive"): transport.get_run_logs("42") @@ -105,7 +100,7 @@ def test_logs_reject_archives_over_the_uncompressed_limit( ) -> None: transport = _transport() archive = _zip_bytes({"large.txt": b"x" * 101}) - monkeypatch.setattr(transport, "_request", lambda *args, **kwargs: archive) + _mock_urlopen(monkeypatch, archive) monkeypatch.setattr("odoo_forge_pipeline_github.transport.MAX_LOG_BYTES", 100) with pytest.raises(RuntimeError, match="log archive"): From a9e21f667bcb1522b9ff71a57e073646122c506e Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Sun, 9 Aug 2026 11:12:09 -0500 Subject: [PATCH 3/4] fix(pipeline): normalize corrupted log entries --- src/odoo_forge_pipeline_github/transport.py | 7 ++++++- tests/pipeline_github/test_rest_transport.py | 22 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/odoo_forge_pipeline_github/transport.py b/src/odoo_forge_pipeline_github/transport.py index fd3da51..8b5f472 100644 --- a/src/odoo_forge_pipeline_github/transport.py +++ b/src/odoo_forge_pipeline_github/transport.py @@ -13,6 +13,7 @@ import json import urllib.request import zipfile +import zlib from pathlib import PurePosixPath from typing import Protocol, runtime_checkable @@ -95,7 +96,11 @@ def get_run_logs(self, run_id: str) -> str: path = PurePosixPath(normalized) if path.is_absolute() or ".." in path.parts or entry.flag_bits & 0x1: raise RuntimeError("log archive contains an unsafe entry") - output.append(archive.read(entry).decode("utf-8", errors="replace")) + try: + content = archive.read(entry) + except (zipfile.BadZipFile, EOFError, NotImplementedError, zlib.error) as exc: + raise RuntimeError("invalid log archive") from exc + output.append(content.decode("utf-8", errors="replace")) return "".join(output) except zipfile.BadZipFile as exc: raise RuntimeError("invalid log archive") from exc diff --git a/tests/pipeline_github/test_rest_transport.py b/tests/pipeline_github/test_rest_transport.py index c283512..5e27cee 100644 --- a/tests/pipeline_github/test_rest_transport.py +++ b/tests/pipeline_github/test_rest_transport.py @@ -20,6 +20,19 @@ def _zip_bytes(entries: dict[str, bytes]) -> bytes: return output.getvalue() +def _corrupted_compressed_entry() -> bytes: + output = io.BytesIO() + with zipfile.ZipFile(output, "w", compression=zipfile.ZIP_DEFLATED) as archive: + archive.writestr("job.txt", b"workflow output\n" * 100) + + corrupted = bytearray(output.getvalue()) + with zipfile.ZipFile(io.BytesIO(corrupted)) as archive: + entry = archive.getinfo("job.txt") + data_offset = entry.header_offset + 30 + len(entry.filename.encode()) + len(entry.extra) + corrupted[data_offset] = (corrupted[data_offset] & ~0x06) | 0x06 + return bytes(corrupted) + + def _mock_urlopen(monkeypatch: pytest.MonkeyPatch, response: bytes) -> list[urllib.request.Request]: requests: list[urllib.request.Request] = [] @@ -56,6 +69,7 @@ def test_dispatch_returns_the_exact_workflow_run_id( requests = _mock_urlopen(monkeypatch, json.dumps({"workflow_run_id": 314}).encode()) assert transport.dispatch_workflow("ci.yml", "main", {"env": "qa"}) == "314" + assert len(requests) == 1 assert requests[0].method == "POST" assert requests[0].data == json.dumps({"ref": "main", "inputs": {"env": "qa"}}).encode() @@ -105,3 +119,11 @@ def test_logs_reject_archives_over_the_uncompressed_limit( with pytest.raises(RuntimeError, match="log archive"): transport.get_run_logs("42") + + +def test_logs_reject_corrupted_compressed_entries(monkeypatch: pytest.MonkeyPatch) -> None: + transport = _transport() + _mock_urlopen(monkeypatch, _corrupted_compressed_entry()) + + with pytest.raises(RuntimeError, match="invalid log archive"): + transport.get_run_logs("42") From 2682cd8b69588cbc5eacda7e470675d1754d5bfd Mon Sep 17 00:00:00 2001 From: Angel Parra <607418+aparragithub@users.noreply.github.com> Date: Sun, 9 Aug 2026 11:20:41 -0500 Subject: [PATCH 4/4] fix(pipeline): normalize malformed archive names --- src/odoo_forge_pipeline_github/transport.py | 2 +- tests/pipeline_github/test_rest_transport.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/odoo_forge_pipeline_github/transport.py b/src/odoo_forge_pipeline_github/transport.py index 8b5f472..9ee41bd 100644 --- a/src/odoo_forge_pipeline_github/transport.py +++ b/src/odoo_forge_pipeline_github/transport.py @@ -102,7 +102,7 @@ def get_run_logs(self, run_id: str) -> str: raise RuntimeError("invalid log archive") from exc output.append(content.decode("utf-8", errors="replace")) return "".join(output) - except zipfile.BadZipFile as exc: + except (zipfile.BadZipFile, UnicodeDecodeError) as exc: raise RuntimeError("invalid log archive") from exc def _request(self, url: str, *, method: str, body: bytes | None = None) -> bytes: diff --git a/tests/pipeline_github/test_rest_transport.py b/tests/pipeline_github/test_rest_transport.py index 5e27cee..36c11e3 100644 --- a/tests/pipeline_github/test_rest_transport.py +++ b/tests/pipeline_github/test_rest_transport.py @@ -127,3 +127,14 @@ def test_logs_reject_corrupted_compressed_entries(monkeypatch: pytest.MonkeyPatc with pytest.raises(RuntimeError, match="invalid log archive"): transport.get_run_logs("42") + + +def test_logs_reject_malformed_utf8_entry_names(monkeypatch: pytest.MonkeyPatch) -> None: + archive = bytearray(_zip_bytes({"job.txt": b"output"})) + central_header = archive.index(b"PK\x01\x02") + archive[central_header + 8 : central_header + 10] = (0x800).to_bytes(2, "little") + archive[central_header + 46] = 0xFF + _mock_urlopen(monkeypatch, bytes(archive)) + + with pytest.raises(RuntimeError, match="invalid log archive"): + _transport().get_run_logs("42")