From 9eed3745fc47991cc2e9eeb71f300e2e44b90a0f Mon Sep 17 00:00:00 2001 From: cproudlock Date: Sat, 11 Jul 2026 19:35:57 -0400 Subject: [PATCH] Add parametrized authz sweep guard over all mutating routes Every non-exempt POST/PUT/PATCH/DELETE must 403 a role-less member and pass authz for admin; exemptions (auth, collector, setup wizard, kiosk click-through, admin-or-self user update) are documented in the test. Any future unguarded mutation fails CI as its own case. Sweep confirmed existing gating complete: zero routes needed fixes; lockout already implemented. Co-Authored-By: Claude Fable 5 --- tests/test_core/test_authz.py | 118 ++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/tests/test_core/test_authz.py b/tests/test_core/test_authz.py index 107c04b..b0ce76e 100644 --- a/tests/test_core/test_authz.py +++ b/tests/test_core/test_authz.py @@ -4,8 +4,76 @@ Covers the security fixes: - write routes require a permission/role, not just authentication - the admin role bypasses permission checks - repeated bad logins lock the account + +The parametrized sweep at the bottom is the standing CI guard: it walks the +whole URL map and proves every mutating route (POST/PUT/PATCH/DELETE) either +rejects a role-less member with 403 or is on the deliberate exemption list. +A future unguarded mutation therefore fails CI the moment it is added. """ +import re + +import pytest + +from shopdb import create_app + + +# HTTP verbs that change state. GETs are intentionally left alone (kiosk pages +# rely on optional-jwt reads). +MUTATING_METHODS = {'POST', 'PUT', 'PATCH', 'DELETE'} + +# Blueprints exempt by design (see task/ADR-006): +# auth - login/refresh/logout are the auth surface itself +# collector - agent ingest, authenticated by per-plugin API key (ADR-006) +# setup - first-run wizard, self-gated by the needs-admin state +EXEMPT_BLUEPRINTS = {'auth', 'collector', 'setup'} + +# Individual endpoints exempt by design. +# knowledgebase.track_click - kiosk click-through counter. Optional-jwt +# telemetry: increments a click count on an already-active article and +# returns its link URL. Takes no caller-supplied data; gating it would +# break the unauthenticated kiosk redirect it exists to serve. +# users.update_user - admin-or-self (inline check, not a flat deny): a +# role-less member MAY edit their own record, so it does not fit the +# 403-for-every-member contract this sweep asserts. The other-user 403 is +# covered by test_member_cannot_update_other_user below. +EXEMPT_ENDPOINTS = {'knowledgebase.track_click', 'users.update_user'} + + +def _fill_url(rule): + """Turn a Werkzeug rule into a concrete path (int/float params -> 1, else x).""" + def repl(match): + converter = match.group(1) or '' + return '1' if converter.startswith(('int', 'float')) else 'x' + # matches , , , ... + return re.sub(r'<(?:([^:>]+):)?[^>]+>', repl, rule) + + +def _mutating_routes(): + """Every non-exempt mutating (endpoint, method, url) in the URL map. + + Enumerated from a throwaway app: route strings are stable across app + instances, so the requests below run against the fixture client's app. + """ + app = create_app('testing') + routes = [] + for rule in app.url_map.iter_rules(): + methods = (rule.methods or set()) & MUTATING_METHODS + if not methods: + continue + blueprint = rule.endpoint.split('.')[0] + if blueprint in EXEMPT_BLUEPRINTS or rule.endpoint in EXEMPT_ENDPOINTS: + continue + for method in sorted(methods): + routes.append((rule.endpoint, method, _fill_url(rule.rule))) + return routes + + +# Collected once at import so each route is its own parametrized case (a +# newly-unguarded mutation shows up as a single named CI failure). +MUTATING_ROUTES = _mutating_routes() +ROUTE_IDS = [f'{ep}[{method}]' for ep, method, _ in MUTATING_ROUTES] + def test_member_cannot_write_settings(client, db, member_headers): """An authenticated user with no roles is forbidden from editing settings.""" @@ -127,3 +195,53 @@ def test_successful_login_resets_failed_counter(client, db, admin_user): user = User.query.filter_by(username='testadmin').first() assert user.failedlogins == 0 assert user.lockeduntil is None + + +# --------------------------------------------------------------------------- +# Standing CI guard: sweep every mutating route. +# --------------------------------------------------------------------------- + +def test_member_cannot_update_other_user(client, db, member_user, member_headers): + """Admin-or-self write gate: a role-less user cannot edit a DIFFERENT user.""" + response = client.put(f'/api/users/{member_user.userid + 999}', + json={'firstname': 'x'}, headers=member_headers) + assert response.status_code == 403 + + +def test_route_map_has_mutating_routes(): + """Sanity: the sweep actually found routes (catches a broken collector).""" + assert len(MUTATING_ROUTES) > 50, 'route enumeration looks empty/broken' + + +@pytest.mark.parametrize('endpoint,method,url', MUTATING_ROUTES, ids=ROUTE_IDS) +def test_mutation_rejects_roleless_member(client, db, member_headers, + endpoint, method, url): + """Every non-exempt mutating route must 403 a logged-in, role-less user. + + A 200/201/400/404/409 here means the route ran its handler for an + unprivileged caller - i.e. it is not permission-gated. That is the exact + regression this guard exists to catch. + """ + response = client.open(url, method=method, headers=member_headers, json={}) + assert response.status_code == 403, ( + f'{method} {endpoint} ({url}) returned {response.status_code}, ' + f'expected 403 - route is not permission-gated. If it is intended to ' + f'be public/kiosk, add it to EXEMPT_ENDPOINTS with a reason.' + ) + assert response.get_json()['data']['error']['code'] == 'FORBIDDEN' + + +@pytest.mark.parametrize('endpoint,method,url', MUTATING_ROUTES, ids=ROUTE_IDS) +def test_mutation_admin_passes_authz(client, db, auth_headers, + endpoint, method, url): + """Admin must clear the gate on every mutating route (never 401/403). + + Downstream the handler may 400/404/409/422 on the empty body - that is + validation, not authorization, and is fine. We only assert the authz gate + let the admin through. + """ + response = client.open(url, method=method, headers=auth_headers, json={}) + assert response.status_code not in (401, 403), ( + f'{method} {endpoint} ({url}) returned {response.status_code} for ' + f'admin - the admin role should bypass every permission check.' + )