Skip to content

Commit 1ea3539

Browse files
author
Tomas Pflanzer
committed
fix: 5 security/reliability bugs from Codex audit round 4
P2: - Export endpoint requires admin auth (was leaking YAML to tenants) - Version history listing no longer auto-creates production version - Idempotency key UNIQUE constraint now composite with tenant_id - Sync run wraps execute in try/except (prevents RUNNING stuck state) P3: - useSSE resets events array on path change (no more mixed run data)
1 parent cfb34cb commit 1ea3539

5 files changed

Lines changed: 52 additions & 28 deletions

File tree

dashboard/src/hooks/useSSE.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ export function useSSE(path: string | null) {
2424
useEffect(() => {
2525
if (!path) return;
2626

27+
// Reset events when path changes so old events from a previous
28+
// run/stream don't mix with new ones.
29+
setEvents([]);
30+
2731
const controller = new AbortController();
2832
abortRef.current = controller;
2933

src/sandcastle/api/routes.py

Lines changed: 34 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2295,6 +2295,7 @@ async def export_workflow(name: str, request: Request) -> ApiResponse:
22952295
22962296
Removes environment variable references and sensitive data.
22972297
"""
2298+
_require_admin(request)
22982299
async with async_session() as session:
22992300
result = await session.execute(
23002301
select(WorkflowVersion)
@@ -4420,14 +4421,28 @@ async def run_workflow_sync(request: WorkflowRunRequest, req: Request) -> ApiRes
44204421
).model_dump(),
44214422
)
44224423

4423-
result = await execute_workflow(
4424-
workflow=workflow,
4425-
plan=plan,
4426-
input_data=request.input,
4427-
run_id=run_id,
4428-
storage=storage,
4429-
max_cost_usd=budget,
4430-
)
4424+
try:
4425+
result = await execute_workflow(
4426+
workflow=workflow,
4427+
plan=plan,
4428+
input_data=request.input,
4429+
run_id=run_id,
4430+
storage=storage,
4431+
max_cost_usd=budget,
4432+
)
4433+
except Exception as exc:
4434+
# Mark the run as FAILED so it doesn't stay stuck in RUNNING.
4435+
try:
4436+
async with async_session() as session:
4437+
db_run = await session.get(Run, uuid.UUID(run_id))
4438+
if db_run:
4439+
db_run.status = RunStatus.FAILED
4440+
db_run.error = f"Engine error: {exc}"
4441+
db_run.completed_at = datetime.now(timezone.utc)
4442+
await session.commit()
4443+
except Exception:
4444+
logger.error("Failed to mark run %s as FAILED after engine error", run_id, exc_info=True)
4445+
raise
44314446

44324447
# Map result status to RunStatus
44334448
status_map = {
@@ -8445,23 +8460,17 @@ async def list_workflow_versions(
84458460
versions = result.scalars().all()
84468461

84478462
if not versions and total == 0:
8448-
# Try auto-import from disk
8449-
try:
8450-
yaml_content = _load_workflow_yaml(name)
8451-
await _auto_import_workflow(name, yaml_content)
8452-
# Re-query
8453-
async with async_session() as session:
8454-
total = 1
8455-
stmt = select(WorkflowVersion).where(WorkflowVersion.workflow_name == name)
8456-
result = await session.execute(stmt)
8457-
versions = result.scalars().all()
8458-
except FileNotFoundError:
8459-
raise HTTPException(
8460-
status_code=404,
8461-
detail=ApiResponse(
8462-
error=ErrorResponse(code="NOT_FOUND", message=f"Workflow '{name}' not found")
8463-
).model_dump(),
8464-
)
8463+
# Return empty list instead of auto-importing (read-only endpoint
8464+
# should not create production versions as a side effect).
8465+
return ApiResponse(
8466+
data={
8467+
"versions": [],
8468+
"total": 0,
8469+
"production_version": None,
8470+
"staging_version": None,
8471+
"draft_version": None,
8472+
}
8473+
)
84658474

84668475
prod_ver = None
84678476
staging_ver = None

src/sandcastle/models/db.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ class Run(Base):
8080
Index("ix_runs_api_key_id", "api_key_id"),
8181
CheckConstraint("total_cost_usd >= 0", name="ck_runs_total_cost_non_negative"),
8282
CheckConstraint("depth >= 0", name="ck_runs_depth_non_negative"),
83+
UniqueConstraint("tenant_id", "idempotency_key", name="uq_tenant_idempotency_key"),
8384
)
8485

8586
id: Mapped[uuid.UUID] = mapped_column(
@@ -97,7 +98,7 @@ class Run(Base):
9798
error: Mapped[str | None] = mapped_column(Text, nullable=True)
9899
callback_url: Mapped[str | None] = mapped_column(String(2048), nullable=True)
99100
tenant_id: Mapped[str | None] = mapped_column(String(255), nullable=True)
100-
idempotency_key: Mapped[str | None] = mapped_column(String(255), nullable=True, unique=True)
101+
idempotency_key: Mapped[str | None] = mapped_column(String(255), nullable=True)
101102
max_cost_usd: Mapped[float | None] = mapped_column(Float, nullable=True)
102103
parent_run_id: Mapped[uuid.UUID | None] = mapped_column(
103104
Uuid, ForeignKey("runs.id", ondelete="SET NULL"), nullable=True

tests/test_api_full_coverage.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1377,7 +1377,11 @@ class TestWorkflowVersions:
13771377

13781378
def test_list_versions_nonexistent_workflow(self):
13791379
response = client.get("/api/workflows/nonexistent-wf/versions")
1380-
assert response.status_code == 404
1380+
# Returns 200 with empty list (read-only endpoint, no auto-import)
1381+
assert response.status_code == 200
1382+
data = response.json()["data"]
1383+
assert data["versions"] == []
1384+
assert data["total"] == 0
13811385

13821386
def test_get_version_nonexistent(self):
13831387
response = client.get("/api/workflows/nonexistent-wf/versions/1")

tests/test_concurrency_wave9.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,14 +234,19 @@ async def add_cost(amount: float):
234234

235235
@pytest.mark.asyncio
236236
async def test_idempotency_key_concurrent_inserts(self):
237-
"""Two concurrent inserts with same idempotency_key - one must fail."""
237+
"""Two concurrent inserts with same tenant + idempotency_key - one must fail.
238+
239+
The unique constraint is composite (tenant_id, idempotency_key) so the
240+
same key can coexist across different tenants but not within one.
241+
"""
238242
from sqlalchemy.exc import IntegrityError
239243

240244
from sandcastle.models.db import Run, RunStatus
241245

242246
eng, sf = await _setup_test_db()
243247
try:
244248
idem_key = "unique-request-123"
249+
tenant = "tenant-idem-test"
245250
results = []
246251

247252
async def insert_with_idem(idx: int):
@@ -253,6 +258,7 @@ async def insert_with_idem(idx: int):
253258
status=RunStatus.QUEUED,
254259
input_data={},
255260
idempotency_key=idem_key,
261+
tenant_id=tenant,
256262
)
257263
session.add(run)
258264
await session.commit()

0 commit comments

Comments
 (0)