Fix significant audit findings: async I/O, public URL, Bearer case, Dockerfile#98
Conversation
…ockerfile license
S1: _try_oauth and _resolve_user now use asyncio.to_thread() — prevents
blocking the async event loop with sync psycopg DB calls
S2: AWARENESS_PUBLIC_URL env var for well-known metadata — fixes
0.0.0.0:8420 in resource URL for Cloudflare tunnel deployments
S3: _sync_custom_prompts documented as DEFAULT_OWNER-only (known limitation)
S4: Bearer scheme check is now case-insensitive per RFC 7235
S5: Dockerfile license label corrected from Apache-2.0 to AGPL-3.0-or-later
S6: Auth/OAuth env vars added to docker-compose.yaml
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Updated test calls to use keyword args after signature change. New test verifies public_url is used in resource metadata. middleware.py at 100% coverage. 491 tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codecov note: 2 lines remain uncovered in |
S7: Per-owner semaphore (max 3 concurrent requests) prevents a single aggressive MCP client from saturating the pool and DOSing other tenants. Returns 429 when all slots are taken. Connection pool default bumped from 5 to 10 for multi-tenant deployments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When AWARENESS_PUBLIC_URL is not set, the resource URL in /.well-known/oauth-protected-resource is now derived from the request's Host header instead of the bind address (0.0.0.0:8420). This handles reverse proxy / tunnel deployments without explicit config. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two new tests: - WellKnownMiddleware derives resource URL from Host header when no public_url - 429 returned when per-owner concurrency slots exhausted middleware.py at 100% coverage. 493 tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #98: Fix significant audit findings
QA Failed — codecov/patch is FAILURE.
All other checks pass (lint, typecheck, test 3.10/3.11/3.12). Coverage gap needs to be addressed before QA can proceed.
Will do full review once codecov is green.
|
Applying QA Failed — codecov/patch check is FAILURE. Dev needs to fix coverage gap before re-review. |
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #98: Fix significant audit findings
Code Review
All 6 significant findings addressed, plus bonus improvements (concurrency limit, pool size):
S1: Sync DB I/O off event loop
_try_oauth():validator.validate(token)→await asyncio.to_thread(validator.validate, token)✅_resolve_user(): called viaawait asyncio.to_thread(self._resolve_user, ...)✅- Both sync psycopg paths now run in thread pool, won't block the async event loop
S2: AWARENESS_PUBLIC_URL
- New env var, read in
server.py, passed toWellKnownMiddleware _resource_url()priority: static public_url → Host header fallback →localhost- Verified:
AWARENESS_PUBLIC_URL=https://mcpawareness.com→resource: https://mcpawareness.com/mcp
S3: Custom prompts multi-tenant
- Documented as DEFAULT_OWNER-only limitation with clear NOTE in
_sync_custom_prompts()docstring ✅
S4: Case-insensitive Bearer
auth_header.startswith("Bearer ")→auth_header.lower().startswith("bearer ")✅- Token extraction still uses
auth_header[7:](correct — scheme length is always 7) - Verified:
bearer,Bearer,BEARERall return 200
S5: Dockerfile license
Apache-2.0→AGPL-3.0-or-later✅- Verified via
docker inspect
S6: docker-compose.yaml auth vars
- 9 auth/OAuth env vars added with sensible defaults ✅
Bonus: Per-owner concurrency limit
max_concurrent_per_owner=3with per-owner semaphores- Returns 429 when all slots taken (
sem._valuecheck for immediate rejection) - Prevents single-client DOS of connection pool
Bonus: Connection pool default
max_pool5 → 10 for multi-tenant
CI Exception
codecov/patch FAILURE — owner-acknowledged exception. The uncovered lines were reviewed and accepted by the repo owner before QA.
Test Results
| Check | Result |
|---|---|
| pytest (493 tests) | ✅ 493/493 pass |
| ruff, mypy | ✅ Clean |
| CI (lint, typecheck, tests) | ✅ All green |
| codecov/patch | |
| Manual #1: Case-insensitive Bearer | ✅ bearer/Bearer/BEARER all 200 |
| Manual #2: Public URL in metadata | ✅ https://mcpawareness.com/mcp |
| Manual #3: docker-compose auth vars | ✅ 9 vars present |
| Manual #4: Dockerfile license | ✅ AGPL-3.0-or-later |
Verdict
Zero findings. codecov exception acknowledged by owner. All manual tests pass. Ready for QA Signoff.
|
Adding Ready for QA Signoff — all 6 significant audit findings + bonus concurrency limit verified. codecov/patch failure reviewed and accepted by owner. CI otherwise green, 493/493 pytest, 4/4 manual tests pass. |
Summary
Addresses 6 significant findings from the multi-tenant/OAuth code audit:
_try_oauth()and_resolve_user()now useasyncio.to_thread()so sync psycopg calls don't block the async event loop during OAuth authenticationAWARENESS_PUBLIC_URL— new env var for well-known metadata resource URL. Fixes0.0.0.0:8420appearing in/.well-known/oauth-protected-resourcefor Cloudflare tunnel deploymentsAuthMiddlewarenow acceptsbearer,Bearer,BEARERper RFC 7235Apache-2.0toAGPL-3.0-or-laterQA
Prerequisites
pip install -e ".[dev]"Manual tests
Expected: accepted (not 401)
Set
AWARENESS_PUBLIC_URL=https://mcpawareness.com, verify:Expected:
resourcefield showshttps://mcpawareness.com/mcp, not0.0.0.0:8420docker compose configshows auth env varsdocker inspectshowsAGPL-3.0-or-later🤖 Generated with Claude Code