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: 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 | diff --git a/VERSION b/VERSION index 93a8d5c6..e5e0ebae 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.113 +1.7.116 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 af439cae..ea6a54f3 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__) @@ -49,7 +51,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/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 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 {} diff --git a/backend/tests/BASELINE_COUNT b/backend/tests/BASELINE_COUNT index 81995be1..a1bb2ea4 100644 --- a/backend/tests/BASELINE_COUNT +++ b/backend/tests/BASELINE_COUNT @@ -1 +1 @@ -4309 +4417 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 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 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 | 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."