Skip to content

fix(geo): reject foreign relay IDs in mutations, and don't cache private tiles - #472

Merged
nossila merged 1 commit into
feature/baseapp_geofrom
feature/baseapp_geo-mutation-auth
Oct 9, 2026
Merged

nossila merged 1 commit into
feature/baseapp_geofrom
feature/baseapp_geo-mutation-auth

Conversation

@nossila

@nossila nossila commented Oct 8, 2026

Copy link
Copy Markdown
Member

Group 1 of the geo code-review findings — the ones that need no schema change or product decision.

The core problem

get_obj_from_relay_id → PublicIdResolverStrategy.resolve_id → DocumentId.get_object_by_public_id(id) with no model_class, which returns the content_object of any registered model. The geo mutations used the result with no type check, and the permission check above it had already run with obj=None, so nothing narrowed it.

Reachable by any user holding the geo model permissions (GeoPermissionsBackend returns False for change/delete, so those fall through to ModelBackend's model-level grant):

  • geoFeatureDelete called obj.delete() on whatever came back. Pass a User's relay ID — AbstractUser carries DocumentIdMixin + RelayModel, so it has one — and that row is deleted.
  • geoFeatureUpdate passed it to GeoJSONFeatureForm(instance=obj). With all four optional inputs supplied the getattr fallback never runs, construct_instance writes every form field the instance shares (name, description), and form.save() persists it.

baseapp/files/graphql/mutations.py already does exactly the missing check (isinstance(file_obj, File) → not_found), so this is a deviation from the reference package, not a new convention.

Changes

  • _get_feature() type-checks the resolved object and raises not_found; used by update and delete. Resolver exceptions are caught and logged rather than surfaced — they leak which IDs exist.
  • geoFeatureCreate rejects an unresolvable targetObjectId with not_found instead of failing later with an IntegrityError on the NOT NULL target_document_id (a 500 to the client).
  • Tiles derived Cache-Control from whether an anonymous user may view, rather than hardcoding public. The view's permission is overridable precisely so projects can hide features from anonymous users; under such a backend a CDN was free to serve one user's tile to the next requester. Vary: Authorization is set too, since token auth never touches the session cookie.
  • baseapp/geo/graphql/__init__.py emptied. It re-exported the ObjectTypes, so importing the FilterSet from rest_framework/views.py pulled in graphene, graphene_django, query_optimizer and baseapp_auth.graphql — making the REST tile endpoint depend on the graphql extra and defeating the laziness filters.py documents. baseapp/files and baseapp/content_feed both ship a 0-line one.

Testing

New tests assert the victim survives, not just that an error is returned:

  • update with another model's relay ID → not_found, and the target's field is unchanged after refresh_from_db()
  • delete with another model's relay ID → not_found, and the row still exists
  • create with an unknown target UUID → not_found, no rows written
  • malformed relay ID → not_found
  • tiles under a non-public permission backend → private, max-age=300

baseapp/geo/tests: 114 passed. black, isort, flake8 clean.

Deliberately not in this PR

  • Object-level authorization on update/delete (review findings 3 and 4). AbstractGeoJSONFeature has no created_by, so there is no owner to authorize against — that needs a model field, a migration, and a permissions-backend change modelled on FilesPermissionsBackend._is_owner_or_has_global. A product decision, not a patch.
  • Renaming GeometryObjectType (finding 8). It collides with the name graphql_geojson's converter produces, but renaming is a public schema change for anything already consuming it.
  • The MVT buffer finding (9), which belongs with the tile rendering work.

🤖 Generated with Claude Code

https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity

…ate tiles

`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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity
@coderabbitai

coderabbitai Bot commented Oct 8, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3f9a5fa9-9dde-4435-82bc-ca7d2d6d6416

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nossila
nossila merged commit 731de71 into feature/baseapp_geo Oct 9, 2026
1 of 2 checks passed
@nossila
nossila deleted the feature/baseapp_geo-mutation-auth branch October 9, 2026 01:06
nossila added a commit that referenced this pull request Oct 9, 2026
Still-valid findings from the Copilot and CodeRabbit reviews. Four others were already
fixed by #472 / #473 and are answered on the threads rather than here.

- Permission codenames were hardcoded to `geojsonfeature`. The model is swappable, so a
  project swapping in `MapFeature` gets `add_mapfeature` and none of the checks would ever
  match. All of them now derive the codename from `_meta.model_name`, across permissions,
  mutations, object types and the REST view/permission.
- `geoFeatures` never enforced the read permission. DjangoConnectionField resolves edges
  straight from the queryset and never calls `get_node`, so a project that overrides the
  public-read backend still had every feature listable. Added `get_queryset`, returning
  `skip_ast_walker(queryset.none())` so the optimizer isn't run over an empty queryset.
- The legacy base64 branch of the target filter validated the type name but never checked
  the row exists, so a well-formed ID for a deleted object returned an empty list instead
  of the documented validation error. It now mirrors the public-id branch.
- README: `baseapp_geo` -> `baseapp.geo` in the INSTALLED_APPS instruction (the label is
  not importable), the auth-backend slot is `baseapp_geo` not `baseapp.geo`, and the
  factory lives at `baseapp.geo.tests.factories`.

baseapp/geo/tests: 124 passed. black, isort and flake8 clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fczpSCpN3kCRs3UBjbity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant