Fix version drift, SSRF-able article fetch, and non-atomic artifact writes
- __version__ was stuck at 0.8.2 after the 0.8.3 release, so every ratings and manifest artifact carried the wrong pipeline_version; bumped to match pyproject.toml and added a regression test pinning the two together. - fetch_article_text followed an RSS entry's <link> wherever it pointed, including through redirects, with no check on the resolved address. A compromised or malicious feed could aim it at an internal address (cloud metadata, LAN service). The host and every redirect hop are now resolved and rejected if they land on a private, loopback, link-local, or otherwise non-public range. - artifacts.write_json wrote the target path in place, so serve.py (a separate process reading the same releases volume) could read a partially written or truncated artifact. It now writes to a temp file in the same directory and swaps it into place with os.replace.
This commit is contained in:
@@ -8,6 +8,23 @@ requires a version bump and, if it changes methodology, a decision record in
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- `__version__` had drifted from `pyproject.toml` (stuck at 0.8.2 through the
|
||||
0.8.3 release), so every ratings/manifest artifact produced since then was
|
||||
stamped with the wrong `pipeline_version`. Bumped to match, and added a
|
||||
regression test pinning the two together.
|
||||
- `fetch_article_text` fetched whatever URL a feed's `<link>` contained, with
|
||||
no check on where that URL actually pointed — a compromised or malicious
|
||||
feed could aim it at an internal address (cloud metadata, LAN service) and
|
||||
have us fetch it on the feed owner's behalf (SSRF). The host, and every
|
||||
redirect hop, is now resolved and rejected if it lands on a private,
|
||||
loopback, link-local, or otherwise non-public address.
|
||||
- `artifacts.write_json` wrote the target path in place, so a reader on the
|
||||
same releases volume (serve.py, a separate process) could observe a
|
||||
partially written or truncated artifact mid-write. It now writes to a temp
|
||||
file in the same directory and swaps it into place with `os.replace`.
|
||||
|
||||
## [0.8.3] - 2026-07-19
|
||||
|
||||
### Fixed
|
||||
|
||||
@@ -7,4 +7,4 @@ Every module in this package opens with a plain-language docstring stating the
|
||||
question it answers. See docs/how-it-works.md for the full plain-language tour.
|
||||
"""
|
||||
|
||||
__version__ = "0.8.2"
|
||||
__version__ = "0.8.3"
|
||||
|
||||
@@ -11,6 +11,8 @@ servable by adding one line here.
|
||||
"""
|
||||
|
||||
import json
|
||||
import os
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
# kind -> filename prefix; API route name == kind
|
||||
@@ -28,13 +30,23 @@ def artifact_path(out_dir: str | Path, kind: str, snapshot_id: str) -> Path:
|
||||
|
||||
|
||||
def write_json(path: str | Path, payload) -> Path:
|
||||
"""Deterministic serialization: same payload, same bytes, any machine."""
|
||||
"""Deterministic serialization: same payload, same bytes, any machine.
|
||||
|
||||
Written to a temp file in the same directory and swapped into place with
|
||||
os.replace, so serve.py — a separate process reading the same releases
|
||||
volume — can never observe a partially written or truncated file.
|
||||
"""
|
||||
path = Path(path)
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(
|
||||
json.dumps(payload, indent=1, sort_keys=True, ensure_ascii=False, default=str) + "\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
data = json.dumps(payload, indent=1, sort_keys=True, ensure_ascii=False, default=str) + "\n"
|
||||
fd, tmp_name = tempfile.mkstemp(dir=path.parent, prefix=f".{path.name}.", suffix=".tmp")
|
||||
try:
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as f:
|
||||
f.write(data)
|
||||
os.replace(tmp_name, path)
|
||||
except BaseException:
|
||||
os.unlink(tmp_name)
|
||||
raise
|
||||
return path
|
||||
|
||||
|
||||
|
||||
+61
-11
@@ -9,15 +9,17 @@ Politeness rules: we identify ourselves with an honest User-Agent, fetch each
|
||||
article at most once ever (URLs are deduplicated), and pause between fetches.
|
||||
"""
|
||||
|
||||
import ipaddress
|
||||
import logging
|
||||
import socket
|
||||
import time
|
||||
from datetime import datetime, timezone
|
||||
from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit
|
||||
from urllib.parse import parse_qsl, urlencode, urljoin, urlsplit, urlunsplit
|
||||
|
||||
import feedparser
|
||||
import requests
|
||||
import trafilatura
|
||||
import yaml
|
||||
from trafilatura.settings import use_config
|
||||
|
||||
from tiltmeter import db
|
||||
|
||||
@@ -60,10 +62,9 @@ FETCH_DELAY_SECONDS = 0.2
|
||||
# Outlets that block us (e.g. paywalled WaPo) time out; cap the wait so one
|
||||
# blocked outlet can't stall a whole ingest run. Headline+summary still land.
|
||||
FETCH_TIMEOUT_SECONDS = 10
|
||||
|
||||
_FETCH_CONFIG = use_config()
|
||||
_FETCH_CONFIG.set("DEFAULT", "DOWNLOAD_TIMEOUT", str(FETCH_TIMEOUT_SECONDS))
|
||||
_FETCH_CONFIG.set("DEFAULT", "USER_AGENTS", USER_AGENT)
|
||||
# A feed entry's <link> is untrusted input. Cap manual redirect-following so a
|
||||
# chain can't loop forever, and re-check the host at each hop (see below).
|
||||
MAX_ARTICLE_REDIRECTS = 5
|
||||
|
||||
|
||||
def load_outlets(config_path: str) -> list[dict]:
|
||||
@@ -73,12 +74,61 @@ def load_outlets(config_path: str) -> list[dict]:
|
||||
return cfg["outlets"]
|
||||
|
||||
|
||||
def _reject_unroutable_host(host: str) -> None:
|
||||
"""Refuse a host that resolves anywhere non-public.
|
||||
|
||||
A feed is an outside party's input; its <link> could point at an
|
||||
internal address (cloud metadata, LAN service) to make us fetch it on
|
||||
the feed owner's behalf. Reject by resolved address, not by hostname
|
||||
pattern, since e.g. "localhost" is only one of many spellings.
|
||||
"""
|
||||
try:
|
||||
infos = socket.getaddrinfo(host, None)
|
||||
except socket.gaierror as exc:
|
||||
raise ValueError(f"cannot resolve {host!r}: {exc}") from None
|
||||
for info in infos:
|
||||
ip = ipaddress.ip_address(info[4][0])
|
||||
if (
|
||||
ip.is_private
|
||||
or ip.is_loopback
|
||||
or ip.is_link_local
|
||||
or ip.is_reserved
|
||||
or ip.is_multicast
|
||||
or ip.is_unspecified
|
||||
):
|
||||
raise ValueError(f"refusing to fetch {host!r}: resolves to {ip} (non-public)")
|
||||
|
||||
|
||||
def fetch_article_text(url: str) -> str | None:
|
||||
"""Download one article page and extract its readable text."""
|
||||
html = trafilatura.fetch_url(url, config=_FETCH_CONFIG)
|
||||
if html is None:
|
||||
return None
|
||||
return trafilatura.extract(html, include_comments=False, include_tables=False)
|
||||
"""Download one article page and extract its readable text.
|
||||
|
||||
Redirects are followed by hand, capped, and re-validated one hop at a
|
||||
time: an automatic redirect follower (trafilatura's, requests', any
|
||||
HTTP client's) would let a feed entry pass the host check and then
|
||||
bounce us to a private address on the next hop.
|
||||
"""
|
||||
for _ in range(MAX_ARTICLE_REDIRECTS + 1):
|
||||
host = urlsplit(url).hostname
|
||||
if not host:
|
||||
return None
|
||||
_reject_unroutable_host(host)
|
||||
resp = requests.get(
|
||||
url,
|
||||
headers={"User-Agent": USER_AGENT},
|
||||
timeout=FETCH_TIMEOUT_SECONDS,
|
||||
allow_redirects=False,
|
||||
)
|
||||
if resp.is_redirect:
|
||||
location = resp.headers.get("Location")
|
||||
if not location:
|
||||
return None
|
||||
url = urljoin(url, location)
|
||||
continue
|
||||
if resp.status_code != 200:
|
||||
return None
|
||||
return trafilatura.extract(resp.content, include_comments=False, include_tables=False)
|
||||
log.warning("too many redirects fetching %s", url)
|
||||
return None
|
||||
|
||||
|
||||
def ingest_outlet(conn, outlet: dict, *, fetch_text: bool = True) -> tuple[int, list[str]]:
|
||||
|
||||
@@ -10,6 +10,7 @@ from pathlib import Path
|
||||
import numpy as np
|
||||
import pytest
|
||||
|
||||
import tiltmeter
|
||||
from tiltmeter import artifacts, db, embed, serve
|
||||
from tiltmeter.stats import spearman
|
||||
|
||||
@@ -235,6 +236,23 @@ def test_artifact_bytes_are_platform_pinned(tmp_path):
|
||||
assert p1.read_bytes().index(b'"a"') < p1.read_bytes().index(b'"z"')
|
||||
|
||||
|
||||
def test_write_json_atomic_never_leaves_a_partial_target(tmp_path, monkeypatch):
|
||||
"""A crash mid-write must never truncate the previous artifact in place —
|
||||
serve.py reads this same file from a separate process, concurrently."""
|
||||
target = tmp_path / "ratings-x.json"
|
||||
artifacts.write_json(target, {"a": 1})
|
||||
original = target.read_bytes()
|
||||
|
||||
def boom(*a, **k):
|
||||
raise RuntimeError("simulated crash mid-write")
|
||||
|
||||
monkeypatch.setattr(artifacts.os, "replace", boom)
|
||||
with pytest.raises(RuntimeError, match="simulated"):
|
||||
artifacts.write_json(target, {"a": 2, "b": "x" * 1000})
|
||||
assert target.read_bytes() == original, "target must be untouched until the atomic swap"
|
||||
assert list(tmp_path.iterdir()) == [target], "a failed write must not leak a temp file"
|
||||
|
||||
|
||||
def test_embedding_cache_key_carries_model_revision():
|
||||
"""A revision bump must miss the cache, never serve stale vectors."""
|
||||
assert embed.MODEL_REVISION in embed.CACHE_MODEL_KEY
|
||||
@@ -250,6 +268,15 @@ def test_dockerfile_model_pins_match_code():
|
||||
assert revision == embed.MODEL_REVISION
|
||||
|
||||
|
||||
def test_version_matches_pyproject():
|
||||
"""__version__ is stamped into every ratings/manifest as pipeline_version;
|
||||
a pyproject bump that leaves it behind ships artifacts under the wrong
|
||||
version."""
|
||||
pyproject = (ROOT / "pyproject.toml").read_text(encoding="utf-8")
|
||||
version = re.search(r'(?m)^version = "([^"]+)"', pyproject).group(1)
|
||||
assert tiltmeter.__version__ == version
|
||||
|
||||
|
||||
def test_serve_routes_cover_every_artifact_kind():
|
||||
"""A new artifact kind must be servable by construction, not by memory."""
|
||||
source = (ROOT / "src/tiltmeter/serve.py").read_text(encoding="utf-8")
|
||||
|
||||
@@ -5,6 +5,8 @@ by the live cron run; what must never regress silently is the storage
|
||||
contract: one row per URL, stable fingerprints, accurate per-outlet counts.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
|
||||
from tiltmeter import db
|
||||
|
||||
|
||||
@@ -70,3 +72,49 @@ def test_counts_group_by_outlet():
|
||||
text="x",
|
||||
)
|
||||
assert db.outlet_counts(conn) == [("left-times", 2), ("right-post", 1)]
|
||||
|
||||
|
||||
def test_reject_unroutable_host_blocks_private_and_link_local_ranges():
|
||||
from tiltmeter.ingest import _reject_unroutable_host
|
||||
|
||||
# RFC1918, loopback, link-local (incl. the cloud metadata address), and
|
||||
# their IPv6 equivalents — a compromised feed pointing <link> here must
|
||||
# never be fetched.
|
||||
for host in (
|
||||
"127.0.0.1", "169.254.169.254", "10.1.2.3", "192.168.1.1", "172.16.0.5",
|
||||
"::1", "fc00::1", "fe80::1",
|
||||
):
|
||||
with pytest.raises(ValueError, match="non-public"):
|
||||
_reject_unroutable_host(host)
|
||||
|
||||
|
||||
def test_reject_unroutable_host_allows_public_addresses():
|
||||
from tiltmeter.ingest import _reject_unroutable_host
|
||||
|
||||
_reject_unroutable_host("8.8.8.8") # a literal IP needs no DNS lookup
|
||||
|
||||
|
||||
def test_fetch_article_text_refuses_private_target_before_any_request(monkeypatch):
|
||||
"""The host check must run before the network call, not after."""
|
||||
from tiltmeter import ingest
|
||||
|
||||
def fail_if_called(*args, **kwargs):
|
||||
raise AssertionError("must not contact the network for a private-IP link")
|
||||
|
||||
monkeypatch.setattr(ingest.requests, "get", fail_if_called)
|
||||
with pytest.raises(ValueError, match="non-public"):
|
||||
ingest.fetch_article_text("http://127.0.0.1:9/admin")
|
||||
|
||||
|
||||
def test_fetch_article_text_revalidates_host_on_redirect(monkeypatch):
|
||||
"""A redirect to a private address must be rejected, not followed."""
|
||||
from tiltmeter import ingest
|
||||
|
||||
class FakeResponse:
|
||||
status_code = 302
|
||||
is_redirect = True
|
||||
headers = {"Location": "http://169.254.169.254/latest/meta-data/"}
|
||||
|
||||
monkeypatch.setattr(ingest.requests, "get", lambda *a, **k: FakeResponse())
|
||||
with pytest.raises(ValueError, match="non-public"):
|
||||
ingest.fetch_article_text("https://example.com/story")
|
||||
|
||||
Reference in New Issue
Block a user