Skip to content

Commit 75992e3

Browse files
author
Test User
committed
fix(security): add path validation and handle branch creation race condition
Security fixes: - Add validate_file_paths() to prevent directory traversal attacks in commit endpoint (rejects absolute paths, '..' segments, and paths that escape the workspace via symlink resolution) - Handle branch creation race condition by returning 409 Conflict instead of 500 when concurrent requests create the same branch Changes: - Add os import for path validation - Add validate_file_paths() function with commonpath check - Call path validation before commit_task_changes() - Update create_branch to return 409 for "already exists" errors - Update test to expect 409 for duplicate branch creation - Add 3 security tests for path validation (absolute, traversal, escape) Test coverage: 50 tests passing
1 parent 4765445 commit 75992e3

File tree

2 files changed

+112
-6
lines changed

2 files changed

+112
-6
lines changed

codeframe/ui/routers/git.py

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
"""
1010

1111
import logging
12+
import os
1213
import re
1314
from datetime import datetime, UTC
1415
from pathlib import Path
@@ -66,6 +67,49 @@ def validate_branch_name(branch_name: str) -> str:
6667
return branch_name
6768

6869

70+
def validate_file_paths(file_paths: List[str], repo_root: str) -> List[str]:
71+
"""Validate file paths to prevent directory traversal attacks.
72+
73+
Args:
74+
file_paths: List of file paths to validate
75+
repo_root: Repository root directory (working tree)
76+
77+
Returns:
78+
List of validated, resolved file paths
79+
80+
Raises:
81+
ValueError: If any path is invalid or attempts to escape the workspace
82+
"""
83+
validated_paths = []
84+
repo_root_resolved = os.path.realpath(repo_root)
85+
86+
for path in file_paths:
87+
# Reject absolute paths
88+
if os.path.isabs(path):
89+
raise ValueError(f"Absolute paths not allowed: {path}")
90+
91+
# Reject paths with '..' segments (directory traversal)
92+
if '..' in path.split(os.sep) or '..' in path.split('/'):
93+
raise ValueError(f"Path traversal not allowed: {path}")
94+
95+
# Resolve the path against repo root
96+
candidate = os.path.join(repo_root_resolved, path)
97+
resolved = os.path.realpath(candidate)
98+
99+
# Ensure resolved path is within repo root
100+
try:
101+
common = os.path.commonpath([repo_root_resolved, resolved])
102+
if common != repo_root_resolved:
103+
raise ValueError(f"Path escapes workspace: {path}")
104+
except ValueError:
105+
# commonpath raises ValueError for paths on different drives (Windows)
106+
raise ValueError(f"Path escapes workspace: {path}")
107+
108+
validated_paths.append(path)
109+
110+
return validated_paths
111+
112+
69113
logger = logging.getLogger(__name__)
70114

71115
router = APIRouter(prefix="/api/projects", tags=["git"])
@@ -263,9 +307,10 @@ async def create_branch(
263307
264308
Raises:
265309
HTTPException:
266-
- 400: Branch already exists or invalid parameters
310+
- 400: Invalid parameters
267311
- 403: Access denied
268312
- 404: Project or issue not found
313+
- 409: Branch already exists (including concurrent creation)
269314
- 500: Git operation failed
270315
"""
271316
# Get project and check access
@@ -314,9 +359,20 @@ async def create_branch(
314359
)
315360

316361
except ValueError as e:
317-
# Branch already exists or validation error
362+
# Branch already exists (pre-check) or validation error
363+
error_msg = str(e).lower()
364+
if "already exists" in error_msg:
365+
raise HTTPException(status_code=409, detail=str(e))
318366
raise HTTPException(status_code=400, detail=str(e))
319367
except git.GitCommandError as e:
368+
# Handle race condition: branch created between check and create_head
369+
error_msg = str(e).lower()
370+
if "already exists" in error_msg or "cannot lock ref" in error_msg:
371+
logger.warning(f"Branch creation race condition detected: {e}")
372+
raise HTTPException(
373+
status_code=409,
374+
detail=f"Branch already exists (concurrent creation detected)"
375+
)
320376
logger.error(f"Git error creating branch: {e}")
321377
raise HTTPException(status_code=500, detail=f"Git operation failed: {e}")
322378

