Skip to content

Commit f050948

Browse files
security: fix critical auth vulnerabilities in bind-aware gateway
- Fix fail-open auth in token mode (now fails closed when expected_token is None) - Fix token file permissions race condition (create with 0o600 atomically) - Fix stale config validation (align bind_host with actual server host) - Add IPv6 bracket handling and remove non-standard 'local' alias - Fix configured auth mode being ignored in assert_external_bind_safe - Add empty credentials rejection on external interfaces - Use constant-time comparison for password checks - Fix test reliability issues (exception swallowing in async tests) - Fix GatewayConfig hardcoded bind_host (now defaults to host) - Update test mocking to work with local chainlit imports Addresses security vulnerabilities identified by CodeRabbit review. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 parent 4be8dac commit f050948

8 files changed

Lines changed: 131 additions & 81 deletions

File tree

src/praisonai-agents/praisonaiagents/gateway/config.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ class GatewayConfig:
210210

211211
host: str = "127.0.0.1"
212212
port: int = 8765
213-
bind_host: str = "127.0.0.1" # For authentication posture resolution
213+
bind_host: str = "" # Defaults to host for authentication posture resolution
214214
cors_origins: List[str] = field(default_factory=lambda: [])
215215
auth_token: Optional[str] = None
216216
max_connections: int = 1000
@@ -222,6 +222,11 @@ class GatewayConfig:
222222
ssl_key: Optional[str] = None
223223
push: PushConfig = field(default_factory=PushConfig)
224224

