keep a precise failure precise, and state one redirect policy

Two problems of the same shape: information about *why* something failed being
replaced by something vaguer.

get_dataset_geojson wrapped the JSON parse, the metadata read, the CRS
resolution and the canonicalisation in one try and reported all of it as
"Stored dataset is not valid JSON" with a 500. An operator whose dataset had an
unusable CRS was sent to inspect a file that parses perfectly well, and the
canonicaliser's own AppError — with its code and its status — never reached
them. Only the parse is now inside that handler; everything after it keeps the
error it raised, and a genuine bug becomes a distinct 500 rather than a
mislabelled client error. A guard finds the same shape elsewhere: catching
Exception around a call into another component and relabelling what it
reported. Wrapping one's own private helper stays legitimate and the guard
says so.

The redirect policy was split without anyone saying so. Two acquisition
services rejected every redirect through a hand-rolled opener, while eight
allowed a same-origin one through the shared guard — and only the latter
checked where the response came from. Both live in the guard now, and the
strict path uses the rejecting handler rather than the guard's after-the-fact
check: objecting to response.url means urllib already opened the connection and
read the body, which for a metadata endpoint is the whole attack. That was a
weakening I introduced in this same commit's first draft.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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]:

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:

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:

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)

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"

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(")

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

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
+- Bounded acquisition refuses any private, loopback or link-local destination,
-  refuses any private, loopback or link-local destination. An official endpoint
+  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.