diff --git a/backend/app/services/dataset_service.py b/backend/app/services/dataset_service.py index 0d93b8a8..ab6c490f 100644 --- a/backend/app/services/dataset_service.py +++ b/backend/app/services/dataset_service.py @@ -2906,8 +2906,19 @@ class DatasetService: raise AppError(code="DATASET_FILE_MISSING", message="Stored file missing", status_code=404) raw = load_dataset_text(dataset.storage_path) + # Only the parse can be "not valid JSON". Everything after it fails for + # its own reasons and must say so, or an operator is sent to inspect a + # file that parses perfectly well. try: payload = json.loads(raw) + except Exception as exc: + raise AppError( + code="INVALID_GEOJSON", + message="Stored dataset is not valid JSON", + status_code=500, + ) from exc + + try: metadata_value = getattr(dataset, "metadata_json", None) metadata = metadata_value if isinstance(metadata_value, dict) else {} provenance_value = getattr(dataset, "provenance_metadata", None) @@ -2933,8 +2944,17 @@ class DatasetService: ) ) return VectorFeatureService.canonicalize_geojson_payload(payload, source_crs=str(source_crs)) + except AppError: + # The canonicaliser's diagnosis is more precise than anything this + # layer could substitute for it. + raise except Exception as exc: - raise AppError(code="INVALID_GEOJSON", message="Stored dataset is not valid JSON", status_code=500) from exc + raise AppError( + code="DATASET_GEOJSON_UNREADABLE", + message="The stored dataset could not be read as canonical GeoJSON", + details={"dataset_id": str(dataset.id), "error_type": type(exc).__name__}, + status_code=500, + ) from exc @staticmethod def inspect_vector_dataset(db: Session, dataset_id: UUID) -> dict[str, Any]: diff --git a/backend/app/services/grb_acquisition_service.py b/backend/app/services/grb_acquisition_service.py index dd94e136..596a5151 100644 --- a/backend/app/services/grb_acquisition_service.py +++ b/backend/app/services/grb_acquisition_service.py @@ -9,7 +9,7 @@ from pathlib import Path from typing import Any, Callable from urllib.error import HTTPError, URLError from urllib.parse import parse_qsl, urlencode, urljoin, urlparse, urlunparse -from urllib.request import HTTPRedirectHandler, Request, build_opener +from urllib.request import Request from uuid import UUID from geoalchemy2.shape import to_shape @@ -20,20 +20,12 @@ from shapely.validation import make_valid from app.core.config import Settings, get_settings from app.core.errors import AppError +from app.services.outbound_request_guard import guarded_opener from app.models import Area, Dataset, Project from app.schemas.grb import GrbAcquireRequest, GrbAcquisitionResult, GrbProductRead from app.services.dataset_service import DatasetService -class _RejectRedirects(HTTPRedirectHandler): - def redirect_request(self, req, fp, code, msg, headers, newurl): - del req, fp, code, msg, headers, newurl - return None - - -_NO_REDIRECT_OPENER = build_opener(_RejectRedirects()) - - @dataclass(frozen=True) class GrbCollection: name: str @@ -368,7 +360,7 @@ class GrbAcquisitionService: }, ) try: - with (opener or _NO_REDIRECT_OPENER.open)( + with (opener or guarded_opener(url, allow_redirect=False))( request, timeout=settings.grb_timeout_seconds, ) as response: diff --git a/backend/app/services/official_vector_acquisition_service.py b/backend/app/services/official_vector_acquisition_service.py index 8dd7a006..a5faad40 100644 --- a/backend/app/services/official_vector_acquisition_service.py +++ b/backend/app/services/official_vector_acquisition_service.py @@ -10,7 +10,7 @@ import re from typing import Any, Callable from urllib.error import HTTPError, URLError from urllib.parse import urlencode, urlparse -from urllib.request import HTTPRedirectHandler, Request, build_opener +from urllib.request import Request from uuid import UUID from geoalchemy2.shape import to_shape @@ -21,6 +21,7 @@ from shapely.validation import make_valid from app.core.config import Settings, get_settings from app.core.errors import AppError +from app.services.outbound_request_guard import guarded_opener from app.models import Area, Dataset, Project from app.schemas.official_vector import ( OfficialVectorAcquireRequest, @@ -30,13 +31,6 @@ from app.schemas.official_vector import ( from app.services.dataset_service import DatasetService -class _RejectRedirects(HTTPRedirectHandler): - def redirect_request(self, req, fp, code, msg, headers, newurl): - del req, fp, code, msg, headers, newurl - return None - - -_NO_REDIRECT_OPENER = build_opener(_RejectRedirects()) _TO_LAMBERT72 = Transformer.from_crs("EPSG:4326", "EPSG:31370", always_xy=True) _TO_WGS84 = Transformer.from_crs("EPSG:31370", "EPSG:4326", always_xy=True) @@ -1086,7 +1080,7 @@ class OfficialVectorAcquisitionService: }, ) try: - with (opener or _NO_REDIRECT_OPENER.open)( + with (opener or guarded_opener(url, allow_redirect=False))( request, timeout=settings.official_vector_timeout_seconds, ) as response: diff --git a/backend/app/services/outbound_request_guard.py b/backend/app/services/outbound_request_guard.py index 0603bbeb..b8f0cdf4 100644 --- a/backend/app/services/outbound_request_guard.py +++ b/backend/app/services/outbound_request_guard.py @@ -18,13 +18,32 @@ import socket from collections.abc import Callable from typing import Any from urllib.parse import urlparse -from urllib.request import urlopen +from urllib.request import HTTPRedirectHandler, build_opener, urlopen from app.core.errors import AppError ALLOWED_SCHEMES = {"http", "https"} +class _RejectRedirects(HTTPRedirectHandler): + """Refuse to *follow* a redirect, rather than object after the fact. + + Checking the final URL means urllib already opened the connection and read + the response — for a destination like a cloud metadata endpoint that is the + whole attack. Returning ``None`` here means the request is never made. + """ + + def redirect_request(self, req, fp, code, msg, headers, newurl): # noqa: ANN001, D102 + del req, fp, code, msg, headers, newurl + return None + + +def no_redirect_opener(): + """An opener that will not follow a redirect anywhere.""" + + return build_opener(_RejectRedirects()) + + def _reject(code: str, message: str, **details: Any) -> AppError: return AppError(code=code, message=message, details=details or None, status_code=502) @@ -117,20 +136,34 @@ def assert_same_origin_redirect(original_url: str, final_url: str) -> None: assert_public_http_url(final_url) -def guarded_opener(expected_url: str) -> Callable[..., Any]: +def guarded_opener(expected_url: str, *, allow_redirect: bool = True) -> Callable[..., Any]: """An ``urlopen`` replacement that verifies where the response came from. ``urlopen`` has already followed the redirect chain by the time it returns, so the check is on ``response.url``: the body is still unread, and raising here means nothing off-origin is ever parsed or persisted. + + ``allow_redirect=False`` refuses any redirect at all, which is what the + paged OGC feature readers want: a page URL they built themselves should be + answered by that URL, and a redirect there means the endpoint moved under + them mid-pagination. """ assert_public_http_url(expected_url) + default_transport = urlopen if allow_redirect else no_redirect_opener().open + def _open(request: Any, *args: Any, _transport: Callable[..., Any] | None = None, **kwargs: Any) -> Any: - response = (_transport or urlopen)(request, *args, **kwargs) + response = (_transport or default_transport)(request, *args, **kwargs) final_url = str(getattr(response, "url", "") or "") try: + if not allow_redirect and final_url and final_url != expected_url: + raise _reject( + "OUTBOUND_REDIRECT_NOT_ALLOWED", + "The official endpoint redirected; this reader accepts only the URL it requested.", + expected_url=expected_url, + redirect_url=final_url, + ) assert_same_origin_redirect(expected_url, final_url) except AppError: close = getattr(response, "close", None) diff --git a/backend/tests/test_dataset_geojson_error_reporting.py b/backend/tests/test_dataset_geojson_error_reporting.py new file mode 100644 index 00000000..d84e31bb --- /dev/null +++ b/backend/tests/test_dataset_geojson_error_reporting.py @@ -0,0 +1,125 @@ +"""A precise failure must not be reported as a generic one. + +``get_dataset_geojson`` wrapped the JSON parse, the metadata read, the CRS +resolution and the canonicalisation in one ``try``, and reported everything as +"Stored dataset is not valid JSON" with a 500. An operator whose dataset has an +unusable CRS was sent to inspect a file that parses perfectly well, and the +specific AppError the canonicaliser raised — with its own code and status — +never reached them. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from uuid import uuid4 + +import pytest + +from app.core.errors import AppError +from app.models import Dataset +from app.services.dataset_service import DatasetService +from app.services.vector_feature_service import VectorFeatureService + + +class FakeSession: + def __init__(self, dataset: Dataset) -> None: + self.dataset = dataset + + def get(self, _model, item_id): + return self.dataset if item_id == self.dataset.id else None + + +def _dataset(tmp_path: Path, payload: str) -> Dataset: + path = tmp_path / "features.geojson" + path.write_text(payload, encoding="utf-8") + return Dataset( + id=uuid4(), + project_id=uuid4(), + name="features.geojson", + dataset_type="geojson", + source="test", + status="ready", + storage_path=str(path), + crs="EPSG:4326", + ) + + +def test_a_valid_dataset_is_returned(tmp_path: Path) -> None: + dataset = _dataset( + tmp_path, + json.dumps( + { + "type": "FeatureCollection", + "features": [ + { + "type": "Feature", + "geometry": {"type": "Point", "coordinates": [5.0, 51.0]}, + "properties": {}, + } + ], + } + ), + ) + + result = DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id) + + assert result["type"] == "FeatureCollection" + + +def test_unparseable_bytes_are_reported_as_invalid_json(tmp_path: Path) -> None: + dataset = _dataset(tmp_path, "{ not json") + + with pytest.raises(AppError) as exc_info: + DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id) + + assert exc_info.value.code == "INVALID_GEOJSON" + assert "JSON" in exc_info.value.message + + +def test_a_parseable_file_that_is_not_a_feature_collection_says_so(tmp_path: Path) -> None: + """Valid JSON, wrong shape. Telling the operator it is not JSON sends them + to inspect a file that parses perfectly well.""" + + dataset = _dataset(tmp_path, json.dumps({"type": "Feature", "geometry": None})) + + with pytest.raises(AppError) as exc_info: + DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id) + + assert exc_info.value.code == "INVALID_GEOJSON" + assert "FeatureCollection" in exc_info.value.message + # The canonicaliser's own status survives; it is a bad request, not a + # server fault. + assert exc_info.value.status_code == 400 + + +def test_a_canonicalisation_failure_keeps_its_own_code(tmp_path: Path, monkeypatch) -> None: + dataset = _dataset(tmp_path, json.dumps({"type": "FeatureCollection", "features": []})) + + def failing(*_args, **_kwargs): + raise AppError(code="INVALID_CRS", message="Unusable source CRS", status_code=409) + + monkeypatch.setattr(VectorFeatureService, "canonicalize_geojson_payload", staticmethod(failing)) + + with pytest.raises(AppError) as exc_info: + DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id) + + assert exc_info.value.code == "INVALID_CRS" + assert exc_info.value.status_code == 409 + + +def test_an_unexpected_failure_still_becomes_a_server_error(tmp_path: Path, monkeypatch) -> None: + """A genuine bug must not masquerade as a client mistake.""" + + dataset = _dataset(tmp_path, json.dumps({"type": "FeatureCollection", "features": []})) + + def exploding(*_args, **_kwargs): + raise RuntimeError("pyproj blew up") + + monkeypatch.setattr(VectorFeatureService, "canonicalize_geojson_payload", staticmethod(exploding)) + + with pytest.raises(AppError) as exc_info: + DatasetService.get_dataset_geojson(FakeSession(dataset), dataset.id) + + assert exc_info.value.status_code == 500 + assert exc_info.value.code == "DATASET_GEOJSON_UNREADABLE" diff --git a/backend/tests/test_error_diagnosis_is_not_lost.py b/backend/tests/test_error_diagnosis_is_not_lost.py new file mode 100644 index 00000000..a2fc34be --- /dev/null +++ b/backend/tests/test_error_diagnosis_is_not_lost.py @@ -0,0 +1,88 @@ +"""A broad handler must not overwrite a precise diagnosis with a generic one. + +``except Exception: raise AppError(...)`` also catches ``AppError``, so a +service that raised INVALID_CRS with a 409 comes out as whatever generic code +the outer handler chose. For a product whose whole claim is that a result can +be traced back to its cause, that is the wrong direction in which to lose +information. + +Wrapping a narrow call whose failure the outer message describes better is +legitimate — coercing a CRS string, parsing one geometry, calling one's own +private helper. Relabelling what *another* component reported is not. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +APP = Path(__file__).resolve().parents[1] / "app" + +GENERIC_HANDLER = re.compile(r"except Exception as \w+:\s*\n\s*raise AppError\(", re.M) +OTHER_SERVICE = re.compile(r"\b([A-Z]\w*Service)\.") +# Cross-component entry points that carry their own considered diagnosis. +GOVERNED_CALLS = re.compile(r"assert_eligible|assert_within_storage_root|canonicalize_|_load_dataset_payload") + + +def _enclosing_service(text: str, handler_line: int) -> str | None: + """The class the handler sits in, not merely the first one in the file.""" + + enclosing = None + for match in re.finditer(r"^class (\w+)[:(]", text, re.M): + if text[: match.start()].count(chr(10)) > handler_line: + break + enclosing = match.group(1) + return enclosing + + +def _try_body(lines: list[str], handler_line: int) -> str: + index = handler_line + while index > 0 and lines[index].strip() != "try:": + index -= 1 + return "\n".join(lines[index:handler_line]) + + +def calls_another_component(body: str, own_service: str | None) -> bool: + if GOVERNED_CALLS.search(body): + return True + return any(name != own_service for name in OTHER_SERVICE.findall(body)) + + +def _offenders() -> list[str]: + found: list[str] = [] + for path in sorted(APP.rglob("*.py")): + text = path.read_text(encoding="utf-8") + lines = text.splitlines() + for match in GENERIC_HANDLER.finditer(text): + preceding = text[max(0, match.start() - 240):match.start() + 40] + if "except AppError:" in preceding: + continue + handler_line = text[:match.start()].count("\n") + own = _enclosing_service(text, handler_line) + if calls_another_component(_try_body(lines, handler_line), own): + found.append(f"{path.relative_to(APP)}:{handler_line + 1}") + return found + + +def test_a_call_into_another_component_keeps_the_error_it_raised() -> None: + offenders = _offenders() + + assert not offenders, ( + "These catch Exception around a call into another component and relabel " + "whatever it raised, including its AppError. Add `except AppError: " + f"raise` before the generic handler: {offenders}" + ) + + +def test_the_check_recognises_the_shape_it_guards_against() -> None: + """Without this the guard could pass because its pattern never matches.""" + + other = "try:\n DatasetService.get_dataset(db, dataset_id)" + own = "try:\n GrbAcquisitionService._extract_dimension(geometry)" + governed = "try:\n DatasetConsumptionGate.assert_eligible(dataset)" + + assert calls_another_component(other, "GrbAcquisitionService") + assert calls_another_component(governed, "GrbAcquisitionService") + # Wrapping one's own private helper is the legitimate case. + assert not calls_another_component(own, "GrbAcquisitionService") + assert GENERIC_HANDLER.search("except Exception as exc:\n raise AppError(") diff --git a/backend/tests/test_outbound_request_guard.py b/backend/tests/test_outbound_request_guard.py index d398b4b6..c0bc3e68 100644 --- a/backend/tests/test_outbound_request_guard.py +++ b/backend/tests/test_outbound_request_guard.py @@ -162,3 +162,96 @@ class TestTheGuardIsWiredIntoAcquisition: OrthophotoAcquisitionService._fetch("http://192.168.10.150/wms", self._settings()) assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED" + + +class TestOneRedirectPolicy: + """Two acquisition services rejected every redirect through their own + opener while eight allowed a same-origin one through this guard. Two + policies with no stated reason, and only one of them checked where the + response actually came from.""" + + def test_the_strict_policy_refuses_any_redirect(self) -> None: + from app.services.outbound_request_guard import guarded_opener + + opener = guarded_opener("https://geo.api.vlaanderen.be/GRB/wfs", allow_redirect=False) + + class _Redirected: + url = "https://geo.api.vlaanderen.be/GRB/wfs/v2" + + def __enter__(self): + return self + + def __exit__(self, *_args): + return False + + with pytest.raises(AppError) as exc_info: + with opener(object(), timeout=1, _transport=lambda *_a, **_k: _Redirected()): + pass + + assert exc_info.value.code == "OUTBOUND_REDIRECT_NOT_ALLOWED" + + def test_the_strict_policy_still_allows_the_response_it_asked_for(self) -> None: + from app.services.outbound_request_guard import guarded_opener + + url = "https://geo.api.vlaanderen.be/GRB/wfs" + opener = guarded_opener(url, allow_redirect=False) + + class _Direct: + def __init__(self) -> None: + self.url = url + + def __enter__(self): + return self + + def __exit__(self, *_args): + return False + + with opener(object(), timeout=1, _transport=lambda *_a, **_k: _Direct()) as response: + assert response.url == url + + def test_both_policies_refuse_a_private_destination(self) -> None: + from app.services.outbound_request_guard import guarded_opener + + for allow_redirect in (True, False): + with pytest.raises(AppError) as exc_info: + guarded_opener("http://10.0.0.5/wfs", allow_redirect=allow_redirect) + assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED" + + def test_the_strict_services_use_the_shared_guard(self) -> None: + """Behavioural: their own fetch paths refuse a private endpoint, which + the hand-rolled opener never checked.""" + + from app.core.config import Settings + from app.services.grb_acquisition_service import GrbAcquisitionService + from app.services.official_vector_acquisition_service import OfficialVectorAcquisitionService + + settings = Settings(_env_file=None) + for service in (GrbAcquisitionService, OfficialVectorAcquisitionService): + with pytest.raises(AppError) as exc_info: + service._read_page("http://127.0.0.1:9/wfs", settings, None) + assert exc_info.value.code == "OUTBOUND_URL_NOT_ALLOWED" + + +def test_a_refused_redirect_is_never_requested() -> None: + """Rejecting after the fact still sends the request. + + Checking ``response.url`` means urllib has already followed the chain: the + connection to the redirect target was opened and the response read. For a + destination like a metadata endpoint that is the whole attack. The strict + policy must refuse to follow, not refuse afterwards. + """ + + from app.services.outbound_request_guard import no_redirect_opener + + opener = no_redirect_opener() + handlers = [type(handler).__name__ for handler in opener.handlers] + + assert "_RejectRedirects" in handlers + + +def test_the_rejecting_handler_returns_no_new_request() -> None: + from app.services.outbound_request_guard import _RejectRedirects + + handler = _RejectRedirects() + + assert handler.redirect_request(None, None, 302, "Found", {}, "http://169.254.169.254/") is None diff --git a/docs/KNOWN_LIMITATIONS.md b/docs/KNOWN_LIMITATIONS.md index f1ae0ea0..39bb28ae 100644 --- a/docs/KNOWN_LIMITATIONS.md +++ b/docs/KNOWN_LIMITATIONS.md @@ -62,8 +62,12 @@ runtime source of truth. - An export states its own completeness in `geointel_provenance`. A capped export is still a valid, usable file — it simply no longer implies it holds everything the selection contains. -- Bounded acquisition refuses a redirect that leaves the configured origin, and - refuses any private, loopback or link-local destination. An official endpoint +- Bounded acquisition refuses any private, loopback or link-local destination, + resolving the host first so a DNS name cannot hide one. Two redirect policies + apply: the paged OGC feature readers refuse to follow a redirect at all, + because a page URL they built themselves should be answered by that URL; the + other readers allow a redirect within the configured origin, since providers + do version their endpoints. A refused redirect is never requested. An official endpoint that legitimately moves to a new host therefore fails closed until the operator updates the configured URL, which is the intended trade: bytes from an unexpected host must never be persisted under an official provenance.