mirror of
https://github.com/simonw/datasette.git
synced 2026-07-09 17:14:35 +02:00
Return canonical JSON error for Forbidden on JSON requests
The default forbidden() hook previously rendered an HTML error page even for .json requests. It now returns the canonical JSON error shape with status 403 when the request path ends in .json or the request sends an Accept: application/json or Content-Type: application/json header. HTML requests still get the error page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GrHZSypDfMnym1tM5XJAFZ
This commit is contained in:
parent
f091b6dab1
commit
ae10a99811
7 changed files with 88 additions and 15 deletions
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 <json_api_errors>` 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 <internals_response>`.
|
||||
|
||||
This example returns a redirect to a ``/-/login`` page:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue