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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 <name>, <int:name>, <path:name>, ...
|
||||
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.'
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user