diff --git a/plugins/endpoints/api/routes.py b/plugins/endpoints/api/routes.py index 58ff0549..01e4a4ca 100644 --- a/plugins/endpoints/api/routes.py +++ b/plugins/endpoints/api/routes.py @@ -1376,8 +1376,13 @@ def enqueue_endpoint_command(asset_id: int): # Destructive actions (for example system_restart) demand a stricter grant # ON TOP OF endpoints.command, so a remote reboot cannot ride the routine # toggle permission. + # caller_holds, not current_user.haspermission: the question is what THIS + # caller may do, and a scoped personal token is a narrower caller than its + # owner. Asking the owner let a token scoped to endpoints.command queue a + # remote restart, a script run and an app deploy - the exact bypass + # require_permission exists to prevent, reintroduced by checking by hand. elevated = elevated_permission_for(action) - if elevated and not current_user.haspermission(elevated): + if elevated and not caller_holds(elevated): return error_response( ErrorCodes.FORBIDDEN, f'Action {action!r} requires the {elevated} permission', diff --git a/plugins/tools/api/routes.py b/plugins/tools/api/routes.py index 8c31acbe..5abd0df1 100644 --- a/plugins/tools/api/routes.py +++ b/plugins/tools/api/routes.py @@ -39,6 +39,7 @@ from flask_jwt_extended import jwt_required, get_jwt_identity, current_user from shopdb.api import ( db, cache, Setting, client_ip, success_response, error_response, ErrorCodes, + caller_holds, ) from ..models import SecureSecret @@ -155,7 +156,9 @@ def create_secret(): ErrorCodes.VALIDATION_ERROR, 'Too many links from this address. Try again shortly.', http_code=429) - elif not current_user.haspermission('tools.secrets'): + # caller_holds, for the same reason as the endpoints elevated gate: a + # scoped token is a narrower caller than the person who minted it. + elif not caller_holds('tools.secrets'): return error_response( ErrorCodes.FORBIDDEN, 'You do not have permission to create secret links.', diff --git a/shopdb/utils/authz.py b/shopdb/utils/authz.py index e9bb4284..b1cc404a 100644 --- a/shopdb/utils/authz.py +++ b/shopdb/utils/authz.py @@ -102,14 +102,27 @@ def caller_holds(permission_name: str) -> bool: Guests are included on purpose: a signed-out visitor holds whatever the guest role was granted, so the answer must come from the same place require_permission gets it. + + Scoped personal tokens are included for the same reason, and this is the + half that was missing: the answer has to be about the CALLER, and a scoped + token is a narrower caller than its owner. Reading the owner's permissions + let a token scoped to endpoints.command queue a remote restart, because the + elevated check asked the owner rather than the token - the one bypass + require_permission exists to prevent, reintroduced by a route that did its + own check. """ user = currentuser() if user is None: return guestholds(permission_name) try: - return bool(user.haspermission(permission_name)) + if not user.haspermission(permission_name): + return False + patscopes = get_jwt().get('patscopes') except Exception: return False + if patscopes is not None: + return permission_name in patscopes + return True def require_permission(permission_name: str): diff --git a/tests/test_core/test_apitokens.py b/tests/test_core/test_apitokens.py index 1b0bdaef..61fb7313 100644 --- a/tests/test_core/test_apitokens.py +++ b/tests/test_core/test_apitokens.py @@ -417,3 +417,45 @@ def test_update_scopes_ceiling_enforced(client, db): headers=headers) assert response.status_code == 400 assert 'applications.delete' in response.get_json()['data']['error']['message'] + + +# --- a route that does its own permission check -------------------------- +# +# require_permission suspends the admin bypass for a scoped token, and the +# tests above pin that. What they could not see is a route that checks a +# SECOND permission by hand: the endpoints command route asked +# current_user.haspermission(elevated), which reads the OWNER. A token scoped +# to endpoints.command alone could therefore queue system_restart, run_script +# and update_app on a shop-floor PC - the exact bypass this file exists to +# prevent, reintroduced one route at a time. +# +# The fix is in caller_holds, so this tests the helper rather than only the one +# route: it is the answer every hand-rolled check should be using. + +def test_a_scoped_token_cannot_pass_a_hand_rolled_permission_check( + client, db, admin_user, auth_headers): + """The secure-send route checks tools.secrets itself rather than through + require_permission. A scoped token whose owner holds that permission must + still be refused, because the caller is the token.""" + secret = _create_token( + client, auth_headers, + scopes=['applications.create']).get_json()['data']['secret'] + + refused = client.post('/api/tools/secrets', json={'content': 'hello'}, + headers=_pat_headers(secret)) + assert refused.status_code == 403, refused.get_json() + + +def test_an_unscoped_token_still_passes_that_check(client, db, admin_user, + auth_headers): + """The narrowing applies ONLY to scoped tokens - an unscoped one is the + person, and must keep working.""" + secret = _create_token( + client, auth_headers, scopes=None).get_json()['data']['secret'] + + allowed = client.post('/api/tools/secrets', json={'content': 'hello'}, + headers=_pat_headers(secret)) + # Not 403 is the whole assertion: the payload is deliberately incomplete + # (secure send wants ciphertext and iv), so a 400 proves the request got + # PAST the permission check, which is what this is about. + assert allowed.status_code != 403, allowed.get_json()