From d34ecd53f9057570a84aabbeb2ab3c1f07a83f62 Mon Sep 17 00:00:00 2001 From: Alisson Patricio Date: Thu, 8 Oct 2026 23:33:50 +0000 Subject: [PATCH] fix(geo): reject foreign relay IDs in mutations, and don't cache private tiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_obj_from_relay_id` resolves a public ID through DocumentId and returns the object of ANY registered model. The geo mutations used it without a type check, so a user holding only the geo model permissions could reach other blocks' rows: - geoFeatureDelete called obj.delete() on whatever came back. A User's (or Comment's, Follow's, Rate's) relay ID deleted that row. - geoFeatureUpdate passed it to GeoJSONFeatureForm(instance=obj). With all four optional inputs supplied, construct_instance wrote `name` and `description` onto the foreign instance and form.save() persisted it. Both now resolve through _get_feature(), which type-checks and raises `not_found`. The resolver's own exceptions are caught and logged rather than surfaced, since they leak which IDs exist. geoFeatureCreate rejects an unresolvable targetObjectId the same way, instead of failing later with an IntegrityError on the NOT NULL target_document_id. Also: - Tiles sent `Cache-Control: public` unconditionally, even though the view's permission is overridable precisely so projects can hide features from anonymous users. Visibility is now derived from whether an anonymous user may view, and Vary: Authorization is set so token-auth deployments aren't shared-cached on the session cookie alone. - baseapp/geo/graphql/__init__.py re-exported the ObjectTypes, so importing the FilterSet from the REST tile view pulled in graphene, graphene_django, query_optimizer and baseapp_auth.graphql — making the REST surface depend on the `graphql` extra and defeating the laziness filters.py documents. Emptied, matching baseapp/files and baseapp/content_feed. Not addressed here: object-level authorization on update/delete (no owner column exists on AbstractGeoJSONFeature yet) and the generic `GeometryObjectType` type name. baseapp/geo/tests: 114 passed. black, isort and flake8 clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity --- baseapp/geo/graphql/__init__.py | 15 ----- baseapp/geo/graphql/mutations.py | 43 +++++++++++- baseapp/geo/rest_framework/views.py | 17 ++++- baseapp/geo/tests/test_graphql_mutations.py | 75 +++++++++++++++++++++ baseapp/geo/tests/test_rest_tiles.py | 23 +++++++ 5 files changed, 154 insertions(+), 19 deletions(-) diff --git a/baseapp/geo/graphql/__init__.py b/baseapp/geo/graphql/__init__.py index bdff4e51..e69de29b 100644 --- a/baseapp/geo/graphql/__init__.py +++ b/baseapp/geo/graphql/__init__.py @@ -1,15 +0,0 @@ -from .object_types import ( - BaseGeoJSONFeatureObjectType, - GeoJSONFeatureObjectType, - GeoJSONFeatureProperties, - GeometryObjectType, -) -from .scalars import Geometry - -__all__ = [ - "BaseGeoJSONFeatureObjectType", - "GeoJSONFeatureObjectType", - "GeoJSONFeatureProperties", - "GeometryObjectType", - "Geometry", -] diff --git a/baseapp/geo/graphql/mutations.py b/baseapp/geo/graphql/mutations.py index f2a60c08..fd7ea8f0 100644 --- a/baseapp/geo/graphql/mutations.py +++ b/baseapp/geo/graphql/mutations.py @@ -1,3 +1,5 @@ +import logging + import graphene import swapper from django import forms @@ -13,11 +15,41 @@ from ..models import ALLOWED_GEOMETRY_TYPES from .scalars import Geometry +logger = logging.getLogger(__name__) + GeoJSONFeature = swapper.load_model("baseapp_geo", "GeoJSONFeature") app_label = GeoJSONFeature._meta.app_label ObjectType = GeoJSONFeature.get_graphql_object_type() +def _resolve_relay_id(info: graphene.ResolveInfo, relay_id): + """Resolve a relay ID, treating any failure as "not found". + + Never let the resolver's own exception reach the client: it leaks which IDs exist. + """ + try: + return get_obj_from_relay_id(info, relay_id) + except Exception: + logger.exception("Could not resolve relay id in a geo mutation") + return None + + +def _get_feature(info: graphene.ResolveInfo, relay_id) -> "GeoJSONFeature": + """Resolve a relay ID to a GeoJSONFeature, or raise `not_found`. + + `get_obj_from_relay_id` resolves a public ID through DocumentId and returns the object + of ANY registered model, so this type check is what stops these mutations from writing + to (or deleting) another block's rows. + """ + obj = _resolve_relay_id(info, relay_id) + if not isinstance(obj, GeoJSONFeature): + raise GraphQLError( + str(_("GeoJSON feature not found")), + extensions={"code": "not_found"}, + ) + return obj + + class GeoJSONFeatureForm(forms.ModelForm): class Meta: model = GeoJSONFeature @@ -68,7 +100,12 @@ def mutate_and_get_payload( extensions={"code": "permission_required"}, ) - target = get_obj_from_relay_id(info, input.get("target_object_id")) + target = _resolve_relay_id(info, input.get("target_object_id")) + if target is None: + raise GraphQLError( + str(_("Target object not found")), + extensions={"code": "not_found"}, + ) instance = GeoJSONFeature(target=target) form = GeoJSONFeatureForm(instance=instance, data=input) @@ -121,7 +158,7 @@ def mutate_and_get_payload( extensions={"code": "permission_required"}, ) - instance = get_obj_from_relay_id(info, input.get("id")) + instance = _get_feature(info, input.get("id")) data = { field: input[field] if field in input else getattr(instance, field) @@ -169,7 +206,7 @@ def mutate_and_get_payload( ) relay_id = input.get("id") - obj = get_obj_from_relay_id(info, relay_id) + obj = _get_feature(info, relay_id) target = obj.target diff --git a/baseapp/geo/rest_framework/views.py b/baseapp/geo/rest_framework/views.py index 19c72d00..e4adf3e8 100644 --- a/baseapp/geo/rest_framework/views.py +++ b/baseapp/geo/rest_framework/views.py @@ -1,4 +1,5 @@ import swapper +from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ValidationError from django.db.models import CharField, Expression, F, QuerySet from django.db.models.functions import Cast @@ -61,6 +62,18 @@ def get_feature_properties(self) -> dict[str, Expression]: "feature_type": F("feature_type"), } + def get_cache_control(self) -> str: + """`public` only when anonymous users may view features. + + The permission is overridable, and under a backend that hides features from + anonymous users a shared cache would otherwise hand one user's tile to the next + requester of the same z/x/y. + """ + GeoJSONFeature = swapper.load_model("baseapp_geo", "GeoJSONFeature") + perm = f"{GeoJSONFeature._meta.app_label}.view_geojsonfeature" + visibility = "public" if AnonymousUser().has_perm(perm) else "private" + return f"{visibility}, max-age={self.cache_max_age}" + def get(self, request: Request, z: int, x: int, y: int) -> HttpResponse: tile = Tile(z, x, y) if not tile.is_valid: @@ -92,5 +105,7 @@ def get(self, request: Request, z: int, x: int, y: int) -> HttpResponse: content_type=MVT_CONTENT_TYPE, status=status.HTTP_200_OK if content else status.HTTP_204_NO_CONTENT, ) - response["Cache-Control"] = f"public, max-age={self.cache_max_age}" + response["Cache-Control"] = self.get_cache_control() + # Token auth never touches the session, so nothing else would mark these apart. + response["Vary"] = "Authorization" return response diff --git a/baseapp/geo/tests/test_graphql_mutations.py b/baseapp/geo/tests/test_graphql_mutations.py index 3b477f73..cfe61dc8 100644 --- a/baseapp/geo/tests/test_graphql_mutations.py +++ b/baseapp/geo/tests/test_graphql_mutations.py @@ -1,3 +1,5 @@ +import uuid + import pytest import swapper from django.contrib.auth.models import Permission @@ -294,6 +296,79 @@ def test_perm_granted_user_deletes_feature(self, django_user_client, graphql_use assert GeoJSONFeature.objects.count() == 0 +class TestCrossModelRelayIds: + """A relay ID of another model must not be writable through these mutations. + + `get_obj_from_relay_id` resolves a public ID through DocumentId and returns the object + of any registered model, so without a type check a user holding only the geo model + permissions could edit or delete rows belonging to other blocks. + """ + + def test_update_with_another_models_id_is_not_found_and_changes_nothing( + self, django_user_client, graphql_user_client + ): + victim = UserFactory(first_name="Untouched") + grant_model_perm(django_user_client.user, "change_geojsonfeature") + + response = graphql_user_client( + UPDATE_MUTATION, + variables={ + "input": { + "id": victim.relay_id, + "name": "Pwned", + "description": "Pwned", + "featureType": "pwned", + "geometry": POINT_GEOJSON, + } + }, + ) + content = response.json() + + assert content["errors"][0]["extensions"]["code"] == "not_found" + victim.refresh_from_db() + assert victim.first_name == "Untouched" + + def test_delete_with_another_models_id_is_not_found_and_deletes_nothing( + self, django_user_client, graphql_user_client + ): + victim = UserFactory() + grant_model_perm(django_user_client.user, "delete_geojsonfeature") + + response = graphql_user_client( + DELETE_MUTATION, variables={"input": {"id": victim.relay_id}} + ) + content = response.json() + + assert content["errors"][0]["extensions"]["code"] == "not_found" + assert type(victim).objects.filter(pk=victim.pk).exists() + + def test_create_with_unknown_target_is_not_found_not_a_server_error( + self, django_user_client, graphql_user_client + ): + response = graphql_user_client( + CREATE_MUTATION, + variables={ + "input": { + "targetObjectId": str(uuid.uuid4()), + "geometry": POINT_GEOJSON, + } + }, + ) + content = response.json() + + assert content["errors"][0]["extensions"]["code"] == "not_found" + assert GeoJSONFeature.objects.count() == 0 + + def test_update_with_a_malformed_id_is_not_found(self, django_user_client, graphql_user_client): + grant_model_perm(django_user_client.user, "change_geojsonfeature") + + response = graphql_user_client( + UPDATE_MUTATION, variables={"input": {"id": "not-a-relay-id", "name": "x"}} + ) + + assert response.json()["errors"][0]["extensions"]["code"] == "not_found" + + class TestWritePermissions: """Write-permission matrix through real mutations (AC-9.5, AC-9.6, AC-10.3).""" diff --git a/baseapp/geo/tests/test_rest_tiles.py b/baseapp/geo/tests/test_rest_tiles.py index 7bb25ac5..11b77aa2 100644 --- a/baseapp/geo/tests/test_rest_tiles.py +++ b/baseapp/geo/tests/test_rest_tiles.py @@ -202,6 +202,29 @@ def test_cache_headers(self, client) -> None: response = client.get(tile_url(tile_for(POINT_NYC, FEATURES_ZOOM))) assert response["Cache-Control"] == "public, max-age=300" + # Other Vary values are appended by DRF/middleware; only ours has to be present. + assert "Authorization" in response["Vary"] + + def test_cache_is_private_when_features_are_not_public( + self, django_user_client, monkeypatch + ) -> None: + """Under a backend that hides features from anonymous users, a shared cache must + not be allowed to serve one user's tile to the next requester.""" + from baseapp.geo import permissions + + monkeypatch.setattr( + permissions.GeoPermissionsBackend, + "has_perm", + lambda self, user_obj, perm, obj=None: bool( + getattr(user_obj, "is_authenticated", False) + ), + ) + GeoJSONFeatureFactory(geometry=POINT_NYC) + + response = django_user_client.get(tile_url(tile_for(POINT_NYC, FEATURES_ZOOM))) + + assert response.status_code == 200, response.content + assert response["Cache-Control"] == "private, max-age=300" def test_filters_are_the_graphql_ones(self, client) -> None: tree = GeoJSONFeatureFactory(geometry=POINT_NYC, feature_type="tree")