From b660495c6f00109043053bea417a436d2a3db3a3 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Tue, 8 Sep 2026 07:37:07 +0900 Subject: [PATCH 1/4] test(artifact): cover the RECOTEM_ARTIFACT_ROOT write-time containment call site Removing the _assert_output_root_containment call from _write_atomic passed the whole suite. Measured against the same tree with only that line replaced by `pass`, a directory that becomes a symlink out of the artifact root after recipe load lets write_artifact complete and land bytes outside the root; with the guard it raises ArtifactError and writes nothing. The existing tests missed it two ways. Every other test around this guard calls _assert_output_root_containment directly, so the helper is covered but its use is not. The one test that drives _write_atomic accepts (ArtifactError, OSError), and its own setup rmtree's the directory holding the temp file, so os.replace raises OSError on a tree with no guard at all -- it passed on both arms. Tighten that assertion to ArtifactError matching RECOTEM_ARTIFACT_ROOT and assert nothing landed outside; the guard runs before os.replace, so on a tree that has it the ArtifactError always wins. Add two tests through the real write_artifact entry point -- parent directory symlinked out, and destination file itself symlinked out. The second is not redundant: a guard pointed at the temp path instead of at dest still refuses the first case, because the temp file is created inside the same escaped directory. A control asserts the same call succeeds against a real in-root directory, so the pair cannot be satisfied by a write_artifact that refuses everything. Mutation matrix at the call site: call deleted -> caught (3 tests); guard watches the temp path instead of dest -> caught (1 test); guard runs only when dest already exists -> caught (3 tests); unmutated -> 35 passed. --- tests/unit/test_artifact_io.py | 122 +++++++++++++++++++++++++++++++-- 1 file changed, 118 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_artifact_io.py b/tests/unit/test_artifact_io.py index 4c2f6141..4c4f1e97 100644 --- a/tests/unit/test_artifact_io.py +++ b/tests/unit/test_artifact_io.py @@ -391,10 +391,14 @@ def _evil_fsync(fd: int) -> None: monkeypatch.setattr(_os, "fsync", _evil_fsync) - with pytest.raises((ArtifactError, OSError)): - # _write_atomic must either detect the escape and raise ArtifactError, - # or raise OSError because the temp file's parent (inside_dir) no longer - # exists as a real directory after the symlink swap. + # ArtifactError specifically, not (ArtifactError, OSError). The swap also + # destroys the directory holding the temp file, so ``os.replace`` would + # raise OSError on its own -- accepting either exception let the test pass + # whether or not the containment re-check ran at all, which is what it + # exists to prove. ``_write_atomic`` calls the check *before* + # ``os.replace``, so on a tree that still has the guard the ArtifactError + # always wins the race to be raised. + with pytest.raises(ArtifactError, match="RECOTEM_ARTIFACT_ROOT"): _write_atomic( None, # type: ignore[arg-type] — not used for local FS path dest, @@ -402,6 +406,116 @@ def _evil_fsync(fd: int) -> None: is_local=True, ) + assert not list(outside_dir.iterdir()), ( + "the containment re-check must refuse before os.replace, so no bytes " + f"may land outside the artifact root; found {list(outside_dir.iterdir())}" + ) + + +def test_write_artifact_refuses_symlinked_parent_out_of_artifact_root( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """``write_artifact`` itself must enforce RECOTEM_ARTIFACT_ROOT containment. + + The recipe loader validates ``output.path`` at load time; the write-time + re-check in ``_write_atomic`` exists because a directory under the root can + become a symlink out of it between those two moments. Every other test + around this guard calls ``_assert_output_root_containment`` directly, so + deleting its *call site* changed nothing they measure -- and the write then + completes, landing artifact bytes outside the root. + + The symlink is in place before the call here (rather than swapped in at + fsync time as in the TOCTOU test above) so that the temp file remains + writable and renameable: the containment re-check is then the only thing + that can refuse, and a pass cannot be borrowed from an incidental OSError. + """ + artifact_root = tmp_path / "root" + artifact_root.mkdir() + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + (artifact_root / "models").symlink_to(outside_dir, target_is_directory=True) + + monkeypatch.setenv("RECOTEM_ARTIFACT_ROOT", str(artifact_root)) + dest = artifact_root / "models" / "model.recotem" + + with pytest.raises(ArtifactError, match="RECOTEM_ARTIFACT_ROOT"): + write_artifact( + {"payload": "x"}, + {"recipe_name": "probe"}, + _make_keyring(), + str(dest), + versioning="always_overwrite", + ) + + assert list(outside_dir.iterdir()) == [], ( + "artifact bytes escaped RECOTEM_ARTIFACT_ROOT through a symlinked " + f"parent: {list(outside_dir.iterdir())}" + ) + + +def test_write_artifact_refuses_dest_that_is_itself_a_symlink_out_of_root( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The final path component is checked too, not only its parent directory. + + ``_assert_output_root_containment`` resolves the parent *and* the + destination. The parent-symlink case above cannot tell the two apart: the + temp file is created inside the same escaped directory, so a check pointed + at the temp path instead of at ``dest`` would refuse there as well. Here + the directory is a genuine in-root directory -- only ``dest`` itself is a + symlink out -- so a check that does not resolve ``dest`` lets the write + through and the bytes follow the symlink. + """ + artifact_root = tmp_path / "root" + (artifact_root / "models").mkdir(parents=True) + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + + dest = artifact_root / "models" / "model.recotem" + dest.symlink_to(outside_dir / "model.recotem") + + monkeypatch.setenv("RECOTEM_ARTIFACT_ROOT", str(artifact_root)) + + with pytest.raises(ArtifactError, match="RECOTEM_ARTIFACT_ROOT"): + write_artifact( + {"payload": "x"}, + {"recipe_name": "probe"}, + _make_keyring(), + str(dest), + versioning="always_overwrite", + ) + + assert not (outside_dir / "model.recotem").exists(), ( + "artifact bytes escaped RECOTEM_ARTIFACT_ROOT through a symlinked " + "destination file" + ) + + +def test_write_artifact_accepts_real_directory_inside_artifact_root( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Control for the test above: the same call succeeds without the symlink. + + Without this, the sibling test would also pass against a ``write_artifact`` + that refused every write. + """ + artifact_root = tmp_path / "root" + (artifact_root / "models").mkdir(parents=True) + + monkeypatch.setenv("RECOTEM_ARTIFACT_ROOT", str(artifact_root)) + dest = artifact_root / "models" / "model.recotem" + + written = write_artifact( + {"payload": "x"}, + {"recipe_name": "probe"}, + _make_keyring(), + str(dest), + versioning="always_overwrite", + ) + + assert Path(written) == dest + assert dest.exists() + def test_assert_output_root_containment_no_op_without_env(tmp_path: Path) -> None: """_assert_output_root_containment is a no-op when RECOTEM_ARTIFACT_ROOT is unset.""" From 059f04c1ac4e84f9c3fffb197df5330f586bcfc7 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Tue, 8 Sep 2026 07:37:43 +0900 Subject: [PATCH 2/4] docs(claude-md): the header_len boundary claim overstates when HMAC: OK appears CLAUDE.md said that moving the 4-byte header_len field "still passes verify_hmac (recotem inspect prints HMAC: OK) and is caught one layer later by the header JSON parse or the deserializer". Measured across ten boundary moves on a real signed artifact, with a tripwire on SafeUnpickler.load: - Only the three shrinking moves (header_len lowered, 0 included) reach verify_hmac and print HMAC: OK. The authenticated run is unchanged, so the verify passes; the header JSON parse then fails on the truncated slice. - All seven enlarging moves are refused earlier, inside parse_header_from_bytes, so verify_hmac is never called and HMAC: OK is never printed: the widened header slice swallows payload bytes and fails the UTF-8 decode, or the file is shorter than the claimed header, or the value trips the 64 KiB MAX_HEADER_LEN cap. - The deserializer never catches this. The header JSON parse always fires first, on the CLI path and on the serve path (ModelRegistry._build_entry decodes the header before unpickle_payload). The load-bearing parts held exactly: every move is exit 5, and the tripwire fired only for the untouched control. Only the description of which layer refuses, and of when HMAC: OK is the tell, was wrong. Rewrite that bullet to split the two directions so nobody reads a missing HMAC: OK as a different class of failure. --- CLAUDE.md | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index acc1f74a..c5b9f4c3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,10 +130,22 @@ Binary container `magic | version | reserved | kid | hmac | header_json | payloa - HMAC scope: `kid_bytes || header_json || payload`. Tampering inside those bytes fails verify. The 4-byte `header_len` field is **not** covered — it only says where the header stops and the payload starts, and both halves - are authenticated as one run of bytes. Moving that boundary therefore - still passes `verify_hmac` (`recotem inspect` prints `HMAC: OK`) and is - caught one layer later by the header JSON parse or the deserializer, - reported as exit 5. It shifts a split point; it cannot inject a byte. + are authenticated as one run of bytes. It shifts a split point; it cannot + inject a byte. Every move of that boundary is refused, and the outcome is + always exit 5 with nothing deserialized, but the layer that refuses depends + on which way it moved, so do not expect `HMAC: OK` as the tell: + - **Shrunk** (`header_len` lowered, `0` included) — reaches `verify_hmac` + and passes it, because the authenticated run is unchanged. `recotem + inspect` prints `HMAC: OK`, then the header JSON parse fails on the + truncated slice. Serve fails the same way, at the `json.loads` in + `ModelRegistry._build_entry`, before `unpickle_payload`. + - **Enlarged** — refused earlier, inside `parse_header_from_bytes`, so + `verify_hmac` is never called and `HMAC: OK` is never printed: the + widened header slice swallows payload bytes and fails the UTF-8 decode, + or the file is shorter than the claimed header, or the value trips the + 64 KiB `MAX_HEADER_LEN` cap. + The deserializer is never the layer that catches this — the header JSON + parse always fires first. - Header JSON carries `recipe_name`, `recipe_hash`, `best_class`, `best_params`, `best_score`, `metric`, `cutoff`, `tuning`, `data_stats`, `recotem_version`, `irspack_version`, `trained_at`. Inspectable without deserialisation via From f8ed373f04574d36441d3c031469e055a5b23f53 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Tue, 8 Sep 2026 07:37:44 +0900 Subject: [PATCH 3/4] docs(http-fetch): replace the stale :doc:`/security` reference with the live URL _is_address_internal's docstring pointed a reader at "MAJOR-3 in :doc:`/security`" -- a Sphinx target in the docs/ tree this repo no longer carries, under an internal audit label that never appeared on the published page. It is the last :doc: reference left in src/. Point at https://recotem.org/2.1/docs/security and name the section heading a reader can actually find, matching how every other cross-reference in the tree now cites the site. --- src/recotem/_http_fetch.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/recotem/_http_fetch.py b/src/recotem/_http_fetch.py index 7cd1d204..91811a4c 100644 --- a/src/recotem/_http_fetch.py +++ b/src/recotem/_http_fetch.py @@ -154,7 +154,8 @@ def _is_address_internal(addr: ipaddress._BaseAddress) -> bool: the IPv6-level properties for these addresses would produce false positives. Evaluating the unwrapped IPv4 directly avoids the ambiguity entirely. The bogon check also uses the unwrapped IPv4 for mapped addresses. - See MAJOR-3 in :doc:`/security`. + See https://recotem.org/2.1/docs/security.html ("IPv4-mapped IPv6 inputs are + explicitly unwrapped"). """ # Primary check for IPv4-mapped IPv6 addresses (``::ffff:a.b.c.d``): # unwrap to the embedded IPv4 and evaluate properties there, bypassing From 8a6c197552d81341ad5f02c02b987ca73e7b3fb2 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Thu, 10 Sep 2026 10:45:30 +0900 Subject: [PATCH 4/4] fix(docs): the security-page URL names 2.1, and main is now 2.2 The reference added in this branch was written before the dev bump. On the current tree `check-release-tag.sh` section 4b refuses a tag whose scanned roots name a documentation line other than the version being released, and `src` is one of those roots -- so this line would have failed the v2.2.0 tag after the tag already existed. The cited section keeps its wording at the new path: 2.2/docs/security.md carries "IPv4-mapped IPv6 inputs are explicitly unwrapped" as a heading of its own. Also merges current main into the branch so its checks run against the tree this will land on rather than a base 34 commits old. --- src/recotem/_http_fetch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/recotem/_http_fetch.py b/src/recotem/_http_fetch.py index 91811a4c..fdb3c5cb 100644 --- a/src/recotem/_http_fetch.py +++ b/src/recotem/_http_fetch.py @@ -154,7 +154,7 @@ def _is_address_internal(addr: ipaddress._BaseAddress) -> bool: the IPv6-level properties for these addresses would produce false positives. Evaluating the unwrapped IPv4 directly avoids the ambiguity entirely. The bogon check also uses the unwrapped IPv4 for mapped addresses. - See https://recotem.org/2.1/docs/security.html ("IPv4-mapped IPv6 inputs are + See https://recotem.org/2.2/docs/security.html ("IPv4-mapped IPv6 inputs are explicitly unwrapped"). """ # Primary check for IPv4-mapped IPv6 addresses (``::ffff:a.b.c.d``):