fix: address codebase-analysis findings across API, SDKs, MCP, and dashboard#276
Conversation
…ault
Derive a per-user org discriminator (personal:${clerkUserId}) when a Clerk
session has no active organization, and drop the "default" fallbacks in
clerk-session.ts and routes/projects.ts. Previously every org-less user
collapsed into one shared sentinel org, allowing cross-tenant reads between
unrelated personal accounts. Adds a test proving user A cannot see user B's
org-less projects.
Closes #254
Co-Authored-By: Duyet Le <me@duyet.net>
Co-Authored-By: duyetbot <bot@duyet.net>
Add a shared url-safety helper that requires https and rejects loopback, private, link-local and cloud-metadata hosts (127/8, 10/8, 172.16/12, 192.168/16, 169.254/16, ::1, fc00::/7, localhost). Enforced in the Create/Update webhook schemas at registration and again in lib/webhook.ts immediately before delivery (defense in depth). Closes #255 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
Add a partial unique index on active leases (project_id, state_key) WHERE released_at IS NULL and turn acquire into a guarded conditional insert that catches the unique violation and returns LEASE_CONFLICT, replacing the racy check-then-insert. Expired leases are released inline before insert so a fresh lease can still be acquired. Migration regenerated via drizzle-kit. Closes #256 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
The public analytics routes mounted only apiKeyAuth + rateLimit, so any valid
key (even one scoped to state:write only) could read aggregate analytics. Add
requireScope("analytics:read").
Closes #257
Co-Authored-By: Duyet Le <me@duyet.net>
Co-Authored-By: duyetbot <bot@duyet.net>
Add an explicit ESCAPE '\\' clause so backslash-escaped wildcards are treated as literals, and stop escaping '[' (a literal in SQLite LIKE, not a wildcard). Paginate on a composite (updated_at, id) cursor with matching ORDER BY so rows sharing a timestamp are neither skipped nor duplicated, mirroring listConversations. Closes #258 Closes #261 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
Mirror listConversations so claims sharing a created_at are not skipped or duplicated across pages; ORDER BY now tie-breaks on id and the cursor encodes both parts. Refs #261 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
GET /conversations/:id loaded all messages unbounded and unordered. Order by (created_at, rowid) and cap at 1000 to prevent an unbounded load, keeping the response shape unchanged. Closes #260 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
deleteProject only removed 5 tables, orphaning state, claims, capability tokens, webhooks, domains and OAuth artifacts (and leaving credentials usable since D1 does not enforce FK cascade by default). Explicitly delete every project-child table and also return capability-token hashes so their auth-cache entries are invalidated alongside API keys. Closes #262 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
Verify the stored client_secret hash (constant-time) for confidential clients using client_secret_basic or client_secret_post at the token endpoint; public (none + PKCE) clients keep working, and unknown clients still surface invalid_grant via the grant handlers. Add per-IP rate limiting to POST /api/oauth/register. Closes #263 Closes #264 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
The Workers KV sliding-window limiter is a non-atomic read-modify-write and is bypassable under burst concurrency, so it stays opt-in behind USE_SLIDING_WINDOW while the atomic D1 fixed-window limiter remains the default path. Document the limitation and recommend a Durable Object / CF Rate Limiting binding for an atomic sliding window. Closes #259 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
TS SDK: restrict automatic retries to idempotent methods (or POSTs carrying an
Idempotency-Key), honor Retry-After on 429 with jittered backoff, and page
LangGraph list() across cursors up to the caller's limit. MCP: add a
configurable AGENTSTATE_TIMEOUT_MS AbortController timeout to apiRequest. Shared:
reconcile ListResponse<T> to the real { data, pagination: { limit, next_cursor } }
shape. Add @langchain/core as an optional peer dependency.
Closes #269
Closes #270
Closes #271
Closes #272
Closes #273
Closes #274
Closes #275
Co-Authored-By: Duyet Le <me@duyet.net>
Co-Authored-By: duyetbot <bot@duyet.net>
Split the langgraph checkpoint-saver imports into separate try/except blocks so
a missing AsyncBaseCheckpointSaver no longer stubs out BaseCheckpointSaver.
Handle 400/403 explicitly and wrap all non-2xx responses in AgentStateError with
the parsed { error: { code, message } } body. Add a timeout constructor param
(~30s) passed to httpx. Unify the default base URL, page LangGraph list() across
cursors, restrict retries to idempotent methods, and honor Retry-After + jitter.
Closes #265
Closes #267
Closes #268
Refs #269
Refs #270
Refs #272
Refs #274
Co-Authored-By: Duyet Le <me@duyet.net>
Co-Authored-By: duyetbot <bot@duyet.net>
Remove the loaded ref that defeated the effect deps array (stale messages persisted when switching conversations) and add an ignore cleanup flag to drop out-of-order async responses from a previous conversation. Closes #266 Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (38)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several security and robustness enhancements across the API, TypeScript SDK, and Python SDK. Key changes include adding SSRF guards for webhooks, isolating personal accounts into per-user internal organizations, implementing client secret verification for confidential OAuth clients, and adding robust retry logic with exponential backoff and jitter to the SDKs. Feedback from the review suggests hardening the SSRF blocklist with CGNAT and site-local IPv6 ranges, preventing client ID enumeration in OAuth authentication, resetting the dashboard's message state immediately when switching conversations to avoid stale UI rendering, and clamping the maximum retry delay in both SDKs to prevent application hangs from large Retry-After headers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function isBlockedIPv4(octets: [number, number, number, number]): boolean { | ||
| const [a, b] = octets; | ||
| if (a === 0) return true; // 0.0.0.0/8 (includes unspecified) | ||
| if (a === 127) return true; // 127.0.0.0/8 loopback | ||
| if (a === 10) return true; // 10.0.0.0/8 private | ||
| if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 private | ||
| if (a === 192 && b === 168) return true; // 192.168.0.0/16 private | ||
| if (a === 169 && b === 254) return true; // 169.254.0.0/16 link-local (+ metadata) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The SSRF protection blocks standard private ranges but misses the Carrier-Grade NAT (CGNAT) range 100.64.0.0/10 (RFC 6598). This range is commonly used in cloud environments, Tailscale/ZeroTier networks, and internal Kubernetes clusters, and is a known vector for SSRF bypasses. Adding a check for this range hardens the SSRF guard.
| function isBlockedIPv4(octets: [number, number, number, number]): boolean { | |
| const [a, b] = octets; | |
| if (a === 0) return true; // 0.0.0.0/8 (includes unspecified) | |
| if (a === 127) return true; // 127.0.0.0/8 loopback | |
| if (a === 10) return true; // 10.0.0.0/8 private | |
| if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 private | |
| if (a === 192 && b === 168) return true; // 192.168.0.0/16 private | |
| if (a === 169 && b === 254) return true; // 169.254.0.0/16 link-local (+ metadata) | |
| return false; | |
| } | |
| function isBlockedIPv4(octets: [number, number, number, number]): boolean { | |
| const [a, b] = octets; | |
| if (a === 0) return true; // 0.0.0.0/8 (includes unspecified) | |
| if (a === 127) return true; // 127.0.0.0/8 loopback | |
| if (a === 10) return true; // 10.0.0.0/8 private | |
| if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 private | |
| if (a === 192 && b === 168) return true; // 192.168.0.0/16 private | |
| if (a === 169 && b === 254) return true; // 169.254.0.0/16 link-local (+ metadata) | |
| if (a === 100 && b >= 64 && b <= 127) return true; // 100.64.0.0/10 CGNAT (RFC 6598) | |
| return false; | |
| } |
| const firstHextet = h.split(":")[0]; | ||
| if (firstHextet.length > 0) { | ||
| // fc00::/7 → first byte 0xfc or 0xfd; fe80::/10 → fe80..febf. | ||
| if (firstHextet.startsWith("fc") || firstHextet.startsWith("fd")) return true; | ||
| if (/^fe[89ab]/.test(firstHextet)) return true; | ||
| } |
There was a problem hiding this comment.
The IPv6 blocklist covers fe80::/10 (link-local) but misses fec0::/10 (site-local), which is deprecated but still private/local and can be used for SSRF. We can expand the regex to /^fe[8-f]/ to cover both link-local and site-local ranges.
| const firstHextet = h.split(":")[0]; | |
| if (firstHextet.length > 0) { | |
| // fc00::/7 → first byte 0xfc or 0xfd; fe80::/10 → fe80..febf. | |
| if (firstHextet.startsWith("fc") || firstHextet.startsWith("fd")) return true; | |
| if (/^fe[89ab]/.test(firstHextet)) return true; | |
| } | |
| const firstHextet = h.split(":")[0]; | |
| if (firstHextet.length > 0) { | |
| // fc00::/7 → first byte 0xfc or 0xfd; fe80::/10 → fe80..febf; fec0::/10 → fec0..feff (site-local) | |
| if (firstHextet.startsWith("fc") || firstHextet.startsWith("fd")) return true; | |
| if (/^fe[8-f]/.test(firstHextet)) return true; | |
| } |
| const client = await getClient(db, clientId); | ||
| if (!client) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the client is not found, returning early (success) allows client ID enumeration. An attacker can distinguish between a valid client ID (which returns 401 invalid_client when the secret is wrong) and an invalid client ID (which returns 400 invalid_grant or similar). Throwing invalid_client for unknown client IDs is more compliant with RFC 6749 §5.2 and prevents enumeration.
| const client = await getClient(db, clientId); | |
| if (!client) { | |
| return; | |
| } | |
| const client = await getClient(db, clientId); | |
| if (!client) { | |
| throw new OAuthError("invalid_client", "invalid client credentials", 401); | |
| } |
There was a problem hiding this comment.
Applied the other five suggestions (CGNAT + IPv6 site-local SSRF ranges, retry-delay clamp in both SDKs, dashboard message reset). Leaving this one as-is intentionally: returning invalid_grant for an unknown/mismatched client_id at the token endpoint is a deliberate design here (see the docstring on authenticateClient) and is asserted by the existing tests (oauth.test.ts — "rejects a refresh token redeemed with the wrong client_id", etc.). The grant handlers bind the client to the code/refresh-token and surface invalid_grant, so an unknown client can't complete a grant regardless. The residual enumeration signal is minor and overriding the maintainer's tested error-semantics choice on this isn't warranted; happy to revisit if you'd prefer strict RFC 6749 §5.2 invalid_client for unknown clients.
Generated by Claude Code
| useEffect(() => { | ||
| if (loaded.current) return; | ||
| loaded.current = true; | ||
| let ignore = false; | ||
| (async () => { | ||
| setLoading(true); |
There was a problem hiding this comment.
When switching between conversations, the messages state is not reset immediately. This causes the UI to display the messages of the previous conversation while the new one is loading. Resetting messages to null at the start of the effect prevents this stale data display.
| useEffect(() => { | |
| if (loaded.current) return; | |
| loaded.current = true; | |
| let ignore = false; | |
| (async () => { | |
| setLoading(true); | |
| useEffect(() => { | |
| let ignore = false; | |
| setMessages(null); | |
| (async () => { | |
| setLoading(true); |
| if (attempt > 0) { | ||
| const delay = this.retryDelayMs * 2 ** (attempt - 1); | ||
| const backoff = this.retryDelayMs * 2 ** (attempt - 1); | ||
| // Full jitter on the exponential backoff to avoid thundering herds. | ||
| const jittered = backoff + Math.floor(Math.random() * backoff); | ||
| const delay = nextDelayMs ?? jittered; | ||
| nextDelayMs = null; | ||
| await new Promise((resolve) => setTimeout(resolve, delay)); | ||
| } |
There was a problem hiding this comment.
If the server returns an extremely large Retry-After header (e.g., due to misconfiguration or long rate limit windows), the client will sleep for that entire duration, potentially hanging the application or exceeding execution limits (especially in serverless environments like Cloudflare Workers). Clamping the maximum retry delay to a reasonable value (e.g., 30 seconds) prevents this.
| if (attempt > 0) { | |
| const delay = this.retryDelayMs * 2 ** (attempt - 1); | |
| const backoff = this.retryDelayMs * 2 ** (attempt - 1); | |
| // Full jitter on the exponential backoff to avoid thundering herds. | |
| const jittered = backoff + Math.floor(Math.random() * backoff); | |
| const delay = nextDelayMs ?? jittered; | |
| nextDelayMs = null; | |
| await new Promise((resolve) => setTimeout(resolve, delay)); | |
| } | |
| if (attempt > 0) { | |
| const backoff = this.retryDelayMs * 2 ** (attempt - 1); | |
| // Full jitter on the exponential backoff to avoid thundering herds. | |
| const jittered = backoff + Math.floor(Math.random() * backoff); | |
| const delay = Math.min(nextDelayMs ?? jittered, 30000); | |
| nextDelayMs = null; | |
| await new Promise((resolve) => setTimeout(resolve, delay)); | |
| } |
| if retry_after is not None: | ||
| base = retry_after | ||
| else: | ||
| base = self.retry_delay_ms * (2 ** attempt) / 1000.0 | ||
| return base + random.uniform(0.0, base * 0.5) |
There was a problem hiding this comment.
Similar to the TypeScript SDK, if the server returns an extremely large Retry-After header, the client will sleep for that entire duration, blocking the thread. Clamping the maximum retry delay to a reasonable value (e.g., 30.0 seconds) prevents hanging the client application.
| if retry_after is not None: | |
| base = retry_after | |
| else: | |
| base = self.retry_delay_ms * (2 ** attempt) / 1000.0 | |
| return base + random.uniform(0.0, base * 0.5) | |
| if retry_after is not None: | |
| base = retry_after | |
| else: | |
| base = self.retry_delay_ms * (2 ** attempt) / 1000.0 | |
| delay = base + random.uniform(0.0, base * 0.5) | |
| return min(delay, 30.0) |
Address review feedback on #276: - url-safety: block CGNAT 100.64.0.0/10 (RFC 6598) and IPv6 site-local fec0::/10 in addition to the existing private/link-local ranges. - SDK + python-sdk: clamp the retry delay to 30s so a misconfigured or huge Retry-After header can't hang the client. - dashboard useMessages: clear messages immediately on conversation switch so the panel doesn't show the previous conversation while loading. Co-Authored-By: Duyet Le <me@duyet.net> Co-Authored-By: duyetbot <bot@duyet.net>
2 participants
Fixes the 22 issues filed from the deep codebase analysis. Quality gates green locally: Biome clean,
tscclean, API vitest 353 passed (+12 new), TS SDK 17 passed, MCP 11 passed, python-sdk 57 passed.Security — API
personal:${clerkUserId}) instead of a shared"default"sentinel; removed the?? "default"fallbacks. Test proves user A cannot see user B's org-less projects and that they map to distinct internal orgs.lib/url-safety.ts(https-only; blocks loopback/private/link-local/::1/fc00::/7/localhost), enforced in the Create/Update webhook schemas and again before delivery inwebhook.ts. Parametrized rejection tests added.requireScope("analytics:read")added to the public analytics routes (403 for astate:read-only key, 200 withanalytics:read).client_secretverification (basic + post) for confidential OAuth clients; public/PKCE and unknown-client paths unchanged.POST /oauth/register. The SSE?token=path was verified: the error handler logsc.req.path(query-stripped), so no token leaks — no code change needed there.Correctness / reliability — API
(project_id, state_key) WHERE released_at IS NULL+ guarded conditional insert →LEASE_CONFLICTon violation. Migration0008_amusing_nightshade.sqlregenerated via drizzle-kit.[→ wrong/empty search results #258 —ESCAPE '\'added to search LIKE; dropped the invalid[escaping.(timestamp, id)cursors + matchingORDER BYfor search and claims.GET /conversations/:idorders messages by(created_at, rowid), bounded at 1000.deleteProjectexplicitly deletes every child table and returns capability-token hashes for cache invalidation.USE_SLIDING_WINDOW. A full Durable-Object sliding window is left as follow-up, so this issue stays open.SDK / MCP / shared / dashboard
AsyncBaseCheckpointSaverdoesn't exist in langgraph-checkpoint 4.x); split imports so a missing symbol no longer discards both base classes. Integration test added.useMessagesno longer keeps stale messages on conversation switch (removed theloadedref, added anignorecleanup flag).AgentStateError, parsing the{error:{code,message}}body.timeout(default ~30s).list()paginates across cursors instead of clamping at 100.apiRequestgains anAbortControllertimeout (AGENTSTATE_TIMEOUT_MS).Idempotency-Keyon retried POSTs.sharedListResponse<T>reconciled to the real{ data, pagination: { limit, next_cursor } }.Retry-Afteron 429 + backoff jitter.@langchain/coreadded as an optional peer dependency.Commits are grouped by subsystem, each with the repo's dual co-author trailers.
🤖 Generated with Claude Code
Generated by Claude Code