Harden zip handling, rate limiter, auth polling, and log streaming

[lfiaschi]

What changed

Small, high-leverage hardening across shared/, the client CLI, and the server API — grouped into one PR because each change is a couple of lines and they all landed while doing a full-codebase review pass. 12 files, ~397 insertions / 38 deletions, +65 tests, no schema change.

Security

• shared/ziputil.py — reject symlink zip entries, reject \ path separators (POSIX normpath doesn't collapse them → a Windows extractor could still escape), and cap entry count + total uncompressed size (zip-bomb defense).
• client/cli/registry._create_zip — skip symlinks on publish. Previously for file in path.rglob("*"): if not file.is_file() followed a link like notes.md -> ~/.aws/credentials and packaged the target's bytes into a public zip on the registry.
• server/api/registry_service.require_org_membership — return the same 404 for "org does not exist" and "caller is not a member", so a differing status code no longer lets any authenticated user enumerate the org namespace. The admin-only role check keeps its 403 (no enumeration leak — caller has already proven membership).

Correctness

• server/api/registry_routes.get_eval_run_logs — skip lines that fail json.loads instead of letting JSONDecodeError bubble. A truncated write, a killed worker, or a partial S3 chunk used to 500 every subsequent poll for that run's logs.
• client/cli/registry._render_skills_table — coerce updated_at=None to "" before slicing. A null timestamp used to crash the list table with NoneType is not subscriptable.
• client/cli/auth._clamp_poll_interval — clamp the server-supplied device-flow interval to [1, 30]s. A malformed / hostile 0, -1, or huge value could spin the CLI in a tight loop or make login hang effectively forever.

Performance

• server/api/rate_limit.RateLimiter — replace the sum(len(v) for v in self._requests.values()) % 100 == 0 purge trigger, which was O(N tracked IPs) on every request while holding the lock, with a monotonic per-limiter counter. Same purge cadence, zero hot-path scan.

Why

Full deep-dive review of the codebase (see appendix at the bottom) surfaced ~40 findings across four axes: architecture, correctness, security, and testing. This PR picks the ones that are (a) real, concrete bugs with a failing scenario, (b) small and safe to implement in one pass, and (c) covered by a new regression test. Larger refactors (split database.py / registry.py / gauntlet.py god-modules, move rate-limit backend to Redis for cross-container correctness, real-Postgres integration tests, Sentry + /metrics) are called out in the appendix but deferred to follow-up PRs.

How to test

make test          # 104 shared + 299 client + 936 server tests pass
make lint          # ruff check + format clean across the repo

The 5 test_docx_integration.py failures observed locally reproduce on main before this branch — they compare hardcoded file names / counts against ~/.claude/skills/docx/ which drifts. Not caused by this PR.

Manual smoke tests that map to each change:

• python -c "import zipfile, io, stat; from dhub_core.ziputil import validate_zip_entries; ..." — inject a symlink entry, assert ValueError.
• dhub publish a directory containing a symlink → check the resulting zip does not include the link's target.
• dhub login against a server returning interval: 0 → CLI polls at 1s cadence instead of tight-looping.
• curl -H 'Authorization: Bearer <valid-jwt>' https://.../v1/skills/no-such-org/foo/1.0.0 (DELETE) → 404, byte-for-byte identical to a genuinely missing org.

Checklist

• Tests pass (make test) — 1,339 tests pass; only pre-existing docx integration brittleness fails.
• No breaking API changes — CLI behavior is strictly stricter (rejects malicious zips it used to accept, skips exfiltration path); the one server behavior change (403→404 on non-member) is defensive.
• Database migration included (if schema changed) — n/a, no schema change.

---

Appendix — Deep code review findings

Reviewed by four parallel principal-engineer sweeps (server domain + infra, API routes, client + shared, tests + CI + observability). This appendix is the synthesis; only the bolded items are addressed in this PR — the rest are the follow-up backlog.

System map

uv workspace monorepo: client/ (dhub-cli, PyPI), server/ (decision-hub-server, Modal), shared/ (dhub-core, source-of-truth models), frontend/ (React 19 + Vite, bundled into the server image). Backend: FastAPI + SQLAlchemy Core (no ORM) + Postgres + S3 + Gemini + Anthropic + Modal. ~54k LOC Python, ~6.7k LOC TS. Router-level Depends(get_current_user) on every write router is a clean defense-in-depth pattern. Boundaries between domain/ and infra/ are respected in shape but leak in practice via inline imports.

Top-15 highest-leverage findings (severity-ordered)

#	Category	Area	Issue	Status
1	Security	client publish	_create_zip follows symlinks → publishes host secrets to public zip	fixed
2	Security	client install	extractall writes through pre-existing symlinks in the target dir (no O_NOFOLLOW in stdlib)	deferred — needs extract-to-tempdir + os.replace
3	Security	shared/ziputil	Backslash separators not detected on POSIX; no size/count caps; symlink entries pass through	fixed
4	Security	API authz	require_org_membership leaks org existence via 404 vs 403	fixed
5	Security	logging	_extract_username_from_jwt reads unverified JWT claim into request log context → log spoofing	deferred
6	Security	client config	~/.dhub/config.*.json written with process umask (world-readable JWT on shared boxes)	deferred
7	Correctness	eval logs	get_eval_run_logs crashes on any malformed chunk line → 500s all subsequent polls	fixed
8	Correctness	tracker svc	_dispatch_changed_trackers re-runs already-published trackers when fn.map iteration raises mid-stream	deferred
9	Correctness	infra/database	Engine leaks: 6 call sites construct Engine per-invocation, never dispose → FD leak under cron load	deferred
10	Correctness	infra/database	Publish pipeline: eval_run row inserted+committed before Modal spawn; failed spawn leaves ghost pending runs	deferred
11	Correctness	client auth	Server-supplied interval accepted unchecked → 0/negative → tight-loop, huge → hang	fixed
12	Correctness	client CLI	_render_skills_table crashes on updated_at=None	fixed
13	Perf	api/rate_limit	O(N) sum(len(v)) % 100 == 0 scan on every request under lock	fixed
14	Perf	api rate limiter	Uses request.client.host (proxy IP behind Modal edge, not client) + per-container storage → effective limit is N× at scale	deferred — needs XFF trust-chain policy + Redis-backed store
15	Perf	api/registry	get_eval_run_logs re-reads every S3 chunk on every poll (quadratic) — cursor is event-seq but not translated to chunk offset	deferred

Testing / CI / observability posture

Repo has ~1,370 tests (886 server, 345 client, 57 shared, 82 frontend). Strengths: excellent gauntlet coverage, thorough manifest / semver / zip-slip cases, JSON-output tests across ~10 CLI commands, per-endpoint route tests. Gaps: the API TestClient uses MagicMock() for the DB engine in every test — no SQL is ever exercised in CI, so _SKILL_SUMMARY_COLUMNS drift or missed RLS grant fails silently until prod. moto is a dev dep but never imported. No E2E test. No /metrics, no Sentry, no tracing — an outage today is grep-through-Modal-logs, not dashboards. CI has no uv cache (every job cold-installs the workspace, ~30-60s each × 6 jobs).

Suggested follow-up PRs (not in this one)

1. Real-Postgres integration for test-server — copy the services: postgres block already used by migrate-check to unlock T1/T2 from the testing review (guard _SKILL_SUMMARY_COLUMNS, publish-e2e, RLS boundary).
2. Split database.py (3,465 LOC), registry_routes.py (1,352 LOC), gauntlet.py (1,355 LOC), client/cli/registry.py (1,912 LOC) into aggregate-scoped modules. Re-export from __init__.py so callers are unaffected.
3. Rate limiter: XFF-aware client IP + Redis/Modal-Dict backend — makes rate limits actually work across replicas.
4. /metrics + Sentry — one env var each, ~50 lines, turns silent 500s into paged alerts.
5. Central client HTTP wrapper (dhub/http.py) with retry, timeout, and friendly error mapping — replaces ~35 ad-hoc httpx.Client(timeout=60) blocks scattered across CLI modules and eliminates the raw-traceback UX for common failure modes.
6. uv cache in CI (cache-dependency-glob: "**/uv.lock") — one-line change per job, ~5 min faster PRs.
7. Unify secret redaction — _CREDENTIAL_PATTERNS in gauntlet.py covers AWS/GitHub/Slack/Stripe/JWTs; logging.py only redacts ?key=; evals.py covers only Anthropic/OpenAI/Google. Move to a single decision_hub.redact module.

---

Generated by Claude Code