Full application build: landing page, auth, chat with MCP tool use, harness management, and OAuth

[DIodide]

Summary

This PR represents the full initial build of Harness, bringing the application from skeleton to a functional AI chat
platform with MCP (Model Context Protocol) integration. Key additions include:

• Landing page — Animated landing page with floating dots background, feature highlights, and call-to-action
• Authentication & onboarding — Clerk-based sign-in page, onboarding flow with API key setup and initial harness
  creation, TanStack Router auth guards for protected routes
• Chat interface — Full streaming chat UI with markdown rendering, reasoning/thinking display, tool call
  visualization, chat interrupts, auto-scroll, and per-conversation state management
• Harness management — Create, configure, and manage harnesses (model + MCP server bundles), harness detail pages,
  decoupled harnesses from conversations with last-used tracking
• MCP integration — Dynamic HTTP-streamable MCP server connectivity, parallel tool call execution, MCP server health
  checks with UI status indicators, and error surfacing to frontend
• OAuth for MCP servers — Full OAuth 2.0 flow for MCP servers requiring authentication (GitHub OAuth App support),
  token storage and refresh via Convex
• FastAPI backend — Chat streaming endpoint via OpenRouter, MCP client service, OAuth routes, Convex persistence
  service, structured logging, config validation, connection pooling via httpx.AsyncClient
• Convex backend — Schema for harnesses, conversations, messages (with parts, tool calls, usage/cost tracking), MCP
  OAuth tokens, and user settings; seed data; composite indexes for recency queries
• UI/UX — shadcn/ui component library, display modes (zen/standard/developer), user settings with profile
  management, Geist font, toast notifications, dark theme with Clerk appearance customization
• Model support — GPT-4o, GPT-4.1, Claude Sonnet/Opus 4, Gemini 2.5 Pro/Flash, DeepSeek R1/V3, Grok 3/3 Mini;
  thinking model variants with reasoning parameter support

Changes

• 81 files changed, ~12,000 insertions, ~670 deletions
• Removed deprecated packages/backend in favor of packages/convex-backend
• Removed old Header component and Clerk integration files, replaced with route-level auth guards and chromeless route
  support

PRs included

• Feat/landing page #3 — Landing page
• Feat/login and onboarding #4 — Login and onboarding
• Feat/initial chat streaming implementation  #5 — Initial chat + OAuth implementation
• Add chat interrupts, auto scroll #6 — Chat interrupts and scroll
• Feat/mcp implementation #7 — MCP implementation

[cole-ramer]

@claude, review this.

[claude]

Claude finished @cole-ramer's task in 6m 57s —— View job

---

PR Review

This is an incremental review on top of the prior Claude review from March 22. I re-read the key files and note what's been fixed, what remains open, and flag new issues introduced by the additions since then (sandbox tools, skills system, etc.).

Tasks

• Gather context
• Review FastAPI backend (auth, chat, MCP, OAuth)
• Review frontend (chat stream, routes, components)
• Review Convex schema and queries
• Post comprehensive review

---

What's Been Fixed

• Session cache now user-scoped — _session_cache is now keyed by (url, user_id, netid) (mcp_client.py:26), resolving the cross-user session sharing bug from item Feat/initial chat streaming implementation  #5 in the prior review.
• Hardcoded personal ngrok URL removed — main.py CORS list no longer contains the personal dev machine ngrok URL or the 127.0.0.1:57609 entry.
• _tool_name_map key format improved — Keys are now servername__toolname (full namespaced name), so single-server collisions are less likely.

---

🔴 Critical — Still Open from Previous Review

1. JWT issuer read from unverified token (packages/fastapi/app/auth.py:44-51)

unverified_claims = jwt.decode(token, options={"verify_signature": False})
issuer = unverified_claims.get("iss", "")
jwks = await _get_jwks(http_client, issuer)

The JWKS URL is still derived from the attacker-controlled iss claim. An attacker can host their own JWKS and sign a valid-looking token. Pin the expected issuer in Settings and reject mismatches before fetching. Fix this →

2. Audience verification disabled (packages/fastapi/app/auth.py:76)

options={"verify_aud": False}

Tokens issued for any Clerk application are accepted. Fix this →

3. postMessage wildcard origin (packages/fastapi/app/routes/mcp_oauth.py:249)

window.opener.postMessage({json.dumps(message_data)}, "*");

The OAuth result (including access token provenance) is broadcast to any origin. Pass settings.frontend_url as the second argument. Fix this →

---

🟠 High — Still Open from Previous Review

4. conversation_id not validated against authenticated user (packages/fastapi/app/routes/chat.py:719)

body.conversation_id is passed directly to save_assistant_message and patch_message_usage with no check that the conversation belongs to user_id. User A could supply User B's conversation ID to inject messages. Validate ownership in the Convex mutation or look up the conversation first.