@@ -491,6 +547,12 @@ async def create_commit(
491547
# Get workflow manager
492548
workflow_manager = get_git_workflow_manager(project, db)
493549

550+
# Validate file paths to prevent directory traversal attacks
551+
try:
552+
validate_file_paths(request.files_modified, workflow_manager.repo.working_tree_dir)
553+
except ValueError as e:
554+
raise HTTPException(status_code=400, detail=str(e))
555+
494556
try:
495557
# Create task dict for commit_task_changes
496558
task_dict = {

tests/api/test_git_api.py

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -259,8 +259,8 @@ def test_create_branch_requires_issue_title(self, api_client, test_project_with_
259259
)
260260
assert response.status_code == 422
261261

262-
def test_create_duplicate_branch_returns_400(self, api_client, test_project_with_issue):
263-
"""Test that creating duplicate branch returns 400."""
262+
def test_create_duplicate_branch_returns_409_conflict(self, api_client, test_project_with_issue):
263+
"""Test that creating duplicate branch returns 409 Conflict."""
264264
project = test_project_with_issue
265265

266266
# Create branch first time
@@ -272,15 +272,16 @@ def test_create_duplicate_branch_returns_400(self, api_client, test_project_with
272272
},
273273
)
274274

275-
# Try to create same branch again
275+
# Try to create same branch again - should return 409 Conflict
276276
response = api_client.post(
277277
f"/api/projects/{project['project_id']}/git/branches",
278278
json={
279279
"issue_number": project["issue_number"],
280280
"issue_title": project["issue_title"],
281281
},
282282
)
283-
assert response.status_code == 400
283+
assert response.status_code == 409
284+
assert "already exists" in response.json()["detail"].lower()
284285

285286
def test_create_branch_issue_not_found(self, api_client, test_project_with_git):
286287
"""Test that creating branch with non-existent issue returns 404."""
@@ -599,6 +600,49 @@ def test_commit_task_not_found(self, api_client, test_project_with_git):
599600
)
600601
assert response.status_code == 404
601602

603+
def test_commit_rejects_absolute_paths(self, api_client, test_project_with_task):
604+
"""Test that absolute file paths are rejected (security)."""
605+
project = test_project_with_task
606+
response = api_client.post(
607+
f"/api/projects/{project['project_id']}/git/commit",
608+
json={
609+
"task_id": project["task_id"],
610+
"files_modified": ["/etc/passwd"],
611+
"agent_id": "backend-worker-001",
612+
},
613+
)
614+
assert response.status_code == 400
615+
assert "absolute" in response.json()["detail"].lower()
616+
617+
def test_commit_rejects_path_traversal(self, api_client, test_project_with_task):
618+
"""Test that path traversal attempts are rejected (security)."""
619+
project = test_project_with_task
620+
response = api_client.post(
621+
f"/api/projects/{project['project_id']}/git/commit",
622+
json={
623+
"task_id": project["task_id"],
624+
"files_modified": ["../../../etc/passwd"],
625+
"agent_id": "backend-worker-001",
626+
},
627+
)
628+
assert response.status_code == 400
629+
assert "traversal" in response.json()["detail"].lower()
630+
631+
def test_commit_rejects_workspace_escape(self, api_client, test_project_with_task):
632+
"""Test that paths escaping workspace are rejected (security)."""
633+
project = test_project_with_task
634+
# Use a path that doesn't have '..' but could resolve outside via symlinks
635+
# This tests the commonpath check
636+
response = api_client.post(
637+
f"/api/projects/{project['project_id']}/git/commit",
638+
json={
639+
"task_id": project["task_id"],
640+
"files_modified": ["subdir/../../../outside.txt"],
641+
"agent_id": "backend-worker-001",
642+
},
643+
)
644+
assert response.status_code == 400
645+
602646

603647
# ============================================================================
604648
# Commit Listing Tests

0 commit comments

Comments
 (0)