From cb9ee72cffd7aadd0af2b259f9160c8c07b3dd0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 18:27:22 +0200 Subject: [PATCH 01/19] docs: enrich API decorators, add /api/location-schema, fix CSRF gaps Enriches spectree decorators so /api/doc carries real parameter and response schemas for every endpoint (previously most had none), using skip_validation to document dynamic query parameters (lat/lon/limit, zoom, category filters) without changing their intentionally-lenient runtime behaviour. Adds GET /api/location-schema so a client can discover what a given instance accepts for a new point instead of relying on http-api.rst's static example. Fixes three CSRF issues found while verifying the docs against the running app: platzky already initializes CSRFProtect, so goodmap's second CSRFProtect(app) call registered a duplicate before_request hook; CSRF failures returned an HTML error page instead of the API's documented JSON shape; and WTF_CSRF_SSL_STRICT rejected scripted https callers that send a valid session-bound token but no Referer header. Shrinks http-api.rst now that the schema covers response shapes, and switches its examples from the old climbing-crag data to the bridges dataset e2e tests actually use. --- docs/conf.py | 5 +- docs/http-api.rst | 117 +++++++------------- goodmap/api_models.py | 177 +++++++++++++++++++++++++++++- goodmap/core_api.py | 83 ++++++++++++-- goodmap/goodmap.py | 22 +++- tests/unit_tests/test_core_api.py | 18 +++ tests/unit_tests/test_goodmap.py | 68 ++++++++++++ 7 files changed, 392 insertions(+), 98 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index 65b878de..39fa526a 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -62,7 +62,10 @@ _ANY_PY_ROLE = "py:.*" nitpick_ignore_regex = [ (_ANY_PY_ROLE, r"ConfigDict|callable"), - (_ANY_PY_ROLE, r"(annotated_types|pymongo)\..*"), + (_ANY_PY_ROLE, r"(annotated_types|pymongo|spectree)\..*"), + # RootModel generics: pydantic's inventory has RootModel but not RootModel[...] + # or RootModelRootType, so the parametrised bases autodoc prints cannot resolve. + (_ANY_PY_ROLE, r"pydantic\.root_model\..*"), (_ANY_PY_ROLE, r"[gl]e=-?\d+"), ( _ANY_PY_ROLE, diff --git a/docs/http-api.rst b/docs/http-api.rst index 09d203b9..7f37c2ae 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -18,8 +18,10 @@ A running instance also serves its own generated OpenAPI schema: * - ``/api/doc/openapi.json`` - raw OpenAPI document -Use this page for the semantics and the schema endpoint for the exact shapes of the -release you are running. +The schema is generated from the code, so it always describes the release you are +running: every endpoint's parameters, status codes and response shapes. **Use it as the +reference.** This page covers the parts a schema cannot state — what the endpoints mean, +how filtering behaves, and which parameters depend on your own data. Conventions ----------- @@ -29,8 +31,9 @@ Conventions on the ``categories`` in your data source (:doc:`data-source`). **Writes need a CSRF token.** CSRF protection is on for the whole app, so ``POST``, -``PUT`` and ``DELETE`` without a token get ``400 The CSRF token is missing``. Send it as -an ``X-CSRFToken`` header. Server-rendered pages expose one in a meta tag: +``PUT`` and ``DELETE`` without a token get ``400 {"message": "The CSRF token is +missing."}``. Send it as an ``X-CSRFToken`` header, from the same session the token was +minted in — the token is bound to the session cookie, so the pair travels together. Server-rendered pages expose one in a meta tag: .. code-block:: html @@ -84,17 +87,8 @@ Query parameters: curl 'http://localhost:5000/api/locations?accessible_by=bikes&lat=51.10&lon=17.05&limit=5' -.. code-block:: json - - [ - { - "uuid": "7c3d5e7f-9a1b-4c3d-8e5f-7a9b1c3d5e7f", - "position": [50.0397, 19.906], - "remark": false - } - ] - -``remark`` is a **boolean** — whether the point has a remark, not the remark itself. +Each point comes back as ``uuid``, ``position`` and ``remark`` — where ``remark`` is a +**boolean**, whether the point has one, not its text. Invalid or unknown query parameters are ignored rather than rejected. @@ -108,27 +102,8 @@ level. This is what the frontend calls instead of ``/api/locations`` when Takes every parameter of :ref:`api-locations`, plus ``zoom`` (integer, **0–16**, default ``7``). A ``zoom`` outside that range is a ``400``. -.. code-block:: json - - [ - { - "type": "cluster", - "position": [50.1026, 19.8240], - "uuid": null, - "cluster_uuid": "34515392-7913-47be-a5b4-0c4b5247ad4c", - "cluster_count": 2 - }, - { - "type": "point", - "position": [50.833, 15.917], - "uuid": "9b1c3d5e-7f9a-4b1c-8d5e-9f1a3b5c7d9e", - "cluster_uuid": null, - "cluster_count": null - } - ] - -Both kinds come back in one list, told apart by ``type``. A ``"point"`` carries a real -``uuid`` you can pass to :ref:`api-location-detail`; a ``"cluster"`` carries a +Points and clusters come back in one list, told apart by ``type``. A ``"point"`` carries +a real ``uuid`` you can pass to :ref:`api-location-detail`; a ``"cluster"`` carries a freshly-generated ``cluster_uuid`` (not stable across requests — it is a render key, not an identifier) and the number of points it stands for. ``position`` is ``[latitude, longitude]``, as everywhere else. @@ -169,20 +144,6 @@ well-formed UUID that does not exist also gives ``404 {"message": "Location not Every category with its options, defaults and filter mode — everything needed to render the filter panel in one request. -.. code-block:: json - - { - "categories": [ - { - "key": "accessible_by", - "name": "accessible_by", - "options": [["bikes", "bikes"], ["cars", "cars"]], - "default_checked": [], - "filter_mode": "or" - } - ] - } - ``key`` is the query-parameter name to filter by; ``name`` is its translated label. ``options`` are ``[value, translated label]`` pairs — send the *value*. ``filter_mode`` tells you which control to draw: checkboxes for ``or``/``and``, radio @@ -192,41 +153,32 @@ buttons for ``exclusive``/``threshold``, a single checkbox for ``boolean`` With ``CATEGORIES_HELP`` on, each category also carries ``options_help``, and the response gains a top-level ``categories_help`` — both lists of ``{option: help text}`` objects. -Prefer this endpoint over the two below, which exist for older clients and cost one -request per category. - -``GET /api/categories`` -~~~~~~~~~~~~~~~~~~~~~~~ +Prefer this endpoint over ``GET /api/categories`` and ``GET /api/category/``, which +exist for older clients and cost one request per category. Both share one trap worth +knowing: with ``CATEGORIES_HELP`` **off** they return a bare list of pairs, and with it +**on** they return an object instead — the response *type* changes with the flag, not just +its contents. The schema documents both shapes. -Category names only, as ``[key, translated name]`` pairs. With ``CATEGORIES_HELP`` on, -returns ``{"categories": [...], "categories_help": [...]}`` instead — note the response -*type* changes with the flag. +.. _api-location-schema: -``GET /api/category/`` +``GET /api/location-schema`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -The options for one category, as ``[value, translated label]`` pairs. With -``CATEGORIES_HELP`` on, returns -``{"categories_options": [...], "categories_options_help": [...]}``. - -``GET /api/languages`` -~~~~~~~~~~~~~~~~~~~~~~ - -The configured interface languages, exactly as given in ``LANGUAGES``: - -.. code-block:: json - - {"en": {"name": "English", "flag": "gb", "country": "GB"}} +What *this* instance accepts for a new point: the fields of its location model (minus the +server-managed ``uuid`` and ``position``), the allowed values per category, the reportable +issue types, and the photo limits. Since the accepted fields are configured per deployment, +this is how a client discovers them rather than assuming — it is the same schema the +built-in suggest form is generated from. -``GET /api/version`` +Other read endpoints ~~~~~~~~~~~~~~~~~~~~ -.. code-block:: json - - {"backend": ""} +``GET /api/languages`` returns the configured languages keyed by language code, exactly as +given in ``LANGUAGES``. -The installed package version, normalised to PEP 440 — a release published as -``2.0.0-alpha.5`` reports as ``2.0.0a5``. Useful as a health check. +``GET /api/version`` returns the installed package version normalised to PEP 440, so a +release published as ``2.0.0-alpha.5`` reports as ``2.0.0a5``. It needs no data source, +which makes it the endpoint to point a load balancer at. Submissions ----------- @@ -244,7 +196,13 @@ the map data. **The request must be ``multipart/form-data``.** The point goes in a single ``location`` form field as a JSON object — not as one form field per property — and the optional photo -goes in a ``photo`` file part. Send the point without a ``uuid``; the server assigns one: +goes in a ``photo`` file part. Send the point without a ``uuid``; the server assigns one. + +That envelope is the same everywhere. **What goes inside the JSON object is not** — the +accepted fields are whatever *your* data source declares in ``location_obligatory_fields`` +and ``categories`` (:doc:`data-source`), so there is no universal payload to copy. The +fields below are the ones the :doc:`quickstart` map happens to declare; substitute your +own: .. code-block:: bash @@ -253,6 +211,9 @@ goes in a ``photo`` file part. Send the point without a ``uuid``; the server ass -F 'location={"name": "Nowy", "position": [51.11, 17.03], "type_of_place": "small bridge", "accessible_by": ["bikes"], "is_free": "true"}' \ -F 'photo=@bridge.jpg' +To find the fields a given instance wants, call :ref:`api-location-schema` — the same +schema the built-in suggest form is generated from. + .. note:: Sending the point as a JSON request body used to work and no longer does — a diff --git a/goodmap/api_models.py b/goodmap/api_models.py index 8de93dfe..be21f852 100644 --- a/goodmap/api_models.py +++ b/goodmap/api_models.py @@ -5,9 +5,10 @@ and request/response validation. """ -from typing import Literal +from typing import Any, Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, RootModel +from spectree import BaseFile class LocationReportRequest(BaseModel): @@ -84,12 +85,176 @@ class SuccessResponse(BaseModel): message: str = Field(..., description="Success message") +class LocationBasicInfo(BaseModel): + """One point as returned by the list endpoint: identity and position only.""" + + uuid: str = Field(..., description="Location UUID") + position: tuple[float, float] = Field(..., description="[latitude, longitude]") + remark: bool = Field(..., description="Whether the point has a remark, not the remark itself") + + +class LocationList(RootModel[list[LocationBasicInfo]]): + """List of points, each with identity and position only.""" + + class ClusterInfo(BaseModel): - """Cluster information for map display.""" + """One entry of the clustered list: either a single point or a cluster of them.""" + + type: Literal["cluster", "point"] = Field(..., description="Which of the two this entry is") + position: tuple[float, float] = Field(..., description="[latitude, longitude]") + uuid: str | None = Field(None, description="Location UUID; null for a cluster") + cluster_uuid: str | None = Field( + None, + description="Render key for a cluster, regenerated per request; null for a point", + ) + cluster_count: int | None = Field( + None, description="Number of points the cluster stands for; null for a point" + ) + + +class ClusterList(RootModel[list[ClusterInfo]]): + """Points and clusters in one list, told apart by ``type``.""" + + +class LocationDetail(BaseModel): + """One point formatted for its map popup.""" + + title: str = Field(..., description="The point's name") + subtitle: str = Field(..., description="The point's type_of_place") + position: tuple[float, float] = Field(..., description="[latitude, longitude]") + data: list[tuple[str, Any]] = Field( + ..., description="[label, value] pairs for the visible_data fields, translated" + ) + metadata: dict[str, Any] = Field(..., description="The meta_data fields") + + +class CategoriesWithHelp(BaseModel): + """Category names plus help text, returned when CATEGORIES_HELP is on.""" + + categories: list[tuple[str, str]] = Field(..., description="[key, translated label] pairs") + categories_help: list[dict[str, str]] = Field(..., description="Help text per category") + + +class CategoriesResponse(RootModel[CategoriesWithHelp | list[tuple[str, str]]]): + """Bare [key, label] pairs, or an object with help text when CATEGORIES_HELP is on.""" + + +class CategoryFull(BaseModel): + """One category with everything needed to render its filter control.""" + + key: str = Field(..., description="Query-parameter name to filter by") + name: str = Field(..., description="Translated label") + options: list[tuple[str, str]] = Field(..., description="[value, translated label] pairs") + default_checked: list[str] = Field(..., description="Values checked on first load") + filter_mode: Literal["or", "and", "exclusive", "boolean", "threshold"] = Field( + ..., description="Which control to draw and how selections combine" + ) + options_help: list[dict[str, str]] | None = Field( + None, description="Present only when the CATEGORIES_HELP feature flag is on" + ) + + +class CategoriesFullResponse(BaseModel): + """Every category with its options, defaults and filter mode.""" + + categories: list[CategoryFull] = Field(..., description="One entry per category") + categories_help: list[dict[str, str]] | None = Field( + None, description="Present only when the CATEGORIES_HELP feature flag is on" + ) + + +class CategoryOptionsWithHelp(BaseModel): + """A category's options plus help text, returned when CATEGORIES_HELP is on.""" + + categories_options: list[tuple[str, str]] = Field( + ..., description="[value, translated label] pairs" + ) + categories_options_help: list[dict[str, str]] = Field(..., description="Help text per option") + + +class CategoryOptionsResponse(RootModel[CategoryOptionsWithHelp | list[tuple[str, str]]]): + """Bare [value, label] pairs, or an object with help text when CATEGORIES_HELP is on.""" + + +class LocationQueryParams(BaseModel): + """Non-filter query parameters of the location list endpoints. + + Filter parameters are *not* listed here: they are named after the categories in the + deployment's own data source, so they differ per instance. Call + ``GET /api/categories-full`` to discover the ones this instance accepts. + """ + + lat: float | None = Field(None, description="Sort by distance from this latitude; requires lon") + lon: float | None = Field( + None, description="Sort by distance from this longitude; requires lat" + ) + limit: int | None = Field( + None, description="Return at most this many points, applied after sorting" + ) + + +class SuggestNewPointForm(BaseModel): + """The multipart/form-data body of a new-point suggestion.""" + + location: str = Field( + ..., + description=( + "The whole point as one JSON object, not one form field per property. " + "Its accepted fields come from this instance's location_obligatory_fields " + "and categories - call GET /api/location-schema to discover them. " + "Omit uuid; the server assigns one." + ), + ) + photo: BaseFile | None = Field(None, description="Optional photo, subject to ATTACHMENT limits") + + +class IssueType(BaseModel): + """One reportable issue type, ready to render in a form.""" + + value: str = Field(..., description="Value to send to /api/report-location") + label: str = Field(..., description="Translated label") + + +class PhotoLimits(BaseModel): + """What a photo attachment may be, from the ATTACHMENT config.""" + + allowed_extensions: list[str] = Field(..., description="Permitted file extensions") + allowed_mime_types: list[str] = Field(..., description="Permitted MIME types") + max_size_bytes: int = Field(..., description="Largest permitted photo, in bytes") + + +class LocationSchemaResponse(BaseModel): + """What this instance accepts for a new point - its schema, not a fixed contract.""" + + fields: dict[str, Any] = Field( + ..., + description=( + "JSON Schema property per accepted field, from the instance's location model, " + "excluding the server-managed uuid and position" + ), + ) + obligatory_fields: list[Any] = Field( + ..., description="[name, type] pairs every point must carry" + ) + categories: dict[str, list[str]] = Field( + ..., description="Filterable fields and their allowed values" + ) + reported_issue_types: list[IssueType] = Field( + ..., description="Accepted values for /api/report-location" + ) + photo: PhotoLimits = Field(..., description="Attachment limits for the photo part") + + +class LanguageInfo(BaseModel): + """One interface language.""" + + name: str = Field(..., description="Language name in that language") + flag: str = Field(..., description="Country code used to pick the flag icon") + country: str = Field(..., description="Country code") + - uuid: str | None = Field(None, description="Location UUID (None for multi-point clusters)") - position: tuple[float, float] = Field(..., description="Cluster center coordinates") - count: int = Field(..., description="Number of locations in cluster") +class LanguagesResponse(RootModel[dict[str, LanguageInfo]]): + """Interface languages, keyed by language code.""" # Note: Full location model is dynamically created from LocationBase diff --git a/goodmap/core_api.py b/goodmap/core_api.py index a6b18680..09545400 100644 --- a/goodmap/core_api.py +++ b/goodmap/core_api.py @@ -7,7 +7,7 @@ import deprecation import numpy import pysupercluster -from flask import Blueprint, jsonify, make_response, request +from flask import Blueprint, current_app, jsonify, make_response, request from flask_babel import gettext from platzky import FeatureFlagSet from platzky.attachment import create_attachment @@ -17,11 +17,22 @@ from werkzeug.exceptions import HTTPException from goodmap.api_models import ( + CategoriesFullResponse, + CategoriesResponse, + CategoryOptionsResponse, + ClusteringParams, + ClusterList, CSRFTokenResponse, ErrorResponse, + LanguagesResponse, + LocationDetail, + LocationList, + LocationQueryParams, LocationReportRequest, LocationReportResponse, + LocationSchemaResponse, SuccessResponse, + SuggestNewPointForm, VersionResponse, ) from goodmap.clustering import ( @@ -137,12 +148,20 @@ def _clean_model_name(model: type) -> str: title="Goodmap API", version="0.1", path="doc", - annotations=True, + # annotations=False: the handlers take no model-annotated parameters, and with + # it on spectree refuses skip_validation, which several routes below rely on. + annotations=False, naming_strategy=_clean_model_name, # Use clean model names without hash ) @core_api_blueprint.route("/suggest-new-point", methods=["POST"]) - @spec.validate(resp=Response(HTTP_200=SuccessResponse, HTTP_400=ErrorResponse)) + # skip_validation: the handler returns its own 400 for a missing or malformed + # `location` field; letting spectree validate would turn that into a 422. + @spec.validate( + form=SuggestNewPointForm, + resp=Response(HTTP_200=SuccessResponse, HTTP_400=ErrorResponse), + skip_validation=True, + ) def suggest_new_point(): """Suggest new location for review. @@ -268,7 +287,11 @@ def report_location(): return make_response(jsonify({"message": gettext("Location reported")}), 200) @core_api_blueprint.route("/locations", methods=["GET"]) - @spec.validate() + # skip_validation: this endpoint ignores invalid and unknown query parameters by + # design, so spectree must document them without rejecting anything. + @spec.validate( + query=LocationQueryParams, resp=Response(HTTP_200=LocationList), skip_validation=True + ) def get_locations(): """Get list of locations with basic info. @@ -279,7 +302,13 @@ def get_locations(): return jsonify(locations) @core_api_blueprint.route("/locations-clustered", methods=["GET"]) - @spec.validate(resp=Response(HTTP_400=ErrorResponse)) + # skip_validation: the handler owns zoom validation and returns 400 with a log line; + # letting spectree validate would turn that into a 422 and skip the log. + @spec.validate( + query=ClusteringParams, + resp=Response(HTTP_200=ClusterList, HTTP_400=ErrorResponse), + skip_validation=True, + ) def get_locations_clustered(): """Get clustered locations for map display. @@ -329,7 +358,7 @@ def get_locations_clustered(): return make_response(jsonify({"message": "An error occurred during clustering"}), 500) @core_api_blueprint.route("/location/", methods=["GET"]) - @spec.validate(resp=Response(HTTP_404=ErrorResponse)) + @spec.validate(resp=Response(HTTP_200=LocationDetail, HTTP_404=ErrorResponse)) def get_location(location_id): """Get detailed information for a single location. @@ -376,8 +405,42 @@ def generate_csrf_token(): csrf_token = csrf_generator() return {"csrf_token": csrf_token} + @core_api_blueprint.route("/location-schema", methods=["GET"]) + @spec.validate(resp=Response(HTTP_200=LocationSchemaResponse)) + def get_location_schema(): + """Get the schema this instance accepts for a new point. + + The fields a point may carry are configured per deployment, so there is no + fixed payload for /api/suggest-new-point. This returns the accepted fields, + the allowed values for each category, the reportable issue types and the + photo limits, as the built-in suggest form uses them. + """ + category_data = database.get_category_data() + properties = location_model.model_json_schema().get("properties", {}) + return jsonify( + { + "fields": { + name: spec_ + for name, spec_ in properties.items() + if name not in ("uuid", "position") + }, + "obligatory_fields": current_app.extensions.get("goodmap", {}).get( + "location_obligatory_fields", [] + ), + "categories": category_data.get("categories", {}), + "reported_issue_types": [ + {"value": t, "label": gettext(t)} for t in database.get_issue_options() + ], + "photo": { + "allowed_extensions": sorted(photo_attachment_config.allowed_extensions or []), + "allowed_mime_types": sorted(photo_attachment_config.allowed_mime_types or []), + "max_size_bytes": photo_attachment_config.max_size, + }, + } + ) + @core_api_blueprint.route("/categories", methods=["GET"]) - @spec.validate() + @spec.validate(resp=Response(HTTP_200=CategoriesResponse)) def get_categories(): """Get all available location categories. @@ -400,7 +463,7 @@ def get_categories(): return jsonify({"categories": categories, "categories_help": proper_categories_help}) @core_api_blueprint.route("/categories-full", methods=["GET"]) - @spec.validate() + @spec.validate(resp=Response(HTTP_200=CategoriesFullResponse)) def get_categories_full(): """Get all categories with their subcategory options in a single request. @@ -450,7 +513,7 @@ def get_categories_full(): return jsonify(response) @core_api_blueprint.route("/languages", methods=["GET"]) - @spec.validate() + @spec.validate(resp=Response(HTTP_200=LanguagesResponse)) def get_languages(): """Get all available interface languages. @@ -459,7 +522,7 @@ def get_languages(): return jsonify(languages) @core_api_blueprint.route("/category/", methods=["GET"]) - @spec.validate() + @spec.validate(resp=Response(HTTP_200=CategoryOptionsResponse)) def get_category_types(category_type): """Get all available options for a specific category. diff --git a/goodmap/goodmap.py b/goodmap/goodmap.py index 5e0395ef..e85bacb4 100644 --- a/goodmap/goodmap.py +++ b/goodmap/goodmap.py @@ -6,9 +6,9 @@ import os from typing import Any -from flask import Blueprint, redirect, render_template, session +from flask import Blueprint, jsonify, redirect, render_template, session from flask_babel import gettext -from flask_wtf.csrf import CSRFProtect, generate_csrf +from flask_wtf.csrf import CSRFError, generate_csrf from platzky import platzky from platzky.config import languages_dict from platzky.models import CmsModule @@ -231,7 +231,23 @@ def create_app_from_config(config: GoodmapConfig) -> platzky.Engine: app.config["PLUGIN_MANIFEST"] = plugin_manifest - CSRFProtect(app) + # CSRF protection itself is initialized by platzky (create_app_from_config runs + # CSRFProtect on the engine); initializing it here again would register a second + # before_request hook and break any future exempt() registered on one instance only. + # + # The token is bound to the session, which is the actual protection. The extra + # referrer check flask-wtf adds on https would reject scripted API callers that + # send a valid token but no Referer header, so it is off. + app.config["WTF_CSRF_SSL_STRICT"] = False + # The map page is typically left open well past the default 3600s, and the frontend + # never refreshes the meta-tag token - so scope the token to the session instead of + # rejecting submissions from any tab older than an hour. + app.config["WTF_CSRF_TIME_LIMIT"] = None + + @app.errorhandler(CSRFError) + def handle_csrf_error(error): + """Return CSRF failures in the API's JSON error shape instead of an HTML page.""" + return jsonify({"message": error.description}), 400 photo_attachment_config = config.attachment diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index 5c88f48c..fa81438b 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -33,6 +33,24 @@ def test_version_endpoint_returns_version(mock_returning_version, test_app): assert response.json == {"backend": "0.1.2"} +def test_location_schema_endpoint_describes_this_instance(test_app): + response = test_app.get("/api/location-schema") + assert response.status_code == 200 + body = response.json + assert set(body) == { + "fields", + "obligatory_fields", + "categories", + "reported_issue_types", + "photo", + } + # uuid and position are server-managed and must not be offered as form fields + assert "uuid" not in body["fields"] + assert "position" not in body["fields"] + assert all(set(t) == {"value", "label"} for t in body["reported_issue_types"]) + assert set(body["photo"]) == {"allowed_extensions", "allowed_mime_types", "max_size_bytes"} + + def test_csrf_token_endpoint_returns_token(test_app): response = test_app.get("/api/generate-csrf-token") assert response.status_code == 200 diff --git a/tests/unit_tests/test_goodmap.py b/tests/unit_tests/test_goodmap.py index 18b09e8e..60cc6213 100644 --- a/tests/unit_tests/test_goodmap.py +++ b/tests/unit_tests/test_goodmap.py @@ -523,6 +523,74 @@ def _make_test_app_config(feature_flags: Any = None, extra_data: Any = None) -> ) +def test_csrf_protect_is_initialized_exactly_once(): + """platzky already initializes CSRFProtect; goodmap must not add a second hook.""" + config = _make_test_app_config(extra_data={"categories": {}}) + app = goodmap.create_app_from_config(config) + hooks = [f.__name__ for f in app.before_request_funcs.get(None, [])] + assert hooks.count("csrf_protect") == 1 + + +def test_csrf_failure_returns_json_error(): + """A rejected write gets the API's JSON error shape, not an HTML error page.""" + config = _make_test_app_config(extra_data={"categories": {}, "data": []}) + app = goodmap.create_app_from_config(config) + client = app.test_client() + + response = client.post("/api/report-location", json={"id": "x", "description": "y"}) + + assert response.status_code == 400 + assert response.content_type.startswith("application/json") + assert response.json == {"message": "The CSRF token is missing."} + + +def test_csrf_accepts_scripted_https_caller_without_referer(): + """A valid token from the caller's own session works over https with no Referer.""" + import re + + config = _make_test_app_config(extra_data={"categories": {}, "data": []}) + app = goodmap.create_app_from_config(config) + client = app.test_client() + + page = client.get("/map", base_url="https://localhost") + token_match = re.search(r'name="csrf-token" content="([^"]+)"', page.data.decode("utf-8")) + assert token_match is not None + + response = client.post( + "/api/report-location", + json={"id": "x", "description": "y"}, + headers={"X-CSRFToken": token_match.group(1)}, + base_url="https://localhost", + ) + + # Past the CSRF layer: the handler itself may reject the payload, but not with + # a CSRF message and never as HTML. + assert response.content_type.startswith("application/json") + assert "CSRF" not in response.get_data(as_text=True) + + +def test_csrf_token_is_session_scoped(): + """The token alone is not enough - it must be paired with the session it was minted in.""" + import re + + config = _make_test_app_config(extra_data={"categories": {}, "data": []}) + app = goodmap.create_app_from_config(config) + assert app.config["WTF_CSRF_TIME_LIMIT"] is None + + victim = app.test_client() + page = victim.get("/map") + token = re.search(r'name="csrf-token" content="([^"]+)"', page.data.decode("utf-8")).group(1) + + attacker = app.test_client() + response = attacker.post( + "/api/report-location", + json={"id": "x", "description": "y"}, + headers={"X-CSRFToken": token}, + ) + assert response.status_code == 400 + assert response.json == {"message": "The CSRF session token is missing."} + + def test_admin_route_disabled(): """Should redirect to / when admin panel feature flag is disabled.""" config = _make_test_app_config() From 4fce39e63e7fb4cf9e880c7a6ad284b946e24f6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 13:16:16 +0200 Subject: [PATCH 02/19] fix!: removed deprecated api method --- frontend/src/utils/csrf.js | 50 +++++++------------------------ goodmap/api_models.py | 6 ---- goodmap/core_api.py | 18 ----------- goodmap/goodmap.py | 3 +- tests/unit_tests/test_core_api.py | 6 ---- 5 files changed, 12 insertions(+), 71 deletions(-) diff --git a/frontend/src/utils/csrf.js b/frontend/src/utils/csrf.js index 0229c95f..a28959fd 100644 --- a/frontend/src/utils/csrf.js +++ b/frontend/src/utils/csrf.js @@ -9,60 +9,32 @@ */ /** - * Gets the CSRF token from the page's meta tag, with fallback to legacy API endpoint. + * Gets the CSRF token from the page's meta tag. * - * Preferred method: The backend sets a meta tag like: + * The backend sets a meta tag like: * * - * Fallback (DEPRECATED): Fetches token from /api/generate-csrf-token endpoint. - * This fallback exists for backward compatibility but will be removed in a future version. - * * This token must be included in the X-CSRFToken header for all * state-changing requests (POST, PUT, PATCH, DELETE). * - * @returns {Promise} The CSRF token - * @throws {Error} If CSRF token cannot be obtained from either source + * @returns {string} The CSRF token + * @throws {Error} If the CSRF token meta tag is missing or empty * * @example - * const csrfToken = await getCsrfToken(); + * const csrfToken = getCsrfToken(); * axios.post('/api/suggest-new-point', data, { * headers: { 'X-CSRFToken': csrfToken } * }); */ -export const getCsrfToken = async () => { +export const getCsrfToken = () => { const metaTag = document.querySelector('meta[name="csrf-token"]'); + const token = metaTag?.getAttribute('content'); - // Try to get token from meta tag first (preferred method) - if (metaTag) { - const token = metaTag.getAttribute('content'); - if (token) { - return token; - } - } - - // Fallback to legacy API endpoint (DEPRECATED) - console.warn( - '⚠️ DEPRECATION WARNING: CSRF token meta tag not found. ' + - 'Falling back to /api/generate-csrf-token endpoint. ' + - 'This fallback is DEPRECATED and will be removed in a future version. ' + - 'Please ensure the backend includes in the page HTML.', - ); - - try { - const response = await fetch('/api/generate-csrf-token'); - if (!response.ok) { - throw new Error(`HTTP ${response.status}: ${response.statusText}`); - } - const data = await response.json(); - if (!data.csrf_token) { - throw new Error('API response missing csrf_token field'); - } - return data.csrf_token; - } catch (error) { - console.error('Failed to fetch CSRF token from legacy endpoint:', error); + if (!token) { throw new Error( - 'CSRF token not found. Neither meta tag nor /api/generate-csrf-token endpoint provided a valid token.', + 'CSRF token not found. Ensure the backend includes in the page HTML.', ); } -}; + return token; +}; diff --git a/goodmap/api_models.py b/goodmap/api_models.py index be21f852..ab923c23 100644 --- a/goodmap/api_models.py +++ b/goodmap/api_models.py @@ -51,12 +51,6 @@ class VersionResponse(BaseModel): backend: str = Field(..., description="Backend version") -class CSRFTokenResponse(BaseModel): - """Response model for CSRF token endpoint (deprecated).""" - - csrf_token: str = Field(..., description="CSRF token") - - class PaginationParams(BaseModel): """Common pagination and filtering parameters.""" diff --git a/goodmap/core_api.py b/goodmap/core_api.py index 09545400..53396428 100644 --- a/goodmap/core_api.py +++ b/goodmap/core_api.py @@ -22,7 +22,6 @@ CategoryOptionsResponse, ClusteringParams, ClusterList, - CSRFTokenResponse, ErrorResponse, LanguagesResponse, LocationDetail, @@ -124,7 +123,6 @@ def core_pages( database, languages: LanguagesMapping, notifier_function, - csrf_generator, location_model, photo_attachment_config: AttachmentConfig, feature_flags: FeatureFlagSet, @@ -389,22 +387,6 @@ def get_version(): version_info = {"backend": importlib.metadata.version("goodmap")} return jsonify(version_info) - @core_api_blueprint.route("/generate-csrf-token", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=CSRFTokenResponse)) - @deprecation.deprecated( - deprecated_in="1.1.8", - details="This endpoint for explicit CSRF token generation is deprecated. " - "CSRF protection remains active in the application.", - ) - def generate_csrf_token(): - """Generate CSRF token (DEPRECATED). - - This endpoint is deprecated and maintained only for backward compatibility. - CSRF protection remains active in the application. - """ - csrf_token = csrf_generator() - return {"csrf_token": csrf_token} - @core_api_blueprint.route("/location-schema", methods=["GET"]) @spec.validate(resp=Response(HTTP_200=LocationSchemaResponse)) def get_location_schema(): diff --git a/goodmap/goodmap.py b/goodmap/goodmap.py index e85bacb4..313a07fd 100644 --- a/goodmap/goodmap.py +++ b/goodmap/goodmap.py @@ -8,7 +8,7 @@ from flask import Blueprint, jsonify, redirect, render_template, session from flask_babel import gettext -from flask_wtf.csrf import CSRFError, generate_csrf +from flask_wtf.csrf import CSRFError from platzky import platzky from platzky.config import languages_dict from platzky.models import CmsModule @@ -269,7 +269,6 @@ def handle_csrf_error(error): app.db, languages_dict(config.languages), app.notify, - generate_csrf, location_model, photo_attachment_config=photo_attachment_config, feature_flags=config.feature_flags, diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index fa81438b..eb27bf01 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -51,12 +51,6 @@ def test_location_schema_endpoint_describes_this_instance(test_app): assert set(body["photo"]) == {"allowed_extensions", "allowed_mime_types", "max_size_bytes"} -def test_csrf_token_endpoint_returns_token(test_app): - response = test_app.get("/api/generate-csrf-token") - assert response.status_code == 200 - assert "csrf_token" in response.json - - def test_api_doc_index(test_app): response = test_app.get("/api/doc") assert response.status_code == 200 From 662459de86a65baf4845e3878860a1bcead28230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 18:33:18 +0200 Subject: [PATCH 03/19] docs: drop docs for the removed generate-csrf-token endpoint --- docs/http-api.rst | 8 -------- 1 file changed, 8 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index 7f37c2ae..1104291c 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -281,11 +281,3 @@ description that satisfies neither rule gives ``400``. The report is stored with ``"status": "pending"`` and ``"priority": "medium"`` in the data source, for triage. - -``GET /api/generate-csrf-token`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -.. deprecated:: 1.1.8 - - Deprecated since 1.1.8 and kept only for backward compatibility. Read the token from - the ``csrf-token`` meta tag instead. CSRF protection itself is unaffected. From f3e06c644f14baf9c938bbfef47075f5d5eea24b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 18:42:54 +0200 Subject: [PATCH 04/19] lint fix --- tests/unit_tests/test_goodmap.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/test_goodmap.py b/tests/unit_tests/test_goodmap.py index 60cc6213..3e68419a 100644 --- a/tests/unit_tests/test_goodmap.py +++ b/tests/unit_tests/test_goodmap.py @@ -579,7 +579,9 @@ def test_csrf_token_is_session_scoped(): victim = app.test_client() page = victim.get("/map") - token = re.search(r'name="csrf-token" content="([^"]+)"', page.data.decode("utf-8")).group(1) + token_match = re.search(r'name="csrf-token" content="([^"]+)"', page.data.decode("utf-8")) + assert token_match is not None + token = token_match.group(1) attacker = app.test_client() response = attacker.post( From 4fbbead79eee1cbac269f554fcfbe50b8ba6dad7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 19:32:23 +0200 Subject: [PATCH 05/19] refactor --- docs/api-reference.rst | 2 +- docs/http-api.rst | 4 +- .../components/MarkerPopup/MarkerPopup.jsx | 8 +-- .../MarkerClusterGroupIntegration.test.jsx | 6 +- .../tests/MarkerPopup/MarkerPopup.test.jsx | 6 +- goodmap/{ => api}/admin_api.py | 2 +- goodmap/{ => api}/api_models.py | 12 ++-- goodmap/{ => api}/core_api.py | 4 +- goodmap/data_models/location.py | 2 +- goodmap/goodmap.py | 4 +- tests/unit_tests/test_admin_api.py | 4 +- tests/unit_tests/test_core_api.py | 65 +++++++++++-------- 12 files changed, 66 insertions(+), 53 deletions(-) rename goodmap/{ => api}/admin_api.py (99%) rename goodmap/{ => api}/api_models.py (95%) rename goodmap/{ => api}/core_api.py (99%) diff --git a/docs/api-reference.rst b/docs/api-reference.rst index 85c8fd9c..76ae08ff 100644 --- a/docs/api-reference.rst +++ b/docs/api-reference.rst @@ -60,7 +60,7 @@ Request and response models Pydantic models for the HTTP layer. These are what generate the OpenAPI document served at ``/api/doc/openapi.json``, so they and the schema endpoint never disagree. -.. automodule:: goodmap.api_models +.. automodule:: goodmap.api.api_models :members: :show-inheritance: diff --git a/docs/http-api.rst b/docs/http-api.rst index 1104291c..aeaededb 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -87,8 +87,8 @@ Query parameters: curl 'http://localhost:5000/api/locations?accessible_by=bikes&lat=51.10&lon=17.05&limit=5' -Each point comes back as ``uuid``, ``position`` and ``remark`` — where ``remark`` is a -**boolean**, whether the point has one, not its text. +Each point comes back as ``uuid``, ``position`` and ``has_remark`` — a **boolean**, whether +the point has a remark, not its text. Invalid or unknown query parameters are ignored rather than rejected. diff --git a/frontend/src/components/MarkerPopup/MarkerPopup.jsx b/frontend/src/components/MarkerPopup/MarkerPopup.jsx index e0c5156f..86174c6c 100644 --- a/frontend/src/components/MarkerPopup/MarkerPopup.jsx +++ b/frontend/src/components/MarkerPopup/MarkerPopup.jsx @@ -81,7 +81,7 @@ const asteriskIcon = new Icon({ * @param {Object} props - Component props * @param {Object} props.place - Location data object * @param {number[]} props.place.position - Coordinates [latitude, longitude] - * @param {boolean} [props.place.remark] - Whether this location has a remark (uses asterisk icon if true) + * @param {boolean} [props.place.has_remark] - Whether this location has a remark (uses asterisk icon if true) * @returns {React.ReactElement} Leaflet Marker component with click-to-show-details functionality */ export const MarkerPopup = ({ place }) => { @@ -113,12 +113,12 @@ export const MarkerPopup = ({ place }) => { eventHandlers: { click: handleMarkerClick, }, - alt: place.remark ? 'Marker-Asterisk' : 'Marker', + alt: place.has_remark ? 'Marker-Asterisk' : 'Marker', }; // Only add icon prop if we have a custom icon (for remarks) // This prevents passing undefined which can cause issues with MarkerClusterGroup - if (place.remark) { + if (place.has_remark) { markerProps.icon = asteriskIcon; } @@ -132,7 +132,7 @@ export const MarkerPopup = ({ place }) => { MarkerPopup.propTypes = { place: PropTypes.shape({ position: PropTypes.arrayOf(PropTypes.number).isRequired, - remark: PropTypes.bool, + has_remark: PropTypes.bool, uuid: PropTypes.string.isRequired, }).isRequired, }; diff --git a/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx b/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx index b58d90f6..b07bfe39 100644 --- a/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx @@ -24,17 +24,17 @@ describe('MarkerPopup integration with MarkerClusterGroup', () => { { position: [51.1095, 17.0525], uuid: 'location-1', - remark: false, + has_remark: false, }, { position: [51.10655, 17.0555], uuid: 'location-2', - remark: true, + has_remark: true, }, { position: [51.1085, 17.0535], uuid: 'location-3', - remark: false, + has_remark: false, }, ]; diff --git a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx index b5b33d8a..46358626 100644 --- a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx @@ -10,7 +10,7 @@ jest.mock('../../src/services/http/httpService'); const location = { position: [51.1095, 17.0525], uuid: '21231', - remark: false, + has_remark: false, }; const locationData = { @@ -106,7 +106,7 @@ describe('MarkerPopup with remark', () => { }); it('should render marker popup with asterisks when remark is true', () => { - const locationWhenRemarkIsTrue = { ...location, remark: true }; + const locationWhenRemarkIsTrue = { ...location, has_remark: true }; act(() => { render( { }); it('should pass custom icon prop when remark is true', () => { - const locationWithRemark = { ...location, remark: true }; + const locationWithRemark = { ...location, has_remark: true }; act(() => { render( dict[str, Any]: def basic_info(self) -> dict[str, Any]: """Get basic location information summary.""" data = self.model_dump(include={"uuid", "position"}) - data["remark"] = bool(self.remark) + data["has_remark"] = bool(self.remark) return data diff --git a/goodmap/goodmap.py b/goodmap/goodmap.py index 313a07fd..0264f172 100644 --- a/goodmap/goodmap.py +++ b/goodmap/goodmap.py @@ -16,9 +16,9 @@ from platzky.shortcodes import Shortcode from pydantic import BaseModel -from goodmap.admin_api import admin_pages +from goodmap.api.admin_api import admin_pages +from goodmap.api.core_api import core_pages from goodmap.config import GoodmapConfig -from goodmap.core_api import core_pages from goodmap.data_models.location import create_location_model from goodmap.db import ( extend_db_with_goodmap_queries, diff --git a/tests/unit_tests/test_admin_api.py b/tests/unit_tests/test_admin_api.py index 680c5a9a..93151ba4 100644 --- a/tests/unit_tests/test_admin_api.py +++ b/tests/unit_tests/test_admin_api.py @@ -11,7 +11,7 @@ # --- Admin location tests --- -@mock.patch("goodmap.admin_api.uuid.uuid4") +@mock.patch("goodmap.api.admin_api.uuid.uuid4") def test_admin_post_location_success(mock_uuid4, test_app): from uuid import UUID @@ -34,7 +34,7 @@ def test_admin_post_location_success(mock_uuid4, test_app): assert isinstance(resp_json["uuid"], str) -@mock.patch("goodmap.admin_api.uuid.uuid4") +@mock.patch("goodmap.api.admin_api.uuid.uuid4") def test_admin_post_location_without_remark_excludes_remark_from_response(mock_uuid4, test_app): from uuid import UUID diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index eb27bf01..fe10fb02 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -3,8 +3,8 @@ import pytest +from goodmap.api.core_api import get_or_none, make_tuple_translation from goodmap.config import GoodmapConfig -from goodmap.core_api import get_or_none, make_tuple_translation from goodmap.feature_flags import CategoriesHelp from goodmap.goodmap import create_app_from_config from tests.unit_tests.conftest import ( @@ -63,7 +63,7 @@ def test_api_doc_index(test_app): # --- Categories endpoint tests --- -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_endpoints_return_expected_data(test_app): # Test /api/categories endpoint response = test_app.get("/api/categories") @@ -82,7 +82,7 @@ def test_categories_endpoints_return_expected_data(test_app): } -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_endpoints_old_format(test_app_without_helpers): # Test /api/categories endpoint (old format) response = test_app_without_helpers.get("/api/categories") @@ -95,7 +95,7 @@ def test_categories_endpoints_old_format(test_app_without_helpers): assert response.json == [["test", "test-translated"], ["test2", "test2-translated"]] -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_categories_endpoint_with_categories_help(): test_app = create_test_app( @@ -111,7 +111,7 @@ def test_categories_endpoint_with_categories_help(): assert data["categories_help"][0] == {"option1": "categories_help_option1-translated"} -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_category_data_endpoint_with_categories_options_help(): test_app = create_test_app( @@ -157,7 +157,7 @@ def test_category_data_endpoint_with_none_categories_options_help(): # --- Categories-full endpoint tests --- -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint(test_app): response = test_app.get("/api/categories-full") assert response.status_code == 200 @@ -189,7 +189,7 @@ def test_categories_full_endpoint(test_app): assert category["filter_mode"] == "or" -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_reports_configured_filter_mode(): test_app = create_test_app( db_overrides={ @@ -204,7 +204,7 @@ def test_categories_full_endpoint_reports_configured_filter_mode(): assert category["filter_mode"] == "exclusive" -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_with_default_checked(): test_app = create_test_app( db_overrides={ @@ -221,7 +221,7 @@ def test_categories_full_endpoint_with_default_checked(): assert category["default_checked"] == ["opt1"] -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_drops_default_checked_not_in_options(): test_app = create_test_app( db_overrides={ @@ -238,7 +238,7 @@ def test_categories_full_endpoint_drops_default_checked_not_in_options(): assert category["default_checked"] == ["opt1"] -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_without_default_checked(): test_app = create_test_app( db_overrides={ @@ -254,7 +254,7 @@ def test_categories_full_endpoint_without_default_checked(): assert category["default_checked"] == [] -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_with_multiple_categories(): test_app = create_test_app( db_overrides={ @@ -274,7 +274,7 @@ def test_categories_full_endpoint_with_multiple_categories(): assert "category2" in keys -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_with_categories_help(): test_app = create_test_app( feature_flags=make_flag_set(CategoriesHelp), @@ -303,7 +303,7 @@ def test_categories_full_endpoint_with_categories_help(): assert category["options_help"][0] == {"opt1": "categories_options_help_opt1-translated"} -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_categories_full_endpoint_without_categories_help(): test_app = create_test_app( feature_flags=make_flag_set(), @@ -329,8 +329,16 @@ def test_get_locations(test_app): response = test_app.get("/api/locations") assert response.status_code == 200 assert response.json == [ - {"uuid": "11111111-1111-1111-1111-111111111111", "position": [50, 50], "remark": True}, - {"uuid": "22222222-2222-2222-2222-222222222222", "position": [60, 60], "remark": False}, + { + "uuid": "11111111-1111-1111-1111-111111111111", + "position": [50, 50], + "has_remark": True, + }, + { + "uuid": "22222222-2222-2222-2222-222222222222", + "position": [60, 60], + "has_remark": False, + }, ] @@ -459,7 +467,7 @@ def test_get_locations_threshold_filter_mode(): } -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("goodmap.formatter.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_get_location(test_app): @@ -501,7 +509,7 @@ def test_reporting_location_success(test_app): assert response.status_code == 200 -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_reporting_returns_error_when_wrong_json(test_app): response = api_post(test_app, "/api/report-location", {"name": "location-id", "position": 50}) @@ -509,7 +517,7 @@ def test_reporting_returns_error_when_wrong_json(test_app): assert isinstance(response.json, list) -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_report_location_notification_success(test_app): response = api_post( @@ -555,7 +563,7 @@ def test_report_description_not_in_options_without_other(test_app): assert "Invalid report description" in response.json["message"] -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_report_description_free_text_with_other_option(): """When 'other' is in options, free text within limit should be accepted.""" @@ -566,7 +574,7 @@ def test_report_description_free_text_with_other_option(): assert response.status_code == 200 -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_report_description_exceeds_max_length(): """Description exceeding max length should be rejected even with 'other'.""" @@ -578,7 +586,7 @@ def test_report_description_exceeds_max_length(): assert response.status_code in (400, 422) -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_report_description_empty_options_uses_fallback(): """Empty issue options should fall back to defaults (which include 'other').""" @@ -588,7 +596,7 @@ def test_report_description_empty_options_uses_fallback(): assert response.status_code == 200 -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) @mock.patch("flask_babel.gettext", fake_translation) def test_report_description_empty_options_allows_free_text(): """Empty issue options fallback includes 'other', so free text is allowed.""" @@ -990,7 +998,8 @@ def test_location_clustering_empty_locations(): def test_location_clustering_exception_handling(test_app): with mock.patch( - "goodmap.core_api.pysupercluster.SuperCluster", side_effect=Exception("Clustering failed") + "goodmap.api.core_api.pysupercluster.SuperCluster", + side_effect=Exception("Clustering failed"), ): response = test_app.get("/api/locations-clustered?zoom=10") assert response.status_code == 500 @@ -998,7 +1007,7 @@ def test_location_clustering_exception_handling(test_app): def test_location_clustering_logs_on_invalid_parameter(test_app): - with mock.patch("goodmap.core_api.logger") as mock_logger: + with mock.patch("goodmap.api.core_api.logger") as mock_logger: test_app.get("/api/locations-clustered?zoom=invalid") mock_logger.warning.assert_called_once() assert "Invalid parameter" in mock_logger.warning.call_args[0][0] @@ -1007,10 +1016,10 @@ def test_location_clustering_logs_on_invalid_parameter(test_app): def test_location_clustering_logs_on_exception(test_app): with ( mock.patch( - "goodmap.core_api.pysupercluster.SuperCluster", + "goodmap.api.core_api.pysupercluster.SuperCluster", side_effect=Exception("Clustering failed"), ), - mock.patch("goodmap.core_api.logger") as mock_logger, + mock.patch("goodmap.api.core_api.logger") as mock_logger, ): test_app.get("/api/locations-clustered?zoom=10") mock_logger.exception.assert_called_once() @@ -1020,7 +1029,7 @@ def test_location_clustering_logs_on_exception(test_app): # --- Helper function tests --- -@mock.patch("goodmap.core_api.gettext", fake_translation) +@mock.patch("goodmap.api.core_api.gettext", fake_translation) def test_make_tuple_translation(): keys = ["alpha", "beta"] assert make_tuple_translation(keys) == [ @@ -1068,7 +1077,7 @@ def test_issue_options_defaults_to_empty_when_missing(): def test_get_locations_from_request_helper(test_app): - from goodmap.core_api import get_locations_from_request + from goodmap.api.core_api import get_locations_from_request class MockArgs: def to_dict(self, flat=False): From 1b6a4f4316c7fe8e18a33663289aa946aadde208 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 19:42:45 +0200 Subject: [PATCH 06/19] lint fix --- frontend/src/components/MarkerPopup/MarkerPopup.jsx | 2 +- .../MarkerPopup/MarkerClusterGroupIntegration.test.jsx | 6 +++--- frontend/tests/MarkerPopup/MarkerPopup.test.jsx | 4 +++- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/MarkerPopup/MarkerPopup.jsx b/frontend/src/components/MarkerPopup/MarkerPopup.jsx index 86174c6c..5c53e582 100644 --- a/frontend/src/components/MarkerPopup/MarkerPopup.jsx +++ b/frontend/src/components/MarkerPopup/MarkerPopup.jsx @@ -132,7 +132,7 @@ export const MarkerPopup = ({ place }) => { MarkerPopup.propTypes = { place: PropTypes.shape({ position: PropTypes.arrayOf(PropTypes.number).isRequired, - has_remark: PropTypes.bool, + has_remark: PropTypes.bool, // eslint-disable-line camelcase -- matches backend API schema property name uuid: PropTypes.string.isRequired, }).isRequired, }; diff --git a/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx b/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx index b07bfe39..eadee6d8 100644 --- a/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerClusterGroupIntegration.test.jsx @@ -24,17 +24,17 @@ describe('MarkerPopup integration with MarkerClusterGroup', () => { { position: [51.1095, 17.0525], uuid: 'location-1', - has_remark: false, + has_remark: false, // eslint-disable-line camelcase }, { position: [51.10655, 17.0555], uuid: 'location-2', - has_remark: true, + has_remark: true, // eslint-disable-line camelcase }, { position: [51.1085, 17.0535], uuid: 'location-3', - has_remark: false, + has_remark: false, // eslint-disable-line camelcase }, ]; diff --git a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx index 46358626..02f82fd6 100644 --- a/frontend/tests/MarkerPopup/MarkerPopup.test.jsx +++ b/frontend/tests/MarkerPopup/MarkerPopup.test.jsx @@ -10,7 +10,7 @@ jest.mock('../../src/services/http/httpService'); const location = { position: [51.1095, 17.0525], uuid: '21231', - has_remark: false, + has_remark: false, // eslint-disable-line camelcase -- matches backend API schema property name }; const locationData = { @@ -106,6 +106,7 @@ describe('MarkerPopup with remark', () => { }); it('should render marker popup with asterisks when remark is true', () => { + // eslint-disable-next-line camelcase -- matches backend API schema property name const locationWhenRemarkIsTrue = { ...location, has_remark: true }; act(() => { render( @@ -125,6 +126,7 @@ describe('MarkerPopup with remark', () => { }); it('should pass custom icon prop when remark is true', () => { + // eslint-disable-next-line camelcase -- matches backend API schema property name const locationWithRemark = { ...location, has_remark: true }; act(() => { render( From abae3cd72cc2c099f39a322f4f661cffff642513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 19:52:09 +0200 Subject: [PATCH 07/19] fix: returned csrf locks --- docs/http-api.rst | 9 ++++++- goodmap/goodmap.py | 4 ---- tests/unit_tests/test_goodmap.py | 41 +++++++++++++++++++++++++------- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index aeaededb..44e21e5a 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -33,7 +33,8 @@ on the ``categories`` in your data source (:doc:`data-source`). **Writes need a CSRF token.** CSRF protection is on for the whole app, so ``POST``, ``PUT`` and ``DELETE`` without a token get ``400 {"message": "The CSRF token is missing."}``. Send it as an ``X-CSRFToken`` header, from the same session the token was -minted in — the token is bound to the session cookie, so the pair travels together. Server-rendered pages expose one in a meta tag: +minted in — the token is bound to the session cookie, so the pair travels together. +Server-rendered pages expose one in a meta tag: .. code-block:: html @@ -48,6 +49,12 @@ minted in — the token is bound to the session cookie, so the pair travels toge body: JSON.stringify({ id: locationUuid, description: 'has a hole' }), }); +Over https, a matching ``Referer`` header is required too — same-origin defense in +depth, on top of the token. Browsers send this automatically for a same-origin request, +so it is invisible in normal use; a scripted client (``curl``, a backend job) must set it +explicitly, e.g. ``-H "Referer: https://your-host/"``, or the request gets +``400 {"message": "The referrer header is missing."}``. + **Errors are ``{"message": "..."}``**, occasionally with an extra ``error`` field. Messages are deliberately generic — the details go to the server log, not the response. diff --git a/goodmap/goodmap.py b/goodmap/goodmap.py index 0264f172..113038e4 100644 --- a/goodmap/goodmap.py +++ b/goodmap/goodmap.py @@ -235,10 +235,6 @@ def create_app_from_config(config: GoodmapConfig) -> platzky.Engine: # CSRFProtect on the engine); initializing it here again would register a second # before_request hook and break any future exempt() registered on one instance only. # - # The token is bound to the session, which is the actual protection. The extra - # referrer check flask-wtf adds on https would reject scripted API callers that - # send a valid token but no Referer header, so it is off. - app.config["WTF_CSRF_SSL_STRICT"] = False # The map page is typically left open well past the default 3600s, and the frontend # never refreshes the meta-tag token - so scope the token to the session instead of # rejecting submissions from any tab older than an hour. diff --git a/tests/unit_tests/test_goodmap.py b/tests/unit_tests/test_goodmap.py index 3e68419a..488fc01a 100644 --- a/tests/unit_tests/test_goodmap.py +++ b/tests/unit_tests/test_goodmap.py @@ -544,8 +544,12 @@ def test_csrf_failure_returns_json_error(): assert response.json == {"message": "The CSRF token is missing."} -def test_csrf_accepts_scripted_https_caller_without_referer(): - """A valid token from the caller's own session works over https with no Referer.""" +def test_csrf_enforces_referer_on_https(): + """WTF_CSRF_SSL_STRICT is left on: https requires a same-origin Referer, on top + of the token/session check, as defense-in-depth against a forged cross-origin + request. A real browser sends a matching Referer automatically for a same-origin + request, so this only rejects scripted callers that omit it or spoof it. + """ import re config = _make_test_app_config(extra_data={"categories": {}, "data": []}) @@ -555,19 +559,40 @@ def test_csrf_accepts_scripted_https_caller_without_referer(): page = client.get("/map", base_url="https://localhost") token_match = re.search(r'name="csrf-token" content="([^"]+)"', page.data.decode("utf-8")) assert token_match is not None + token = token_match.group(1) + payload = {"id": "x", "description": "y"} + # Same-origin Referer, as a real browser sends by default: past the CSRF layer. + # The handler may still reject the payload, but not with a CSRF message. response = client.post( "/api/report-location", - json={"id": "x", "description": "y"}, - headers={"X-CSRFToken": token_match.group(1)}, + json=payload, + headers={"X-CSRFToken": token, "Referer": "https://localhost/map"}, base_url="https://localhost", ) - - # Past the CSRF layer: the handler itself may reject the payload, but not with - # a CSRF message and never as HTML. - assert response.content_type.startswith("application/json") assert "CSRF" not in response.get_data(as_text=True) + # No Referer at all: rejected. + response = client.post( + "/api/report-location", + json=payload, + headers={"X-CSRFToken": token}, + base_url="https://localhost", + ) + assert response.status_code == 400 + assert response.json == {"message": "The referrer header is missing."} + + # Cross-origin Referer: rejected. This is the actual attack the check defends + # against - a request that carries a valid token but did not originate here. + response = client.post( + "/api/report-location", + json=payload, + headers={"X-CSRFToken": token, "Referer": "https://evil.example/"}, + base_url="https://localhost", + ) + assert response.status_code == 400 + assert response.json == {"message": "The referrer does not match the host."} + def test_csrf_token_is_session_scoped(): """The token alone is not enough - it must be paired with the session it was minted in.""" From e17bb3cf40925f907d1097ab4fa7869e5581214d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 20:04:53 +0200 Subject: [PATCH 08/19] fixes after review --- docs/http-api.rst | 13 +-- examples/e2e_test_data.json | 155 +----------------------------- goodmap/api/core_api.py | 21 ++-- tests/unit_tests/test_core_api.py | 20 +++- 4 files changed, 35 insertions(+), 174 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index 44e21e5a..0d207690 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -31,8 +31,8 @@ Conventions on the ``categories`` in your data source (:doc:`data-source`). **Writes need a CSRF token.** CSRF protection is on for the whole app, so ``POST``, -``PUT`` and ``DELETE`` without a token get ``400 {"message": "The CSRF token is -missing."}``. Send it as an ``X-CSRFToken`` header, from the same session the token was +``PUT``, ``PATCH`` and ``DELETE`` without a token get ``400 {"message": "The CSRF token +is missing."}``. Send it as an ``X-CSRFToken`` header, from the same session the token was minted in — the token is bound to the session cookie, so the pair travels together. Server-rendered pages expose one in a meta tag: @@ -172,10 +172,11 @@ its contents. The schema documents both shapes. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ What *this* instance accepts for a new point: the fields of its location model (minus the -server-managed ``uuid`` and ``position``), the allowed values per category, the reportable -issue types, and the photo limits. Since the accepted fields are configured per deployment, -this is how a client discovers them rather than assuming — it is the same schema the -built-in suggest form is generated from. +server-assigned ``uuid`` — ``position`` is required and client-supplied, same as +``/api/suggest-new-point``), the allowed values per category, the reportable issue types, +and the photo limits. Since the accepted fields are configured per deployment, this is how +a client discovers them rather than assuming — it is the same schema the built-in suggest +form is generated from. Other read endpoints ~~~~~~~~~~~~~~~~~~~~ diff --git a/examples/e2e_test_data.json b/examples/e2e_test_data.json index 60fc6126..329589b7 100644 --- a/examples/e2e_test_data.json +++ b/examples/e2e_test_data.json @@ -1,154 +1 @@ -{ - "map": { - "data": [ - { - "name": "Grunwaldzki", - "position": [ - 51.1095, - 17.0525 - ], - "accessible_by": [ - "pedestrians", - "cars" - ], - "type_of_place": "big bridge", - "uuid": "9264286a-5d33-4e38-ab11-c8e179a7754a", - "CTA": { - "type": "CTA", - "value": "https://www.example.com", - "displayValue": "Visit example.org!" - } - }, - { - "name": "Zwierzyniecka", - "position": [ - 51.10655, - 17.0555 - ], - "accessible_by": [ - "bikes", - "pedestrians" - ], - "type_of_place": "small bridge", - "uuid": "c8ecf476-5968-40da-ba5c-e810ad9ff203", - "remark": "very old bridge" - } - ], - "location_obligatory_fields": [ - [ - "name", - "str" - ], - [ - "accessible_by", - "list" - ], - [ - "type_of_place", - "str" - ] - ], - "reported_issue_types": ["under construction", "has a hole"], - "categories": { - "accessible_by": [ - "bikes", - "cars", - "pedestrians" - ], - "type_of_place": [ - "big bridge", - "small bridge" - ] - }, - "categories_help": [ - "accessible_by" - ], - "categories_options_help": { - "type_of_place": [ - "small bridge" - ], - "accessible_by": [ - "cars", - "pedestrians" - ] - }, - "categories_default_checked": { - "accessible_by": [ - "cars" - ] - }, - "visible_data": [ - "remark", - "accessible_by", - "type_of_place", - "CTA" - ], - "meta_data": [ - "uuid" - ] - }, - "site_content": { - "home_page_path": "/map", - "pages": [ - { - "title": "O nas", - "slug": "o-nas", - "coverImage": { - "url": "", - "alternateText": "" - }, - "date": "01-01-2024", - "author": "", - "comments": [], - "excerpt": "", - "tags": [], - "language": "pl", - "contentInMarkdown": "o nas" - }, - { - "title": "About", - "slug": "about", - "coverImage": { - "url": "", - "alternateText": "" - }, - "date": "01-01-2024", - "author": "", - "comments": [], - "excerpt": "", - "tags": [], - "language": "en", - "contentInMarkdown": "about" - } - ], - "menu_items": { - "pl": [ - { - "name": "Mapa", - "url": "/" - }, - { - "name": "O nas", - "url": "/blog/page/o-nas" - } - ], - "en": [ - { - "name": "Map", - "url": "/" - }, - { - "name": "About", - "url": "/blog/page/about" - } - ] - }, - "logo_url": "", - "font": { - "name": "Poppins", - "url": "https://fonts.googleapis.com/css2?family=Poppins" - }, - "primary_color": "#FFFFFF", - "secondary_color": "#245466" - } -} +{"map": {"data": [{"name": "Grunwaldzki", "position": [51.1095, 17.0525], "accessible_by": ["pedestrians", "cars"], "type_of_place": "big bridge", "uuid": "9264286a-5d33-4e38-ab11-c8e179a7754a", "CTA": {"type": "CTA", "value": "https://www.example.com", "displayValue": "Visit example.org!"}}, {"name": "Zwierzyniecka", "position": [51.10655, 17.0555], "accessible_by": ["bikes", "pedestrians"], "type_of_place": "small bridge", "uuid": "c8ecf476-5968-40da-ba5c-e810ad9ff203", "remark": "very old bridge"}], "location_obligatory_fields": [["name", "str"], ["accessible_by", "list"], ["type_of_place", "str"]], "reported_issue_types": ["under construction", "has a hole"], "categories": {"accessible_by": ["bikes", "cars", "pedestrians"], "type_of_place": ["big bridge", "small bridge"]}, "categories_help": ["accessible_by"], "categories_options_help": {"type_of_place": ["small bridge"], "accessible_by": ["cars", "pedestrians"]}, "categories_default_checked": {"accessible_by": ["cars"]}, "visible_data": ["remark", "accessible_by", "type_of_place", "CTA"], "meta_data": ["uuid"], "suggestions": [{"position": [51.11, 17.03], "uuid": "033dd579-91e5-4279-9aa0-b986ccfbf513", "name": "Nowy", "accessible_by": ["bikes"], "type_of_place": "small bridge", "is_free": "true", "status": "pending"}]}, "site_content": {"home_page_path": "/map", "pages": [{"title": "O nas", "slug": "o-nas", "coverImage": {"url": "", "alternateText": ""}, "date": "01-01-2024", "author": "", "comments": [], "excerpt": "", "tags": [], "language": "pl", "contentInMarkdown": "o nas"}, {"title": "About", "slug": "about", "coverImage": {"url": "", "alternateText": ""}, "date": "01-01-2024", "author": "", "comments": [], "excerpt": "", "tags": [], "language": "en", "contentInMarkdown": "about"}], "menu_items": {"pl": [{"name": "Mapa", "url": "/"}, {"name": "O nas", "url": "/blog/page/o-nas"}], "en": [{"name": "Map", "url": "/"}, {"name": "About", "url": "/blog/page/about"}]}, "logo_url": "", "font": {"name": "Poppins", "url": "https://fonts.googleapis.com/css2?family=Poppins"}, "primary_color": "#FFFFFF", "secondary_color": "#245466"}} \ No newline at end of file diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index 0926b685..c5e147b6 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -393,26 +393,25 @@ def get_location_schema(): """Get the schema this instance accepts for a new point. The fields a point may carry are configured per deployment, so there is no - fixed payload for /api/suggest-new-point. This returns the accepted fields, - the allowed values for each category, the reportable issue types and the - photo limits, as the built-in suggest form uses them. + fixed payload for /api/suggest-new-point. This returns the accepted fields + (excluding only the server-assigned uuid - position is required and must be + supplied by the client), the allowed values for each category, the reportable + issue types and the photo limits, as the built-in suggest form uses them. """ category_data = database.get_category_data() properties = location_model.model_json_schema().get("properties", {}) + # Matches the fallback /api/report-location applies: an unconfigured + # reported_issue_types must not make this endpoint advertise fewer accepted + # values than the report endpoint actually accepts. + issue_options = database.get_issue_options() or get_default_issue_options() return jsonify( { - "fields": { - name: spec_ - for name, spec_ in properties.items() - if name not in ("uuid", "position") - }, + "fields": {name: spec_ for name, spec_ in properties.items() if name != "uuid"}, "obligatory_fields": current_app.extensions.get("goodmap", {}).get( "location_obligatory_fields", [] ), "categories": category_data.get("categories", {}), - "reported_issue_types": [ - {"value": t, "label": gettext(t)} for t in database.get_issue_options() - ], + "reported_issue_types": [{"value": t, "label": gettext(t)} for t in issue_options], "photo": { "allowed_extensions": sorted(photo_attachment_config.allowed_extensions or []), "allowed_mime_types": sorted(photo_attachment_config.allowed_mime_types or []), diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index fe10fb02..4787a318 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -3,7 +3,7 @@ import pytest -from goodmap.api.core_api import get_or_none, make_tuple_translation +from goodmap.api.core_api import get_default_issue_options, get_or_none, make_tuple_translation from goodmap.config import GoodmapConfig from goodmap.feature_flags import CategoriesHelp from goodmap.goodmap import create_app_from_config @@ -44,13 +44,27 @@ def test_location_schema_endpoint_describes_this_instance(test_app): "reported_issue_types", "photo", } - # uuid and position are server-managed and must not be offered as form fields + # uuid is server-assigned and must not be offered as a form field; position is + # required and client-supplied, same as /api/suggest-new-point, so it must be. assert "uuid" not in body["fields"] - assert "position" not in body["fields"] + assert "position" in body["fields"] assert all(set(t) == {"value", "label"} for t in body["reported_issue_types"]) assert set(body["photo"]) == {"allowed_extensions", "allowed_mime_types", "max_size_bytes"} +def test_location_schema_endpoint_falls_back_to_default_issue_options(): + """An unconfigured reported_issue_types must not undersell what + /api/report-location actually accepts (it falls back to the same defaults). + """ + test_app = create_test_app(db_overrides={"reported_issue_types": []}) + response = test_app.get("/api/location-schema") + assert response.status_code == 200 + data = response.json + assert data is not None + values = {t["value"] for t in data["reported_issue_types"]} + assert values == set(get_default_issue_options()) + + def test_api_doc_index(test_app): response = test_app.get("/api/doc") assert response.status_code == 200 From dd7e641da11a5ca29aa948d1752f7b55e19121ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Tue, 18 Aug 2026 21:01:14 +0200 Subject: [PATCH 09/19] fixes for sonarqube --- examples/e2e_test_data.json | 155 +++++++++++++++++++++++++++++++++++- goodmap/api/api_models.py | 8 +- 2 files changed, 159 insertions(+), 4 deletions(-) diff --git a/examples/e2e_test_data.json b/examples/e2e_test_data.json index 329589b7..60fc6126 100644 --- a/examples/e2e_test_data.json +++ b/examples/e2e_test_data.json @@ -1 +1,154 @@ -{"map": {"data": [{"name": "Grunwaldzki", "position": [51.1095, 17.0525], "accessible_by": ["pedestrians", "cars"], "type_of_place": "big bridge", "uuid": "9264286a-5d33-4e38-ab11-c8e179a7754a", "CTA": {"type": "CTA", "value": "https://www.example.com", "displayValue": "Visit example.org!"}}, {"name": "Zwierzyniecka", "position": [51.10655, 17.0555], "accessible_by": ["bikes", "pedestrians"], "type_of_place": "small bridge", "uuid": "c8ecf476-5968-40da-ba5c-e810ad9ff203", "remark": "very old bridge"}], "location_obligatory_fields": [["name", "str"], ["accessible_by", "list"], ["type_of_place", "str"]], "reported_issue_types": ["under construction", "has a hole"], "categories": {"accessible_by": ["bikes", "cars", "pedestrians"], "type_of_place": ["big bridge", "small bridge"]}, "categories_help": ["accessible_by"], "categories_options_help": {"type_of_place": ["small bridge"], "accessible_by": ["cars", "pedestrians"]}, "categories_default_checked": {"accessible_by": ["cars"]}, "visible_data": ["remark", "accessible_by", "type_of_place", "CTA"], "meta_data": ["uuid"], "suggestions": [{"position": [51.11, 17.03], "uuid": "033dd579-91e5-4279-9aa0-b986ccfbf513", "name": "Nowy", "accessible_by": ["bikes"], "type_of_place": "small bridge", "is_free": "true", "status": "pending"}]}, "site_content": {"home_page_path": "/map", "pages": [{"title": "O nas", "slug": "o-nas", "coverImage": {"url": "", "alternateText": ""}, "date": "01-01-2024", "author": "", "comments": [], "excerpt": "", "tags": [], "language": "pl", "contentInMarkdown": "o nas"}, {"title": "About", "slug": "about", "coverImage": {"url": "", "alternateText": ""}, "date": "01-01-2024", "author": "", "comments": [], "excerpt": "", "tags": [], "language": "en", "contentInMarkdown": "about"}], "menu_items": {"pl": [{"name": "Mapa", "url": "/"}, {"name": "O nas", "url": "/blog/page/o-nas"}], "en": [{"name": "Map", "url": "/"}, {"name": "About", "url": "/blog/page/about"}]}, "logo_url": "", "font": {"name": "Poppins", "url": "https://fonts.googleapis.com/css2?family=Poppins"}, "primary_color": "#FFFFFF", "secondary_color": "#245466"}} \ No newline at end of file +{ + "map": { + "data": [ + { + "name": "Grunwaldzki", + "position": [ + 51.1095, + 17.0525 + ], + "accessible_by": [ + "pedestrians", + "cars" + ], + "type_of_place": "big bridge", + "uuid": "9264286a-5d33-4e38-ab11-c8e179a7754a", + "CTA": { + "type": "CTA", + "value": "https://www.example.com", + "displayValue": "Visit example.org!" + } + }, + { + "name": "Zwierzyniecka", + "position": [ + 51.10655, + 17.0555 + ], + "accessible_by": [ + "bikes", + "pedestrians" + ], + "type_of_place": "small bridge", + "uuid": "c8ecf476-5968-40da-ba5c-e810ad9ff203", + "remark": "very old bridge" + } + ], + "location_obligatory_fields": [ + [ + "name", + "str" + ], + [ + "accessible_by", + "list" + ], + [ + "type_of_place", + "str" + ] + ], + "reported_issue_types": ["under construction", "has a hole"], + "categories": { + "accessible_by": [ + "bikes", + "cars", + "pedestrians" + ], + "type_of_place": [ + "big bridge", + "small bridge" + ] + }, + "categories_help": [ + "accessible_by" + ], + "categories_options_help": { + "type_of_place": [ + "small bridge" + ], + "accessible_by": [ + "cars", + "pedestrians" + ] + }, + "categories_default_checked": { + "accessible_by": [ + "cars" + ] + }, + "visible_data": [ + "remark", + "accessible_by", + "type_of_place", + "CTA" + ], + "meta_data": [ + "uuid" + ] + }, + "site_content": { + "home_page_path": "/map", + "pages": [ + { + "title": "O nas", + "slug": "o-nas", + "coverImage": { + "url": "", + "alternateText": "" + }, + "date": "01-01-2024", + "author": "", + "comments": [], + "excerpt": "", + "tags": [], + "language": "pl", + "contentInMarkdown": "o nas" + }, + { + "title": "About", + "slug": "about", + "coverImage": { + "url": "", + "alternateText": "" + }, + "date": "01-01-2024", + "author": "", + "comments": [], + "excerpt": "", + "tags": [], + "language": "en", + "contentInMarkdown": "about" + } + ], + "menu_items": { + "pl": [ + { + "name": "Mapa", + "url": "/" + }, + { + "name": "O nas", + "url": "/blog/page/o-nas" + } + ], + "en": [ + { + "name": "Map", + "url": "/" + }, + { + "name": "About", + "url": "/blog/page/about" + } + ] + }, + "logo_url": "", + "font": { + "name": "Poppins", + "url": "https://fonts.googleapis.com/css2?family=Poppins" + }, + "primary_color": "#FFFFFF", + "secondary_color": "#245466" + } +} diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index 8a92448f..288245e5 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -12,6 +12,8 @@ from goodmap.data_models.location import Latitude, Longitude +_POSITION_DESCRIPTION = "[latitude, longitude]" + class LocationReportRequest(BaseModel): """Request model for reporting a location issue.""" @@ -85,7 +87,7 @@ class LocationBasicInfo(BaseModel): """One point as returned by the list endpoint: identity and position only.""" uuid: str = Field(..., description="Location UUID") - position: tuple[Latitude, Longitude] = Field(..., description="[latitude, longitude]") + position: tuple[Latitude, Longitude] = Field(..., description=_POSITION_DESCRIPTION) has_remark: bool = Field( ..., description="Whether the point has a remark, not the remark itself" ) @@ -99,7 +101,7 @@ class ClusterInfo(BaseModel): """One entry of the clustered list: either a single point or a cluster of them.""" type: Literal["cluster", "point"] = Field(..., description="Which of the two this entry is") - position: tuple[Latitude, Longitude] = Field(..., description="[latitude, longitude]") + position: tuple[Latitude, Longitude] = Field(..., description=_POSITION_DESCRIPTION) uuid: str | None = Field(None, description="Location UUID; null for a cluster") cluster_uuid: str | None = Field( None, @@ -119,7 +121,7 @@ class LocationDetail(BaseModel): title: str = Field(..., description="The point's name") subtitle: str = Field(..., description="The point's type_of_place") - position: tuple[Latitude, Longitude] = Field(..., description="[latitude, longitude]") + position: tuple[Latitude, Longitude] = Field(..., description=_POSITION_DESCRIPTION) data: list[tuple[str, Any]] = Field( ..., description="[label, value] pairs for the visible_data fields, translated" ) From b0a330a5cce9f5f8ece6f5377e90813492747b0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 00:27:11 +0200 Subject: [PATCH 10/19] removed another api method --- docs/configuration.rst | 8 +- docs/http-api.rst | 25 ++++-- goodmap/api/api_models.py | 24 ------ goodmap/api/core_api.py | 64 ---------------- goodmap/templates/goodmap-admin.html | 10 +-- tests/unit_tests/conftest.py | 11 --- tests/unit_tests/test_core_api.py | 109 +-------------------------- 7 files changed, 28 insertions(+), 223 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 27d8c986..4562b0d2 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -187,10 +187,10 @@ frontend to decide what to render. Both are set the same way. suggest form has no fields — see the note below. * - ``CATEGORIES_HELP`` - both - - Enables the help-tooltip data in ``/api/categories``, ``/api/categories-full`` and - ``/api/category/``, and makes the frontend render the tooltips. Without it - the ``categories_help`` and ``categories_options_help`` keys in your data are - ignored. See :ref:`data-source-help`. + - Enables the help-tooltip data in ``/api/categories-full``, and makes the frontend + render the tooltips. Without it the ``categories_help`` and + ``categories_options_help`` keys in your data are ignored. See + :ref:`data-source-help`. * - ``USE_SERVER_SIDE_CLUSTERING`` - both - The frontend fetches ``/api/locations-clustered`` instead of ``/api/locations``, diff --git a/docs/http-api.rst b/docs/http-api.rst index 0d207690..3569b7bf 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -49,6 +49,22 @@ Server-rendered pages expose one in a meta tag: body: JSON.stringify({ id: locationUuid, description: 'has a hole' }), }); +A scripted client has no meta tag to read, so it needs both pieces the browser gets for +free: the token, and the session cookie it is bound to. **A bare token is not enough** — +without the matching cookie the request fails with a different error, +``400 {"message": "The CSRF session token is missing."}``. Fetch a page first to get +both, keeping cookies in a jar to reuse on the write: + +.. code-block:: bash + + JAR=$(mktemp) + TOKEN=$(curl -s -c "$JAR" http://localhost:5000/ | grep -oP 'name="csrf-token" content="\K[^"]+') + curl -X POST http://localhost:5000/api/report-location \ + -b "$JAR" \ + -H "Content-Type: application/json" \ + -H "X-CSRFToken: $TOKEN" \ + -d '{"id": "9264286a-5d33-4e38-ab11-c8e179a7754a", "description": "has a hole"}' + Over https, a matching ``Referer`` header is required too — same-origin defense in depth, on top of the token. Browsers send this automatically for a same-origin request, so it is invisible in normal use; a scripted client (``curl``, a backend job) must set it @@ -160,12 +176,6 @@ buttons for ``exclusive``/``threshold``, a single checkbox for ``boolean`` With ``CATEGORIES_HELP`` on, each category also carries ``options_help``, and the response gains a top-level ``categories_help`` — both lists of ``{option: help text}`` objects. -Prefer this endpoint over ``GET /api/categories`` and ``GET /api/category/``, which -exist for older clients and cost one request per category. Both share one trap worth -knowing: with ``CATEGORIES_HELP`` **off** they return a bare list of pairs, and with it -**on** they return an object instead — the response *type* changes with the flag, not just -its contents. The schema documents both shapes. - .. _api-location-schema: ``GET /api/location-schema`` @@ -215,10 +225,13 @@ own: .. code-block:: bash curl -X POST http://localhost:5000/api/suggest-new-point \ + -b "$JAR" \ -H "X-CSRFToken: $TOKEN" \ -F 'location={"name": "Nowy", "position": [51.11, 17.03], "type_of_place": "small bridge", "accessible_by": ["bikes"], "is_free": "true"}' \ -F 'photo=@bridge.jpg' +(``$JAR`` and ``$TOKEN`` as obtained above.) + To find the fields a given instance wants, call :ref:`api-location-schema` — the same schema the built-in suggest form is generated from. diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index 288245e5..f66bbf23 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -128,17 +128,6 @@ class LocationDetail(BaseModel): metadata: dict[str, Any] = Field(..., description="The meta_data fields") -class CategoriesWithHelp(BaseModel): - """Category names plus help text, returned when CATEGORIES_HELP is on.""" - - categories: list[tuple[str, str]] = Field(..., description="[key, translated label] pairs") - categories_help: list[dict[str, str]] = Field(..., description="Help text per category") - - -class CategoriesResponse(RootModel[CategoriesWithHelp | list[tuple[str, str]]]): - """Bare [key, label] pairs, or an object with help text when CATEGORIES_HELP is on.""" - - class CategoryFull(BaseModel): """One category with everything needed to render its filter control.""" @@ -163,19 +152,6 @@ class CategoriesFullResponse(BaseModel): ) -class CategoryOptionsWithHelp(BaseModel): - """A category's options plus help text, returned when CATEGORIES_HELP is on.""" - - categories_options: list[tuple[str, str]] = Field( - ..., description="[value, translated label] pairs" - ) - categories_options_help: list[dict[str, str]] = Field(..., description="Help text per option") - - -class CategoryOptionsResponse(RootModel[CategoryOptionsWithHelp | list[tuple[str, str]]]): - """Bare [value, label] pairs, or an object with help text when CATEGORIES_HELP is on.""" - - class LocationQueryParams(BaseModel): """Non-filter query parameters of the location list endpoints. diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index c5e147b6..4f2273d7 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -18,8 +18,6 @@ from goodmap.api.api_models import ( CategoriesFullResponse, - CategoriesResponse, - CategoryOptionsResponse, ClusteringParams, ClusterList, ErrorResponse, @@ -81,15 +79,6 @@ def make_tuple_translation(keys_to_translate): return [(x, gettext(x)) for x in keys_to_translate] -def get_or_none(data, *keys): - for key in keys: - if isinstance(data, dict): - data = data.get(key) - else: - return None - return data - - def get_locations_from_request(database, request_args): """ Shared helper to fetch locations from database based on request arguments. @@ -420,29 +409,6 @@ def get_location_schema(): } ) - @core_api_blueprint.route("/categories", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=CategoriesResponse)) - def get_categories(): - """Get all available location categories. - - Returns list of categories with optional help text - if CATEGORIES_HELP feature flag is enabled. - """ - raw_categories = database.get_categories() - categories = make_tuple_translation(raw_categories) - - if CategoriesHelp not in feature_flags: - return jsonify(categories) - - category_data = database.get_category_data() - categories_help = category_data.get("categories_help") - proper_categories_help = [] - if categories_help is not None: - for option in categories_help: - proper_categories_help.append({option: gettext(f"categories_help_{option}")}) - - return jsonify({"categories": categories, "categories_help": proper_categories_help}) - @core_api_blueprint.route("/categories-full", methods=["GET"]) @spec.validate(resp=Response(HTTP_200=CategoriesFullResponse)) def get_categories_full(): @@ -502,36 +468,6 @@ def get_languages(): """ return jsonify(languages) - @core_api_blueprint.route("/category/", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=CategoryOptionsResponse)) - def get_category_types(category_type): - """Get all available options for a specific category. - - Returns list of category options with optional help text - if CATEGORIES_HELP feature flag is enabled. - """ - category_data = database.get_category_data(category_type) - local_data = make_tuple_translation(category_data["categories"][category_type]) - - categories_options_help = get_or_none( - category_data, "categories_options_help", category_type - ) - proper_categories_options_help = [] - if categories_options_help is not None: - for option in categories_options_help: - proper_categories_options_help.append( - {option: gettext(f"categories_options_help_{option}")} - ) - if CategoriesHelp not in feature_flags: - return jsonify(local_data) - - return jsonify( - { - "categories_options": local_data, - "categories_options_help": proper_categories_options_help, - } - ) - # Register Spectree with blueprint after all routes are defined spec.register(core_api_blueprint) diff --git a/goodmap/templates/goodmap-admin.html b/goodmap/templates/goodmap-admin.html index 0efba827..b2b2e0f1 100644 --- a/goodmap/templates/goodmap-admin.html +++ b/goodmap/templates/goodmap-admin.html @@ -201,12 +201,10 @@