5. OAuth PKCE state in-process only (packages/fastapi/app/services/mcp_oauth.py:40)

_pending_oauth is module-level. Multi-worker deployments (e.g. uvicorn --workers N) will fail OAuth callbacks that land on a different worker. Move to Redis or Convex.

---

🟠 High — New Findings

6. Redirect URI built from unvalidated X-Forwarded-* headers (packages/fastapi/app/routes/mcp_oauth.py:52-55, 175-177)

forwarded_proto = request.headers.get("x-forwarded-proto", request.url.scheme)
forwarded_host = request.headers.get("x-forwarded-host") or request.headers.get("host", "localhost:8000")
redirect_uri = f"{forwarded_proto}://{forwarded_host}/api/mcp/oauth/callback"

If the FastAPI server is directly internet-exposed (without a trusted reverse proxy stripping these headers), a client can inject an arbitrary x-forwarded-host to redirect OAuth callbacks to an attacker-controlled host. Use settings.fastapi_base_url (already in config) instead of trusting forwarded headers, or configure a trusted proxy list.

7. Skill content fetching allows arbitrary GitHub requests (packages/fastapi/app/routes/chat.py:142-238)

_fetch_skill_md_from_repo makes HTTP requests to raw.githubusercontent.com and the GitHub API for any source derived from the skill name in the user's harness config. If a user configures a harness with a skill from a repo they control, they can cause the backend to make requests to GitHub on their behalf. While allowed_skills gates which names are accepted, the path components of the skill name (source, skill_id) flow directly into URLs without sanitization. A crafted skill name like evil-org/evil-repo/../../etc won't traverse the filesystem but could target arbitrary GitHub paths. Consider validating that source matches owner/repo format before constructing URLs.

---

🟡 Medium — Still Open from Previous Review

8. JWKS cache has no TTL (packages/fastapi/app/auth.py:9-22)

_jwks_cache is a global with no expiry. Clerk key rotation only triggers a retry when a specific kid is missing; the cached set of keys can grow stale permanently.

9. Duplicate model IDs in AVAILABLE_MODELS (packages/fastapi/app/config.py:54-55)

{"id": "anthropic/claude-sonnet-4", "name": "Claude Sonnet 4"},
{"id": "anthropic/claude-sonnet-4", "name": "Claude Sonnet 4 (Thinking)"},

Two entries share the same id. Same for Claude Opus 4. Frontend model selectors that key by id will de-duplicate these. Use distinct IDs (e.g. anthropic/claude-sonnet-4:thinking). Fix this →

---

🟡 Medium — New Findings

10. GitHub OAuth PKCE code_verifier generated but never sent (packages/fastapi/app/services/mcp_oauth.py:802-803)

code_verifier, code_challenge = _generate_pkce_pair()

