diff --git a/datasette/extras.py b/datasette/extras.py index 36014185..fb8c2e06 100644 --- a/datasette/extras.py +++ b/datasette/extras.py @@ -5,6 +5,8 @@ from typing import ClassVar from asyncinject import Registry +from datasette.utils.asgi import BadRequest + def extra_names_from_request(request): extra_bits = request.args.getlist("_extra") @@ -113,6 +115,17 @@ class ExtraRegistry: self._allowed_names[key] = names return names + def validate_requested(self, requested, scope): + """ + Raise BadRequest if any requested extra name is not a public extra + for this scope. Used by data formats such as .json - HTML pages + silently ignore unknown names instead. + """ + allowed = self._allowed_names_for_scope(scope, include_internal=False) + unknown = sorted(name for name in requested if name not in allowed) + if unknown: + raise BadRequest("Unknown _extra: {}".format(", ".join(unknown))) + async def resolve(self, requested, context, scope, include_internal=False): allowed_names = self._allowed_names_for_scope(scope, include_internal) requested_names = [name for name in requested if name in allowed_names] diff --git a/datasette/views/database.py b/datasette/views/database.py index 99e85d2b..b7131a05 100644 --- a/datasette/views/database.py +++ b/datasette/views/database.py @@ -8,7 +8,7 @@ import markupsafe import os import textwrap -from datasette.extras import extra_names_from_request +from datasette.extras import extra_names_from_request, ExtraScope from datasette.database import QueryInterrupted from datasette.resources import DatabaseResource, QueryResource from datasette.stored_queries import StoredQuery, stored_query_to_dict @@ -855,6 +855,7 @@ class QueryView(View): raise DatasetteError("?sql= is required", status=400) data = {"ok": True, "rows": rows, "columns": columns} extras = extra_names_from_request(request) + table_extra_registry.validate_requested(extras, ExtraScope.QUERY) if extras: query_extra_context = QueryExtraContext( datasette=datasette, diff --git a/datasette/views/row.py b/datasette/views/row.py index 71539f34..296a0ac1 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -596,6 +596,9 @@ class RowView(BaseView): } extras = extra_names_from_request(request) + if request.url_vars.get("format"): + # Data formats reject unknown extras; HTML ignores them + table_extra_registry.validate_requested(extras, ExtraScope.ROW) # Process extras row_extra_context = RowExtraContext( diff --git a/datasette/views/table.py b/datasette/views/table.py index 44919281..ed3b1276 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -2254,6 +2254,10 @@ async def table_view_data( # Resolve extras extras = extra_names_from_request(request) + if not extra_extras: + # Data formats reject unknown extras; the HTML path (which passes + # extra_extras={"_html"}) resolves internal extras of its own + table_extra_registry.validate_requested(extras, ExtraScope.TABLE) if any(k for k in request.args.keys() if k == "_facet" or k.startswith("_facet_")): extras.add("facet_results") if request.args.get("_shape") == "object": diff --git a/docs/json_api.rst b/docs/json_api.rst index 2a859ba2..0069055b 100644 --- a/docs/json_api.rst +++ b/docs/json_api.rst @@ -283,6 +283,8 @@ These can be repeated or comma-separated: ?_extra=columns&_extra=count,next_url +Requesting an ``_extra`` name that does not exist returns a ``400`` error in the :ref:`standard error format `, for example ``{"ok": false, "error": "Unknown _extra: nope", ...}``. + .. [[[cog from json_api_doc import table_extras table_extras(cog) diff --git a/existing-api.md b/existing-api.md index 6ce29bf5..cf456921 100644 --- a/existing-api.md +++ b/existing-api.md @@ -176,9 +176,10 @@ build JSON directly): Table, row and query JSON responses support `?_extra=` (repeatable and/or comma-separated, extras.py:9-14) to add keys to the response. Extras are scope-registered (`ExtraScope.TABLE` / `ROW` / `QUERY`) and only **public** -extras are available over JSON (extras.py:73-92). Unknown extra names are -silently ignored. The available names per scope are listed with the relevant -endpoints below. +extras are available over JSON (extras.py:73-92). Unknown extra names (and +internal HTML-only names) on data formats return 400 +`Unknown _extra: `; HTML pages ignore them. The available names per +scope are listed with the relevant endpoints below. --- diff --git a/stable-api-recommendations.md b/stable-api-recommendations.md index a92fbfbd..1bc2412e 100644 --- a/stable-api-recommendations.md +++ b/stable-api-recommendations.md @@ -166,7 +166,11 @@ Endpoints disagree about the success envelope: matches the read API).~~ ✅ **Done** — row update now returns `rows: [{...}]`. -### 2b. `_extra`/`_shape` support is uneven (P2) +### 2b. `_extra`/`_shape` support is uneven (P2) — partially implemented + +> **Status:** unknown `_extra` names on data formats now return 400 +> `Unknown _extra: ` (HTML pages still ignore them). Extending +> extras/shaping to database/instance scope remains open. The extras system (`?_extra=`, scope-registered) is the 1.0 mechanism for response shaping — but it only exists on table, row and query endpoints. The diff --git a/tests/test_error_shape.py b/tests/test_error_shape.py index 55370c18..f0ff0a42 100644 --- a/tests/test_error_shape.py +++ b/tests/test_error_shape.py @@ -570,3 +570,37 @@ async def test_schema_endpoints_no_existence_oracle(tmp_path_factory): async def test_table_schema_unknown_database_is_404_not_500(ds_client): response = await ds_client.get("/no_such_db/some_table/-/schema.json") assert_canonical_error(response, 404) + + +# Unknown _extra names are a 400, not silently ignored + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "path", + ( + "/fixtures/facetable.json?_extra=nope", + "/fixtures/facetable.json?_extra=count,nope", + "/fixtures/simple_primary_key/1.json?_extra=nope", + "/fixtures/-/query.json?sql=select+1&_extra=nope", + ), +) +async def test_unknown_extra_is_400(ds_client, path): + response = await ds_client.get(path) + data = assert_canonical_error(response, 400) + assert data["errors"] == ["Unknown _extra: nope"] + + +@pytest.mark.asyncio +async def test_html_only_extra_via_json_is_400(ds_client): + # display_rows exists for the HTML view but is not part of the JSON API + response = await ds_client.get("/fixtures/facetable.json?_extra=display_rows") + data = assert_canonical_error(response, 400) + assert data["errors"] == ["Unknown _extra: display_rows"] + + +@pytest.mark.asyncio +async def test_unknown_extra_ignored_on_html_pages(ds_client): + response = await ds_client.get("/fixtures/facetable?_extra=nope") + assert response.status_code == 200 + assert response.headers["content-type"].startswith("text/html") diff --git a/tests/test_table_api.py b/tests/test_table_api.py index e3737ee6..ff413be8 100644 --- a/tests/test_table_api.py +++ b/tests/test_table_api.py @@ -123,8 +123,8 @@ async def test_html_only_extras_are_not_available_via_json(ds_client, extra): # These extras exist for the HTML view; their values are not JSON # serializable so they are internal, not part of the JSON API response = await ds_client.get(f"/fixtures/facetable.json?_extra={extra}") - assert response.status_code == 200 - assert extra not in response.json() + assert response.status_code == 400 + assert response.json()["errors"] == [f"Unknown _extra: {extra}"] @pytest.mark.asyncio