diff --git a/datasette/forbidden.py b/datasette/forbidden.py index 41c48396..3a81ac4f 100644 --- a/datasette/forbidden.py +++ b/datasette/forbidden.py @@ -1,9 +1,19 @@ from datasette import hookimpl, Response +from .utils import add_cors_headers, error_body @hookimpl(trylast=True) def forbidden(datasette, request, message): async def inner(): + if ( + request.path.split("?")[0].endswith(".json") + or "application/json" in (request.headers.get("accept") or "") + or request.headers.get("content-type") == "application/json" + ): + headers = {} + if datasette.cors: + add_cors_headers(headers) + return Response.json(error_body(message, 403), status=403, headers=headers) return Response.html( await datasette.render_template( "error.html", diff --git a/docs/json_api.rst b/docs/json_api.rst index 27d2d705..a561aa9c 100644 --- a/docs/json_api.rst +++ b/docs/json_api.rst @@ -81,6 +81,11 @@ Some endpoints add extra context keys. For example, a SQL error from a ``"rows"`` and ``"truncated"`` keys of the response it was unable to produce. +Permission errors use the same format: a request that fails a permission +check receives a ``403`` with this JSON error body when the URL ends in +``.json`` or the request sends an ``Accept: application/json`` or +``Content-Type: application/json`` header. + .. _json_api_custom_sql: Executing custom SQL diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index 81ef4acd..8614a15c 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -1685,6 +1685,8 @@ forbidden(datasette, request, message) Plugins can use this to customize how Datasette responds when a 403 Forbidden error occurs - usually because a page failed a permission check, see :ref:`authentication_permissions`. +Datasette's default behavior returns the :ref:`standard JSON error format ` with a 403 status when the request path ends in ``.json`` or the request has an ``Accept: application/json`` or ``Content-Type: application/json`` header; other requests get an HTML error page. + If a plugin hook wishes to react to the error, it should return a :ref:`Response object `. This example returns a redirect to a ``/-/login`` page: diff --git a/existing-api.md b/existing-api.md index 722331cd..90a931d1 100644 --- a/existing-api.md +++ b/existing-api.md @@ -98,13 +98,13 @@ Method-not-allowed responses return HTTP 405 with the canonical shape when the path ends in `.json` or the request content type is `application/json`; plain text otherwise (views/base.py). -**`Forbidden` is special:** when a view raises `Forbidden` (e.g. via -`ensure_permission`), the default `forbidden()` plugin hook renders an **HTML -error page with status 403 even for `.json` requests** -(forbidden.py:4-19, app.py:2895-2904). Endpoints that check permissions -themselves and return `_error(..., 403)` produce JSON instead. So a JSON -client may receive either an HTML 403 page or a JSON 403 body depending on -the endpoint. +**`Forbidden` handling:** when a view raises `Forbidden` (e.g. via +`ensure_permission`), the default `forbidden()` plugin hook returns the +canonical JSON error with status 403 when the path ends in `.json` or the +request carries an `Accept: application/json` / `Content-Type: +application/json` header; other requests get an HTML error page +(forbidden.py). Endpoints that check permissions themselves return +`_error(..., 403)` JSON directly. ### CORS diff --git a/stable-api-recommendations.md b/stable-api-recommendations.md index a8690f51..d68bd32e 100644 --- a/stable-api-recommendations.md +++ b/stable-api-recommendations.md @@ -26,9 +26,9 @@ Findings are grouped by theme. Each carries a priority: > The `title` key is no longer emitted in JSON; the bare `{"error": ...}` > debug-endpoint shape is gone; `_shape=object` misuse now returns HTTP 400 > (part of §1b). Covered by `tests/test_error_shape.py` and documented in -> the "Error responses" section of `docs/json_api.rst`. Still open from -> this section's sub-items: §1a (`Forbidden` → HTML), the write -> canned-query 200 (§1b), and the §1c status outliers. +> the "Error responses" section of `docs/json_api.rst`. §1a (`Forbidden` → +> JSON) is now also implemented. Still open from this section's sub-items: +> the write canned-query 200 (§1b) and the §1c status outliers. The API currently produces four distinct JSON error shapes depending on which internal layer generates the error: @@ -58,7 +58,11 @@ defaults). At minimum, eliminate the bare `{"error": ...}` shape and the `status`/`title` keys nobody else emits (`title` is a template-rendering concern that leaked into the API). -### 1a. `Forbidden` returns an HTML 403 to JSON clients (P1) +### 1a. `Forbidden` returns an HTML 403 to JSON clients (P1) — ✅ IMPLEMENTED + +> **Status:** implemented — the default `forbidden()` hook now returns the +> canonical JSON error for requests whose path ends in `.json` or that send +> `Accept: application/json` / `Content-Type: application/json`. Read endpoints that deny access via `ensure_permission`/`check_visibility` raise `Forbidden`, and the default `forbidden()` hook renders an **HTML error @@ -373,7 +377,7 @@ Two details make tiering urgent rather than optional: ## 10. Summary of P1 items (the pre-1.0 checklist) 1. ~~One canonical JSON error shape; retire the other three (§1).~~ ✅ Done. -2. `Forbidden` → JSON 403 for JSON requests (§1a). +2. ~~`Forbidden` → JSON 403 for JSON requests (§1a).~~ ✅ Done. 3. No `ok: false` with HTTP 200 (§1b: `_shape=object`, write canned-query SQL errors). 4. ~~Wrap `/-/plugins`, `/-/databases`, `/-/actions` top-level arrays in diff --git a/tests/test_cli.py b/tests/test_cli.py index 7b528a8e..cbd8edad 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -385,7 +385,9 @@ def test_setting_boolean_validation_false_values(value): ) # Should be forbidden (setting is false) assert result.exit_code == 1, result.output - assert "Forbidden" in result.output + error = json.loads(result.output) + assert error["ok"] is False + assert error["status"] == 403 @pytest.mark.parametrize("value", ("on", "true", "1")) @@ -425,8 +427,9 @@ def test_setting_default_allow_sql(default_allow_sql): assert json.loads(result.output)["rows"][0] == {"21": 21} else: assert result.exit_code == 1, result.output - # This isn't JSON at the moment, maybe it should be though - assert "Forbidden" in result.output + error = json.loads(result.output) + assert error["ok"] is False + assert error["status"] == 403 def test_sql_errors_logged_to_stderr(): diff --git a/tests/test_error_shape.py b/tests/test_error_shape.py index 6747e47a..7f12abc6 100644 --- a/tests/test_error_shape.py +++ b/tests/test_error_shape.py @@ -183,3 +183,52 @@ async def test_method_not_allowed_error_shape(ds_client): async def test_schema_unknown_database_error_shape(ds_client): response = await ds_client.get("/no_such_db/-/schema.json") assert_canonical_error(response, 404) + + +# Forbidden responses (the default forbidden() hook) + + +@pytest.fixture +def ds_forbidden(tmp_path_factory): + db_directory = tmp_path_factory.mktemp("dbs") + db_path = str(db_directory / "data.db") + conn = sqlite3.connect(db_path) + conn.execute("vacuum") + conn.execute("create table docs (id integer primary key, title text)") + conn.close() + ds = Datasette( + [db_path], + config={"databases": {"data": {"tables": {"docs": {"allow": {"id": "root"}}}}}}, + ) + ds.root_enabled = True + yield ds + ds.close() + + +@pytest.mark.asyncio +async def test_forbidden_json_path_returns_canonical_json(ds_forbidden): + response = await ds_forbidden.client.get("/data/docs.json") + data = assert_canonical_error(response, 403) + assert "permission" in data["error"].lower() + + +@pytest.mark.asyncio +async def test_forbidden_accept_json_returns_canonical_json(ds_forbidden): + response = await ds_forbidden.client.get( + "/data/docs", headers={"Accept": "application/json"} + ) + assert_canonical_error(response, 403) + + +@pytest.mark.asyncio +async def test_forbidden_html_path_still_returns_html(ds_forbidden): + response = await ds_forbidden.client.get("/data/docs") + assert response.status_code == 403 + assert response.headers["content-type"].startswith("text/html") + + +@pytest.mark.asyncio +async def test_forbidden_json_path_allowed_actor_still_works(ds_forbidden): + response = await ds_forbidden.client.get("/data/docs.json", actor={"id": "root"}) + assert response.status_code == 200 + assert response.json()["ok"] is True