For the standalone GitHub OAuth flow, code_challenge is not included in the authorization URL params and code_verifier is not sent during token exchange (GitHub OAuth Apps don't support PKCE). The generated PKCE data is stored in _pending_oauth but silently ignored. This is dead code — remove it to avoid confusion about security posture.

11. _tools_cache keyed by server URL only (packages/fastapi/app/services/mcp_client.py:105)

The tools list cache is not user-scoped. For most servers this is fine (tool schemas are the same for all users), but for any server that returns per-user tool definitions, user A's cached tools will be served to user B. Low risk currently, worth a comment.

12. No message content size limit (packages/fastapi/app/routes/chat.py:523)

The full body.messages list is forwarded to OpenRouter on every request with no cap on total tokens or message count. Long conversations can generate unexpectedly large token costs or cause upstream timeouts.

---

🔵 Low / Nits

13. import re inside _extract_summary (packages/fastapi/app/routes/chat.py:64)

def _extract_summary(detail: str, max_chars: int = 300) -> str:
    import re

re is already imported at the top of the file (line 4). Remove the redundant in-function import.

14. import os inside resolve_princeton_netid (packages/fastapi/app/services/mcp_client.py:73)

Same pattern — os is used once inside a function. Hoist to module level.

15. import base64 inside call_tool (packages/fastapi/app/services/mcp_client.py:503)

Same pattern — move to top-level imports.

16. Bearer tokens stored in plaintext in Convex (packages/convex-backend/convex/schema.ts:17)

authToken on MCP server configs is a plain string. Flagged for future work: these should be encrypted at rest or stored in a secrets manager.

17. messages.userId optional (packages/convex-backend/convex/schema.ts:97)

userId: v.optional(v.string()),

userId should likely be required on messages to prevent orphaned records and simplify user-scoped queries.

18. Harness delete doesn't cascade to conversations (packages/convex-backend/convex/harnesses.ts)

Deleting a harness leaves conversations with a stale lastHarnessId. Acceptable for now but worth a comment or a compensating query to null out the references.

---

Summary

The codebase has grown considerably and remains well-structured. The session cache isolation fix (item 5 from the prior review) is a meaningful improvement. The two highest-priority unfixed issues remain JWT issuer pinning (#1) and postMessage wildcard (#3). The new redirect URI from unvalidated forwarded headers (#6) is worth fixing before deploying behind anything less than a fully trusted reverse proxy, and the skill content SSRF vector (#7) deserves a sanity-check on the source path before it goes to untrusted users.

[cole-ramer]

@claude, review this, focus on notifying REAL issues

[claude]

Claude finished @cole-ramer's task in 1m 43s —— View job

---

PR Review

Tasks

• Gather context
• Read auth.py, chat.py, mcp_client.py, mcp_oauth.py, config.py
• Review Convex backend
• Post findings

---

What's Been Fixed Since Prior Reviews

• JWT issuer pinned — settings.clerk_issuer is now used directly; unverified iss claim no longer controls JWKS URL ✅
• Hardcoded ngrok/dev URLs removed from CORS config ✅
• Session cache user-scoped — keyed by (url, user_id, netid) ✅
• OAuth redirect URI — now uses settings.fastapi_base_url, not forwarded headers ✅

---

🔴 Critical — Still Open

1. Audience verification disabled (packages/fastapi/app/auth.py:76)

options={"verify_aud": False}

This accepts JWTs issued for any Clerk application, not just this one. A token from a completely different Clerk app (e.g. a user's personal project) will pass verification. Set clerk_jwt_audience in Settings and verify it.

Fix this →

2. postMessage wildcard origin (packages/fastapi/app/routes/mcp_oauth.py:244)

window.opener.postMessage({json.dumps(message_data)}, "*");

The OAuth result — including the server URL that was just authorized — is broadcast to every open window/tab, regardless of origin. Any malicious page the user has open can intercept it and learn which MCP servers they authenticated with. Replace "*" with settings.frontend_url.

Fix this →

---

🟠 High — Still Open

3. conversation_id not validated against the authenticated user (packages/fastapi/app/routes/chat.py:719)

body.conversation_id from the client is passed directly to save_assistant_message and patch_message_usage. The Convex saveAssistantMessage is an internalMutation that checks the conversation exists but does not verify convo.userId === user_id (messages.ts:204-205). User A can supply User B's conversation_id and the assistant's full response gets saved into User B's conversation history.

Fix: add an ownership check in the FastAPI route before the agentic loop starts, or add userId validation to the saveAssistantMessage internalMutation.

Fix this →

4. OAuth PKCE state stored in process memory only (packages/fastapi/app/services/mcp_oauth.py:40)

_pending_oauth: dict[str, "PendingOAuth"] = {}

Under multi-worker deployments (uvicorn --workers N), the OAuth callback will randomly land on a different process from the one that stored the state, causing "Invalid or expired OAuth state" errors for ~(N-1)/N of attempts. This is a functional bug under any non-single-process deployment. Move _pending_oauth to Redis or Convex.

---

🟡 Medium

5. Duplicate model IDs in AVAILABLE_MODELS (packages/fastapi/app/config.py:62–67)

{"id": "anthropic/claude-sonnet-4", "name": "Claude Sonnet 4"},
{"id": "anthropic/claude-sonnet-4", "name": "Claude Sonnet 4 (Thinking)"},
{"id": "anthropic/claude-opus-4",   "name": "Claude Opus 4"},
{"id": "anthropic/claude-opus-4",   "name": "Claude Opus 4 (Thinking)"},

Any frontend code that deduplicates by id (e.g. a <select> keyed by model ID) will silently drop one of the two variants. The correct fix is already present in MODEL_MAP — use "claude-sonnet-4-thinking" / "claude-opus-4-thinking" as the IDs here too.

Fix this →

---

🔵 Nits (3 in-function imports that should be hoisted)

• import re inside _extract_summary — re is already imported at top of file (chat.py:4, chat.py:62)
• import os inside resolve_princeton_netid — used once, should be module-level (mcp_client.py:73)
• import base64 inside call_tool — should be module-level (mcp_client.py:503)

---

Summary

Items 1 (audience verification) and 2 (postMessage wildcard) are the two unfixed security issues from prior reviews. Item 3 (conversation ownership) is a real data isolation bug — the saveAssistantMessage internal mutation has no user check. Item 4 is a functional reliability bug under any multi-process deployment. The duplicate model IDs (item 5) will silently break any model selector that keys by ID.

[cole-ramer]

Investigating the issues flagged, they appear not to be real issues (sorry if I am wrong)