claim analysis jobs atomically and keep acquisition on its official host
Two defects of the same kind: work that is supposed to be bounded is not. The analysis worker selected queued jobs and then set them to running in a second statement. A restarted process overlapping the previous one, or a second replica, could both select the same row and both start tiled GPU inference on it — duplicate analysis runs and double the GPU load. The AOI worker beside it already claims with FOR UPDATE SKIP LOCKED; this uses a conditional update, which is the same guarantee in one statement. run_once now reports jobs it actually claimed rather than jobs it looked at. urlopen follows redirects, so although every acquisition URL is built from settings and cannot be steered by a request payload, a misconfigured or compromised upstream could send the runtime to the loopback interface, to another container on the compose network, or to a cloud metadata endpoint — and the bytes would then be persisted under an official provenance. That is exactly the substitution the product forbids. All eight fetch sites now open through a guard that refuses private, loopback and link-local destinations (resolving the host first, so a DNS name cannot hide one) and refuses a redirect that leaves the configured origin or downgrades from HTTPS. The guard is proven by calling the services' own fetch paths, not by grepping for the call: every existing acquisition test injects an opener, which bypasses it by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Jens
@@ -0,0 +1,135 @@
|
||||
"""A queued run must be claimed once, even if two workers look at it.
|
||||
|
||||
The worker selected queued jobs and then set them to running in a second
|
||||
statement. Two workers — an API restart overlapping the previous process, or a
|
||||
second replica — could both select the same row and both start tiled GPU
|
||||
inference on it, producing duplicate analysis runs and doubling the GPU load.
|
||||
|
||||
The AOI worker beside it already claims with ``FOR UPDATE SKIP LOCKED``. This
|
||||
uses a conditional update, which is the same guarantee expressed in one
|
||||
statement: exactly one caller sees a row count of 1.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from uuid import uuid4
|
||||
|
||||
from app.models import Job
|
||||
from app.services.analysis_job_worker import AnalysisJobWorker
|
||||
|
||||
|
||||
class _Update:
|
||||
"""Mimics a conditional UPDATE: the first caller wins, the rest see zero."""
|
||||
|
||||
def __init__(self, store: dict, job_id):
|
||||
self.store = store
|
||||
self.job_id = job_id
|
||||
|
||||
def update(self, values, **_kwargs) -> int:
|
||||
if self.store.get(self.job_id) != "queued":
|
||||
return 0
|
||||
self.store[self.job_id] = "running"
|
||||
return 1
|
||||
|
||||
|
||||
class _Query:
|
||||
def __init__(self, session, model):
|
||||
self.session = session
|
||||
self.model = model
|
||||
self.job_id = None
|
||||
|
||||
def filter(self, *criteria):
|
||||
for criterion in criteria:
|
||||
right = getattr(criterion, "right", None)
|
||||
value = getattr(right, "value", None)
|
||||
if isinstance(value, type(uuid4())):
|
||||
self.job_id = value
|
||||
return self
|
||||
|
||||
def update(self, values, **kwargs) -> int:
|
||||
return _Update(self.session.statuses, self.job_id).update(values, **kwargs)
|
||||
|
||||
def order_by(self, *_args):
|
||||
return self
|
||||
|
||||
def limit(self, _count):
|
||||
return self
|
||||
|
||||
def all(self):
|
||||
return list(self.session.rows)
|
||||
|
||||
|
||||
class _Session:
|
||||
def __init__(self, rows: list[Job]):
|
||||
self.rows = rows
|
||||
self.statuses = {row.id: row.status for row in rows}
|
||||
self.committed = 0
|
||||
|
||||
def query(self, model):
|
||||
return _Query(self, model)
|
||||
|
||||
def get(self, _model, item_id):
|
||||
return next((row for row in self.rows if row.id == item_id), None)
|
||||
|
||||
def add(self, _item):
|
||||
return None
|
||||
|
||||
def commit(self):
|
||||
self.committed += 1
|
||||
|
||||
def rollback(self):
|
||||
return None
|
||||
|
||||
def close(self):
|
||||
return None
|
||||
|
||||
|
||||
def _job() -> Job:
|
||||
return Job(
|
||||
id=uuid4(),
|
||||
job_type="detection.run",
|
||||
status="queued",
|
||||
project_id=uuid4(),
|
||||
parameters_json={},
|
||||
)
|
||||
|
||||
|
||||
def test_the_first_claim_wins() -> None:
|
||||
job = _job()
|
||||
session = _Session([job])
|
||||
|
||||
assert AnalysisJobWorker.claim(session, job) is True
|
||||
assert job.status == "running"
|
||||
|
||||
|
||||
def test_a_second_claim_on_the_same_job_is_refused() -> None:
|
||||
job = _job()
|
||||
session = _Session([job])
|
||||
|
||||
assert AnalysisJobWorker.claim(session, job) is True
|
||||
assert AnalysisJobWorker.claim(session, job) is False
|
||||
|
||||
|
||||
def test_a_job_that_is_no_longer_queued_cannot_be_claimed() -> None:
|
||||
job = _job()
|
||||
session = _Session([job])
|
||||
session.statuses[job.id] = "success"
|
||||
|
||||
assert AnalysisJobWorker.claim(session, job) is False
|
||||
|
||||
|
||||
def test_an_unclaimable_job_is_skipped_rather_than_run(monkeypatch) -> None:
|
||||
job = _job()
|
||||
session = _Session([job])
|
||||
session.statuses[job.id] = "running"
|
||||
dispatched: list[Job] = []
|
||||
monkeypatch.setattr(
|
||||
AnalysisJobWorker,
|
||||
"_dispatch",
|
||||
staticmethod(lambda _db, item: dispatched.append(item)),
|
||||
)
|
||||
|
||||
processed = AnalysisJobWorker.run_once(db=session)
|
||||
|
||||
assert processed == 0
|
||||
assert dispatched == []
|
||||
@@ -26,6 +26,16 @@ class FakeQuery:
|
||||
def filter(self, *criteria):
|
||||
return self
|
||||
|
||||
def update(self, values, **_kwargs) -> int:
|
||||
"""Stand in for the conditional claim: succeeds while still queued."""
|
||||
|
||||
claimed = 0
|
||||
for row in self.rows:
|
||||
if getattr(row, "status", None) == "queued":
|
||||
row.status = "running"
|
||||
claimed += 1
|
||||
return claimed
|
||||
|
||||
def order_by(self, *_args):
|
||||
return self
|
||||
|
||||
|
||||
@@ -0,0 +1,164 @@
|
||||
"""Bounded acquisition must stay bounded to the official host.
|
||||
|
||||
Every acquisition service builds its URL from configured settings, so the
|
||||
request payload cannot point the runtime anywhere. The redirect chain can:
|
||||
``urlopen`` follows redirects by default, so a misconfigured or compromised
|
||||
upstream can send the runtime to ``127.0.0.1``, to the container network, or to
|
||||
a cloud metadata endpoint — and the response is then persisted as if it were
|
||||
official source data.
|
||||
|
||||
The product's stated rule is that acquisition fails closed and never
|
||||
substitutes fabricated data for official data. A redirect off the configured
|
||||
host is exactly that substitution.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from app.core.errors import AppError
|
||||
from app.services.outbound_request_guard import (
|
||||
assert_public_http_url,
|
||||
assert_same_origin_redirect,
|
||||
)
|
||||
|
||||
|
||||
class TestUrlShape:
|
||||
def test_an_official_https_endpoint_is_accepted(self) -> None:
|
||||
assert_public_http_url("https://geo.api.vlaanderen.be/dhmv/wcs?SERVICE=WCS")
|
||||
|
||||
def test_a_non_http_scheme_is_refused(self) -> None:
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
assert_public_http_url("file:///etc/passwd")
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED"
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"url",
|
||||
[
|
||||
"http://127.0.0.1:8000/internal",
|
||||
"http://localhost/internal",
|
||||
"http://10.1.2.3/internal",
|
||||
"http://192.168.10.150/internal",
|
||||
"http://172.16.0.9/internal",
|
||||
"http://169.254.169.254/latest/meta-data/",
|
||||
"http://[::1]/internal",
|
||||
],
|
||||
)
|
||||
def test_private_and_loopback_destinations_are_refused(self, url: str) -> None:
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
assert_public_http_url(url)
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED"
|
||||
|
||||
def test_a_url_without_a_host_is_refused(self) -> None:
|
||||
with pytest.raises(AppError):
|
||||
assert_public_http_url("https:///no-host")
|
||||
|
||||
|
||||
class TestRedirects:
|
||||
def test_a_redirect_within_the_same_origin_is_allowed(self) -> None:
|
||||
assert_same_origin_redirect(
|
||||
"https://geo.api.vlaanderen.be/dhmv/wcs",
|
||||
"https://geo.api.vlaanderen.be/dhmv/wcs/v2?x=1",
|
||||
)
|
||||
|
||||
def test_a_redirect_to_another_host_is_refused(self) -> None:
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
assert_same_origin_redirect(
|
||||
"https://geo.api.vlaanderen.be/dhmv/wcs",
|
||||
"https://cdn.example.net/payload.tif",
|
||||
)
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_REDIRECT_NOT_ALLOWED"
|
||||
assert "cdn.example.net" in str(exc_info.value.details)
|
||||
|
||||
def test_a_downgrade_to_plain_http_is_refused(self) -> None:
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
assert_same_origin_redirect(
|
||||
"https://geo.api.vlaanderen.be/wcs",
|
||||
"http://geo.api.vlaanderen.be/wcs",
|
||||
)
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_REDIRECT_NOT_ALLOWED"
|
||||
|
||||
def test_a_redirect_to_the_loopback_is_refused_even_on_the_same_scheme(self) -> None:
|
||||
with pytest.raises(AppError):
|
||||
assert_same_origin_redirect("https://geo.api.vlaanderen.be/wcs", "https://127.0.0.1/wcs")
|
||||
|
||||
def test_an_upgrade_to_https_stays_allowed(self) -> None:
|
||||
assert_same_origin_redirect("http://geo.example.be/wcs", "https://geo.example.be/wcs")
|
||||
|
||||
|
||||
def test_the_guard_opener_refuses_a_cross_host_redirect() -> None:
|
||||
"""The opener is what the acquisition services actually call."""
|
||||
|
||||
from app.services.outbound_request_guard import guarded_opener
|
||||
|
||||
opener = guarded_opener("https://geo.api.vlaanderen.be/wcs")
|
||||
|
||||
class _Redirecting:
|
||||
def __init__(self, location: str) -> None:
|
||||
self.url = location
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, *_args):
|
||||
return False
|
||||
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
with opener(
|
||||
type("Req", (), {"full_url": "https://geo.api.vlaanderen.be/wcs"})(),
|
||||
timeout=1,
|
||||
_transport=lambda *_a, **_k: _Redirecting("https://evil.example.net/x"),
|
||||
):
|
||||
pass
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_REDIRECT_NOT_ALLOWED"
|
||||
|
||||
|
||||
class TestTheGuardIsWiredIntoAcquisition:
|
||||
"""Behavioural, not a grep: each service is called on its real fetch path.
|
||||
|
||||
Every existing acquisition test injects an ``opener``, which bypasses the
|
||||
guard by design — that is how those tests stub the network. These call the
|
||||
production default instead.
|
||||
"""
|
||||
|
||||
def _settings(self):
|
||||
from app.core.config import Settings
|
||||
|
||||
return Settings(_env_file=None)
|
||||
|
||||
def test_dhmv_refuses_a_loopback_endpoint(self) -> None:
|
||||
from app.services.dhmv_acquisition_service import DhmvAcquisitionService
|
||||
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
DhmvAcquisitionService._fetch("http://127.0.0.1:9/wcs", self._settings())
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED"
|
||||
|
||||
def test_flood_hazard_refuses_a_link_local_endpoint(self) -> None:
|
||||
from app.services.flood_hazard_acquisition_service import FloodHazardAcquisitionService
|
||||
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
FloodHazardAcquisitionService._fetch("http://169.254.169.254/latest/", self._settings())
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED"
|
||||
|
||||
def test_thematic_raster_refuses_a_private_endpoint(self) -> None:
|
||||
from app.services.thematic_raster_acquisition_service import ThematicRasterAcquisitionService
|
||||
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
ThematicRasterAcquisitionService._fetch("http://10.0.0.5/product.tif", self._settings())
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED"
|
||||
|
||||
def test_orthophoto_refuses_a_private_endpoint(self) -> None:
|
||||
from app.services.orthophoto_acquisition_service import OrthophotoAcquisitionService
|
||||
|
||||
with pytest.raises(AppError) as exc_info:
|
||||
OrthophotoAcquisitionService._fetch("http://192.168.10.150/wms", self._settings())
|
||||
|
||||
assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED"
|
||||
Reference in New Issue
Block a user