From 3d3395d3ad1c85263b4b95a6bcc1613118cfce14 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 17:34:32 -0400 Subject: [PATCH 01/12] ci(release): cap the tarball build at 30 minutes The amd64 tarball job on run 32270325886 hung inside apt-get update (unreachable azure apt mirror on the runner) and sat for GitHub's 6-hour default job timeout. Normal runs finish in ~13 minutes. Co-Authored-By: Claude Fable 5 --- .github/workflows/release.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index a9c6c184..ef820aea 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -77,6 +77,9 @@ jobs: name: Build Release Tarball runs-on: ${{ matrix.runner }} needs: version + # Past runs finish in ~13 min; without this cap a hung apt mirror once + # held the job for GitHub's 6-hour default. + timeout-minutes: 30 strategy: fail-fast: true matrix: From a92d40a042af282bb646315c1b68bf6591531b4e Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 19:16:42 -0400 Subject: [PATCH 02/12] fix(workspaces): require admin role to create workspaces POST /api/v1/workspaces/ was gated only by @jwt_required(), so any authenticated user - including the read-only viewer role - could create a workspace, automatically become its owner, then add arbitrary members and mint workspace-scoped API keys (GHSA-r4q7-x795-vr4j, CWE-862). Add @admin_required, matching the owner/admin checks every other mutating workspace route already enforces, plus a regression test asserting viewer and developer JWTs get 403 while an admin still creates. Co-authored-by: Alex <134992700+campguyuz@users.noreply.github.com> --- backend/app/api/workspaces.py | 5 ++++- backend/tests/test_workspace_scope.py | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/backend/app/api/workspaces.py b/backend/app/api/workspaces.py index af439cae..040d55fb 100644 --- a/backend/app/api/workspaces.py +++ b/backend/app/api/workspaces.py @@ -5,7 +5,9 @@ from app.services.audit_service import AuditService from app.models.audit_log import AuditLog # The workspace guards now live in middleware/rbac.py so any resource blueprint # can reuse them (plan 19 Decision 2). Re-exported here for existing callers. -from app.middleware.rbac import get_current_user, require_workspace_access, require_workspace_role +from app.middleware.rbac import ( + admin_required, get_current_user, require_workspace_access, require_workspace_role, +) workspaces_bp = Blueprint('workspaces', __name__) @@ -50,6 +52,7 @@ def get_workspace(workspace_id): @workspaces_bp.route('/', methods=['POST']) @jwt_required() +@admin_required def create_workspace(): user = get_current_user() data = request.get_json() diff --git a/backend/tests/test_workspace_scope.py b/backend/tests/test_workspace_scope.py index 8eb828b9..301c6cc9 100644 --- a/backend/tests/test_workspace_scope.py +++ b/backend/tests/test_workspace_scope.py @@ -509,3 +509,22 @@ def test_list_workspaces_surfaces_effective_role(app, client): rows = {w['name']: w for w in r.get_json()['workspaces']} assert rows['LW']['my_role'] == 'viewer' assert rows['LW']['my_effective_role'] == 'viewer' + + +def test_create_workspace_requires_admin(app, client): + """GHSA-r4q7-x795-vr4j: POST /api/v1/workspaces/ was gated only by + @jwt_required(), so a read-only viewer could create a workspace, become its + owner, then add arbitrary members and mint workspace API keys. It now + carries @admin_required like every other panel-wide create.""" + from app import db + + for role in ('viewer', 'developer'): + u = _mk_user(db, f'cw_{role}', role) + r = client.post('/api/v1/workspaces/', headers=_token(u.id), + json={'name': f'cw-{role}', 'slug': f'cw-{role}'}) + assert r.status_code == 403, f'{role} reached workspace creation: {r.status_code}' + + admin = _mk_user(db, 'cw_admin', 'admin') + r = client.post('/api/v1/workspaces/', headers=_token(admin.id), + json={'name': 'cw-ok', 'slug': 'cw-ok'}) + assert r.status_code == 201 From 031142111d66b0e64ac729f5397b3a65433752c8 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 21:44:17 -0400 Subject: [PATCH 03/12] fix(authz): give every mutating route an explicit authorization decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-r4q7-x795-vr4j was one forgotten check, but the audit behind it found the same shape across the surface: `@jwt_required()` authenticates, it does not authorize, and ServerKit has no default-deny to catch the difference. This closes the routes that audit left open — fleet-monitor alert acknowledge/resolve, recycle-bin restore, nginx test/diff, registrar sync/test, server-template drift checks, uptime tracking on/off, server ping and remote compose ps/logs, refresh-capabilities, template test-db-connection, deploy branches, buildpack detect — plus three inline gaps: a viewer could widen their own API key's scopes, the demo-deploy flag was doing double duty as an authorization one, and tagging a shared resource had no write check at all while attaching one did. `_ensure_group_mutable` only ever blocked *system* queue groups, so every queue_bus mutation now also calls `_ensure_group_accessible` (owner-or-admin) — renaming, draining or deleting another user's group was open to any authenticated caller. The rbac policy decorators authenticate through `auth_required()`, so they replace `@jwt_required()` rather than stack under it; stacking rejects API-key callers before the policy can authorize them (tests/test_auth_decorator_boundaries.py enforces this). Where a route then needed the caller it reads `get_current_user()` — `get_jwt_identity()` raises for an API-key request. The decorators also carry a `_sk_authz` marker so a static test can see the decision from the source. `migrations.get_migration_history` moves to `require_admin_user()` for the same reason its siblings did: the inline `User.ROLE_ADMIN` check it kept was already reading two names this module no longer imports, so the route raised NameError for every caller, admin included. Co-Authored-By: Claude Opus 5 (1M context) --- backend/app/api/api_keys.py | 6 ++++-- backend/app/api/auth.py | 8 ++------ backend/app/api/buildpacks.py | 10 +++++++--- backend/app/api/deploy.py | 4 ++-- backend/app/api/deployment_jobs.py | 4 ++++ backend/app/api/docker.py | 8 ++++---- backend/app/api/fleet_monitor.py | 13 ++++++++----- backend/app/api/migrations.py | 15 ++++----------- backend/app/api/monitors.py | 17 +++++++++-------- backend/app/api/nginx_advanced.py | 10 ++++++++++ backend/app/api/queue_bus.py | 8 ++++++++ backend/app/api/recycle_bin.py | 2 +- backend/app/api/registrars.py | 6 +++--- backend/app/api/server_templates.py | 4 ++-- backend/app/api/servers.py | 8 ++++---- backend/app/api/shared_resources.py | 7 +++++++ backend/app/api/templates.py | 4 ++-- backend/app/api/uptime.py | 5 +++-- backend/app/api/workspaces.py | 1 - backend/app/middleware/rbac.py | 8 ++++++++ 20 files changed, 92 insertions(+), 56 deletions(-) diff --git a/backend/app/api/api_keys.py b/backend/app/api/api_keys.py index 88f77142..5801c362 100644 --- a/backend/app/api/api_keys.py +++ b/backend/app/api/api_keys.py @@ -93,9 +93,11 @@ def get_key(key_id): @jwt_required() def update_key(key_id): """Update API key metadata.""" + # Same bar as create_key: a viewer (e.g. demoted after key creation) must + # not widen an existing key's scopes/tier. user = get_current_user() - if not user: - return jsonify({'error': 'User not found'}), 404 + if not user or not user.is_developer: + return jsonify({'error': 'Developer access required'}), 403 data = request.get_json() or {} api_key = ApiKeyService.update_key( diff --git a/backend/app/api/auth.py b/backend/app/api/auth.py index 677545d6..92dbc08c 100644 --- a/backend/app/api/auth.py +++ b/backend/app/api/auth.py @@ -12,7 +12,7 @@ from flask_jwt_extended import ( from app import db, limiter from app.models import User, AuditLog, SystemSettings # Aliased: this module already has a `get_current_user` route handler. -from app.middleware.rbac import admin_required, get_current_user as get_request_user +from app.middleware.rbac import admin_required, get_current_user as get_request_user, require_admin_user from app.services.settings_service import SettingsService from app.services.audit_service import AuditService from app.services import login_link_service @@ -168,11 +168,7 @@ ALLOWED_USE_CASES = {'wordpress', 'web-apps', 'self-hosted', 'devops'} @jwt_required() def complete_onboarding(): """Complete the onboarding wizard and mark setup as done.""" - current_user_id = get_jwt_identity() - user = User.query.get(current_user_id) - - if not user or user.role != User.ROLE_ADMIN: - return jsonify({'error': 'Admin access required'}), 403 + user = require_admin_user() data = request.get_json() or {} use_cases = data.get('use_cases', []) diff --git a/backend/app/api/buildpacks.py b/backend/app/api/buildpacks.py index edfbf2c6..f9c74b37 100644 --- a/backend/app/api/buildpacks.py +++ b/backend/app/api/buildpacks.py @@ -17,11 +17,12 @@ import shutil import tempfile from flask import Blueprint, request, jsonify -from flask_jwt_extended import jwt_required, get_jwt_identity +from flask_jwt_extended import jwt_required from app.services.buildpack_service import BuildpackService from app.services.git_service import GitService from app.services.source_connection_service import SourceConnectionService +from app.middleware.rbac import developer_required, get_current_user buildpacks_bp = Blueprint('buildpacks', __name__) @@ -36,7 +37,7 @@ def _build_response(plan: dict, app_name: str = 'app') -> dict: @buildpacks_bp.route('/detect', methods=['POST']) -@jwt_required() +@developer_required def detect(): """Detect the build pack for a repository. @@ -46,7 +47,10 @@ def detect(): cloned shallowly to a temp workspace, inspected, and the workspace removed. """ data = request.get_json() or {} - current_user_id = get_jwt_identity() + # Through the one identity door: @developer_required admits API-key + # callers, for whom reading the JWT directly raises. + caller = get_current_user() + current_user_id = caller.id if caller else None explicit_path = (data.get('path') or '').strip() or None repo_url = (data.get('repo_url') or '').strip() diff --git a/backend/app/api/deploy.py b/backend/app/api/deploy.py index ae30bde6..6b269059 100644 --- a/backend/app/api/deploy.py +++ b/backend/app/api/deploy.py @@ -6,7 +6,7 @@ import os from flask import Blueprint, request, jsonify from flask_jwt_extended import jwt_required, get_jwt_identity from app import paths -from app.middleware.rbac import admin_required, app_access_tier +from app.middleware.rbac import admin_required, app_access_tier, developer_required from app.models import User, Application from app.services.git_service import GitService from app.services.resource_grant_service import ResourceGrantService @@ -225,7 +225,7 @@ def get_branches(app_id): @deploy_bp.route('/branches', methods=['POST']) -@jwt_required() +@developer_required def get_branches_from_url(): """Get list of branches from a repository URL (before cloning).""" data = request.get_json() diff --git a/backend/app/api/deployment_jobs.py b/backend/app/api/deployment_jobs.py index 1a5f5169..2612c5bf 100644 --- a/backend/app/api/deployment_jobs.py +++ b/backend/app/api/deployment_jobs.py @@ -57,6 +57,10 @@ def simulate_deployment(): synchronously (tests).""" if not current_app.config.get('DEMO_DEPLOYS_ENABLED'): return jsonify({'error': 'Not found'}), 404 + # Even with the demo flag on, creating jobs is not a viewer capability. + user = get_current_user() + if not user or not user.is_developer: + return jsonify({'error': 'Developer access required'}), 403 from flask_jwt_extended import get_jwt_identity from app.services.demo_deploy_service import DemoDeployService diff --git a/backend/app/api/docker.py b/backend/app/api/docker.py index a39eac63..7d1f17e8 100644 --- a/backend/app/api/docker.py +++ b/backend/app/api/docker.py @@ -402,7 +402,7 @@ def compose_down(): @docker_bp.route('/compose/ps', methods=['POST']) -@jwt_required() +@admin_required def compose_ps(): """List Docker Compose services.""" data = request.get_json() @@ -415,7 +415,7 @@ def compose_ps(): @docker_bp.route('/compose/logs', methods=['POST']) -@jwt_required() +@admin_required def compose_logs(): """Get Docker Compose logs.""" data = request.get_json() @@ -633,7 +633,7 @@ def compose_pull(): @docker_bp.route('/compose/validate', methods=['POST']) -@jwt_required() +@admin_required def compose_validate(): """Validate a Docker Compose file.""" data = request.get_json() @@ -646,7 +646,7 @@ def compose_validate(): @docker_bp.route('/compose/config', methods=['POST']) -@jwt_required() +@admin_required def compose_config(): """Get parsed Docker Compose configuration.""" data = request.get_json() diff --git a/backend/app/api/fleet_monitor.py b/backend/app/api/fleet_monitor.py index 7c781273..c3c580bd 100644 --- a/backend/app/api/fleet_monitor.py +++ b/backend/app/api/fleet_monitor.py @@ -8,10 +8,10 @@ fleet search, and metrics export. import os from flask import Blueprint, request, jsonify, Response -from flask_jwt_extended import jwt_required, get_jwt_identity +from flask_jwt_extended import jwt_required from app.services.fleet_monitor_service import fleet_monitor_service -from app.middleware.rbac import admin_required +from app.middleware.rbac import admin_required, get_current_user fleet_monitor_bp = Blueprint('fleet_monitor', __name__) @@ -58,10 +58,13 @@ def get_alerts(): @fleet_monitor_bp.route('/alerts//acknowledge', methods=['POST']) -@jwt_required() +@admin_required def acknowledge_alert(alert_id): """Acknowledge an active alert.""" - user_id = get_jwt_identity() + # Through the one identity door: @admin_required admits API-key callers, + # for whom reading the JWT directly raises. + caller = get_current_user() + user_id = caller.id if caller else None success = fleet_monitor_service.acknowledge_alert(alert_id, user_id) if not success: return jsonify({'error': 'Cannot acknowledge alert'}), 400 @@ -69,7 +72,7 @@ def acknowledge_alert(alert_id): @fleet_monitor_bp.route('/alerts//resolve', methods=['POST']) -@jwt_required() +@admin_required def resolve_alert(alert_id): """Resolve an alert.""" success = fleet_monitor_service.resolve_alert(alert_id) diff --git a/backend/app/api/migrations.py b/backend/app/api/migrations.py index 58ba5d25..a49f5fba 100644 --- a/backend/app/api/migrations.py +++ b/backend/app/api/migrations.py @@ -1,9 +1,8 @@ from flask import Blueprint, jsonify, current_app from flask_jwt_extended import jwt_required -from app.models import User from app.services.migration_service import MigrationService -from app.middleware.rbac import get_current_user +from app.middleware.rbac import require_admin_user migrations_bp = Blueprint('migrations', __name__) @@ -19,9 +18,7 @@ def get_migration_status(): @jwt_required() def create_backup(): """Create a database backup before applying migrations. Admin only.""" - user = get_current_user() - if not user or user.role != User.ROLE_ADMIN: - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() result = MigrationService.create_backup(current_app) if result['success']: @@ -33,9 +30,7 @@ def create_backup(): @jwt_required() def apply_migrations(): """Apply all pending migrations. Admin only.""" - user = get_current_user() - if not user or user.role != User.ROLE_ADMIN: - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() result = MigrationService.apply_migrations(current_app) if result['success']: @@ -47,9 +42,7 @@ def apply_migrations(): @jwt_required() def get_migration_history(): """Return all migration revisions. Admin only.""" - user = get_current_user() - if not user or user.role != User.ROLE_ADMIN: - return jsonify({'error': 'Admin access required'}), 403 + require_admin_user() history = MigrationService.get_migration_history(current_app) return jsonify({'revisions': history}), 200 diff --git a/backend/app/api/monitors.py b/backend/app/api/monitors.py index 330f3c0a..a4493572 100644 --- a/backend/app/api/monitors.py +++ b/backend/app/api/monitors.py @@ -10,6 +10,7 @@ from flask import Blueprint, request, jsonify from flask_jwt_extended import jwt_required from app.exceptions import NotFoundError +from app.middleware.rbac import permission_required from app.services.monitor_service import MonitorService monitors_bp = Blueprint('monitors', __name__) @@ -49,7 +50,7 @@ def get_stats(): @monitors_bp.route('', methods=['POST']) @monitors_bp.route('/', methods=['POST']) -@jwt_required() +@permission_required('monitoring', 'write') def create_monitor(): data = request.get_json() or {} monitor = MonitorService.create(data) @@ -66,7 +67,7 @@ def get_monitor(monitor_id): @monitors_bp.route('/', methods=['PATCH', 'PUT']) -@jwt_required() +@permission_required('monitoring', 'write') def update_monitor(monitor_id): data = request.get_json() or {} monitor = MonitorService.update(monitor_id, data) @@ -76,7 +77,7 @@ def update_monitor(monitor_id): @monitors_bp.route('/', methods=['DELETE']) -@jwt_required() +@permission_required('monitoring', 'write') def delete_monitor(monitor_id): if not MonitorService.delete(monitor_id): return {'error': 'Monitor not found'}, 404 @@ -84,7 +85,7 @@ def delete_monitor(monitor_id): @monitors_bp.route('//check', methods=['POST']) -@jwt_required() +@permission_required('monitoring', 'write') def run_check(monitor_id): """Probe now, out of band from the scheduler.""" hc = MonitorService.run_check(monitor_id) @@ -95,7 +96,7 @@ def run_check(monitor_id): @monitors_bp.route('//pause', methods=['POST']) -@jwt_required() +@permission_required('monitoring', 'write') def set_paused(monitor_id): data = request.get_json() or {} monitor = MonitorService.set_paused(monitor_id, data.get('paused', True)) @@ -148,7 +149,7 @@ def list_incidents(): @monitors_bp.route('/incidents', methods=['POST']) -@jwt_required() +@permission_required('monitoring', 'write') def create_incident(): data = request.get_json() or {} if not data.get('title'): @@ -158,7 +159,7 @@ def create_incident(): @monitors_bp.route('/incidents/', methods=['PATCH', 'PUT']) -@jwt_required() +@permission_required('monitoring', 'write') def update_incident(incident_id): incident = MonitorService.update_incident(incident_id, request.get_json() or {}) if not incident: @@ -167,7 +168,7 @@ def update_incident(incident_id): @monitors_bp.route('/incidents/', methods=['DELETE']) -@jwt_required() +@permission_required('monitoring', 'write') def delete_incident(incident_id): if not MonitorService.delete_incident(incident_id): return {'error': 'Incident not found'}, 404 diff --git a/backend/app/api/nginx_advanced.py b/backend/app/api/nginx_advanced.py index 67010785..48d92a8d 100644 --- a/backend/app/api/nginx_advanced.py +++ b/backend/app/api/nginx_advanced.py @@ -33,6 +33,9 @@ def create_proxy(): @nginx_advanced_bp.route('/test', methods=['POST']) @jwt_required() def test_config(): + user = get_current_user() + if not user or not user.is_admin: + return jsonify({'error': 'Admin access required'}), 403 result = NginxAdvancedService.test_config() return jsonify(result) @@ -50,12 +53,19 @@ def reload(): @nginx_advanced_bp.route('/diff', methods=['POST']) @jwt_required() def preview_diff(): + # Admin-only: the diff includes the CURRENT vhost file's contents, so this + # is a file read as much as a preview. + user = get_current_user() + if not user or not user.is_admin: + return jsonify({'error': 'Admin access required'}), 403 data = request.get_json() or {} domain = data.get('domain') new_config = data.get('config', '') if not domain: return jsonify({'error': 'domain required'}), 400 result = NginxAdvancedService.preview_diff(domain, new_config) + if 'error' in result: + return jsonify(result), 400 return jsonify(result) diff --git a/backend/app/api/queue_bus.py b/backend/app/api/queue_bus.py index 0fc758ee..2dccf51d 100644 --- a/backend/app/api/queue_bus.py +++ b/backend/app/api/queue_bus.py @@ -134,6 +134,7 @@ def get_group(group_slug): def update_group(group_slug): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) data = request.get_json() or {} group = QueueBusService.update_group( slug=group_slug, @@ -151,6 +152,7 @@ def update_group(group_slug): def delete_group(group_slug): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) result = QueueBusService.delete_group(group_slug) return jsonify(result), 200 except Exception as e: @@ -178,6 +180,7 @@ def list_queues(group_slug): def create_queue(group_slug): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) data = request.get_json() or {} slug = data.get('slug') name = data.get('name') @@ -216,6 +219,7 @@ def get_queue(group_slug, queue_slug): def update_queue(group_slug, queue_slug): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) data = request.get_json() or {} queue = QueueBusService.update_queue( group_slug=group_slug, @@ -234,6 +238,7 @@ def update_queue(group_slug, queue_slug): def delete_queue(group_slug, queue_slug): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) result = QueueBusService.delete_queue(group_slug, queue_slug) return jsonify(result), 200 except Exception as e: @@ -269,6 +274,7 @@ def list_messages(group_slug, queue_slug): def send_message(group_slug, queue_slug): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) data = request.get_json() or {} payload = data.get('payload') priority = data.get('priority', 0) @@ -358,6 +364,7 @@ def fail_message(group_slug, queue_slug, message_id): def requeue_message(group_slug, queue_slug, message_id): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) message = QueueBusService.requeue(group_slug, queue_slug, message_id) return jsonify({'message': message}), 200 except Exception as e: @@ -369,6 +376,7 @@ def requeue_message(group_slug, queue_slug, message_id): def delete_message(group_slug, queue_slug, message_id): try: _ensure_group_mutable(group_slug) + _ensure_group_accessible(group_slug) result = QueueBusService.delete_message(group_slug, queue_slug, message_id) return jsonify(result), 200 except Exception as e: diff --git a/backend/app/api/recycle_bin.py b/backend/app/api/recycle_bin.py index 4ebdd5fd..9fc1b89d 100644 --- a/backend/app/api/recycle_bin.py +++ b/backend/app/api/recycle_bin.py @@ -31,7 +31,7 @@ def list_deleted(): @recycle_bin_bp.route('///restore', methods=['POST']) -@jwt_required() +@admin_required def restore(kind, record_id): try: item, err = recycle_bin_service.restore(kind, record_id) diff --git a/backend/app/api/registrars.py b/backend/app/api/registrars.py index dcf6317b..3f62a49a 100644 --- a/backend/app/api/registrars.py +++ b/backend/app/api/registrars.py @@ -9,7 +9,7 @@ from flask import Blueprint, request, jsonify from flask_jwt_extended import jwt_required from app.services.registrar_service import RegistrarService -from app.middleware.rbac import get_current_user +from app.middleware.rbac import admin_required, get_current_user registrars_bp = Blueprint('registrars', __name__) @@ -54,7 +54,7 @@ def delete_connection(cid): @registrars_bp.route('/connections//test', methods=['POST']) -@jwt_required() +@admin_required def test_connection(cid): conn = RegistrarService.get_connection(cid) if not conn: @@ -71,7 +71,7 @@ def list_domains(): @registrars_bp.route('/sync', methods=['POST']) -@jwt_required() +@admin_required def sync(): """Force-refresh the portfolio (and stamp last_synced_at).""" return jsonify({'domains': RegistrarService.sync_now()}) diff --git a/backend/app/api/server_templates.py b/backend/app/api/server_templates.py index 8c7d0ed4..6e1c9fc6 100644 --- a/backend/app/api/server_templates.py +++ b/backend/app/api/server_templates.py @@ -3,7 +3,7 @@ from flask_jwt_extended import jwt_required from app.services.server_template_service import ServerTemplateService from app.services.audit_service import AuditService from app.models.audit_log import AuditLog -from app.middleware.rbac import get_current_user +from app.middleware.rbac import admin_required, get_current_user server_templates_bp = Blueprint('server_templates', __name__) @@ -150,7 +150,7 @@ def unassign(assignment_id): @server_templates_bp.route('/assignments//check', methods=['POST']) -@jwt_required() +@admin_required def check_drift(assignment_id): assignment = ServerTemplateService.check_drift(assignment_id) if not assignment: diff --git a/backend/app/api/servers.py b/backend/app/api/servers.py index 9458f29b..ed93e71d 100644 --- a/backend/app/api/servers.py +++ b/backend/app/api/servers.py @@ -795,7 +795,7 @@ def get_server_status(server_id): @servers_bp.route('//ping', methods=['POST']) -@jwt_required() +@developer_required def ping_server(server_id): """Force a ping/health check on a server""" server = Server.query.get(server_id) @@ -1521,7 +1521,7 @@ def list_remote_compose_projects(server_id): @servers_bp.route('//docker/compose/ps', methods=['POST']) -@jwt_required() +@developer_required def remote_compose_ps(server_id): """List containers for a compose project""" user_id = get_jwt_identity() @@ -1591,7 +1591,7 @@ def remote_compose_down(server_id): @servers_bp.route('//docker/compose/logs', methods=['POST']) -@jwt_required() +@developer_required def remote_compose_logs(server_id): """Get logs from a compose project""" user_id = get_jwt_identity() @@ -2686,7 +2686,7 @@ def remote_cloudflared_delete(server_id, tunnel_ref): # Capabilities message so all panel listeners stay in sync. @servers_bp.route('//refresh-capabilities', methods=['POST']) -@jwt_required() +@developer_required def remote_refresh_capabilities(server_id): user_id = get_jwt_identity() result = dispatch_agent_command( diff --git a/backend/app/api/shared_resources.py b/backend/app/api/shared_resources.py index 6f5eeb3f..5a164a47 100644 --- a/backend/app/api/shared_resources.py +++ b/backend/app/api/shared_resources.py @@ -208,6 +208,9 @@ def add_tag(): missing = _require(data, 'resource_type', 'resource_id', 'tag') if missing: return _bad(f'{missing} is required') + denied = _check_resource_write(data['resource_type'], data['resource_id']) + if denied: + return denied try: row = SharedResourceService.add_tag( data['resource_type'], data['resource_id'], data['tag'] @@ -231,6 +234,10 @@ def remove_tag(): if not resource_type or resource_id in (None, '') or not tag: return _bad('resource_type, resource_id and tag are required') + denied = _check_resource_write(resource_type, resource_id) + if denied: + return denied + removed = SharedResourceService.remove_tag(resource_type, resource_id, tag) return jsonify({'removed': removed}) diff --git a/backend/app/api/templates.py b/backend/app/api/templates.py index 2079ee48..47c3ea6e 100644 --- a/backend/app/api/templates.py +++ b/backend/app/api/templates.py @@ -12,7 +12,7 @@ Provides REST endpoints for: from flask import Blueprint, current_app, request, jsonify from flask_jwt_extended import jwt_required, get_jwt_identity -from app.middleware.rbac import admin_required +from app.middleware.rbac import admin_required, developer_required from app.models import User, Application from app.services.deployment_job_service import DeploymentJobService from app.services.template_service import TemplateService @@ -451,7 +451,7 @@ def validate_installation(): @templates_bp.route('/test-db-connection', methods=['POST']) -@jwt_required() +@developer_required def test_db_connection(): """Test database connection before template installation. diff --git a/backend/app/api/uptime.py b/backend/app/api/uptime.py index 2e5a06cf..18196407 100644 --- a/backend/app/api/uptime.py +++ b/backend/app/api/uptime.py @@ -8,6 +8,7 @@ from flask import Blueprint, request, jsonify from flask_jwt_extended import jwt_required from app.services.uptime_service import UptimeService +from app.middleware.rbac import admin_required uptime_bp = Blueprint('uptime', __name__) @@ -66,7 +67,7 @@ def get_uptime_history(): @uptime_bp.route('/tracking/start', methods=['POST']) -@jwt_required() +@admin_required def start_tracking(): """Start uptime tracking.""" result = UptimeService.start_tracking() @@ -76,7 +77,7 @@ def start_tracking(): @uptime_bp.route('/tracking/stop', methods=['POST']) -@jwt_required() +@admin_required def stop_tracking(): """Stop uptime tracking.""" result = UptimeService.stop_tracking() diff --git a/backend/app/api/workspaces.py b/backend/app/api/workspaces.py index 040d55fb..ea6a54f3 100644 --- a/backend/app/api/workspaces.py +++ b/backend/app/api/workspaces.py @@ -51,7 +51,6 @@ def get_workspace(workspace_id): @workspaces_bp.route('/', methods=['POST']) -@jwt_required() @admin_required def create_workspace(): user = get_current_user() diff --git a/backend/app/middleware/rbac.py b/backend/app/middleware/rbac.py index 098d359e..3b82c851 100644 --- a/backend/app/middleware/rbac.py +++ b/backend/app/middleware/rbac.py @@ -62,6 +62,10 @@ def require_role(*allowed_roles): if user.role not in allowed_roles: return jsonify({'error': 'Insufficient permissions'}), 403 return fn(*args, **kwargs) + # Marks this view as carrying an explicit authorization decision, so the + # default-deny sweep test (tests/test_route_authz_sweep.py) recognizes it + # as gated rather than flagging it as an unguarded mutating route. + wrapper._sk_authz = ('require_role', allowed_roles) return wrapper return decorator @@ -86,6 +90,7 @@ def admin_required(fn): if not user.is_admin: return jsonify({'error': 'Admin access required'}), 403 return fn(*args, **kwargs) + wrapper._sk_authz = ('admin_required',) return wrapper @@ -136,6 +141,7 @@ def developer_required(fn): if not user.is_developer: return jsonify({'error': 'Developer access required'}), 403 return fn(*args, **kwargs) + wrapper._sk_authz = ('developer_required',) return wrapper @@ -162,6 +168,7 @@ def permission_required(feature, level='read'): 'error': f'Permission denied: {level} access to {feature} required' }), 403 return fn(*args, **kwargs) + wrapper._sk_authz = ('permission_required', feature, level) return wrapper return decorator @@ -278,5 +285,6 @@ def require_app_member(min_role='viewer', arg='app_id'): return jsonify({'error': 'Access denied'}), 403 g.current_application = application return fn(*args, **kwargs) + wrapper._sk_authz = ('require_app_member', min_role) return wrapper return decorator From 47ec555915e408853208ee211372b2914bf338ef Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 21:44:28 -0400 Subject: [PATCH 04/12] fix(security): a webhook with no signature is as unauthenticated as a wrong one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inbound gateway receiver is public by design — the HMAC signature IS the credential — but the reject branch read `if not signature_valid and signature_header`, so it only fired for a request that bothered to sign. A delivery with no `X-Hub-Signature-256` at all skipped the branch entirely and was logged, filtered and forwarded to the endpoint's `forward_url`. Every endpoint row carries a non-null secret, so there is no legitimate unsigned caller to preserve; the check is now unconditional and reports which of the two cases it refused. `preview_diff` joins a request-body `domain` straight onto SITES_AVAILABLE and returns the current file's contents inside the diff, which makes a traversing value an arbitrary file read. The guard lives in the service, not only in the route, so every future caller inherits it, and the route now surfaces the refusal as a 400 instead of handing the error dict back as a 200. Co-Authored-By: Claude Opus 5 (1M context) --- backend/app/services/nginx_advanced_service.py | 5 +++++ backend/app/services/webhook_gateway_service.py | 9 ++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/backend/app/services/nginx_advanced_service.py b/backend/app/services/nginx_advanced_service.py index 399aa546..570f7bdc 100644 --- a/backend/app/services/nginx_advanced_service.py +++ b/backend/app/services/nginx_advanced_service.py @@ -153,6 +153,11 @@ class NginxAdvancedService: @staticmethod def preview_diff(domain, new_config): """Preview config changes as a diff.""" + # `domain` is joined onto SITES_AVAILABLE below — refuse anything that + # is not a plain filename so ../ can't turn this into an arbitrary + # file read (the API layer gates admins, but never trust one layer). + if not domain or os.path.basename(domain) != domain or domain in ('.', '..'): + return {'error': 'invalid domain'} conf_path = os.path.join(NginxAdvancedService.SITES_AVAILABLE, domain) old_config = '' if os.path.isfile(conf_path): diff --git a/backend/app/services/webhook_gateway_service.py b/backend/app/services/webhook_gateway_service.py index 4f3cb200..e7e50b7a 100644 --- a/backend/app/services/webhook_gateway_service.py +++ b/backend/app/services/webhook_gateway_service.py @@ -164,12 +164,15 @@ class WebhookGatewayService: db.session.add(delivery) db.session.commit() - if not signature_valid and signature_header: + # Reject unconditionally: every endpoint has a secret, so a request with + # a missing OR wrong signature is equally unauthenticated. The old + # `and signature_header` clause let unsigned requests through. + if not signature_valid: delivery.status = 'failed' - delivery.error_message = 'Invalid signature' + delivery.error_message = 'Invalid signature' if signature_header else 'Missing signature' delivery.completed_at = datetime.utcnow() db.session.commit() - return {'success': False, 'error': 'Invalid signature', 'event_id': event_id}, 401 + return {'success': False, 'error': delivery.error_message, 'event_id': event_id}, 401 try: payload_json = json.loads(payload.decode('utf-8', errors='replace')) if payload else {} From 7aa3286932ac7e644cb62c1cacf3d62db593c22c Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 21:44:45 -0400 Subject: [PATCH 05/12] test(authz): default-deny backstops plus proving tests for the gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two nets, deliberately complementary, because neither alone would have caught the advisory: - `test_route_authz_sweep.py` fires every mutating route as a viewer and fails on any 2xx outside a reviewed `VIEWER_WRITABLE` list. It proves *enforcement*, but a route that 400s or 404s on a dummy request before its gate runs is invisible to it — `POST /workspaces/` 400s on an empty body, so the net would never have seen the bug it exists for. Hence the targeted Part A: the fixed routes must answer a viewer and a developer with 403, and must not block an admin. - `test_route_authz_static.py` proves *coverage* from the source instead: every mutating route in `url_map` either carries a `_sk_authz` decorator, reaches an authorization primitive through its module's call graph, or sits in SELF_SCOPED / PUBLIC_ALT_AUTH / STATELESS with a written justification. Staleness checks keep an allowlist slot from outliving the route that earned it, and a SELF_SCOPED entry must still resolve the caller's identity — calling `get_current_user()` is necessary but not sufficient, which is exactly how `create_workspace` looked self-scoped while creating a global row. Both nets are blind below the API layer and to public routes, so `test_authz_round3_guards.py` covers the three fixes whose shape they cannot express (API-key scope widening, demo-deploy job creation, nginx diff containment), the webhook cases land beside the rest of the gateway surface, and the shared-resource tag gate lands with its own blueprint's tests. Each comes with its non-vacuity case: the same call succeeds for a caller the gate admits. `migrations/history` gets tests because it had none — which is why the NameError in it shipped. Ratchets: JWT-only census 605 -> 577 (the routes that moved to policy decorators), the controller-boundary baseline loses the one persistence crossing `complete_onboarding` no longer makes, and the collected-test floor rises to 4431. Co-Authored-By: Claude Opus 5 (1M context) --- backend/tests/BASELINE_COUNT | 2 +- backend/tests/JWT_ONLY_CEILING | 2 +- .../api_controller_boundary_baseline.json | 3 +- backend/tests/test_authz_round3_guards.py | 127 +++++++ backend/tests/test_migration_status.py | 28 ++ backend/tests/test_route_authz_static.py | 335 ++++++++++++++++++ backend/tests/test_route_authz_sweep.py | 292 +++++++++++++++ backend/tests/test_secrets_webhooks_authz.py | 55 +++ backend/tests/test_shared_resources.py | 25 ++ 9 files changed, 865 insertions(+), 4 deletions(-) create mode 100644 backend/tests/test_authz_round3_guards.py create mode 100644 backend/tests/test_route_authz_static.py create mode 100644 backend/tests/test_route_authz_sweep.py diff --git a/backend/tests/BASELINE_COUNT b/backend/tests/BASELINE_COUNT index 81995be1..91b7ddfe 100644 --- a/backend/tests/BASELINE_COUNT +++ b/backend/tests/BASELINE_COUNT @@ -1 +1 @@ -4309 +4431 diff --git a/backend/tests/JWT_ONLY_CEILING b/backend/tests/JWT_ONLY_CEILING index 695668dd..3fa81504 100644 --- a/backend/tests/JWT_ONLY_CEILING +++ b/backend/tests/JWT_ONLY_CEILING @@ -1 +1 @@ -605 +577 diff --git a/backend/tests/api_controller_boundary_baseline.json b/backend/tests/api_controller_boundary_baseline.json index b3470f89..5f454f13 100644 --- a/backend/tests/api_controller_boundary_baseline.json +++ b/backend/tests/api_controller_boundary_baseline.json @@ -3,7 +3,7 @@ "policy": "Exact reviewed inventory of direct persistence, subprocess, and filesystem mutation calls in Flask API controllers. New calls are forbidden; remove entries as controllers delegate to services.", "counts": { "filesystem": 11, - "persistence": 499 + "persistence": 498 }, "owners": { "api/admin.py": { @@ -369,7 +369,6 @@ "api/apps.py::upload_app_archive::persistence::db.session.commit()::1", "api/apps.py::upload_app_archive::persistence::db.session.commit()::2", "api/apps.py::wake_app::persistence::Application.query_active().filter_by(id=app_id).first()::1", - "api/auth.py::complete_onboarding::persistence::User.query.get(current_user_id)::1", "api/auth.py::create_login_link::persistence::User.query.get(target_id)::1", "api/auth.py::create_login_link::persistence::db.session.commit()::1", "api/auth.py::demo_info::persistence::User.query.filter_by(username='demo').first()::1", diff --git a/backend/tests/test_authz_round3_guards.py b/backend/tests/test_authz_round3_guards.py new file mode 100644 index 00000000..9318f517 --- /dev/null +++ b/backend/tests/test_authz_round3_guards.py @@ -0,0 +1,127 @@ +"""Proving tests for the inline guards added in the round-3 authz audit. + +The two sweep tests can only see so much: ``test_route_authz_static.py`` proves +a route *reaches* an authorization primitive, and +``test_route_authz_sweep.py`` proves a viewer does not get a 2xx. Neither can +tell whether the guard admits the right callers, and neither looks below the +API layer at all. These are the behavioural checks for the three fixes in that +round whose shape the sweeps cannot express: + +* ``api_keys.update_key`` — the *only* key route with no role bar, so a viewer + demoted after minting a key could still widen its scopes to ``['*']``. + Invisible to the sweep because the dummy key id 404s first. +* ``deployment_jobs.simulate_deployment`` — gated only by the demo flag, and + ``DEMO_DEPLOYS_ENABLED`` is on in testing, so a viewer could create real + ``DeploymentJob`` rows. +* ``nginx_advanced_service.preview_diff`` — a body-supplied ``domain`` joined + straight onto ``SITES_AVAILABLE``. The API layer now admits only admins, but + the traversal guard belongs in the service, where every future caller + inherits it. +""" +import pytest + +from factories import make_user, headers_for + + +# --------------------------------------------------------- API-key metadata + +def _key_for(db, user): + from app.services.api_key_service import ApiKeyService + api_key, _raw = ApiKeyService.create_key(user.id, name='round3', scopes=['read']) + return api_key + + +def test_viewer_cannot_widen_the_scopes_of_their_own_api_key(client, db_session): + """Ownership is not the question — capability is. + + ``ApiKeyService.update_key`` already filters by ``user_id``, so this was + never an IDOR; the hole was that a viewer could take a key they legally own + and hand it ``scopes=['*']``, which every API-key-authenticated route then + honours. + """ + viewer = make_user(db_session, role='viewer') + key = _key_for(db_session, viewer) + + resp = client.put(f'/api/v1/api-keys/{key.id}', + json={'scopes': ['*'], 'tier': 'unlimited'}, + headers=headers_for(viewer)) + + assert resp.status_code == 403 + db_session.session.refresh(key) + assert key.get_scopes() == ['read'], 'the viewer widened their key anyway' + + +def test_developer_can_still_update_their_own_api_key(client, db_session): + """Non-vacuity: the gate is a role bar, not a freeze on the route.""" + dev = make_user(db_session, role='developer') + key = _key_for(db_session, dev) + + resp = client.put(f'/api/v1/api-keys/{key.id}', + json={'name': 'renamed'}, + headers=headers_for(dev)) + + assert resp.status_code == 200 + assert resp.get_json()['name'] == 'renamed' + + +# ------------------------------------------------------ demo deployment jobs + +def test_viewer_cannot_start_a_simulated_deployment(app, client, db_session): + """``DEMO_DEPLOYS_ENABLED`` is a *visibility* flag, not an authorization + one — with it on, job creation still has to be a developer capability.""" + from app.models.deployment_job import DeploymentJob + + app.config['DEMO_DEPLOYS_ENABLED'] = True + before = DeploymentJob.query.count() + + resp = client.post('/api/v1/deployment-jobs/simulate?wait=true', + json={'scenario': 'success', 'speed': 'instant'}, + headers=headers_for(make_user(db_session, role='viewer'))) + + assert resp.status_code == 403 + assert DeploymentJob.query.count() == before, 'a viewer created a job row' + + +# --------------------------------------------- nginx vhost path containment + +@pytest.mark.parametrize('domain', [ + '../../../../etc/passwd', + '..', + '.', + 'sub/dir/example.com', + '', + None, +]) +def test_preview_diff_refuses_anything_that_is_not_a_plain_filename(app, domain): + """``domain`` is joined onto ``/etc/nginx/sites-available``; a traversing + value turns a config preview into an arbitrary file read, since the diff + body carries the current file's contents verbatim.""" + from app.services.nginx_advanced_service import NginxAdvancedService + + result = NginxAdvancedService.preview_diff(domain, 'new config') + + assert result == {'error': 'invalid domain'} + + +def test_preview_diff_still_diffs_a_plain_domain(app, tmp_path, monkeypatch): + """Non-vacuity: a normal vhost name is accepted and really is diffed.""" + from app.services.nginx_advanced_service import NginxAdvancedService + + monkeypatch.setattr(NginxAdvancedService, 'SITES_AVAILABLE', str(tmp_path)) + (tmp_path / 'example.com').write_text('server { listen 80; }\n', encoding='utf-8') + + result = NginxAdvancedService.preview_diff('example.com', 'server { listen 443; }\n') + + assert 'error' not in result + assert result['has_changes'] is True + assert 'listen 443' in result['diff'] + + +def test_preview_diff_route_reports_the_refusal_as_a_400(client, db_session): + """The API must not hand the service's error dict back as a 200 success.""" + resp = client.post('/api/v1/nginx/advanced/diff', + json={'domain': '../../etc/passwd', 'config': 'x'}, + headers=headers_for(make_user(db_session, role='admin'))) + + assert resp.status_code == 400 + assert resp.get_json() == {'error': 'invalid domain'} diff --git a/backend/tests/test_migration_status.py b/backend/tests/test_migration_status.py index 364ca47b..69f1a8e9 100644 --- a/backend/tests/test_migration_status.py +++ b/backend/tests/test_migration_status.py @@ -95,3 +95,31 @@ def test_no_revision_at_all_is_not_treated_as_orphaned(migration_state): status = migration_state(None, '080_dashboard_boards') assert status['orphaned_revision'] is False assert status['needs_migration'] is False + + +# --- authorization --------------------------------------------------------- +# `/migrations/history` is the one admin-gated route on this blueprint with no +# side effect, which made it the one whose gate nothing exercised: when the +# module moved to `require_admin_user()` it kept an inline `User.ROLE_ADMIN` +# check whose imports had already been deleted, so every call — admin included +# — raised NameError. The gate has to be fired, not just read. + +def test_history_is_admin_only(client, db_session): + from factories import make_user, headers_for + + for role in ('viewer', 'developer'): + response = client.get('/api/v1/migrations/history', + headers=headers_for(make_user(db_session, role=role))) + assert response.status_code == 403, ( + f'a {role} read the migration history (got {response.status_code})' + ) + + +def test_history_answers_an_admin(client, db_session): + """Non-vacuity for the test above: the same route succeeds for an admin.""" + from factories import make_user, headers_for + + response = client.get('/api/v1/migrations/history', + headers=headers_for(make_user(db_session, role='admin'))) + assert response.status_code == 200 + assert isinstance(response.get_json()['revisions'], list) diff --git a/backend/tests/test_route_authz_static.py b/backend/tests/test_route_authz_static.py new file mode 100644 index 00000000..9a7c7c6e --- /dev/null +++ b/backend/tests/test_route_authz_static.py @@ -0,0 +1,335 @@ +"""Static authorization-coverage test — every mutating route must carry an +authorization *decision*, proven from the source, not from firing requests. + +Companion to ``test_route_authz_sweep.py``. The two are deliberately +complementary: + +* The sweep (live-fire) proves **enforcement**: a real viewer JWT is refused. + It cannot prove coverage — routes that 400/404 on a dummy request before the + gate runs are invisible to it (GHSA-r4q7-x795-vr4j's ``POST /workspaces/`` + 400s on an empty body, so the net would never have caught it). +* This test proves **coverage**: every mutating route in ``app.url_map`` either + carries a recognized gate, reaches an authorization primitive in its module's + call graph, or sits in a small reviewed allowlist with a written + justification. It cannot prove a gate actually rejects — that stays with the + sweep. + +How a route passes (first match wins): + +1. ``_sk_authz`` marker on the view function — set by the rbac decorators + (``@admin_required``, ``@developer_required``, ``@permission_required``, + ``@require_role``, ``@require_app_member``). ``functools.wraps`` propagates + the marker outward through ``@jwt_required``. +2. AST reachability: the view's module-level call graph reaches a name in + ``AUTHORIZATION_PRIMITIVES`` (inline helpers like ``require_admin_user``, + ``_check_scope``, ``can_edit_app``…) or inspects ``.is_admin`` / + ``.is_developer``. +3. ``SELF_SCOPED[endpoint]`` — a *reviewed* route that acts only on the + caller's own rows. Listing is explicit (never inferred) because calling + ``get_current_user()`` is necessary but NOT sufficient: the advisory's + ``create_workspace`` used the caller id as the *owner of a new global row*, + which is not self-scoping. The route must still reach an identity primitive. +4. ``PUBLIC_ALT_AUTH[endpoint]`` — public by design; the value must name the + alternative mechanism (webhook signature, pairing code, enrollment secret). +5. ``STATELESS[endpoint]`` — pure computation: no persistence, no outbound + connections, no command execution on attacker-controlled input. + +Anything else fails with instructions. Scope note: routes served from +``builtin-extensions/`` (mounted under ``/api/v1/``) are excluded — they +use the ``auth_required`` API-key model, a different surface that deserves its +own pass. +""" +import ast +import inspect + +import pytest + + +# --- authorization vocabulary ---------------------------------------------- + +# Inline primitives: calling any of these (directly or through a module-local +# helper) counts as an authorization decision. Names are matched against both +# plain calls (``require_admin_user()``) and method calls +# (``WorkspaceService.get_user_role(...)``). +AUTHORIZATION_PRIMITIVES = frozenset({ + # app/middleware/rbac.py + 'require_admin_user', + 'require_workspace_access', + 'require_workspace_role', + # workspace / app / server access checks + 'get_user_role', + 'check_app_access', + 'can_access_app', + 'can_edit_app', + 'can_operate_app', + 'can_write_in_workspace', + 'verify_workspace_access', + 'verify_server_access', + 'check_server_access', + 'can_access_server', + 'require_app_access', + # per-file gate helpers established by the audit + '_check_scope', + '_check_resource_write', + '_endpoint_gate', + '_vault_gate', + '_ensure_group_accessible', + '_load_app_for', + '_require_admin', +}) + +# Inspecting these attributes on the current user is an inline role check. +ATTRIBUTE_GATES = frozenset({'is_admin', 'is_developer'}) + +# Identity resolution — required for SELF_SCOPED, never sufficient alone. +IDENTITY_PRIMITIVES = frozenset({ + 'get_current_user', 'get_jwt_identity', 'get_jwt', +}) + + +# --- reviewed allowlists (endpoint -> justification) ------------------------- +# Every entry was reviewed against the code (audit of 2026-08-19; evidence +# file:line recorded in the review). The justification is load-bearing: it is +# the reason this route may legitimately lack a role gate. Keep it specific. + +SELF_SCOPED = { + # Acts only on the caller's own rows, ownership enforced in the service. + 'ai.chat': 'conversation ownership via _load_or_create/_owned_conversation', + 'ai.chat_cancel': 'ownership checked before cancel', + 'ai.chat_confirm': 'conversation ownership + pending action belongs to caller', + 'ai.chat_stream': 'conversation ownership via _load_or_create', + 'ai.create_conversation': 'row stamped with caller user_id', + 'ai.delete_conversation': '_owned_conversation gate', + 'ai.rename_conversation': '_owned_conversation gate', + 'api_keys.revoke_key': 'service query filtered by caller user_id', + 'api_keys.rotate_key': 'service query filtered by caller user_id', + 'auth.passkey_delete': 'credential lookup filtered by id + caller user_id', + 'auth.passkey_register': 'registers a passkey for the JWT user only', + 'auth.passkey_register_options': 'challenge issued for the JWT user only', + 'auth.refresh': 'mints an access token only for the refresh-token identity', + 'auth.update_current_user': 'mutates only the caller\'s own user row', + 'dashboards.create_board': 'board created with caller user_id', + 'dashboards.delete_board': 'get_board_for_user scopes id + user_id', + 'dashboards.reset_board': 'get_board_for_user scopes id + user_id', + 'dashboards.update_board': 'get_board_for_user scopes id + user_id', + 'event_subscriptions.delete_subscription': 'service get_for_user enforces admin-or-owner', + 'event_subscriptions.retry_delivery': 'routes through get_for_user (owner check)', + 'event_subscriptions.test_subscription': 'routes through get_for_user (owner check)', + 'event_subscriptions.update_subscription': 'routes through get_for_user (owner check)', + 'mobile.register_push': 'writes the caller\'s own push_subscriptions_json', + 'notifications.mark_inbox_all_read': 'mark_all_read scoped to caller user_id', + 'notifications.mark_inbox_read': 'inbox query scoped to caller user_id', + 'notifications.test_user_notification': 'sends only to the caller\'s own channels', + 'notifications.unmute_own_email': 'unmutes the caller\'s own email only', + 'notifications.update_user_preferences': 'preferences row keyed to caller', + 'queue_bus.create_group': 'non-admin owner_type/owner_id forced to the caller', + 'secrets_webhooks.create_webhook_endpoint': 'workspace resolved via membership-checked resolve_workspace_id; endpoint stamped with caller user_id', + 'source_connections.bitbucket_callback': 'completes OAuth for the caller\'s user_id', + 'source_connections.disconnect_bitbucket': 'disconnect scoped to caller user_id', + 'source_connections.disconnect_github': 'disconnect scoped to caller user_id', + 'source_connections.disconnect_gitlab': 'disconnect scoped to caller user_id', + 'source_connections.github_callback': 'completes OAuth for the caller\'s user_id', + 'source_connections.gitlab_callback': 'completes OAuth for the caller\'s user_id', + 'sso.link_provider': 'links identity to the JWT user with cross-account conflict check', + 'sso.unlink_provider': 'unlink_identity scoped to caller user_id', + 'two_factor.confirm_2fa_setup': 'own user + valid TOTP required', + 'two_factor.disable_2fa': 'own user, requires TOTP/backup code', + 'two_factor.initiate_2fa_setup': 'own user 2FA setup only', + 'two_factor.regenerate_backup_codes': 'own user, TOTP required', + 'views.create_view': 'view created with caller user_id', + 'views.delete_view': 'query filtered by id + caller user_id', + 'views.update_view': 'query filtered by id + caller user_id', +} + +PUBLIC_ALT_AUTH = { + # Public by design; the value names the mechanism that authenticates the call. + 'agent_poll.connect': 'agent HMAC handshake + per-IP throttle', + 'agent_poll.disconnect': 'X-Session-Token lookup', + 'agent_poll.poll': 'X-Session-Token resolves the ConnectedAgent', + 'agent_poll.result': 'X-Session-Token resolves the ConnectedAgent', + 'auth.login': 'password credential + per-IP/per-user throttles + lockout', + 'auth.passkey_authenticate': 'WebAuthn assertion verification, 5/min', + 'auth.redeem_login_link': 'single-use login-link token + per-IP throttle', + 'auth.register': 'invite token / first-user / registration-enabled flag, 3/min', + 'ddns.update': 'per-host token is the credential (DynDNS protocol)', + 'deploy.webhook': 'URL token + provider signature verified before handling', + 'error_logs.client_error_log': 'unauthenticated client error intake: per-IP rate limit + strict field validation', + 'git.receive_webhook': 'webhook token lookup + HMAC signature before processing', + 'notifications.inbound_email_webhook': 'HMAC-SHA256 over raw body; 404 when secret unset', + 'pairing.code_freeze': 'enrollment_id + enrollment_secret', + 'pairing.code_refresh': 'enrollment_id + enrollment_secret', + 'pairing.enroll': 'pubkey+passphrase enrollment; produces a PENDING enrollment needing developer-gated claim', + 'pairing.poll': 'enrollment creds + optional Ed25519 proof-of-possession', + 'preview_webhooks.receive_pull_request': 'webhook token lookup + HMAC signature before any action', + 'secrets_webhooks.receive_webhook': 'HMAC signature mandatory since the 2026-08 fix (missing or wrong signature -> 401)', + 'servers.register_agent': 'single-use hashed registration token, 5/min', + 'sso.callback': 'OAuth authorization-code + state exchange with the provider', + 'sso.saml_callback': 'SAML assertion validated by the SSO service', + 'two_factor.verify_2fa_code': 'short-lived 2fa_pending token + TOTP/backup code + throttle', +} + +STATELESS = { + # Persists nothing, dials nothing attacker-chosen, executes nothing + # attacker-controlled. + 'agent_plugins.validate_manifest': 'pure manifest schema validation', + 'auth.passkey_auth_options': 'challenge issuance only, no mutation', + 'buildpacks.generate': 'pure plan -> Dockerfile/compose templating', + 'cron.preview_schedule': 'pure cron-expression computation', + 'dns_cutover.ttl_guidance': 'pure computation over supplied records', + 'dns_cutover.verify': 'read-only DNS lookups against public resolvers', + 'docker.get_containers_stats': 'read-via-POST of container stats; viewer holds docker:read', + 'firewall.rule_removal_preflight': 'pure analysis against current rules', + 'htaccess_tools.htaccess_convert': 'pure text transform, 256KB cap', + 'mobile.execute_quick_action': 'stub: canned no-op responses / live metrics read only', + 'mobile.unregister_push': 'stub: returns success without touching the DB', + 'servers.check_agent_version': 'agent-facing version check; outbound only to a fixed GitHub releases URL, cached', + 'templates.validate_installation': 'validation only (name-clash read + variable checks)', +} + + +# --- AST machinery ----------------------------------------------------------- + +_MUTATING = {'POST', 'PUT', 'PATCH', 'DELETE'} + + +def _mutating_rules(app): + """Mutating rules served from backend/app/api (the core surface). + + Yields ``(rule, view, unwrapped)``: ``view`` is the registered (possibly + decorator-wrapped) endpoint function — where ``_sk_authz`` markers live — + and ``unwrapped`` is the original view reached through ``__wrapped__`` + chains, which is what source-file attribution and AST analysis must use + (``inspect.getsourcefile`` on a wrapped view returns the *decorator's* + module, e.g. flask_jwt_extended, not the route's file). + """ + for rule in app.url_map.iter_rules(): + if not (rule.methods & _MUTATING): + continue + view = app.view_functions.get(rule.endpoint) + if view is None: + continue + try: + unwrapped = inspect.unwrap(view) + except ValueError: # wrapper cycle + unwrapped = view + src = inspect.getsourcefile(unwrapped) + if src is None or '\\app\\api\\' not in src.replace('/', '\\'): + continue # builtin-extensions and flask internals: separate surface + yield rule, view, unwrapped + + +def _module_functions(path): + """name -> FunctionDef for every function defined in the module.""" + with open(path, encoding='utf-8') as fh: + tree = ast.parse(fh.read()) + return {n.name: n for n in ast.walk(tree) + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef))} + + +def _called_names(fn): + """Every called name plus gated-attribute lookups in one function body.""" + names = set() + for node in ast.walk(fn): + if isinstance(node, ast.Call): + func = node.func + if isinstance(func, ast.Name): + names.add(func.id) + elif isinstance(func, ast.Attribute): + names.add(func.attr) + elif isinstance(node, ast.Attribute) and node.attr in ATTRIBUTE_GATES: + names.add(node.attr) + return names + + +def _reachable_names(funcs, start): + """Transitive closure of names callable from `start` within the module.""" + calls = {name: _called_names(f) for name, f in funcs.items()} + seen, stack = set(), [start] + while stack: + for callee in calls.get(stack.pop(), ()): + if callee not in seen: + seen.add(callee) + stack.append(callee) + return seen + + +# --- the test ---------------------------------------------------------------- + +def test_every_mutating_route_carries_an_authorization_decision(app): + # Parse each api module at most once. + module_funcs = {} + + def reachable(unwrapped): + path = inspect.getsourcefile(unwrapped) + if path not in module_funcs: + module_funcs[path] = _module_functions(path) + return _reachable_names(module_funcs[path], unwrapped.__name__) + + ungated = [] + for rule, view, unwrapped in _mutating_rules(app): + endpoint = rule.endpoint + + # 1. Decorator gate (marker propagates through functools.wraps). + if getattr(view, '_sk_authz', None): + continue + + names = reachable(unwrapped) + + # 2. Inline primitive or attribute role check. + if names & AUTHORIZATION_PRIMITIVES or names & ATTRIBUTE_GATES: + continue + + # 3. Reviewed self-scoped route — must resolve the caller's identity. + if endpoint in SELF_SCOPED and names & IDENTITY_PRIMITIVES: + continue + + # 4/5. Reviewed public-with-alt-auth / stateless routes. + if endpoint in PUBLIC_ALT_AUTH or endpoint in STATELESS: + continue + + methods = sorted(rule.methods & _MUTATING) + ungated.append(f'{methods} {rule.rule} ({endpoint})') + + assert not ungated, ( + 'Mutating route(s) with no authorization decision found:\n ' + + '\n '.join(sorted(ungated)) + + '\n\nFix one of:' + '\n - add a gate decorator (@admin_required / @developer_required /' + ' @permission_required), or' + '\n - call an inline authorization primitive (require_admin_user,' + ' check_app_access, _check_scope, ...), or' + '\n - if the route is genuinely self-scoped / public-with-alt-auth /' + ' stateless, add it to the matching allowlist in this file with a' + ' specific justification.' + ) + + +def test_static_allowlists_are_not_stale(app): + """Every allowlisted endpoint must still exist and still need its listing. + + A stale entry is a hiding place: if the route is renamed or gains a real + gate, the entry must leave so a future route cannot reuse the slot. + """ + live = {rule.endpoint for rule, _, _ in _mutating_rules(app)} + listed = set(SELF_SCOPED) | set(PUBLIC_ALT_AUTH) | set(STATELESS) + stale = listed - live + assert not stale, f'Allowlist entries for endpoints that no longer exist: {sorted(stale)}' + + +def test_self_scoped_entries_actually_resolve_identity(app): + """A SELF_SCOPED entry whose route no longer resolves the caller's identity + is not self-scoped by any definition — drop the entry or fix the route.""" + module_funcs = {} + bogus = [] + for rule, _, unwrapped in _mutating_rules(app): + if rule.endpoint not in SELF_SCOPED: + continue + path = inspect.getsourcefile(unwrapped) + if path not in module_funcs: + module_funcs[path] = _module_functions(path) + names = _reachable_names(module_funcs[path], unwrapped.__name__) + if not (names & IDENTITY_PRIMITIVES): + bogus.append(rule.endpoint) + assert not bogus, ( + f'SELF_SCOPED entries whose view never resolves the caller identity: {sorted(bogus)}' + ) diff --git a/backend/tests/test_route_authz_sweep.py b/backend/tests/test_route_authz_sweep.py new file mode 100644 index 00000000..a17c7949 --- /dev/null +++ b/backend/tests/test_route_authz_sweep.py @@ -0,0 +1,292 @@ +"""Authorization sweep — the default-deny backstop for mutating routes. + +Motivation +---------- +GHSA-r4q7-x795-vr4j: ``POST /api/v1/workspaces/`` shipped gated only by +``@jwt_required()``, so a read-only *viewer* could create and administer +workspaces. It was one forgotten authorization check, and it was not the only +one — an audit of every mutating route turned up the same class in the docker +compose read routes (arbitrary-path log/config disclosure) and the whole +monitors surface (viewer could create/edit/delete monitors and incidents). + +The root cause is architectural: ServerKit has **no enforced default-deny**. +Authorization is opt-in per route, via a decorator *or* an inline helper +(``_can_edit_app``, ``require_workspace_role``, ``_check_scope``, ...). Nothing +fails closed when an author forgets, so each new feature file is one missing +line away from re-introducing the bug. + +This module is the CI backstop for that class. It has two parts: + +* ``test_fixed_routes_reject_non_admins`` — a precise, hermetic proof that the + specific routes fixed alongside the advisory (docker compose + monitors) + reject a viewer *and* a developer. These use ``@permission_required`` / + ``@admin_required``, which run before the view body, so a dummy id still + exercises the gate (403, not a 404 for the missing row). + +* ``test_no_mutating_route_succeeds_for_a_viewer`` — the regression net. It + fires every mutating route as a viewer and asserts none returns a 2xx unless + its endpoint is in ``VIEWER_WRITABLE`` (a reviewed allowlist). A newly-added + route that forgets to gate and happily answers a viewer with ``201`` trips + this test until the author either gates it or consciously allowlists it. + +Known limitation: a route with a path parameter that resolves its resource +(and 404s) *before* checking authorization is invisible to the live-fire net +— the 404 masks a missing gate. The net therefore catches the clearest +regressions (creates and global toggles with no path param); resource-scoped +routes need their own per-feature tests. See ``VIEWER_WRITABLE`` notes for the +handful of endpoints that remain viewer-writable and are tracked as follow-ups. +""" +import pytest + +from factories import make_user, headers_for + + +# --- Part A: the routes fixed alongside the advisory ----------------------- + +# (endpoint path, method) — every mutating route on the docker compose read +# surface and the monitors surface. A viewer and a developer must both be +# refused; only an admin gets through. +_MONITORS_WRITE = [ + ('/api/v1/monitors/', 'POST'), + ('/api/v1/monitors/1', 'PUT'), + ('/api/v1/monitors/1', 'DELETE'), + ('/api/v1/monitors/1/check', 'POST'), + ('/api/v1/monitors/1/pause', 'POST'), + ('/api/v1/monitors/incidents', 'POST'), + ('/api/v1/monitors/incidents/1', 'PUT'), + ('/api/v1/monitors/incidents/1', 'DELETE'), +] + +_DOCKER_COMPOSE_READ = [ + ('/api/v1/docker/compose/ps', 'POST'), + ('/api/v1/docker/compose/logs', 'POST'), + ('/api/v1/docker/compose/validate', 'POST'), + ('/api/v1/docker/compose/config', 'POST'), +] + +# The advisory fix itself plus the viewer-write gaps the same audit surfaced: +# workspace creation, the global uptime on/off toggles, registrar sync/test +# (their add/delete siblings were already admin-only), and template drift checks. +_ADMIN_GATED_ROUND2 = [ + ('/api/v1/workspaces/', 'POST'), + ('/api/v1/uptime/tracking/start', 'POST'), + ('/api/v1/uptime/tracking/stop', 'POST'), + ('/api/v1/registrars/sync', 'POST'), + ('/api/v1/registrars/connections/1/test', 'POST'), + ('/api/v1/server-templates/assignments/1/check', 'POST'), +] + +_FIXED_ROUTES = _MONITORS_WRITE + _DOCKER_COMPOSE_READ + _ADMIN_GATED_ROUND2 + +# Round 3 — gaps surfaced by the static-coverage audit (see +# test_route_authz_static.py, seeded 2026-08-19). Admin-gated: both a viewer +# and a developer must be refused. +_ADMIN_GATED_ROUND3 = [ + ('/api/v1/fleet-monitor/alerts/a1/acknowledge', 'POST'), + ('/api/v1/fleet-monitor/alerts/a1/resolve', 'POST'), + ('/api/v1/recycle-bin/apps/1/restore', 'POST'), + ('/api/v1/nginx/advanced/test', 'POST'), + ('/api/v1/nginx/advanced/diff', 'POST'), +] + +# Developer-gated: a viewer is refused; a developer is admitted (the body may +# still 400/404/500 on the dummy payload — only the gate must not fire). +_DEVELOPER_GATED_ROUND3 = [ + ('/api/v1/servers/s1/docker/compose/ps', 'POST'), + ('/api/v1/servers/s1/docker/compose/logs', 'POST'), + ('/api/v1/servers/s1/refresh-capabilities', 'POST'), + ('/api/v1/servers/s1/ping', 'POST'), + ('/api/v1/templates/test-db-connection', 'POST'), + ('/api/v1/deploy/branches', 'POST'), + ('/api/v1/buildpacks/detect', 'POST'), +] + +_FIXED_ROUTES += _ADMIN_GATED_ROUND3 + + +@pytest.mark.parametrize('path,method', _FIXED_ROUTES) +@pytest.mark.parametrize('role', ['viewer', 'developer']) +def test_fixed_routes_reject_non_admins(client, db_session, path, method, role): + """A viewer and a developer are refused (403) on every fixed route. + + The gate is a decorator, so it fires before the view body — the dummy id + ``1`` never matters; we never reach the row lookup. + """ + headers = headers_for(make_user(db_session, role=role)) + resp = client.open(path, method=method, headers=headers, json={}) + assert resp.status_code == 403, ( + f'{method} {path} let a {role} through with {resp.status_code}; ' + f'expected 403 from the authorization gate' + ) + + +@pytest.mark.parametrize('path,method', _FIXED_ROUTES) +def test_fixed_routes_admit_admin(client, db_session, path, method): + """Sanity: an admin is NOT stopped by the authorization gate. + + The body may still 400/404/500 on the dummy payload — we only assert the + gate itself did not reject the admin (never 401/403). + """ + if path == '/api/v1/uptime/tracking/start': + # Side-effecting for an admin (spawns the tracking daemon thread); + # the viewer/developer 403 above is the security assertion here. + return + headers = headers_for(make_user(db_session, role='admin')) + resp = client.open(path, method=method, headers=headers, json={}) + assert resp.status_code not in (401, 403), ( + f'{method} {path} wrongly blocked an admin with {resp.status_code}' + ) + + +@pytest.mark.parametrize('path,method', _DEVELOPER_GATED_ROUND3) +def test_developer_gated_routes_reject_viewers(client, db_session, path, method): + """A viewer is refused (403) on every developer-gated round-3 route.""" + headers = headers_for(make_user(db_session, role='viewer')) + resp = client.open(path, method=method, headers=headers, json={}) + assert resp.status_code == 403, ( + f'{method} {path} let a viewer through with {resp.status_code}; ' + f'expected 403 from @developer_required' + ) + + +@pytest.mark.parametrize('path,method', _DEVELOPER_GATED_ROUND3) +def test_developer_gated_routes_admit_developers(client, db_session, path, method): + """Sanity: a developer is NOT stopped by the gate (body may 400/404/500).""" + headers = headers_for(make_user(db_session, role='developer')) + resp = client.open(path, method=method, headers=headers, json={}) + assert resp.status_code not in (401, 403), ( + f'{method} {path} wrongly blocked a developer with {resp.status_code}' + ) + + +def test_queue_group_mutations_are_owner_scoped(app, client, db_session): + """queue_bus cross-user fix: `_ensure_group_mutable` alone only blocked + *system* groups — any authenticated user could rename/delete another + user's groups, queues and messages. Every mutating route now also calls + `_ensure_group_accessible` (owner-or-admin).""" + from app.queue_bus.service import QueueBusService + + owner = make_user(db_session, role='developer') + intruder = make_user(db_session, role='developer') + group = QueueBusService.create_group( + slug='owned-group', name='Owned', owner_type='user', owner_id=str(owner.id), + ) + slug = group['slug'] if isinstance(group, dict) else group.slug + + headers = headers_for(intruder) + attempts = [ + ('PATCH', f'/api/v1/queue/groups/{slug}'), + ('DELETE', f'/api/v1/queue/groups/{slug}'), + ('POST', f'/api/v1/queue/groups/{slug}/queues'), + ('POST', f'/api/v1/queue/groups/{slug}/queues/q1/messages'), + ] + for method, url in attempts: + resp = client.open(url, method=method, headers=headers, + json={'name': 'x', 'payload': {}}) + assert resp.status_code == 403, ( + f'{method} {url} let a non-owner through with {resp.status_code}' + ) + + # The owner themselves is NOT blocked by the accessibility guard. + resp = client.open(f'/api/v1/queue/groups/{slug}', method='PATCH', + headers=headers_for(owner), json={'description': 'mine'}) + assert resp.status_code != 403 + + +# --- Part B: the default-deny regression net ------------------------------- + +# Endpoints where a viewer legitimately receives a 2xx today. Each is reviewed +# and categorized. Adding a NEW mutating route that answers a viewer with a 2xx +# will fail the sweep below until it is gated or deliberately listed here. +VIEWER_WRITABLE = { + # SELF — the viewer acts only on their own account/data. + 'ai.create_conversation', + 'auth.update_current_user', + 'notifications.mark_inbox_all_read', + 'notifications.test_user_notification', + 'notifications.unmute_own_email', + 'two_factor.initiate_2fa_setup', + 'mobile.execute_quick_action', + # STATELESS — parse/validate/dry-run helpers; no persisted mutation. + 'cron.preview_schedule', + 'dns_cutover.ttl_guidance', + 'firewall.rule_removal_preflight', + 'nginx_advanced.test_config', + # READ-VIA-POST — a read the permission model already grants a viewer + # (viewer has docker:read). POST only because it takes a body of ids. + 'docker.get_containers_stats', + # PUBLIC transport — agent long-poll fallback; user JWT is ignored here. + 'agent_poll.disconnect', +} + +# Endpoints skipped by the live-fire net because firing them reaches out to the +# network / external providers (slow, flaky in CI). Their gating is asserted by +# dedicated per-feature tests, not here. +_SKIP_ENDPOINTS = { + 'servers.check_agent_version', +} + + +def _mutating_rules(app): + for rule in app.url_map.iter_rules(): + methods = rule.methods - {'GET', 'HEAD', 'OPTIONS'} + if not methods: + continue + if rule.endpoint in _SKIP_ENDPOINTS: + continue + method = 'POST' if 'POST' in methods else sorted(methods)[0] + yield rule, method + + +def _fill(rule): + """Concrete URL for a rule, dummy values for path params.""" + url = rule.rule + for arg in rule.arguments: + conv = type(rule._converters.get(arg)).__name__ + val = '1' if 'Integer' in conv or 'Float' in conv else 'x' + for token in (f'<{arg}>', f'', f'', + f'', f'', f''): + url = url.replace(token, val) + return url + + +def test_no_mutating_route_succeeds_for_a_viewer(app, client, db_session): + """No mutating route may answer a viewer with a 2xx unless allowlisted. + + This is the default-deny backstop. A 2xx here means the authorization gate + definitively did NOT stop a read-only user — the exact shape of the + workspace advisory. Non-2xx (401/403/400/404/5xx) is fine: either the gate + refused, or the body rejected the dummy payload before mutating anything. + """ + headers = headers_for(make_user(db_session, role='viewer')) + offenders = [] + for rule, method in _mutating_rules(app): + url = _fill(rule) + try: + resp = client.open(url, method=method, headers=headers, json={}) + code = resp.status_code + except Exception: + # A crash in the view body is not a viewer succeeding — the gate + # question is answered "not 2xx". Real gating bugs surface as 2xx. + continue + if 200 <= code < 300 and rule.endpoint not in VIEWER_WRITABLE: + offenders.append(f'{method} {url} ({rule.endpoint}) -> {code}') + + assert not offenders, ( + 'A read-only viewer received a success response on mutating ' + 'route(s) with no authorization gate:\n ' + '\n '.join(sorted(offenders)) + + '\n\nGate the route (a decorator or an inline role/ownership check), ' + 'or — if a viewer is genuinely allowed — add the endpoint to ' + 'VIEWER_WRITABLE with a category comment.' + ) + + +def test_viewer_writable_allowlist_is_not_stale(app): + """Every allowlisted endpoint must still exist — stale entries hide gaps. + + If a route is renamed/removed and its VIEWER_WRITABLE entry is left behind, + a future viewer-writable route could reuse a nearby name and slip through. + """ + live = {r.endpoint for r in app.url_map.iter_rules()} + stale = (VIEWER_WRITABLE | _SKIP_ENDPOINTS) - live + assert not stale, f'Allowlist references endpoints that no longer exist: {sorted(stale)}' diff --git a/backend/tests/test_secrets_webhooks_authz.py b/backend/tests/test_secrets_webhooks_authz.py index e1723c75..9d8ba228 100644 --- a/backend/tests/test_secrets_webhooks_authz.py +++ b/backend/tests/test_secrets_webhooks_authz.py @@ -246,3 +246,58 @@ def test_delivery_replay_requires_owner_role(client, vault_rbac, monkeypatch): assert client.post(url, headers=vault_rbac.ws_owner).status_code == 200 assert client.post('/api/v1/webhooks/deliveries/999999/replay', headers=s.admin).status_code == 404 + + +# ------------------- inbound receiver (public, signature-authenticated) ---- + +def _sign(secret, payload): + import hashlib + import hmac + return 'sha256=' + hmac.new(secret.encode(), payload, hashlib.sha256).hexdigest() + + +def test_inbound_webhook_without_a_signature_is_rejected(client, vault_rbac, monkeypatch): + """The receiver is unauthenticated by design — the HMAC signature IS the + credential — so an *absent* signature has to fail exactly like a wrong one. + + It did not: the reject branch read ``if not signature_valid and + signature_header``, which only fires for a request that bothered to sign. + Sending no ``X-Hub-Signature-256`` at all skipped the branch entirely and + the payload was logged, filtered and forwarded to ``forward_url``. + """ + from app.services.webhook_gateway_service import WebhookGatewayService + forwarded = [] + monkeypatch.setattr(WebhookGatewayService, '_forward', + classmethod(lambda cls, e, d, p, h: forwarded.append(p) or + {'success': True, 'status': 200, 'body': ''})) + + resp = client.post('/api/v1/webhooks/receive/scope-endpoint', + data=b'{"a": 1}', content_type='application/json') + assert resp.status_code == 401 + assert resp.get_json()['error'] == 'Missing signature' + assert forwarded == [], 'an unsigned payload was forwarded upstream' + + +def test_inbound_webhook_with_a_wrong_signature_is_rejected(client, vault_rbac): + resp = client.post('/api/v1/webhooks/receive/scope-endpoint', + data=b'{"a": 1}', content_type='application/json', + headers={'X-Hub-Signature-256': _sign('not-the-secret', b'{"a": 1}')}) + assert resp.status_code == 401 + assert resp.get_json()['error'] == 'Invalid signature' + + +def test_inbound_webhook_with_a_valid_signature_is_accepted(client, vault_rbac, monkeypatch): + """Non-vacuity for the two rejections: a correctly signed delivery still + reaches the forwarder.""" + from app.services.webhook_gateway_service import WebhookGatewayService + forwarded = [] + monkeypatch.setattr(WebhookGatewayService, '_forward', + classmethod(lambda cls, e, d, p, h: forwarded.append(p) or + {'success': True, 'status': 200, 'body': ''})) + + payload = b'{"a": 1}' + resp = client.post('/api/v1/webhooks/receive/scope-endpoint', + data=payload, content_type='application/json', + headers={'X-Hub-Signature-256': _sign('whsec', payload)}) + assert resp.status_code == 200 + assert forwarded == [payload] diff --git a/backend/tests/test_shared_resources.py b/backend/tests/test_shared_resources.py index 46dd19d3..80410fe3 100644 --- a/backend/tests/test_shared_resources.py +++ b/backend/tests/test_shared_resources.py @@ -413,3 +413,28 @@ def test_audit_rows_written_on_mutations(app): assert {'tag.add', 'tag.remove', 'group.create', 'variable.create', 'group.attach', 'group.detach', 'group.delete'} <= ops assert AuditLog.query.count() > before + + +# ------------------------------------------------- tag write authorization + +def test_api_tag_write_is_authorized_not_just_authenticated(app, client, db_session): + """Tagging is a mutation on someone else's resource, so it needs the same + ``_check_resource_write`` gate as attach/detach. + + ``POST``/``DELETE /shared/tags`` shipped behind ``@jwt_required()`` alone: + any authenticated account could label any resource of any type. ``service`` + is the sharpest case — it has no ownership seam, so the gate resolves it to + admin-only and a developer must be refused. + """ + from factories import make_user, headers_for + + dev = headers_for(make_user(db_session, role='developer')) + admin = headers_for(make_user(db_session, role='admin')) + body = {'resource_type': 'service', 'resource_id': 'web-1', 'tag': 'prod'} + + assert client.post('/api/v1/shared/tags', json=body, headers=dev).status_code == 403 + assert client.delete('/api/v1/shared/tags', json=body, headers=dev).status_code == 403 + + # Non-vacuity: the same calls succeed for a caller the gate admits. + assert client.post('/api/v1/shared/tags', json=body, headers=admin).status_code == 201 + assert client.delete('/api/v1/shared/tags', json=body, headers=admin).status_code == 200 From 9e18e354948b9f9542630c13dedbd093c2fa9d88 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 21:44:56 -0400 Subject: [PATCH 06/12] docs(authz): write down the default-deny and one-boundary rules Both are now enforced by tests that fail the build, so they belong where the "adding a feature" checklist is rather than in a commit message nobody re-reads. The one-boundary rule is the trap this round fell into: stacking `@jwt_required()` outside a policy decorator looks like belt-and-braces and is actually a regression, because it rejects API-key callers before the policy can authorize them. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 24 ++++++++++++++++++++++++ CLAUDE.md | 11 +++++++++++ 2 files changed, 35 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 0459f9f1..0a427e82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -191,6 +191,30 @@ PageTopbar (tabs + actions: Add/Refresh/…, FilterButton, SearchField) 5. **Page**: Create page component in `frontend/src/pages/`, add route in `App.jsx` 6. **Styles**: Add SCSS file in `frontend/src/styles/pages/`, import in `main.scss` +### Authorization (default-deny, test-enforced) + +Every **mutating** route (POST/PUT/PATCH/DELETE) must carry an explicit +authorization decision — `@jwt_required()` alone is never sufficient +(GHSA-r4q7-x795-vr4j). Three tests enforce this and will fail your PR: + +- `backend/tests/test_route_authz_static.py` (coverage): the route must carry a + gate decorator (`@admin_required` / `@developer_required` / + `@permission_required`), reach an inline authorization primitive + (`require_admin_user`, `check_app_access`, `_check_scope`, …), or be listed in + a reviewed allowlist (`SELF_SCOPED` / `PUBLIC_ALT_AUTH` / `STATELESS`) with a + specific justification. +- `backend/tests/test_route_authz_sweep.py` (enforcement): no mutating route may + answer a viewer with a 2xx unless allowlisted in `VIEWER_WRITABLE`. +- `backend/tests/test_auth_decorator_boundaries.py` (one boundary): the policy + decorators already authenticate through `auth_required()`, so **do not stack + `@jwt_required()` outside one** — it rejects API-key callers before the policy + can authorize them. Add the gate *instead of* `@jwt_required()`, and read the + caller with `get_current_user()`, never `get_jwt_identity()` (which raises for + an API-key request). + +When in doubt: admin-only for panel-wide resources, `@developer_required` for +operational actions, the file's own inline ownership helper for user data. + ## Key Environment Variables | Variable | Purpose | diff --git a/CLAUDE.md b/CLAUDE.md index ad32b09d..08330cf9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,6 +118,17 @@ ServerKit deploys on Linux (bare metal, VPS, or Docker). Development may happen 5. **Page**: Create page component in `frontend/src/pages/`, add route in `App.jsx` 6. **Styles**: Add SCSS file in `frontend/src/styles/pages/`, import in `main.scss` +### Authorization (default-deny, test-enforced) + +Every mutating route (POST/PUT/PATCH/DELETE) must carry an explicit +authorization decision — `@jwt_required()` alone is never sufficient +(GHSA-r4q7-x795-vr4j). Use one authentication boundary: the rbac policy +decorators (`@admin_required` / `@developer_required` / `@permission_required`) +already authenticate via `auth_required()`, so never stack `@jwt_required()` +outside one. `backend/tests/test_route_authz_static.py` (coverage), +`test_route_authz_sweep.py` (enforcement) and `test_auth_decorator_boundaries.py` +(one boundary) all fail the build otherwise. See AGENTS.md for the allowlists. + ## Key Environment Variables | Variable | Purpose | From c8afc7ccd27f45036c073d17580c8d7af9cf800a Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 21:44:56 -0400 Subject: [PATCH 07/12] fix(workspaces): hide the creation entry points from non-admins Creating a workspace became admin-only server-side, so the topbar button, the empty-state action and the `?focus=create` deep link were all offers of a guaranteed 403. Non-admins get an empty state that says who to ask instead. Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/pages/Workspaces.jsx | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/frontend/src/pages/Workspaces.jsx b/frontend/src/pages/Workspaces.jsx index e1f4e378..59ab392c 100644 --- a/frontend/src/pages/Workspaces.jsx +++ b/frontend/src/pages/Workspaces.jsx @@ -12,6 +12,7 @@ import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; import { Textarea } from '@/components/ui/textarea'; import { useWorkspace } from '../contexts/WorkspaceContext'; +import { useAuth } from '../contexts/AuthContext'; import { useServerMutation, useServerQuery } from '../hooks/useServerQuery'; // Preset views. `servers` and `users` are quota CEILINGS, not usage, so there @@ -84,11 +85,14 @@ const Workspaces = () => { const toast = useToast(); const navigate = useNavigate(); const { activeWorkspaceId } = useWorkspace(); + // Workspace creation is admin-only server-side (@admin_required); hide the + // entry points for everyone else instead of letting them hit a 403. + const { isAdmin } = useAuth(); const [search, setSearch] = useState(''); const [selectedIds, setSelectedIds] = useState(new Set()); const [showCreateModal, setShowCreateModal] = useState(false); // Quick-create deep link: /workspaces?focus=create:workspace opens the modal. - useFocusParam('create', () => setShowCreateModal(true)); + useFocusParam('create', () => { if (isAdmin) setShowCreateModal(true); }); const [form, setForm] = useState({ name: '', description: '', max_servers: 0, max_users: 0, primary_color: '#6d7cff' }); const loadWorkspaces = useCallback( @@ -109,17 +113,19 @@ const Workspaces = () => { useTopbarActions(() => ( <> - + {isAdmin && ( + + )} - ), [search]); + ), [search, isAdmin]); const handleCreate = async () => { try { @@ -239,12 +245,14 @@ const Workspaces = () => { onClearSelection={() => setSelectedIds(new Set())} emptyIcon={LayoutGrid} emptyTitle="No workspaces yet" - emptyDescription="Create one to isolate servers by team or project." - emptyAction={ + emptyDescription={isAdmin + ? 'Create one to isolate servers by team or project.' + : 'Workspaces isolate servers by team or project. Ask an admin to create one.'} + emptyAction={isAdmin ? ( - } + ) : null} filteredEmptyIcon={LayoutGrid} filteredEmptyTitle="No workspaces found" filteredEmptyDescription="Try adjusting your search or filter." From 343b4e3b14e553bedb32cd3079d833419cb81dc4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 20 Aug 2026 01:54:09 +0000 Subject: [PATCH 08/12] chore: bump version to 1.7.114 [skip ci] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 93a8d5c6..0773f194 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.113 +1.7.114 From 0d42baf678570e65fc7643a2b116944a6421e892 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 22:09:59 -0400 Subject: [PATCH 09/12] fix(tests): set the test-count floor to the clean-checkout number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4431 was measured on a dev box that has four git-ignored extension copies installed under backend/app/plugins/ (cloud-provision, ftp, remote-access, status). test_builtin_extension_drift.py parametrizes over the on-disk intersection of builtin-extensions//backend and app/plugins/, so those four contribute 3+3+5+3 = 14 cases that no clean checkout can collect — CI collects 4417, and the ratchet went red for a suite that had lost nothing. This is the failure mode check_test_count.py's own docstring warns about (a floor --update'd above what a clean checkout can reach). 4417 is still a raise of 108 over main's 4309, matching the 107 tests this branch adds. Co-Authored-By: Claude Opus 5 (1M context) --- backend/tests/BASELINE_COUNT | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/tests/BASELINE_COUNT b/backend/tests/BASELINE_COUNT index 91b7ddfe..a1bb2ea4 100644 --- a/backend/tests/BASELINE_COUNT +++ b/backend/tests/BASELINE_COUNT @@ -1 +1 @@ -4431 +4417 From 1c28afca44013dd52a6f69dfac294e535fc6d08b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 20 Aug 2026 02:16:48 +0000 Subject: [PATCH 10/12] chore: bump version to 1.7.115 [skip ci] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 0773f194..79c02183 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.114 +1.7.115 From 7311abe61f6d0e3c35c8a6475543b2f02ef14ab0 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Wed, 19 Aug 2026 22:29:52 -0400 Subject: [PATCH 11/12] docs(inventory): regenerate the migration inventory after the authz sweep The workspace-authz round converted 28 routes off bare @jwt_required() onto role decorators and ratcheted JWT_ONLY_CEILING 605 -> 577, but the generated inventory was never regenerated, so CI's freshness test failed. Regenerated from the same censuses that enforce the ratchets: bare @jwt_required() 605 -> 577 (ceiling 605 -> 577) hand-shaped error bodies 1159 -> 1158 (ceiling stays 1159, 1 of 25 slack) controller-boundary 510 -> 509 (baseline json already at 509) Co-Authored-By: Claude Opus 5 (1M context) --- docs/MIGRATION_INVENTORY.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/MIGRATION_INVENTORY.md b/docs/MIGRATION_INVENTORY.md index 3ec2f353..30b51e64 100644 --- a/docs/MIGRATION_INVENTORY.md +++ b/docs/MIGRATION_INVENTORY.md @@ -13,12 +13,12 @@ debt. Regenerate with `python scripts/generate-migration-inventory.py`. | Concern | Door | Remaining | Ceiling | Policy | |---|---|---:|---:|---| | Identity lookups outside rbac.get_current_user() | rbac.get_current_user() | 69 | 69 | JWT-only routes; the API-key-capable population is held at 0 by a companion assertion | -| Routes on bare @jwt_required() | auth_required() / role decorators | 605 | 605 | REGISTERED EXCEPTION: JWT-only is the deliberate default; conversion grants API-key access and happens per route, on decision | +| Routes on bare @jwt_required() | auth_required() / role decorators | 577 | 577 | REGISTERED EXCEPTION: JWT-only is the deliberate default; conversion grants API-key access and happens per route, on decision | | HTTP statuses chosen by sniffing error text | typed errors from app.exceptions | 0 | 0 | INVARIANT at 0 - migration completed 2026-08-19 | | API crashes swallowed without recording | app.error_reporting | 0 | 0 | INVARIANT at 0 | -| Hand-shaped {'error': ...} bodies in app/api | typed errors + the global handler | 1159 | 1159 | migrate when touched; new endpoints raise | +| Hand-shaped {'error': ...} bodies in app/api | typed errors + the global handler | 1158 | 1159 | migrate when touched; new endpoints raise | | Raw subprocess calls outside the runners | app/utils/system.py runners | 24 | 24 | migrate when touched | -| Controller-boundary violations (routes doing service work) | service layer extraction | 510 | 510 | migrate when touched (first-wave ratchet) | +| Controller-boundary violations (routes doing service work) | service layer extraction | 509 | 509 | migrate when touched (first-wave ratchet) | | raw api.* calls in pages/ | E1: useServerQuery/useServerMutation | 460 | 460 | migrate when touched | | per-page toast.error extractions in pages/ | E1: query-layer error presentation | 264 | 264 | migrate when touched | | hand-rolled form-group blocks | F2: FormField/useForm | 343 | 343 | migrate when touched | From 9e0f19c378b5064638e691371dc8a0ca301e5cd0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 20 Aug 2026 02:33:07 +0000 Subject: [PATCH 12/12] chore: bump version to 1.7.116 [skip ci] --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 79c02183..e5e0ebae 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.115 +1.7.116