fix(review): 4 issues found in agent review of PR #45
BUG-1 (medium): _validate_profile_name() used re.match() with a $ anchor.
re.match() with $ is truthy for 'name\n' because match() allows trailing
content after the $ in multiline mode. Changed to re.fullmatch() which
requires the entire string to match — trailing newlines now correctly rejected.
BUG-2 (medium/defense-in-depth): create_profile_api() validated 'name' via
_validate_profile_name() but passed clone_from directly to hermes_cli and
_create_profile_fallback() without validation. Added clone_from validation
inside create_profile_api() (skipping 'default' which is a valid clone source).
routes.py already validates it at the HTTP layer; this adds API-layer defense.
BUG-3 (low): When hermes_cli is not importable (the exact Docker case this PR
targets), list_profiles_api() also returns only the stub default dict and
can't find the newly created profile by name. The fallback return was a
2-key dict {name, path} — incomplete vs the 9-key schema everywhere else.
Expanded to the full profile dict with all fields so API clients get
consistent data regardless of hermes_cli availability.
OBS-4 (low/TOCTOU): _create_profile_fallback() checked profile_dir.exists()
then called mkdir(exist_ok=True). If a concurrent request created the dir
between those two calls, mkdir silently succeeded — defeating the
FileExistsError guard. Changed to mkdir(exist_ok=False) so the OS raises
FileExistsError atomically if the dir appears in the race window.
Tests: 423 passed, 0 failed.
This commit is contained in:
@@ -259,7 +259,8 @@ def _validate_profile_name(name: str):
|
|||||||
"""Validate profile name format (matches hermes_cli.profiles upstream)."""
|
"""Validate profile name format (matches hermes_cli.profiles upstream)."""
|
||||||
if name == 'default':
|
if name == 'default':
|
||||||
raise ValueError("Cannot create a profile named 'default' -- it is the built-in profile.")
|
raise ValueError("Cannot create a profile named 'default' -- it is the built-in profile.")
|
||||||
if not _PROFILE_ID_RE.match(name):
|
# Use fullmatch (not match) so a trailing newline can't sneak past the $ anchor
|
||||||
|
if not _PROFILE_ID_RE.fullmatch(name):
|
||||||
raise ValueError(
|
raise ValueError(
|
||||||
f"Invalid profile name {name!r}. "
|
f"Invalid profile name {name!r}. "
|
||||||
"Must match [a-z0-9][a-z0-9_-]{0,63}"
|
"Must match [a-z0-9][a-z0-9_-]{0,63}"
|
||||||
@@ -273,8 +274,8 @@ def _create_profile_fallback(name: str, clone_from: str = None,
|
|||||||
if profile_dir.exists():
|
if profile_dir.exists():
|
||||||
raise FileExistsError(f"Profile '{name}' already exists.")
|
raise FileExistsError(f"Profile '{name}' already exists.")
|
||||||
|
|
||||||
# Bootstrap directory structure
|
# Bootstrap directory structure (exist_ok=False so a concurrent create raises)
|
||||||
profile_dir.mkdir(parents=True, exist_ok=True)
|
profile_dir.mkdir(parents=True, exist_ok=False)
|
||||||
for subdir in _PROFILE_DIRS:
|
for subdir in _PROFILE_DIRS:
|
||||||
(profile_dir / subdir).mkdir(parents=True, exist_ok=True)
|
(profile_dir / subdir).mkdir(parents=True, exist_ok=True)
|
||||||
|
|
||||||
@@ -297,6 +298,10 @@ def create_profile_api(name: str, clone_from: str = None,
|
|||||||
clone_config: bool = False) -> dict:
|
clone_config: bool = False) -> dict:
|
||||||
"""Create a new profile. Returns the new profile info dict."""
|
"""Create a new profile. Returns the new profile info dict."""
|
||||||
_validate_profile_name(name)
|
_validate_profile_name(name)
|
||||||
|
# Defense-in-depth: validate clone_from here too, even though routes.py
|
||||||
|
# also validates it. Any caller that bypasses the HTTP layer gets protection.
|
||||||
|
if clone_from is not None and clone_from != 'default':
|
||||||
|
_validate_profile_name(clone_from)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
from hermes_cli.profiles import create_profile
|
from hermes_cli.profiles import create_profile
|
||||||
@@ -310,11 +315,25 @@ def create_profile_api(name: str, clone_from: str = None,
|
|||||||
except ImportError:
|
except ImportError:
|
||||||
_create_profile_fallback(name, clone_from, clone_config)
|
_create_profile_fallback(name, clone_from, clone_config)
|
||||||
|
|
||||||
# Find and return the newly created profile info
|
# Find and return the newly created profile info.
|
||||||
|
# When hermes_cli is not importable, list_profiles_api() also falls back
|
||||||
|
# to the stub default-only list and won't find the new profile by name.
|
||||||
|
# In that case, return a complete profile dict directly.
|
||||||
|
profile_path = _DEFAULT_HERMES_HOME / 'profiles' / name
|
||||||
for p in list_profiles_api():
|
for p in list_profiles_api():
|
||||||
if p['name'] == name:
|
if p['name'] == name:
|
||||||
return p
|
return p
|
||||||
return {'name': name, 'path': str(_DEFAULT_HERMES_HOME / 'profiles' / name)}
|
return {
|
||||||
|
'name': name,
|
||||||
|
'path': str(profile_path),
|
||||||
|
'is_default': False,
|
||||||
|
'is_active': _active_profile == name,
|
||||||
|
'gateway_running': False,
|
||||||
|
'model': None,
|
||||||
|
'provider': None,
|
||||||
|
'has_env': (profile_path / '.env').exists(),
|
||||||
|
'skill_count': 0,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
def delete_profile_api(name: str) -> dict:
|
def delete_profile_api(name: str) -> dict:
|
||||||
|
|||||||
Reference in New Issue
Block a user