{{ gettext("Reports") }}

// Category fetch function initCategories() { - return Promise.all([ - fetch('/api/category/accessible_by').then(r => r.json()), - fetch('/api/category/type_of_place').then(r => r.json()) - ]).then(([accessOpts, typeOpts]) => { - categories.accessible_by = accessOpts.map(o => o[0]); - categories.type_of_place = typeOpts.map(o => o[0]); + return fetch('/api/categories-full').then(r => r.json()).then(data => { + const byKey = Object.fromEntries(data.categories.map(c => [c.key, c.options])); + categories.accessible_by = (byKey.accessible_by || []).map(o => o[0]); + categories.type_of_place = (byKey.type_of_place || []).map(o => o[0]); }); } diff --git a/tests/unit_tests/conftest.py b/tests/unit_tests/conftest.py index 2ab2b7c6..55155568 100644 --- a/tests/unit_tests/conftest.py +++ b/tests/unit_tests/conftest.py @@ -1,7 +1,6 @@ import json from typing import Any -import deprecation import pytest from platzky import FeatureFlag, FeatureFlagSet @@ -122,13 +121,3 @@ def create_test_app( @pytest.fixture def test_app(): return create_test_app() - - -@pytest.fixture -@deprecation.deprecated( - deprecated_in="0.5.3", - removed_in="0.6.0", - details="actually categories_help should be integrated as true in future major release", -) -def test_app_without_helpers(): - return create_test_app(feature_flags=make_flag_set(UseLazyLoading, EnableAdminPanel)) diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index 4787a318..0541894e 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -3,7 +3,7 @@ import pytest -from goodmap.api.core_api import get_default_issue_options, get_or_none, make_tuple_translation +from goodmap.api.core_api import get_default_issue_options, make_tuple_translation from goodmap.config import GoodmapConfig from goodmap.feature_flags import CategoriesHelp from goodmap.goodmap import create_app_from_config @@ -74,100 +74,6 @@ def test_api_doc_index(test_app): assert b"/api/doc/openapi.json" in response.data -# --- Categories endpoint tests --- - - -@mock.patch("goodmap.api.core_api.gettext", fake_translation) -def test_categories_endpoints_return_expected_data(test_app): - # Test /api/categories endpoint - response = test_app.get("/api/categories") - assert response.status_code == 200 - assert response.json == { - "categories": [["test-category", "test-category-translated"]], - "categories_help": [{"test-category": "categories_help_test-category-translated"}], - } - - # Test /api/category/ endpoint - response = test_app.get("/api/category/test-category") - assert response.status_code == 200 - assert response.json == { - "categories_options": [["test", "test-translated"], ["test2", "test2-translated"]], - "categories_options_help": [{"test": "categories_options_help_test-translated"}], - } - - -@mock.patch("goodmap.api.core_api.gettext", fake_translation) -def test_categories_endpoints_old_format(test_app_without_helpers): - # Test /api/categories endpoint (old format) - response = test_app_without_helpers.get("/api/categories") - assert response.status_code == 200 - assert response.json == [["test-category", "test-category-translated"]] - - # Test /api/category/ endpoint (old format) - response = test_app_without_helpers.get("/api/category/test-category") - assert response.status_code == 200 - assert response.json == [["test", "test-translated"], ["test2", "test2-translated"]] - - -@mock.patch("goodmap.api.core_api.gettext", fake_translation) -@mock.patch("flask_babel.gettext", fake_translation) -def test_categories_endpoint_with_categories_help(): - test_app = create_test_app( - feature_flags=make_flag_set(CategoriesHelp), - db_overrides={"categories_help": ["option1", "option2"]}, - ) - response = test_app.get("/api/categories") - assert response.status_code == 200 - data = response.json - assert data is not None - assert "categories_help" in data - assert len(data["categories_help"]) == 2 - assert data["categories_help"][0] == {"option1": "categories_help_option1-translated"} - - -@mock.patch("goodmap.api.core_api.gettext", fake_translation) -@mock.patch("flask_babel.gettext", fake_translation) -def test_category_data_endpoint_with_categories_options_help(): - test_app = create_test_app( - feature_flags=make_flag_set(CategoriesHelp), - db_overrides={"categories_options_help": {"test-category": ["help1", "help2"]}}, - ) - response = test_app.get("/api/category/test-category") - assert response.status_code == 200 - data = response.json - assert data is not None - assert "categories_options_help" in data - assert len(data["categories_options_help"]) == 2 - assert data["categories_options_help"][0] == { - "help1": "categories_options_help_help1-translated" - } - - -def test_categories_endpoint_with_none_categories_help(): - test_app = create_test_app( - feature_flags=make_flag_set(CategoriesHelp), db_overrides={"categories_help": None} - ) - response = test_app.get("/api/categories") - assert response.status_code == 200 - data = response.json - assert data is not None - assert data["categories_help"] == [] - - -def test_category_data_endpoint_with_none_categories_options_help(): - config_data = get_test_config_data() - config_data["FEATURE_FLAGS"] = make_flag_set(CategoriesHelp) - config_data["DB"]["DATA"].pop("categories_options_help", None) - config = GoodmapConfig.model_validate(config_data) - app = create_app_from_config(config) - test_client = app.test_client() - response = test_client.get("/api/category/test-category") - assert response.status_code == 200 - data = response.json - assert data is not None - assert data["categories_options_help"] == [] - - # --- Categories-full endpoint tests --- @@ -1052,19 +958,6 @@ def test_make_tuple_translation(): ] -@pytest.mark.parametrize( - "data,keys,expected", - [ - ({"a": {"b": {"c": "value"}}}, ("a", "b", "c"), "value"), - ({"a": "not_a_dict"}, ("a", "b"), None), - ({"a": {"b": "value"}}, ("a", "missing_key"), None), - ], -) -def test_get_or_none(data, keys, expected): - result = get_or_none(data, *keys) - assert result == expected - - def test_issue_options_from_db(test_app): db = test_app.application.db assert db.get_issue_options() == ["test issue 1", "test issue 2"] From 79d442d1a082b51a8991eb0aa6b4d8b424683e45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 08:23:47 +0200 Subject: [PATCH 11/19] some cuts --- docs/http-api.rst | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index 3569b7bf..4cd19f9d 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -33,8 +33,21 @@ on the ``categories`` in your data source (:doc:`data-source`). **Writes need a CSRF token.** CSRF protection is on for the whole app, so ``POST``, ``PUT``, ``PATCH`` and ``DELETE`` without a token get ``400 {"message": "The CSRF token is missing."}``. Send it as an ``X-CSRFToken`` header, from the same session the token was -minted in — the token is bound to the session cookie, so the pair travels together. -Server-rendered pages expose one in a meta tag: +minted in. There is no endpoint that issues a token on its own — a script needs to fetch +a page first, the same as a browser does (:ref:`api-csrf-scripted`). + +**Errors are ``{"message": "..."}``**, occasionally with an extra ``error`` field. +Messages are deliberately generic — the details go to the server log, not the response. + +**Strings are translated** to the request's language before being returned, so category +keys and field names come back as display text (:ref:`config-translations`). + +.. _api-csrf-scripted: + +Calling writes from a script +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +A browser gets the token for free, from a meta tag on every server-rendered page: .. code-block:: html @@ -71,12 +84,6 @@ so it is invisible in normal use; a scripted client (``curl``, a backend job) mu explicitly, e.g. ``-H "Referer: https://your-host/"``, or the request gets ``400 {"message": "The referrer header is missing."}``. -**Errors are ``{"message": "..."}``**, occasionally with an extra ``error`` field. -Messages are deliberately generic — the details go to the server log, not the response. - -**Strings are translated** to the request's language before being returned, so category -keys and field names come back as display text (:ref:`config-translations`). - Reading the map --------------- From d83e52c250a14cbd90af0dff6762ba598efe22a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 09:37:07 +0200 Subject: [PATCH 12/19] cleanup health check --- docs/deployment.rst | 9 --------- docs/http-api.rst | 3 +-- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/docs/deployment.rst b/docs/deployment.rst index 2a474f98..dc474f21 100644 --- a/docs/deployment.rst +++ b/docs/deployment.rst @@ -122,15 +122,6 @@ incoming reports change it while the app runs. Schema keys (``categories``, ``visible_data``, ``location_obligatory_fields``) are read at startup, so changing them means restarting the app. Point data is re-read per request. -Health checks -------------- - -``GET /api/version`` is cheap and needs no data source, returning -``{"backend": ""}``. Point your load balancer at it. - -For a check that also proves the data source is reachable, use ``GET /api/locations`` — -it touches the backend, though on a large map it is not free. - Upgrading --------- diff --git a/docs/http-api.rst b/docs/http-api.rst index 4cd19f9d..8890e8e6 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -202,8 +202,7 @@ Other read endpoints given in ``LANGUAGES``. ``GET /api/version`` returns the installed package version normalised to PEP 440, so a -release published as ``2.0.0-alpha.5`` reports as ``2.0.0a5``. It needs no data source, -which makes it the endpoint to point a load balancer at. +release published as ``2.0.0-alpha.5`` reports as ``2.0.0a5``. Submissions ----------- From 44685af69fe600206090bbc8dd650f6a1a0fed14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 11:37:00 +0200 Subject: [PATCH 13/19] fixes after review --- docs/http-api.rst | 55 +++++++++++++++++++++++++-------------- goodmap/api/api_models.py | 27 +------------------ goodmap/api/core_api.py | 54 +++++++++++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 57 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index 8890e8e6..71371e72 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -20,8 +20,15 @@ A running instance also serves its own generated OpenAPI schema: The schema is generated from the code, so it always describes the release you are running: every endpoint's parameters, status codes and response shapes. **Use it as the -reference.** This page covers the parts a schema cannot state — what the endpoints mean, -how filtering behaves, and which parameters depend on your own data. +reference.** This page covers what a schema cannot state — what the endpoints mean and +how they behave. + +**The API surface is the same in every deployment**: same paths, same methods, same +response envelopes, same status codes. That part is documented here in full. The *values* +moving through it are not — filters, the fields a point may carry, the issues that can be +reported all come from each deployment's own data source. Those are documented by your +running instance rather than by this page; see +`Discovery: what your deployment declares`_. Conventions ----------- @@ -168,38 +175,48 @@ in neither list are not returned at all. The path segment must be a valid UUID; anything else fails routing with ``404``. A well-formed UUID that does not exist also gives ``404 {"message": "Location not found"}``. +Discovery: what your deployment declares +---------------------------------------- + +The endpoints above have a fixed shape, but the *values* moving through them do not. +Which filters apply, which fields a point may carry, which issues can be reported — all +of that comes from your own data source (:doc:`data-source`), so it differs between +instances. Rather than enumerating one instance's values here, these endpoints report +what yours actually declares. They are grouped under the ``discovery`` tag in +``/api/doc``, and a running instance is always the authority. + ``GET /api/categories-full`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Every category with its options, defaults and filter mode — everything needed to render -the filter panel in one request. +the filter panel in one request, and the way to learn which filter parameters +:ref:`api-locations` accepts on this instance. -``key`` is the query-parameter name to filter by; ``name`` is its translated label. -``options`` are ``[value, translated label]`` pairs — send the *value*. -``filter_mode`` tells you which control to draw: checkboxes for ``or``/``and``, radio +``key`` is the query-parameter name to filter by; ``options`` are +``[value, translated label]`` pairs — send the *value*. ``filter_mode`` is one of the five +fixed modes and tells you which control to draw: checkboxes for ``or``/``and``, radio buttons for ``exclusive``/``threshold``, a single checkbox for ``boolean`` (:ref:`categories-filter-mode`). -With ``CATEGORIES_HELP`` on, each category also carries ``options_help``, and the response -gains a top-level ``categories_help`` — both lists of ``{option: help text}`` objects. - .. _api-location-schema: ``GET /api/location-schema`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -What *this* instance accepts for a new point: the fields of its location model (minus the -server-assigned ``uuid`` — ``position`` is required and client-supplied, same as -``/api/suggest-new-point``), the allowed values per category, the reportable issue types, -and the photo limits. Since the accepted fields are configured per deployment, this is how -a client discovers them rather than assuming — it is the same schema the built-in suggest -form is generated from. +What this instance accepts for a new point: the fields of its location model (all of them +except the server-assigned ``uuid``), the allowed values per category, the reportable +issue types, and the photo limits. This is how a client learns what to put in +``/api/suggest-new-point``'s ``location`` payload rather than assuming — it is the same +schema the built-in suggest form is generated from. + +``GET /api/languages`` +~~~~~~~~~~~~~~~~~~~~~~ -Other read endpoints -~~~~~~~~~~~~~~~~~~~~ +The configured interface languages, keyed by language code, exactly as given in +``LANGUAGES``. -``GET /api/languages`` returns the configured languages keyed by language code, exactly as -given in ``LANGUAGES``. +Fixed everywhere +---------------- ``GET /api/version`` returns the installed package version normalised to PEP 440, so a release published as ``2.0.0-alpha.5`` reports as ``2.0.0a5``. diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index f66bbf23..67c10625 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -8,7 +8,6 @@ from typing import Any, Literal from pydantic import BaseModel, Field, RootModel -from spectree import BaseFile from goodmap.data_models.location import Latitude, Longitude @@ -55,15 +54,6 @@ class VersionResponse(BaseModel): backend: str = Field(..., description="Backend version") -class PaginationParams(BaseModel): - """Common pagination and filtering parameters.""" - - page: int | None = Field(None, ge=1, description="Page number (1-indexed)") - per_page: int | None = Field(None, ge=1, le=100, description="Items per page") - sort_by: str | None = Field(None, description="Field to sort by") - sort_order: Literal["asc", "desc"] | None = Field(None, description="Sort direction") - - class ClusteringParams(BaseModel): """Parameters for clustering request.""" @@ -169,21 +159,6 @@ class LocationQueryParams(BaseModel): ) -class SuggestNewPointForm(BaseModel): - """The multipart/form-data body of a new-point suggestion.""" - - location: str = Field( - ..., - description=( - "The whole point as one JSON object, not one form field per property. " - "Its accepted fields come from this instance's location_obligatory_fields " - "and categories - call GET /api/location-schema to discover them. " - "Omit uuid; the server assigns one." - ), - ) - photo: BaseFile | None = Field(None, description="Optional photo, subject to ATTACHMENT limits") - - class IssueType(BaseModel): """One reportable issue type, ready to render in a form.""" @@ -206,7 +181,7 @@ class LocationSchemaResponse(BaseModel): ..., description=( "JSON Schema property per accepted field, from the instance's location model, " - "excluding the server-managed uuid and position" + "excluding only the server-assigned uuid" ), ) obligatory_fields: list[Any] = Field( diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index 4f2273d7..1c926e02 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -14,6 +14,7 @@ from platzky.config import AttachmentConfig, LanguagesMapping from platzky.shortcodes import Shortcode from spectree import Response, SpecTree +from spectree.models import Tag from werkzeug.exceptions import HTTPException from goodmap.api.api_models import ( @@ -29,7 +30,6 @@ LocationReportResponse, LocationSchemaResponse, SuccessResponse, - SuggestNewPointForm, VersionResponse, ) from goodmap.clustering import ( @@ -63,6 +63,28 @@ logger = logging.getLogger(__name__) +# The API surface - paths, methods, response envelopes, status codes - is the same in +# every deployment. The *values* flowing through it are not: filters, accepted point +# fields and reportable issues all come from that deployment's own data source. These +# tags group the endpoints in /api/doc so that split is visible, and point at the +# discovery endpoints that report what a given instance actually declares. +TAG_DISCOVERY = Tag( + name="discovery", + description="What this particular deployment declares - call these to find out, " + "rather than assuming; the answers differ between instances.", +) +TAG_MAP_DATA = Tag( + name="map data", + description="Reading points. Response envelopes are fixed; which filters apply and " + "which fields come back depend on this deployment's data source.", +) +TAG_SUBMISSIONS = Tag( + name="submissions", + description="Visitor-submitted points and reports. Both land in a moderation queue " + "and need a CSRF token.", +) +TAG_META = Tag(name="meta", description="Fixed in every deployment.") + @deprecation.deprecated( deprecated_in="1.5.0", @@ -139,15 +161,16 @@ def _clean_model_name(model: type) -> str: # it on spectree refuses skip_validation, which several routes below rely on. annotations=False, naming_strategy=_clean_model_name, # Use clean model names without hash + tags=[TAG_DISCOVERY, TAG_MAP_DATA, TAG_SUBMISSIONS, TAG_META], ) @core_api_blueprint.route("/suggest-new-point", methods=["POST"]) - # skip_validation: the handler returns its own 400 for a missing or malformed - # `location` field; letting spectree validate would turn that into a 422. + # No form= model: the point's fields are the deployment's own location_model, so a + # static schema could only say "location is a string" - which the docstring already + # says, in words, without spectree then 500ing on an attached photo it cannot + # serialize into a validation error. The handler validates against location_model. @spec.validate( - form=SuggestNewPointForm, - resp=Response(HTTP_200=SuccessResponse, HTTP_400=ErrorResponse), - skip_validation=True, + tags=[TAG_SUBMISSIONS], resp=Response(HTTP_200=SuccessResponse, HTTP_400=ErrorResponse) ) def suggest_new_point(): """Suggest new location for review. @@ -230,6 +253,7 @@ def suggest_new_point(): @core_api_blueprint.route("/report-location", methods=["POST"]) @spec.validate( + tags=[TAG_SUBMISSIONS], json=LocationReportRequest, resp=Response(HTTP_200=LocationReportResponse, HTTP_400=ErrorResponse), ) @@ -277,7 +301,10 @@ def report_location(): # skip_validation: this endpoint ignores invalid and unknown query parameters by # design, so spectree must document them without rejecting anything. @spec.validate( - query=LocationQueryParams, resp=Response(HTTP_200=LocationList), skip_validation=True + tags=[TAG_MAP_DATA], + query=LocationQueryParams, + resp=Response(HTTP_200=LocationList), + skip_validation=True, ) def get_locations(): """Get list of locations with basic info. @@ -292,6 +319,7 @@ def get_locations(): # skip_validation: the handler owns zoom validation and returns 400 with a log line; # letting spectree validate would turn that into a 422 and skip the log. @spec.validate( + tags=[TAG_MAP_DATA], query=ClusteringParams, resp=Response(HTTP_200=ClusterList, HTTP_400=ErrorResponse), skip_validation=True, @@ -345,7 +373,9 @@ def get_locations_clustered(): return make_response(jsonify({"message": "An error occurred during clustering"}), 500) @core_api_blueprint.route("/location/", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=LocationDetail, HTTP_404=ErrorResponse)) + @spec.validate( + tags=[TAG_MAP_DATA], resp=Response(HTTP_200=LocationDetail, HTTP_404=ErrorResponse) + ) def get_location(location_id): """Get detailed information for a single location. @@ -367,7 +397,7 @@ def get_location(location_id): return jsonify(formatted_data) @core_api_blueprint.route("/version", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=VersionResponse)) + @spec.validate(tags=[TAG_META], resp=Response(HTTP_200=VersionResponse)) def get_version(): """Get backend version information. @@ -377,7 +407,7 @@ def get_version(): return jsonify(version_info) @core_api_blueprint.route("/location-schema", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=LocationSchemaResponse)) + @spec.validate(tags=[TAG_DISCOVERY], resp=Response(HTTP_200=LocationSchemaResponse)) def get_location_schema(): """Get the schema this instance accepts for a new point. @@ -410,7 +440,7 @@ def get_location_schema(): ) @core_api_blueprint.route("/categories-full", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=CategoriesFullResponse)) + @spec.validate(tags=[TAG_DISCOVERY], resp=Response(HTTP_200=CategoriesFullResponse)) def get_categories_full(): """Get all categories with their subcategory options in a single request. @@ -460,7 +490,7 @@ def get_categories_full(): return jsonify(response) @core_api_blueprint.route("/languages", methods=["GET"]) - @spec.validate(resp=Response(HTTP_200=LanguagesResponse)) + @spec.validate(tags=[TAG_DISCOVERY], resp=Response(HTTP_200=LanguagesResponse)) def get_languages(): """Get all available interface languages. From 82ade6f82a64a8da5df1ea4c86d4cab21b94915f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 12:13:27 +0200 Subject: [PATCH 14/19] change naming --- docs/http-api.rst | 12 ++++++------ goodmap/api/core_api.py | 18 +++++++++--------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index 71371e72..724143e8 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -24,11 +24,11 @@ reference.** This page covers what a schema cannot state — what the endpoints how they behave. **The API surface is the same in every deployment**: same paths, same methods, same -response envelopes, same status codes. That part is documented here in full. The *values* +response shapes, same status codes. That part is documented here in full. The *values* moving through it are not — filters, the fields a point may carry, the issues that can be reported all come from each deployment's own data source. Those are documented by your running instance rather than by this page; see -`Discovery: what your deployment declares`_. +`Deployment-specific: what your instance declares`_. Conventions ----------- @@ -175,14 +175,14 @@ in neither list are not returned at all. The path segment must be a valid UUID; anything else fails routing with ``404``. A well-formed UUID that does not exist also gives ``404 {"message": "Location not found"}``. -Discovery: what your deployment declares ----------------------------------------- +Deployment-specific: what your instance declares +------------------------------------------------ The endpoints above have a fixed shape, but the *values* moving through them do not. Which filters apply, which fields a point may carry, which issues can be reported — all of that comes from your own data source (:doc:`data-source`), so it differs between instances. Rather than enumerating one instance's values here, these endpoints report -what yours actually declares. They are grouped under the ``discovery`` tag in +what yours actually declares. They are grouped under the ``deployment_specific`` tag in ``/api/doc``, and a running instance is always the authority. ``GET /api/categories-full`` @@ -239,7 +239,7 @@ the map data. form field as a JSON object — not as one form field per property — and the optional photo goes in a ``photo`` file part. Send the point without a ``uuid``; the server assigns one. -That envelope is the same everywhere. **What goes inside the JSON object is not** — the +That shape is the same everywhere. **What goes inside the JSON object is not** — the accepted fields are whatever *your* data source declares in ``location_obligatory_fields`` and ``categories`` (:doc:`data-source`), so there is no universal payload to copy. The fields below are the ones the :doc:`quickstart` map happens to declare; substitute your diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index 1c926e02..b2c3f4e7 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -63,19 +63,19 @@ logger = logging.getLogger(__name__) -# The API surface - paths, methods, response envelopes, status codes - is the same in +# The API surface - paths, methods, response shapes, status codes - is the same in # every deployment. The *values* flowing through it are not: filters, accepted point # fields and reportable issues all come from that deployment's own data source. These # tags group the endpoints in /api/doc so that split is visible, and point at the -# discovery endpoints that report what a given instance actually declares. -TAG_DISCOVERY = Tag( - name="discovery", +# endpoints that report what a given instance actually declares. +TAG_DEPLOYMENT_SPECIFIC = Tag( + name="deployment_specific", description="What this particular deployment declares - call these to find out, " "rather than assuming; the answers differ between instances.", ) TAG_MAP_DATA = Tag( name="map data", - description="Reading points. Response envelopes are fixed; which filters apply and " + description="Reading points. Response shapes are fixed; which filters apply and " "which fields come back depend on this deployment's data source.", ) TAG_SUBMISSIONS = Tag( @@ -161,7 +161,7 @@ def _clean_model_name(model: type) -> str: # it on spectree refuses skip_validation, which several routes below rely on. annotations=False, naming_strategy=_clean_model_name, # Use clean model names without hash - tags=[TAG_DISCOVERY, TAG_MAP_DATA, TAG_SUBMISSIONS, TAG_META], + tags=[TAG_DEPLOYMENT_SPECIFIC, TAG_MAP_DATA, TAG_SUBMISSIONS, TAG_META], ) @core_api_blueprint.route("/suggest-new-point", methods=["POST"]) @@ -407,7 +407,7 @@ def get_version(): return jsonify(version_info) @core_api_blueprint.route("/location-schema", methods=["GET"]) - @spec.validate(tags=[TAG_DISCOVERY], resp=Response(HTTP_200=LocationSchemaResponse)) + @spec.validate(tags=[TAG_DEPLOYMENT_SPECIFIC], resp=Response(HTTP_200=LocationSchemaResponse)) def get_location_schema(): """Get the schema this instance accepts for a new point. @@ -440,7 +440,7 @@ def get_location_schema(): ) @core_api_blueprint.route("/categories-full", methods=["GET"]) - @spec.validate(tags=[TAG_DISCOVERY], resp=Response(HTTP_200=CategoriesFullResponse)) + @spec.validate(tags=[TAG_DEPLOYMENT_SPECIFIC], resp=Response(HTTP_200=CategoriesFullResponse)) def get_categories_full(): """Get all categories with their subcategory options in a single request. @@ -490,7 +490,7 @@ def get_categories_full(): return jsonify(response) @core_api_blueprint.route("/languages", methods=["GET"]) - @spec.validate(tags=[TAG_DISCOVERY], resp=Response(HTTP_200=LanguagesResponse)) + @spec.validate(tags=[TAG_DEPLOYMENT_SPECIFIC], resp=Response(HTTP_200=LanguagesResponse)) def get_languages(): """Get all available interface languages. From b5b550f08ad5c2cc69e8f35dd1317fa1b708725c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 12:57:53 +0200 Subject: [PATCH 15/19] fixes --- docs/http-api.rst | 19 ++++--- frontend/src/services/http/httpService.js | 27 ++++++++-- goodmap/api/api_models.py | 20 ++++---- goodmap/api/core_api.py | 60 +++++++++++++--------- tests/unit_tests/test_core_api.py | 62 ++++++++++++++++++----- 5 files changed, 134 insertions(+), 54 deletions(-) diff --git a/docs/http-api.rst b/docs/http-api.rst index 724143e8..215558f3 100644 --- a/docs/http-api.rst +++ b/docs/http-api.rst @@ -44,7 +44,9 @@ minted in. There is no endpoint that issues a token on its own — a script need a page first, the same as a browser does (:ref:`api-csrf-scripted`). **Errors are ``{"message": "..."}``**, occasionally with an extra ``error`` field. -Messages are deliberately generic — the details go to the server log, not the response. +Messages are deliberately generic — the offending values go to the server log, not the +response. A rejected query parameter names which one it was, without echoing the value: +``{"message": "Invalid request data", "error": "invalid or out of range: zoom"}``. **Strings are translated** to the request's language before being returned, so category keys and field names come back as display text (:ref:`config-translations`). @@ -115,10 +117,10 @@ Query parameters: - Filter value; repeat for several. Combined per :ref:`categories-filter-mode`. * - ``lat``, ``lon`` - Sort results by distance from this coordinate, nearest first. Both required, or - neither applies. + neither applies. Ranges are the usual **−90..90** and **−180..180**. * - ``limit`` - - Return at most this many points. Applied after sorting, so ``lat``/``lon``/``limit`` - together give "the N nearest". + - Return at most this many points, **1 or more**. Applied after sorting, so + ``lat``/``lon``/``limit`` together give "the N nearest". .. code-block:: bash @@ -127,7 +129,11 @@ Query parameters: Each point comes back as ``uuid``, ``position`` and ``has_remark`` — a **boolean**, whether the point has a remark, not its text. -Invalid or unknown query parameters are ignored rather than rejected. +A ``lat``, ``lon`` or ``limit`` that cannot mean anything — not a number, or outside the +range above — is a ``400 {"message": "Invalid request data"}`` rather than a silently +different result. Any *other* parameter is passed through to the filters untouched: the +valid filter names come from your own ``categories`` and cannot be checked against a fixed +list, so an unknown one is simply a filter that matches nothing. ``GET /api/locations-clustered`` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -137,7 +143,8 @@ level. This is what the frontend calls instead of ``/api/locations`` when ``USE_SERVER_SIDE_CLUSTERING`` is on. Takes every parameter of :ref:`api-locations`, plus ``zoom`` (integer, **0–16**, default -``7``). A ``zoom`` outside that range is a ``400``. +``7``), and rejects unusable values the same way — a ``zoom`` outside that range, like a +bad ``lat``, is a ``400``. Points and clusters come back in one list, told apart by ``type``. A ``"point"`` carries a real ``uuid`` you can pass to :ref:`api-location-detail`; a ``"cluster"`` carries a diff --git a/frontend/src/services/http/httpService.js b/frontend/src/services/http/httpService.js index e734563d..620fb6d7 100644 --- a/frontend/src/services/http/httpService.js +++ b/frontend/src/services/http/httpService.js @@ -13,6 +13,27 @@ import { useMapStore } from '../../components/Map/store/map.store'; // sanitizers for request-URL construction, not third-party validators. const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +/** + * Returns the parsed JSON body, or throws if the response was not a success. + * + * Without this a rejected request (e.g. 400 for an out-of-range lat) resolves to the + * API's `{message}` error object, which then reaches the callers as if it were the + * data they asked for and fails later with a confusing shape error. + * + * @param {Response} response - fetch response + * @param {string} what - short description of the request, used in the error message + * @returns {Promise} The parsed JSON body + * @throws {Error} If the response status is not ok + */ +const jsonOrThrow = async (response, what) => { + if (!response.ok) { + const body = await response.json().catch(() => null); + const detail = body?.message ? `: ${body.message}` : ''; + throw new Error(`Failed to fetch ${what} (HTTP ${response.status})${detail}`); + } + return response.json(); +}; + /** * Converts filter object to URL query string parameters. * Also includes map configuration (zoom, bounds) if server-side clustering is enabled. @@ -96,7 +117,7 @@ export const httpService = { 'Content-Type': 'application/json', }, }); - return response.json(); + return jsonOrThrow(response, 'locations'); }, /** @@ -119,7 +140,7 @@ export const httpService = { }, }, ); - return response.json(); + return jsonOrThrow(response, 'nearby locations'); }, /** @@ -143,7 +164,7 @@ export const httpService = { 'Content-Type': 'application/json', }, }); - return response.json(); + return jsonOrThrow(response, 'location details'); }, /** diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index 67c10625..8c49daaa 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -54,12 +54,6 @@ class VersionResponse(BaseModel): backend: str = Field(..., description="Backend version") -class ClusteringParams(BaseModel): - """Parameters for clustering request.""" - - zoom: int = Field(7, ge=0, le=16, description="Map zoom level for clustering") - - class ErrorResponse(BaseModel): """Standard error response.""" @@ -150,15 +144,23 @@ class LocationQueryParams(BaseModel): ``GET /api/categories-full`` to discover the ones this instance accepts. """ - lat: float | None = Field(None, description="Sort by distance from this latitude; requires lon") - lon: float | None = Field( + lat: Latitude | None = Field( + None, description="Sort by distance from this latitude; requires lon" + ) + lon: Longitude | None = Field( None, description="Sort by distance from this longitude; requires lat" ) limit: int | None = Field( - None, description="Return at most this many points, applied after sorting" + None, ge=1, description="Return at most this many points, applied after sorting" ) +class ClusteredQueryParams(LocationQueryParams): + """Query parameters of the clustered list: the plain list's, plus ``zoom``.""" + + zoom: int = Field(7, ge=0, le=16, description="Map zoom level for clustering") + + class IssueType(BaseModel): """One reportable issue type, ready to render in a form.""" diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index b2c3f4e7..6b485606 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -19,7 +19,7 @@ from goodmap.api.api_models import ( CategoriesFullResponse, - ClusteringParams, + ClusteredQueryParams, ClusterList, ErrorResponse, LanguagesResponse, @@ -130,6 +130,32 @@ def safe_location_loads(raw_location: str) -> dict[str, Any]: return parsed +def _validation_error_to_api_shape(req, resp, req_validation_error, instance): + """Rewrite spectree's raw pydantic error into the API's documented error shape. + + Spectree answers a failed request model with a bare list of pydantic error dicts, + which contradicts the "errors are {"message": ...}" contract every other endpoint + keeps, and leaks input values and errors.pydantic.dev links to the caller. The + detail goes to the log instead, same as the handlers' own error paths. + """ + if req_validation_error is None or resp is None: + return + errors = req_validation_error.errors() + logger.warning( + "Request validation failed: %s", + errors, + extra={"path": getattr(req, "path", None)}, + ) + # Name the offending parameters but not their values or pydantic's internals: the + # caller sent those names, so echoing them back leaks nothing and saves a guess. + fields = sorted({str(loc) for e in errors for loc in e.get("loc", ())}) + body = {"message": ERROR_INVALID_REQUEST_DATA} + if fields: + body["error"] = f"invalid or out of range: {', '.join(fields)}" + resp.set_data(json_lib.dumps(body)) + resp.content_type = "application/json" + + def core_pages( database, languages: LanguagesMapping, @@ -157,18 +183,14 @@ def _clean_model_name(model: type) -> str: title="Goodmap API", version="0.1", path="doc", - # annotations=False: the handlers take no model-annotated parameters, and with - # it on spectree refuses skip_validation, which several routes below rely on. - annotations=False, naming_strategy=_clean_model_name, # Use clean model names without hash tags=[TAG_DEPLOYMENT_SPECIFIC, TAG_MAP_DATA, TAG_SUBMISSIONS, TAG_META], + validation_error_status=400, + validation_error_model=ErrorResponse, + before=_validation_error_to_api_shape, ) @core_api_blueprint.route("/suggest-new-point", methods=["POST"]) - # No form= model: the point's fields are the deployment's own location_model, so a - # static schema could only say "location is a string" - which the docstring already - # says, in words, without spectree then 500ing on an attached photo it cannot - # serialize into a validation error. The handler validates against location_model. @spec.validate( tags=[TAG_SUBMISSIONS], resp=Response(HTTP_200=SuccessResponse, HTTP_400=ErrorResponse) ) @@ -298,13 +320,13 @@ def report_location(): return make_response(jsonify({"message": gettext("Location reported")}), 200) @core_api_blueprint.route("/locations", methods=["GET"]) - # skip_validation: this endpoint ignores invalid and unknown query parameters by - # design, so spectree must document them without rejecting anything. + # lat/lon/limit are validated: a value that cannot mean anything (lat=abc, lat=999, + # limit=-3) is a caller mistake worth reporting, not worth silently ignoring. The + # deployment's own category filters are not declared here and pass through untouched. @spec.validate( tags=[TAG_MAP_DATA], query=LocationQueryParams, - resp=Response(HTTP_200=LocationList), - skip_validation=True, + resp=Response(HTTP_200=LocationList, HTTP_400=ErrorResponse), ) def get_locations(): """Get list of locations with basic info. @@ -316,13 +338,11 @@ def get_locations(): return jsonify(locations) @core_api_blueprint.route("/locations-clustered", methods=["GET"]) - # skip_validation: the handler owns zoom validation and returns 400 with a log line; - # letting spectree validate would turn that into a 422 and skip the log. + # Same contract as /api/locations, plus zoom - validated for the same reason. @spec.validate( tags=[TAG_MAP_DATA], - query=ClusteringParams, + query=ClusteredQueryParams, resp=Response(HTTP_200=ClusterList, HTTP_400=ErrorResponse), - skip_validation=True, ) def get_locations_clustered(): """Get clustered locations for map display. @@ -332,15 +352,9 @@ def get_locations_clustered(): """ try: query_params = request.args.to_dict(flat=False) + # Range-checked by ClusteredQueryParams before the handler runs. zoom = int(query_params.get("zoom", [7])[0]) - # Validate zoom level (aligned with SuperCluster min_zoom/max_zoom) - if not MIN_ZOOM <= zoom <= MAX_ZOOM: - return make_response( - jsonify({"message": f"Zoom must be between {MIN_ZOOM} and {MAX_ZOOM}"}), - 400, - ) - points = get_locations_from_request(database, request.args) if not points: return jsonify([]) diff --git a/tests/unit_tests/test_core_api.py b/tests/unit_tests/test_core_api.py index 0541894e..06844bb7 100644 --- a/tests/unit_tests/test_core_api.py +++ b/tests/unit_tests/test_core_api.py @@ -262,6 +262,39 @@ def test_get_locations(test_app): ] +@pytest.mark.parametrize( + "query", + [ + "lat=abc", # not a number + "lat=999", # outside -90..90 + "lon=999", # outside -180..180 + "limit=notanumber", + "limit=0", # a limit of nothing is a caller mistake, not an empty map + "limit=-3", + ], +) +def test_get_locations_rejects_unusable_parameters(test_app, query): + """lat/lon/limit values that cannot mean anything are reported, not ignored.""" + response = test_app.get(f"/api/locations?{query}") + assert response.status_code == 400 + assert response.json["message"] == "Invalid request data" + + +@pytest.mark.parametrize( + "query", + [ + "", + "lat=51.1&lon=17.05&limit=5", + "unknown_param=x", # not declared, and cannot be - filters are per-deployment + "test-category=test", + ], +) +def test_get_locations_accepts_valid_and_undeclared_parameters(test_app, query): + """Declared params are checked; anything else passes through to the filters.""" + response = test_app.get(f"/api/locations?{query}") + assert response.status_code == 200 + + def test_get_locations_multi_value_same_category_uses_or_semantics(): """Selecting several checkboxes within one category should return the union of matches, not only entries that have every selected value.""" @@ -433,8 +466,8 @@ def test_reporting_location_success(test_app): @mock.patch("flask_babel.gettext", fake_translation) def test_reporting_returns_error_when_wrong_json(test_app): response = api_post(test_app, "/api/report-location", {"name": "location-id", "position": 50}) - assert response.status_code == 422 - assert isinstance(response.json, list) + assert response.status_code == 400 + assert response.json["message"] == "Invalid request data" @mock.patch("goodmap.api.core_api.gettext", fake_translation) @@ -451,7 +484,8 @@ def test_report_location_with_invalid_json(test_app): response = test_app.post( "/api/report-location", data="invalid json", content_type="application/json" ) - assert response.status_code == 422 + assert response.status_code == 400 + assert response.json["message"] == "Invalid request data" def test_report_location_unexpected_error(test_app): @@ -893,20 +927,21 @@ def test_location_clustering_low_zoom_creates_clusters(test_app): @pytest.mark.parametrize( - "zoom,expected_status,check_message", + "zoom,expected_status", [ - ("invalid", 400, "Invalid parameters provided"), - ("-1", 400, "Zoom must be between 0 and 16"), - ("17", 400, "Zoom must be between 0 and 16"), - ("0", 200, None), - ("16", 200, None), + ("invalid", 400), + ("-1", 400), + ("17", 400), + ("0", 200), + ("16", 200), ], ) -def test_location_clustering_zoom_validation(test_app, zoom, expected_status, check_message): +def test_location_clustering_zoom_validation(test_app, zoom, expected_status): response = test_app.get(f"/api/locations-clustered?zoom={zoom}") assert response.status_code == expected_status - if check_message: - assert check_message in response.json["message"] + if expected_status == 400: + assert response.json["message"] == "Invalid request data" + assert "zoom" in response.json["error"] def test_location_clustering_empty_locations(): @@ -927,10 +962,11 @@ def test_location_clustering_exception_handling(test_app): def test_location_clustering_logs_on_invalid_parameter(test_app): + """The rejected value itself goes to the log, not to the caller.""" with mock.patch("goodmap.api.core_api.logger") as mock_logger: test_app.get("/api/locations-clustered?zoom=invalid") mock_logger.warning.assert_called_once() - assert "Invalid parameter" in mock_logger.warning.call_args[0][0] + assert "Request validation failed" in mock_logger.warning.call_args[0][0] def test_location_clustering_logs_on_exception(test_app): From 79fe22deb4b4129e0f4aa9e58d88f952c2d2a740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Ko=C5=82odzi=C5=84ski?= Date: Wed, 19 Aug 2026 13:17:12 +0200 Subject: [PATCH 16/19] little refactor --- goodmap/api/api_models.py | 3 +- goodmap/api/core_api.py | 124 ++++++++++++++++++++------------------ goodmap/clustering.py | 6 ++ 3 files changed, 75 insertions(+), 58 deletions(-) diff --git a/goodmap/api/api_models.py b/goodmap/api/api_models.py index 8c49daaa..fefb06a5 100644 --- a/goodmap/api/api_models.py +++ b/goodmap/api/api_models.py @@ -9,6 +9,7 @@ from pydantic import BaseModel, Field, RootModel +from goodmap.clustering import MAX_ZOOM, MIN_ZOOM from goodmap.data_models.location import Latitude, Longitude _POSITION_DESCRIPTION = "[latitude, longitude]" @@ -158,7 +159,7 @@ class LocationQueryParams(BaseModel): class ClusteredQueryParams(LocationQueryParams): """Query parameters of the clustered list: the plain list's, plus ``zoom``.""" - zoom: int = Field(7, ge=0, le=16, description="Map zoom level for clustering") + zoom: int = Field(7, ge=MIN_ZOOM, le=MAX_ZOOM, description="Map zoom level for clustering") class IssueType(BaseModel): diff --git a/goodmap/api/core_api.py b/goodmap/api/core_api.py index 6b485606..ad72b89e 100644 --- a/goodmap/api/core_api.py +++ b/goodmap/api/core_api.py @@ -33,6 +33,8 @@ VersionResponse, ) from goodmap.clustering import ( + MAX_ZOOM, + MIN_ZOOM, map_clustering_data_to_proper_lazy_loading_object, match_clusters_uuids, ) @@ -46,9 +48,8 @@ safe_json_loads, ) -# SuperCluster configuration constants -MIN_ZOOM = 0 -MAX_ZOOM = 16 +# SuperCluster configuration constants (MIN_ZOOM/MAX_ZOOM live in clustering.py, so the +# request model and the clusterer cannot disagree about the accepted range) CLUSTER_RADIUS = 200 CLUSTER_EXTENT = 512 @@ -60,6 +61,10 @@ ERROR_INVALID_LOCATION_DATA = "Invalid location data" ERROR_LOCATION_NOT_FOUND = "Location not found" ERROR_INVALID_DESCRIPTION = "Invalid report description" +ERROR_INVALID_PARAMETERS = "Invalid parameters provided" +ERROR_PAYLOAD_TOO_COMPLEX = "Invalid request: JSON payload too complex or too large" +ERROR_CLUSTERING_FAILED = "An error occurred during clustering" +ERROR_SUGGESTION_FAILED = "An error occurred while processing your suggestion" logger = logging.getLogger(__name__) @@ -97,6 +102,20 @@ def get_default_issue_options(): return ["notHere", "overload", "broken", "other"] +def translated_help(options, prefix: str) -> list[dict[str, str]]: + """Build the ``[{option: help text}]`` shape the help fields use. + + The help text is looked up under ``_