diff --git a/datasette/__init__.py b/datasette/__init__.py index eb18e59e..de46861c 100644 --- a/datasette/__init__.py +++ b/datasette/__init__.py @@ -1,7 +1,7 @@ from datasette.permissions import Permission # noqa from datasette.version import __version_info__, __version__ # noqa from datasette.events import Event # noqa -from datasette.tokens import TokenHandler, TokenRestrictions # noqa +from datasette.tokens import TokenHandler, TokenInvalid, TokenRestrictions # noqa from datasette.utils.asgi import Forbidden, NotFound, Request, Response # noqa from datasette.utils import actor_matches_allow # noqa from datasette.views import Context # noqa diff --git a/datasette/app.py b/datasette/app.py index dd5d7e8c..9982c58e 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -111,7 +111,9 @@ from .utils import ( baseconv, call_with_supported_arguments, detect_json1, + add_cors_headers, display_actor, + error_body, escape_css_string, escape_sqlite, find_spatialite, @@ -130,6 +132,7 @@ from .utils import ( redact_keys, row_sql_params_pks, ) +from .tokens import TokenInvalid from .utils.asgi import ( AsgiLifespan, Forbidden, @@ -905,7 +908,9 @@ class Datasette: Verify an API token by trying all registered token handlers. Returns an actor dict from the first handler that recognizes the - token, or None if no handler accepts it. + token, or None if no handler accepts it. A handler may raise + TokenInvalid for a token it recognizes but rejects (bad signature, + expired) - Datasette turns that into a 401 response. """ for token_handler in self._token_handlers(): result = await token_handler.verify_token(self, token) @@ -2887,13 +2892,24 @@ class DatasetteRouter: # Handle authentication default_actor = scope.get("actor") or None actor = None + token_error = None results = pm.hook.actor_from_request(datasette=self.ds, request=request) for result in results: - result = await await_me_maybe(result) + try: + result = await await_me_maybe(result) + except TokenInvalid as ex: + # A presented token was recognized but rejected - fail the + # request with a 401 even if another credential is valid, + # but keep awaiting the remaining coroutines first + if token_error is None: + token_error = ex + continue if result and actor is None: actor = result # Don't break — we must await all coroutines to avoid # "coroutine was never awaited" warnings + if token_error is not None: + return await self.handle_401(request, send, token_error) scope_modifications["actor"] = actor or default_actor scope = dict(scope, **scope_modifications) @@ -2925,6 +2941,17 @@ class DatasetteRouter: except Exception as exception: return await self.handle_exception(request, send, exception) + async def handle_401(self, request, send, exception): + # A presented bearer token was recognized by a handler but rejected. + # Bearer tokens are API credentials, so this is always JSON. + headers = {"www-authenticate": 'Bearer error="invalid_token"'} + if self.ds.cors: + add_cors_headers(headers) + response = Response.json( + error_body([str(exception)], 401), status=401, headers=headers + ) + await response.asgi_send(send) + async def handle_404(self, request, send, exception=None): # If path contains % encoding, redirect to tilde encoding if "%" in request.path: diff --git a/datasette/tokens.py b/datasette/tokens.py index 38a55529..4f905339 100644 --- a/datasette/tokens.py +++ b/datasette/tokens.py @@ -18,6 +18,21 @@ if TYPE_CHECKING: from datasette.app import Datasette +class TokenInvalid(Exception): + """ + Raised by a TokenHandler when a token it recognizes is invalid - + for example a bad signature, malformed payload or expired token. + + Datasette responds to this with an HTTP 401 error. Handlers should + return None instead for tokens they do not recognize at all, so that + other registered handlers get a chance to verify them. + """ + + def __init__(self, message="Invalid token"): + self.message = message + super().__init__(message) + + @dataclasses.dataclass class TokenRestrictions: """ @@ -108,8 +123,12 @@ class TokenHandler: async def verify_token(self, datasette: "Datasette", token: str) -> Optional[dict]: """ - Verify a token and return an actor dict, or None if this handler - does not recognize the token. + Verify a token and return an actor dict. + + Return None if this handler does not recognize the token at all, + so other handlers can try it. Raise TokenInvalid if the token is + recognized but invalid (bad signature, malformed, expired) - the + request will fail with a 401 error. """ raise NotImplementedError @@ -147,29 +166,32 @@ class SignedTokenHandler(TokenHandler): async def verify_token(self, datasette: "Datasette", token: str) -> Optional[dict]: prefix = "dstok_" - if not datasette.setting("allow_signed_tokens"): + if not token.startswith(prefix): + # Not one of our tokens - leave it for other handlers return None + if not datasette.setting("allow_signed_tokens"): + raise TokenInvalid( + "Signed tokens are not enabled for this Datasette instance" + ) + max_signed_tokens_ttl = datasette.setting("max_signed_tokens_ttl") - if not token.startswith(prefix): - return None - raw = token[len(prefix) :] try: decoded = datasette.unsign(raw, namespace="token") except itsdangerous.BadSignature: - return None + raise TokenInvalid("Invalid token signature") if "t" not in decoded: - return None + raise TokenInvalid("Invalid token: no timestamp") created = decoded["t"] if not isinstance(created, int): - return None + raise TokenInvalid("Invalid token: invalid timestamp") duration = decoded.get("d") if duration is not None and not isinstance(duration, int): - return None + raise TokenInvalid("Invalid token: invalid duration") if (duration is None and max_signed_tokens_ttl) or ( duration is not None @@ -180,7 +202,7 @@ class SignedTokenHandler(TokenHandler): if duration: if time.time() - created > duration: - return None + raise TokenInvalid("Token has expired") actor = {"id": decoded["a"], "token": "dstok"} diff --git a/docs/authentication.rst b/docs/authentication.rst index 8101699c..72ac5fa6 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -991,7 +991,9 @@ The ``/-/create-token`` page cannot be accessed by actors that are authenticated Datasette plugins that implement their own form of API token authentication should follow this convention. -You can disable the signed token feature entirely using the :ref:`allow_signed_tokens ` setting. +If a request presents a token that a token handler recognizes but rejects - an invalid signature, a malformed payload or an expired token - Datasette responds with a ``401`` status, the :ref:`standard JSON error format ` and a ``WWW-Authenticate: Bearer error="invalid_token"`` header. This means API clients can distinguish "your token needs to be renewed" (``401``) from "your token does not grant this permission" (``403``). A ``Bearer`` token that no registered handler recognizes at all is ignored, since it may be intended for an authentication plugin. + +You can disable the signed token feature entirely using the :ref:`allow_signed_tokens ` setting. Requests presenting a ``dstok_`` token while the feature is disabled receive a ``401``. .. _authentication_cli_create_token: diff --git a/docs/plugin_hooks.rst b/docs/plugin_hooks.rst index 8614a15c..049cb292 100644 --- a/docs/plugin_hooks.rst +++ b/docs/plugin_hooks.rst @@ -2546,6 +2546,10 @@ The default ``SignedTokenHandler`` uses itsdangerous signed tokens (``dstok_`` p async def verify_token(self, datasette, token): # Look up token in database, return actor dict or None + # if this handler does not recognize the token. Raise + # datasette.TokenInvalid for a token this handler + # recognizes but rejects (revoked, expired) - Datasette + # will respond with a 401 error. ... diff --git a/existing-api.md b/existing-api.md index d600179e..8c6e1c4e 100644 --- a/existing-api.md +++ b/existing-api.md @@ -1156,14 +1156,17 @@ registered via `register_token_handler`; the default is - **Format:** `dstok_` + itsdangerous-signed payload (namespace `token`) containing `a` (actor id), `t` (creation Unix time), optional `d` (duration seconds), optional `_r` (restrictions). -- **Verification** returns no actor when: `allow_signed_tokens` is off, the - signature is invalid, `t` is missing/non-integer, or the token is expired. - The effective duration is `d` capped by `max_signed_tokens_ttl` (default 0 - = no cap; a non-zero setting also imposes a TTL on tokens without `d`). +- **Verification:** a `dstok_`-prefixed token that fails verification — + `allow_signed_tokens` off, invalid signature, missing/non-integer `t`, + malformed `d`, or expired — raises `TokenInvalid`, and the request fails + with **401**, the canonical error body and a + `WWW-Authenticate: Bearer error="invalid_token"` header (even if a valid + `ds_actor` cookie is also present). Tokens with prefixes no registered + handler recognizes are ignored (they may belong to an auth plugin). The + effective duration is `d` capped by `max_signed_tokens_ttl` (default 0 = + no cap; a non-zero setting also imposes a TTL on tokens without `d`). - **Resulting actor:** `{"id": , "token": "dstok"}` plus `"_r"` and - `"token_expires"` when applicable. Invalid/expired tokens silently produce - an anonymous request (no 401) — the failure then surfaces as a 403 from - whatever permission check the request hits. + `"token_expires"` when applicable. **Restrictions (`_r`)** (default_permissions/restrictions.py): diff --git a/stable-api-recommendations.md b/stable-api-recommendations.md index 4fec5a00..273a272c 100644 --- a/stable-api-recommendations.md +++ b/stable-api-recommendations.md @@ -96,19 +96,23 @@ request prefers JSON, mirroring `handle_exception`. completed, SQL is invalid" is defensible but should then not reuse the `ok` key — see §2.) -### 1c. Wrong-status outliers (P2) +### 1c. Wrong-status outliers (P2) — ✅ IMPLEMENTED - ~~Row **delete** write failures return **500** (views/row.py:757) while row **update** write failures return **400** (views/row.py:832-835). Same failure class, different status; pick 400 (or 409 for constraint violations) for both.~~ ✅ **Done** — delete now returns 400, matching update and the rest of the write API. -- Invalid or expired bearer tokens silently degrade the request to anonymous, +- ~~Invalid or expired bearer tokens silently degrade the request to anonymous, so clients see a 403 permission error (or worse, anonymous-permitted data) rather than a 401 (tokens.py:147-193). For 1.0, a malformed/expired `Authorization: Bearer dstok_...` header should produce **401** with a distinguishable error, so clients can tell "renew your token" apart from - "you lack permission". + "you lack permission".~~ ✅ **Done** — token handlers can raise + `TokenInvalid`; Datasette responds 401 with the canonical body and a + `WWW-Authenticate: Bearer error="invalid_token"` header. Unrecognized + token prefixes still fall through to anonymous so auth plugins keep + working. --- @@ -391,7 +395,8 @@ Two details make tiering urgent rather than optional: objects (§2).~~ ✅ Done. 5. ~~Filter `/-/databases.json` by `view-database` or gate it behind `permissions-debug` (§6).~~ ✅ Done. -6. 401 (not silent-anonymous) for invalid/expired bearer tokens (§1c). +6. ~~401 (not silent-anonymous) for invalid/expired bearer tokens (§1c).~~ + ✅ Done. 7. Publish explicit stability tiers, including extras and pagination-token opacity (§9). 8. Resolve the looks-like-a-bug list (§8), especially trusted-query delete diff --git a/tests/test_api_write.py b/tests/test_api_write.py index 17542d4b..a29f5c99 100644 --- a/tests/test_api_write.py +++ b/tests/test_api_write.py @@ -202,8 +202,8 @@ async def test_insert_rows(ds_write, return_rows): "/data/docs/-/insert", {"rows": [{"title": "Test"} for i in range(10)]}, "bad_token", - 403, - ["Permission denied"], + 401, + ["Invalid token signature"], ), ( "/data/docs/-/insert", @@ -410,12 +410,13 @@ async def test_insert_or_upsert_row_errors( }, ) - actor_response = ( - await ds_write.client.get("/-/actor.json", headers=kwargs["headers"]) - ).json() - assert set((actor_response["actor"] or {}).get("_r", {}).get("a") or []) == set( - token_permissions - ) + if special_case != "bad_token": + actor_response = ( + await ds_write.client.get("/-/actor.json", headers=kwargs["headers"]) + ).json() + assert set((actor_response["actor"] or {}).get("_r", {}).get("a") or []) == set( + token_permissions + ) if special_case == "invalid_json": del kwargs["json"] diff --git a/tests/test_auth.py b/tests/test_auth.py index 8e83d397..d2913ecc 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -236,7 +236,9 @@ def test_auth_create_token( @pytest.mark.asyncio async def test_auth_create_token_not_allowed_for_tokens(ds_client): - ds_tok = ds_client.ds.sign({"a": "test", "token": "dstok"}, "token") + ds_tok = ds_client.ds.sign( + {"a": "test", "token": "dstok", "t": int(time.time())}, "token" + ) response = await ds_client.get( "/-/create-token", headers={"Authorization": "Bearer dstok_{}".format(ds_tok)}, @@ -304,8 +306,16 @@ async def test_auth_with_dstok_token(ds_client, scenario, should_work): assert actor["token"] == "dstok" if scenario != "valid_unlimited_token": assert isinstance(actor["token_expires"], int) - else: + elif scenario == "no_token": + # No credentials presented - request proceeds as anonymous assert response.json() == {"ok": True, "actor": None} + else: + # Invalid credentials presented - hard 401 + assert response.status_code == 401 + data = response.json() + assert data["ok"] is False + assert data["status"] == 401 + assert response.headers["www-authenticate"].startswith("Bearer") finally: ds_client.ds._settings["allow_signed_tokens"] = True @@ -339,8 +349,9 @@ def test_cli_create_token(app_client, expires): expected_actor["token_expires"] = details["t"] + expires assert response.json == {"ok": True, "actor": expected_actor} else: - expected_actor = None - assert response.json == {"ok": True, "actor": expected_actor} + # Expired token - hard 401 + assert response.status == 401 + assert response.json["ok"] is False @pytest.mark.asyncio diff --git a/tests/test_error_shape.py b/tests/test_error_shape.py index 21d37cc2..49b2c314 100644 --- a/tests/test_error_shape.py +++ b/tests/test_error_shape.py @@ -18,6 +18,7 @@ https://github.com/simonw/datasette/issues - 1.0 API consistency """ import pytest +import time from datasette.app import Datasette from datasette.utils import sqlite3 @@ -395,3 +396,90 @@ async def test_row_delete_write_failure_is_400(tmp_path_factory): assert "deletes are blocked" in data["error"] finally: ds.close() + + +# Invalid bearer tokens must produce 401, not silent anonymous access + + +@pytest.mark.asyncio +async def test_expired_token_returns_401(ds_error_shape): + token = "dstok_{}".format( + ds_error_shape.sign( + {"a": "root", "t": int(time.time()) - 2000, "d": 1000}, + namespace="token", + ) + ) + response = await ds_error_shape.client.get( + "/-/actor.json", headers={"Authorization": "Bearer {}".format(token)} + ) + data = assert_canonical_error(response, 401) + assert "expired" in data["error"].lower() + assert response.headers["www-authenticate"].startswith("Bearer") + + +@pytest.mark.asyncio +async def test_bad_signature_token_returns_401(ds_error_shape): + response = await ds_error_shape.client.get( + "/-/actor.json", headers={"Authorization": "Bearer dstok_garbage"} + ) + data = assert_canonical_error(response, 401) + assert response.headers["www-authenticate"].startswith("Bearer") + + +@pytest.mark.asyncio +async def test_unrecognized_token_prefix_stays_anonymous(ds_error_shape): + # No registered handler claims this token - it might belong to a + # plugin's actor_from_request hook, so it must not hard-fail + response = await ds_error_shape.client.get( + "/-/actor.json", headers={"Authorization": "Bearer sometoken_abc"} + ) + assert response.status_code == 200 + assert response.json() == {"ok": True, "actor": None} + + +@pytest.mark.asyncio +async def test_valid_token_still_authenticates(ds_error_shape): + token = "dstok_{}".format( + ds_error_shape.sign( + {"a": "root", "t": int(time.time())}, + namespace="token", + ) + ) + response = await ds_error_shape.client.get( + "/-/actor.json", headers={"Authorization": "Bearer {}".format(token)} + ) + assert response.status_code == 200 + assert response.json()["actor"]["id"] == "root" + + +@pytest.mark.asyncio +async def test_bad_token_beats_valid_cookie(ds_error_shape): + # A malformed Authorization header is a hard error even if a valid + # ds_actor cookie is also present + response = await ds_error_shape.client.get( + "/-/actor.json", + headers={"Authorization": "Bearer dstok_garbage"}, + cookies={"ds_actor": ds_error_shape.client.actor_cookie({"id": "root"})}, + ) + assert_canonical_error(response, 401) + + +@pytest.mark.asyncio +async def test_token_when_signed_tokens_disabled_returns_401(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.close() + ds = Datasette([db_path], settings={"allow_signed_tokens": False}) + try: + token = "dstok_{}".format( + ds.sign({"a": "root", "t": int(time.time())}, namespace="token") + ) + response = await ds.client.get( + "/-/actor.json", headers={"Authorization": "Bearer {}".format(token)} + ) + data = assert_canonical_error(response, 401) + assert "not enabled" in data["error"] + finally: + ds.close() diff --git a/tests/test_token_handler.py b/tests/test_token_handler.py index 5c87f577..f5bbfead 100644 --- a/tests/test_token_handler.py +++ b/tests/test_token_handler.py @@ -5,7 +5,12 @@ Tests for the register_token_handler plugin hook. from datasette.app import Datasette from datasette.hookspecs import hookimpl from datasette.plugins import pm -from datasette.tokens import TokenHandler, TokenRestrictions, SignedTokenHandler +from datasette.tokens import ( + TokenHandler, + TokenInvalid, + TokenRestrictions, + SignedTokenHandler, +) import pytest @@ -66,10 +71,10 @@ async def test_verify_token_unknown_returns_none(datasette): @pytest.mark.asyncio -async def test_verify_token_bad_signature_returns_none(datasette): - """verify_token() should return None for tokens with bad signatures.""" - result = await datasette.verify_token("dstok_tampered_data_here") - assert result is None +async def test_verify_token_bad_signature_raises(datasette): + """verify_token() should raise TokenInvalid for tokens with bad signatures.""" + with pytest.raises(TokenInvalid): + await datasette.verify_token("dstok_tampered_data_here") @pytest.mark.asyncio @@ -334,5 +339,6 @@ async def test_signed_tokens_disabled(): ds = Datasette(settings={"allow_signed_tokens": False}) with pytest.raises(ValueError, match="Signed tokens are not enabled"): await ds.create_token("test_actor", handler="signed") - # verify_token should return None rather than raising - assert await ds.verify_token("dstok_anything") is None + # verify_token should raise TokenInvalid for a dstok_ token + with pytest.raises(TokenInvalid, match="not enabled"): + await ds.verify_token("dstok_anything")