225+
def __post_init__(self) -> None:
226+
"""Set bind_host to host if not explicitly provided."""
227+
if not self.bind_host:
228+
self.bind_host = self.host
229+
225230
def to_dict(self) -> Dict[str, Any]:
226231
"""Convert to dictionary (hides sensitive data)."""
227232
return {

src/praisonai-agents/praisonaiagents/gateway/protocols.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -723,16 +723,20 @@ def is_loopback(host: str) -> bool:
723723

724724
# Handle common string representations
725725
host = host.lower().strip()
726-
if host in ("localhost", "local"):
726+
if host == "localhost":
727727
return True
728728

729+
# Strip brackets from IPv6 literal forms like "[::1]"
730+
if host.startswith("[") and host.endswith("]"):
731+
host = host[1:-1]
732+
729733
try:
730734
import ipaddress
731735
# Try to parse as IP address
732736
addr = ipaddress.ip_address(host)
733737
return addr.is_loopback
734-
except (ValueError, ImportError):
735-
# Not a valid IP address or ipaddress not available
738+
except ValueError:
739+
# Not a valid IP literal
736740
return False
737741

738742

src/praisonai/praisonai/gateway/auth.py

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,9 @@ def check_request_auth(
9999

100100
if auth_mode == "token":
101101
if not expected_token:
102-
# No token configured, allow (should not happen after validation)
103-
logger.warning("Token mode requested but no expected token configured")
104-
return True
102+
# Fail closed: token mode with no expected token is a misconfiguration
103+
logger.error("Token mode requested but no expected token configured; denying request")
104+
return False
105105

106106
if not request_token:
107107
return False
@@ -128,9 +128,10 @@ def assert_external_bind_safe(config) -> None:
128128
# Use bind_host if available, otherwise fall back to host
129129
bind_host = getattr(config, 'bind_host', None) or config.host
130130
auth_token = getattr(config, 'auth_token', None)
131+
configured_mode = getattr(config, 'auth_mode', None)
131132

132133
# Resolve authentication mode based on bind interface
133-
auth_mode = resolve_auth_mode(bind_host, configured=None)
134+
auth_mode = resolve_auth_mode(bind_host, configured=configured_mode)
134135

135136
# Create enforcer and validate
136137
enforcer = GatewayAuthEnforcer()
@@ -181,13 +182,15 @@ def ensure_token_env_file(token: str, env_file_path: Optional[str] = None) -> No
181182
env_file_path = os.path.join(praisonai_dir, ".env")
182183

183184
try:
184-
# Ensure directory exists
185-
os.makedirs(os.path.dirname(env_file_path), mode=0o700, exist_ok=True)
185+
# Ensure directory exists (skip when env_file_path has no directory component)
186+
parent = os.path.dirname(env_file_path)
187+
if parent:
188+
os.makedirs(parent, mode=0o700, exist_ok=True)
186189

187190
# Read existing content
188191
existing_lines = []
189192
if os.path.exists(env_file_path):
190-
with open(env_file_path, 'r') as f:
193+
with open(env_file_path, 'r', encoding='utf-8') as f:
191194
existing_lines = f.readlines()
192195

193196
# Check if GATEWAY_AUTH_TOKEN already exists
@@ -202,11 +205,11 @@ def ensure_token_env_file(token: str, env_file_path: Optional[str] = None) -> No
202205
if not token_line_found:
203206
existing_lines.append(f"GATEWAY_AUTH_TOKEN={token}\n")
204207

205-
# Write back to file with secure permissions
206-
with open(env_file_path, 'w') as f:
208+
# Write atomically with secure permissions from the start
209+
fd = os.open(env_file_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
210+
with os.fdopen(fd, 'w', encoding='utf-8') as f:
207211
f.writelines(existing_lines)
208-
209-
# Set secure file permissions (owner read/write only)
212+
# Defensive chmod in case the file pre-existed with looser perms
210213
os.chmod(env_file_path, 0o600)
211214

212215
logger.debug(f"Gateway auth token persisted to {env_file_path}")

src/praisonai/praisonai/gateway/server.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,12 @@ async def start(self) -> None:
279279
logger.warning("Gateway already running")
280280
return
281281

282+
# Keep authentication posture aligned with the host Uvicorn will bind
283+
if getattr(self.config, "host", None) != self._host:
284+
self.config.host = self._host
285+
if not getattr(self.config, "bind_host", None) or self.config.bind_host != self._host:
286+
self.config.bind_host = self._host
287+
282288
# Validate authentication configuration before starting
283289
from .auth import assert_external_bind_safe
284290
try:

src/praisonai/praisonai/ui/_auth.py

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
import logging
1010
import os
11+
import secrets
1112
from typing import Optional
1213

1314
# Import protocols from core SDK
@@ -52,6 +53,12 @@ def validate_credentials_config(
5253
"""
5354
is_default_creds = (username == "admin" and password == "admin")
5455
is_external = not is_loopback(bind_host)
56+
57+
if is_external and (not username or not password):
58+
raise UIStartupError(
59+
f"Cannot use empty Chainlit credentials on external interface {bind_host}.\n"
60+
f" Fix: Set non-empty CHAINLIT_USERNAME and CHAINLIT_PASSWORD environment variables"
61+
)
5562

5663
if is_default_creds and is_external and not allow_defaults:
5764
raise UIStartupError(
@@ -68,8 +75,8 @@ def validate_credentials_config(
6875
)
6976
else:
7077
logger.warning(
71-
f"⚠️ Using default admin/admin credentials on localhost. "
72-
f"Set CHAINLIT_USERNAME and CHAINLIT_PASSWORD for production."
78+
"⚠️ Using default admin/admin credentials on localhost. "
79+
"Set CHAINLIT_USERNAME and CHAINLIT_PASSWORD for production."
7380
)
7481

7582
def check_auth_callback(
@@ -92,7 +99,21 @@ def check_auth_callback(
9299
Returns:
93100
True if credentials are valid, False otherwise
94101
"""
95-
return (provided_username, provided_password) == (expected_username, expected_password)
102+
if not all(
103+
isinstance(value, str)
104+
for value in (
105+
provided_username,
106+
provided_password,
107+
expected_username,
108+
expected_password,
109+
)
110+
):
111+
return False
112+
113+
return (
114+
provided_username == expected_username
115+
and secrets.compare_digest(provided_password, expected_password)
116+
)
96117

97118

98119
def register_password_auth(app, *, bind_host: str) -> None:
@@ -126,12 +147,12 @@ def register_password_auth(app, *, bind_host: str) -> None:
126147
)
127148
except UIStartupError as e:
128149
# Convert to runtime error to prevent startup
129-
raise RuntimeError(str(e))
150+
raise RuntimeError(str(e)) from e
130151

131152
@cl.password_auth_callback
132153
def auth_callback(username: str, password: str):
133154
"""Shared password authentication callback."""
134-
logger.debug(f"Auth attempt: username='{username}', expected='{expected_username}'")
155+
logger.debug("Password auth attempt")
135156

136157
# Use enforcer to check credentials
137158
is_valid = enforcer.check_auth_callback(
@@ -143,10 +164,10 @@ def auth_callback(username: str, password: str):
143164
)
144165

145166
if is_valid:
146-
logger.info(f"Login successful for user: {username}")
167+
logger.info("Password auth successful")
147168
return cl.User(identifier=username, metadata={"role": "admin", "provider": "credentials"})
148169
else:
149-
logger.warning(f"Login failed for user: {username}")
170+
logger.warning("Password auth failed")
150171
return None
151172

152173
return auth_callback

src/praisonai/tests/integration/gateway/test_bind_aware_e2e.py

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,17 +87,22 @@ async def _start_gateway_briefly(self, gateway):
8787
# Give it a moment to start and perform validation
8888
await asyncio.sleep(0.1)
8989

90+
# If startup already failed (e.g. validation raised), surface it now
91+
if start_task.done():
92+
# Re-raises any exception stored on the task
93+
start_task.result()
94+
return
95+
9096
# Stop the gateway
9197
if gateway.is_running:
9298
await gateway.stop()
9399

94100
# Cancel the start task if it's still running
95-
if not start_task.done():
96-
start_task.cancel()
97-
try:
98-
await start_task
99-
except asyncio.CancelledError:
100-
pass
101+
start_task.cancel()
102+
try:
103+
await start_task
104+
except asyncio.CancelledError:
105+
pass
101106

102107

103108
class TestRealAgentExecution:

src/praisonai/tests/unit/gateway/test_bind_aware_auth.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ def test_ipv4_loopback_addresses(self):
3232
def test_ipv6_loopback_addresses(self):
3333
"""Test IPv6 loopback addresses are correctly identified."""
3434
assert is_loopback("::1") is True
35+
assert is_loopback("[::1]") is True
3536
assert is_loopback("0:0:0:0:0:0:0:1") is True
3637

3738
def test_localhost_strings(self):
@@ -167,14 +168,13 @@ def test_check_request_auth_token_mode_requires_token(self):
167168
expected_token="secret"
168169
) is True
169170

170-
def test_check_request_auth_no_expected_token_allows_all(self):
171-
"""Test that token mode with no expected token allows all requests."""
172-
# This handles the case where validation failed to catch the issue
171+
def test_check_request_auth_no_expected_token_fails_closed(self):
172+
"""Test that token mode with no expected token denies requests."""
173173
assert self.enforcer.check_request_auth(
174174
auth_mode="token",
175175
request_token=None,
176176
expected_token=None
177-
) is True
177+
) is False
178178

179179

180180
class TestAssertExternalBindSafe:

src/praisonai/tests/unit/ui/test_ui_bind_aware_creds.py

Lines changed: 55 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import pytest
99
import os
10+
import sys
1011
from unittest.mock import patch, MagicMock
1112

1213
from praisonai.ui._auth import (
@@ -117,75 +118,80 @@ def test_check_auth_callback_with_incorrect_credentials(self):
117118
class TestRegisterPasswordAuth:
118119
"""Test the register_password_auth function."""
119120

120-
@patch('praisonai.ui._auth.cl')
121121
@patch.dict(os.environ, {}, clear=True)
122-
def test_default_credentials_on_loopback(self, mock_cl):
122+
def test_default_credentials_on_loopback(self):
123123
"""Test registration with default credentials on loopback."""
124-
# Should not raise
125-
register_password_auth(None, bind_host="127.0.0.1")
126-
127-
# Should have registered a callback
128-
mock_cl.password_auth_callback.assert_called_once()
124+
mock_cl = MagicMock()
125+
with patch.dict(sys.modules, {"chainlit": mock_cl}):
126+
# Should not raise
127+
register_password_auth(None, bind_host="127.0.0.1")
128+
129+
# Should have registered a callback
130+
mock_cl.password_auth_callback.assert_called_once()
129131

130-
@patch('praisonai.ui._auth.cl')
131132
@patch.dict(os.environ, {}, clear=True)
132-
def test_default_credentials_on_external_raises_error(self, mock_cl):
133+
def test_default_credentials_on_external_raises_error(self):
133134
"""Test registration with default credentials on external interface raises error."""
134-
with pytest.raises(RuntimeError) as excinfo:
135-
register_password_auth(None, bind_host="0.0.0.0")
136-
137-
assert "Cannot use default admin/admin credentials on external interface" in str(excinfo.value)
135+
mock_cl = MagicMock()
136+
with patch.dict(sys.modules, {"chainlit": mock_cl}):
137+
with pytest.raises(RuntimeError) as excinfo:
138+
register_password_auth(None, bind_host="0.0.0.0")
139+
140+
assert "Cannot use default admin/admin credentials on external interface" in str(excinfo.value)
138141

139-
@patch('praisonai.ui._auth.cl')
140142
@patch.dict(os.environ, {"PRAISONAI_ALLOW_DEFAULT_CREDS": "1"}, clear=True)
141-
def test_default_credentials_on_external_with_escape_hatch(self, mock_cl):
143+
def test_default_credentials_on_external_with_escape_hatch(self):
142144
"""Test registration with default credentials on external interface with escape hatch."""
143-
# Should not raise with escape hatch
144-
register_password_auth(None, bind_host="0.0.0.0")
145-
146-
# Should have registered a callback
147-
mock_cl.password_auth_callback.assert_called_once()
145+
mock_cl = MagicMock()
146+
with patch.dict(sys.modules, {"chainlit": mock_cl}):
147+
# Should not raise with escape hatch
148+
register_password_auth(None, bind_host="0.0.0.0")
149+
150+
# Should have registered a callback
151+
mock_cl.password_auth_callback.assert_called_once()
148152

149-
@patch('praisonai.ui._auth.cl')
150153
@patch.dict(os.environ, {
151154
"CHAINLIT_USERNAME": "myuser",
152155
"CHAINLIT_PASSWORD": "mypassword"
153156
}, clear=True)
154-
def test_custom_credentials_on_external(self, mock_cl):
157+
def test_custom_credentials_on_external(self):
155158
"""Test registration with custom credentials on external interface."""
156-
# Should not raise
157-
register_password_auth(None, bind_host="0.0.0.0")
158-
159-
# Should have registered a callback
160-
mock_cl.password_auth_callback.assert_called_once()
159+
mock_cl = MagicMock()
160+
with patch.dict(sys.modules, {"chainlit": mock_cl}):
161+
# Should not raise
162+
register_password_auth(None, bind_host="0.0.0.0")
163+
164+
# Should have registered a callback
165+
mock_cl.password_auth_callback.assert_called_once()
161166

162-
@patch('praisonai.ui._auth.cl')
163167
@patch.dict(os.environ, {
164168
"CHAINLIT_USERNAME": "myuser",
165169
"CHAINLIT_PASSWORD": "mypassword"
166170
}, clear=True)
167-
def test_auth_callback_functionality(self, mock_cl):
171+
def test_auth_callback_functionality(self):
168172
"""Test that the registered auth callback works correctly."""
169-
register_password_auth(None, bind_host="127.0.0.1")
170-
171-
# Get the registered callback function
172-
callback = mock_cl.password_auth_callback.call_args[0][0]
173-
174-
# Mock User class
175-
mock_user = MagicMock()
176-
mock_cl.User.return_value = mock_user
177-
178-
# Test correct credentials
179-
result = callback("myuser", "mypassword")
180-
assert result == mock_user
181-
mock_cl.User.assert_called_with(
182-
identifier="myuser",
183-
metadata={"role": "admin", "provider": "credentials"}
184-
)
185-
186-
# Test incorrect credentials
187-
result = callback("wrong", "credentials")
188-
assert result is None
173+
mock_cl = MagicMock()
174+
with patch.dict(sys.modules, {"chainlit": mock_cl}):
175+
register_password_auth(None, bind_host="127.0.0.1")
176+
177+
# Get the registered callback function
178+
callback = mock_cl.password_auth_callback.call_args[0][0]
179+
180+
# Mock User class
181+
mock_user = MagicMock()
182+
mock_cl.User.return_value = mock_user
183+
184+
# Test correct credentials
185+
result = callback("myuser", "mypassword")
186+
assert result == mock_user
187+
mock_cl.User.assert_called_with(
188+
identifier="myuser",
189+
metadata={"role": "admin", "provider": "credentials"}
190+
)
191+
192+
# Test incorrect credentials
193+
result = callback("wrong", "credentials")
194+
assert result is None
189195

190196

191197
class TestEnvironmentVariables:

0 commit comments

Comments
 (0)