-
-
Notifications
You must be signed in to change notification settings - Fork 273
feat(api): add CORS headers for browser clients #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add convex/lib/cors.ts with shared CORS headers and preflight handler - Update httpApi.ts and httpApiV1.ts json/text helpers to include CORS headers - Update downloads.ts to include CORS headers on all responses - Add single OPTIONS handler for /api/* prefix in http.ts This enables browser-based clients (like Cove WebUI) to fetch skills directly from the ClawHub API without CORS errors. Allows: GET, POST, DELETE, OPTIONS from any origin (*)
|
@MaudeBot is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| // CORS preflight handler for all API routes | ||
| http.route({ | ||
| pathPrefix: '/api/', | ||
| method: 'OPTIONS', | ||
| handler: corsPreflightHttp, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] OPTIONS preflight only registered for /api/ prefix
This adds an OPTIONS handler only for pathPrefix: '/api/' (convex/http.ts:199-204). However, this router also exposes LegacyApiRoutes.* (still under /api in many setups) and potentially other non-/api/ HTTP routes (e.g., auth routes). If any of those endpoints are called cross-origin and trigger a preflight, they’ll still fail with a missing/404 OPTIONS route even though the GET/POST responses now include CORS headers.
If the intent is “all API endpoints,” consider also covering legacy routes (and/or the auth routes) with an OPTIONS handler that matches their prefixes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/http.ts
Line: 199:204
Comment:
[P1] OPTIONS preflight only registered for `/api/` prefix
This adds an OPTIONS handler only for `pathPrefix: '/api/'` (`convex/http.ts:199-204`). However, this router also exposes `LegacyApiRoutes.*` (still under `/api` in many setups) and potentially other non-`/api/` HTTP routes (e.g., auth routes). If any of those endpoints are called cross-origin and trigger a preflight, they’ll still fail with a missing/404 OPTIONS route even though the GET/POST responses now include CORS headers.
If the intent is “all API endpoints,” consider also covering legacy routes (and/or the auth routes) with an OPTIONS handler that matches their prefixes.
How can I resolve this? If you propose a fix, please make it concise.| export const CORS_HEADERS = { | ||
| 'Access-Control-Allow-Origin': '*', | ||
| 'Access-Control-Allow-Methods': 'GET, POST, DELETE, OPTIONS', | ||
| 'Access-Control-Allow-Headers': 'Content-Type, Authorization', | ||
| 'Access-Control-Max-Age': '86400', // 24 hours | ||
| } as const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Access-Control-Allow-Headers may be too narrow for real browser clients
CORS_HEADERS hard-codes 'Access-Control-Allow-Headers': 'Content-Type, Authorization' (convex/lib/cors.ts:8-13). In practice, browser requests can include other headers that trigger a preflight (e.g., Accept, If-None-Match, Range, X-Requested-With). If a client sends any additional header, the preflight will be rejected even though the server is otherwise willing to serve the request.
Consider either echoing Access-Control-Request-Headers or broadening this list to the headers your clients actually send.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/lib/cors.ts
Line: 8:13
Comment:
[P2] `Access-Control-Allow-Headers` may be too narrow for real browser clients
`CORS_HEADERS` hard-codes `'Access-Control-Allow-Headers': 'Content-Type, Authorization'` (`convex/lib/cors.ts:8-13`). In practice, browser requests can include other headers that trigger a preflight (e.g., `Accept`, `If-None-Match`, `Range`, `X-Requested-With`). If a client sends any additional header, the preflight will be rejected even though the server is otherwise willing to serve the request.
Consider either echoing `Access-Control-Request-Headers` or broadening this list to the headers your clients actually send.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Adds CORS headers to all API endpoints, enabling browser-based clients to fetch skills directly from the ClawHub API.
Motivation
Browser-based tools like Cove (OpenClaw WebUI) want to display and install skills from ClawHub. Currently, browsers block these requests due to missing CORS headers.
This is standard practice for public package registries — npm, crates.io, and PyPI all allow cross-origin requests.
Changes
convex/lib/cors.ts— Shared CORS headers and preflight response helperconvex/http.ts— Single OPTIONS handler for all/api/*routesconvex/httpApi.ts— CORS headers in json/text response helpersconvex/httpApiV1.ts— CORS headers in json/text helpers + file content endpointsconvex/downloads.ts— CORS headers on download responsesHeaders Added
Safety
Allow-Credentials, so cookies aren't sentGreptile Overview
Greptile Summary
This PR introduces shared CORS configuration (
convex/lib/cors.ts) and applies it across the Convex HTTP API layer by:CORS_HEADERSto JSON/text helper responses in both legacy (convex/httpApi.ts) and v1 (convex/httpApiV1.ts) APIs.convex/downloads.ts)./api/*routes in the HTTP router (convex/http.ts).Overall, this aligns the API with browser-based clients that need cross-origin access to public registry endpoints, while keeping auth behavior unchanged (no credentials mode).
Confidence Score: 4/5
/api/prefix) and overly restrictive allowed request headers that could still block some browser clients.Context used:
dashboard- AGENTS.md (source)