Conversation
…e DoS (#13 #17 #18 #6) - #17: Split /health into public (status + bootstrap only) and authenticated /health/details (version, db, redis, workers, agents). Dashboard settings page uses the authenticated endpoint. - #18 L2: SSE stream returns 401 (not 422) when Authorization header is missing — require_api_key param changed to optional. - #18 L3: CORS allow_methods/allow_headers use explicit lists instead of wildcards (includes X-Edictum-Agent-Id for agents). - #18 L4: RequestValidationError handler strips ctx/type fields from Pydantic errors to avoid leaking framework internals. - #13: Startup warning when base_url is HTTPS but trusted_proxies is empty — surfaces the redirect downgrade and rate-limit keying issue. - #6: Evaluate endpoint wrapped in 5s timeout + semaphore (max 4 concurrent). Pre-parse YAML document limit (10) and post-parse contract count limit (100). All 507 existing tests pass. TypeScript compiles clean.
- Ruff: fix import sorting for RequestValidationError in main.py - Ruff B904: add `from exc` to TimeoutError raises in evaluate.py - login.tsx: remove health.version (no longer in public HealthResponse)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2868fb4beb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| except ValueError as exc: | ||
| raise HTTPException(status_code=422, detail=str(exc)) from exc | ||
| finally: |
There was a problem hiding this comment.
The finally block always releases _EVALUATE_SEMAPHORE immediately, but asyncio.wait_for(asyncio.to_thread(...), timeout=5) only times out the awaiter and does not stop the underlying worker thread; that means expensive evaluations can keep running after a timeout while new requests are still admitted, so repeated timeout payloads can still exhaust the thread pool despite this guard.
Useful? React with 👍 / 👎.
src/edictum_server/main.py
Outdated
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"], |
There was a problem hiding this comment.
Restricting CORS methods to GET/POST/PUT/DELETE/OPTIONS drops PATCH, but the app exposes PATCH routes (for example assignment rules and agent registrations) and the dashboard client calls them (dashboard/src/lib/api/agents.ts), so those browser requests will fail preflight in any cross-origin deployment even though they previously worked with wildcard methods.
Useful? React with 👍 / 👎.
![edictum-reviewer[bot]](https://avatars.githubusercontent.com/in/2923429?s=60&v=4){kind=small}
src/edictum_server/main.py
Outdated
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"], |
There was a problem hiding this comment.
🟡 Warning: allow_methods omits PATCH.
Two existing routes use PATCH:
routes/assignment_rules.py:101—PATCH /{rule_id}routes/agent_registrations.py:60—PATCH /{agent_id}
Cross-origin callers hitting these routes will fail CORS preflight — OPTIONS won't include PATCH in Access-Control-Allow-Methods and the browser will block the request before it reaches FastAPI.
Fix:
allow_methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"],
|
| # | File | Issue | Violates |
|---|---|---|---|
| 1 | src/edictum_server/main.py:408 |
allow_methods omits PATCH — two existing routes use it |
CLAUDE.md — Boring technology / no regressions |
Details
1. src/edictum_server/main.py:408 — CORS allow_methods excludes PATCH
The PR tightens CORS from ["*"] to an explicit list, which is the right move. But the new list omits PATCH:
allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],Two routes in the codebase respond to PATCH and were not touched by this PR:
src/edictum_server/routes/assignment_rules.py:101—PATCH /{rule_id}src/edictum_server/routes/agent_registrations.py:60—PATCH /{agent_id}
Browser-based cross-origin callers (edictum-hub, or any third-party integration) sending a PATCH request will fail the CORS preflight (OPTIONS will not include PATCH in Access-Control-Allow-Methods). The actual request will be blocked before it reaches FastAPI.
Suggested fix:
allow_methods=["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"],Review details
| Files reviewed | 13 |
| Checks applied | Security boundaries (S1–S8) · Tenant isolation · Input bounds · Timing-safe comparisons · CORS/headers · Resource lifecycle · DDD layer rules · shadcn compliance · Code quality |
| Issues found | 1 |
| Criteria sources | CLAUDE.md · code-reviewer.md |
Files
- ✏️
dashboard/src/lib/api/auth.ts(modified) - ✏️
dashboard/src/lib/api/index.ts(modified) - ✏️
dashboard/src/pages/login.tsx(modified) - ✏️
dashboard/src/pages/settings.tsx(modified) - ✏️
dashboard/src/pages/settings/notifications-section.tsx(modified) - ✏️
dashboard/src/pages/settings/system-section.tsx(modified) - ✏️
src/edictum_server/auth/dependencies.py(modified) - ✏️
src/edictum_server/main.py(modified) - ✏️
src/edictum_server/routes/evaluate.py(modified) - ✏️
src/edictum_server/routes/health.py(modified) - ✏️
src/edictum_server/services/evaluate_service.py(modified) - ✏️
tests/test_health.py(modified) - ✏️
tests/test_settings/test_health_enriched.py(modified)
- CORS: add PATCH to allow_methods (assignment rules + agent registrations use PATCH routes) - Evaluate semaphore: use asyncio.shield + done callback so the semaphore slot stays acquired until the worker thread actually exits, not when the HTTP timeout fires
✅ Code ReviewAll checks passed. No issues found in this PR. ✅ Checks passed
Review details
Files
Review notes (for context, not findings)
Semaphore lifecycle in
CORS tightening is correct. Validation error handler position. Registered after
|
1 participant
Summary
Fixes four pentest findings from the 2026-03-10 penetration test:
/healthreturns only{"status":"ok","bootstrap_complete":bool}. Full operational details (version, db, redis, workers, agents) moved to authenticated/health/detailsctx/typefieldsasyncio.to_thread, semaphore caps 4 concurrent evaluations, pre-parse YAML document limit (10), post-parse contract count limit (100)Review-caught regression fixed
Adversarial code review caught
X-Edictum-Agent-Idmissing from the new CORSallow_headerslist — added before merge.Test plan
pnpm tsc --noEmit)/health/detailsrequires auth, returns full infoGET /healthreturns minimal JSONX-Edictum-Agent-IdCloses #13, closes #17, closes #18, closes #6