diff --git a/app/app.py b/app/app.py index 8a603f2..1252f0a 100644 --- a/app/app.py +++ b/app/app.py @@ -103,6 +103,19 @@ def create_app(config=None): allowed_origins = str(app.config.get('CORS_ALLOWED_ORIGINS') or '').split(',') allowed_origins = [origin.strip() for origin in allowed_origins if origin.strip()] + # CORS is only configured when origins are named explicitly. + # + # The previous else-branch called CORS(app, supports_credentials=True) + # with no origins argument. flask-cors then defaults to '*' and, because + # credentials are allowed, echoes back whatever Origin the caller sent + # together with Access-Control-Allow-Credentials: true — the opposite of + # the "allow all (development) or none (production)" the comment claimed. + # + # Exploitation was blocked by SESSION_COOKIE_SAMESITE = 'Lax', which stops + # the browser attaching the session cookie to a cross-site fetch. That is + # a single setting standing between a misconfiguration and a cross-origin + # data leak. This application renders server-side HTML on one origin and + # needs no CORS policy at all. if allowed_origins: CORS( app, @@ -111,15 +124,6 @@ def create_app(config=None): methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], max_age=3600, # Cache preflight for 1 hour ) - else: - # When no origins specified, allow all (development) or none (production) - # In production with a reverse proxy, CORS is handled at the Nginx level - CORS( - app, - supports_credentials=True, - methods=['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], - max_age=3600, - ) db.init_app(app) login_manager.init_app(app) diff --git a/app/routes/users.py b/app/routes/users.py index 147d8b3..3e86767 100644 --- a/app/routes/users.py +++ b/app/routes/users.py @@ -166,6 +166,25 @@ def edit_user(user_id): flash('Email already in use by another account.', 'danger') return _rerender() + if user.role != role: + # Two ways to lock everyone out of administration, neither of + # which any interface can undo afterwards. + if user.id == actor_id: + flash('You cannot change your own role. Ask another president ' + 'to do it.', 'danger') + return _rerender() + + if user.role == 'admin': + remaining_admins = User.query.filter( + User.role == 'admin', + User.is_active_account.is_(True), + User.id != user.id, + ).count() + if remaining_admins == 0: + flash('This is the last active president. Promote another ' + 'account before changing this one.', 'danger') + return _rerender() + # Change role via raw SQL to avoid polymorphic identity corruption. # Must discard the entire session because the polymorphic discriminator # change invalidates the identity map for this instance and anything diff --git a/tests/test_authorization.py b/tests/test_authorization.py index bf6d860..1df8eb9 100644 --- a/tests/test_authorization.py +++ b/tests/test_authorization.py @@ -302,11 +302,6 @@ class TestInputValidation: class TestAdminSafety: - @pytest.mark.xfail( - strict=True, - reason='SEC-AUTHZ-007: the role change excludes neither the current ' - 'user nor the last remaining admin', - ) def test_the_last_admin_cannot_demote_itself(self, app, client, as_role): admin_id = as_role('admin') @@ -322,6 +317,34 @@ class TestAdminSafety: ) + def test_an_admin_cannot_change_its_own_role_even_with_others_present( + self, app, client, as_role, make_user + ): + """Self-demotion is refused on its own, not only when last.""" + make_user('admin') + admin_id = as_role('admin') + + client.post(f'/users/{admin_id}/edit', data={ + 'full_name': 'Admin', 'email': 'self@example.test', 'role': 'player', + }, follow_redirects=True) + + with app.app_context(): + assert db.session.get(User, admin_id).role == 'admin' + + def test_another_admin_can_still_be_demoted(self, app, client, as_role, make_user): + """Guard against over-correcting: the legitimate path must work.""" + other_id = make_user('admin') + as_role('admin') + + client.post(f'/users/{other_id}/edit', data={ + 'full_name': 'Other', 'email': 'other-demoted@example.test', + 'role': 'coach', 'is_active_account': 'on', + }, follow_redirects=True) + + with app.app_context(): + assert db.session.get(User, other_id).role == 'coach' + + class TestCsrf: def test_state_changing_post_without_a_token_is_rejected(self, app_with_csrf): """CSRFProtect is global. This pins that down so a future @@ -331,3 +354,25 @@ class TestCsrf: 'username': 'someone', 'password': 'Password123', }) assert response.status_code == 400 + +class TestCorsPolicy: + """SEC-WEB-003 — with no origins configured, flask-cors defaulted to '*' + and, credentials being allowed, echoed back the caller's Origin.""" + + def test_no_cors_headers_without_explicit_configuration(self, client): + response = client.get('/auth/login', headers={'Origin': 'https://evil.test'}) + assert 'Access-Control-Allow-Origin' not in response.headers + assert 'Access-Control-Allow-Credentials' not in response.headers + + def test_configured_origins_are_still_honoured(self, app_with_csrf): + from app.app import create_app + + application = create_app({ + 'SECRET_KEY': 'test', 'SQLALCHEMY_DATABASE_URI': 'sqlite:///:memory:', + 'TESTING': True, 'FORCE_HTTPS': False, 'ENABLE_DISCORD_BOT': False, + 'AUTO_CREATE_TABLES': False, 'CORS_ALLOWED_ORIGINS': 'https://trusted.test', + }) + response = application.test_client().get( + '/auth/login', headers={'Origin': 'https://trusted.test'}) + + assert response.headers.get('Access-Control-Allow-Origin') == 'https://trusted.test'