-
-
Notifications
You must be signed in to change notification settings - Fork 390
fix(cors): add Access-Control-Allow-Origin headers to API and downloads #146
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
|
@Grenghis-Khan 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
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: convex/downloads.ts
Line: 13:20
Comment:
**CORS missing on errors**
`downloadZip` only sets `Access-Control-Allow-Origin` on the 200 response (`convex/downloads.ts:61-69`). All early error returns (400/404/410) omit it (e.g. `convex/downloads.ts:13-20`, `37-42`), which will present browser clients with an opaque CORS failure whenever an error happens. Add the CORS header consistently to *all* responses from this action (including error paths).
How can I resolve this? If you propose a fix, please make it concise.
The PR adds Prompt To Fix With AIThis is a comment left during a code review.
Path: convex/httpApiV1.ts
Line: 381:398
Comment:
**Bypassing CORS helpers**
The PR adds `Access-Control-Allow-Origin` only via `json()`/`text()` (`convex/httpApiV1.ts:860-885`), but this raw file endpoint returns a `Response` directly (`convex/httpApiV1.ts:398`) using headers built from `rate.headers` + file headers (`convex/httpApiV1.ts:381-397`). Since that header set doesn’t include the new CORS header, cross-origin reads to `/api/v1/skills/*/file` will still be blocked. Add the CORS header to the `headers` used here (same issue also appears in the souls file endpoint at `convex/httpApiV1.ts:1136-1154`).
How can I resolve this? If you propose a fix, please make it concise. |
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.
I addressed these in commit 5fc3d0c |
Addressed in commit 8f20a72 |
|
Partially a duplicate of #94 |
|
This should fix #131 too. Ready to test once it's deployed. |
Description
This PR resolves "Server Error" / CORS failures in the Web UI and "Skill not found" errors in the CLI for newly published skills.
Fixes #143
Context
Access-Control-Allow-Originheaders. This caused failures in "GitHub Import" verification and skill downloading.clawhub installcommand was making anonymous requests. This caused "Skill not found" errors for skills that were "pending scan" or otherwise not yet fully public, even for the owner.Changes
Access-Control-Allow-Origin: *tojson()andtext()helpers inconvex/httpApiV1.ts.Access-Control-Allow-Origin: *todownloadZipsuccess AND error responses (400/404/410) inconvex/downloads.ts.Access-Control-Allow-Origin: *to raw file endpoints (skill/soul files) inconvex/httpApiV1.ts.clawhub):cmdInstallto retrieve the authenticated user's token.downloadZipandfetchBinaryViaCurlto pass theAuthorization: Bearer <token>header if present.Verification Results
clawhub installuses authentication, allowing owners to install pending skills.