-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: added spotlight SDK #1140
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
BYK
left a comment
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.
The AI reviewers' concerns seem legit
Co-authored-by: Burak Yigit Kaya <[email protected]>
packages/spotlight/src/server/sdk.ts
Outdated
| if (options.query.sentry_client?.startsWith("sentry.javascript.browser") && options.headers.origin) { | ||
| // This is a correction we make as Sentry Browser SDK may send messages with text/plain to avoid CORS issues | ||
| contentType = "application/x-sentry-envelope"; |
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.
We should move this check outside as it is specific to the server implementation. It has no place in the SDK API.
packages/spotlight/src/server/sdk.ts
Outdated
| const body = options.overrideBufferDecoding | ||
| ? options.body | ||
| : decompressBody(options.body, options.headers.contentEncoding); |
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.
Let's just not pass contentEncoding here.
packages/spotlight/src/server/sdk.ts
Outdated
| spotlightBuffer: MessageBuffer<EventContainer>; | ||
| body: Buffer; | ||
| overrideBufferDecoding?: boolean; | ||
| headers: { |
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.
This API interface should not get server-specific stuff like headers or query. We should not pass them in the first place if possible or pass normalized/derived values to help with anything else. That said I see the value of a simpler interface so I guess it can stay this way. My ideal version would have had source field calculated before being passed to this method, but to calculate that we need both the user agent and the decoded envelope. Still, let's just use userAgent and contentType outside of headers, make userAgent optional, and keep server-specific stuff to a minimum with all of them being optional with sensible defaults.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @spotlightjs/[email protected] ### Minor Changes - Added spotlight sdk for helping others to build on top of it ([#1140](#1140)) - Support COMPOSE_FILE environment variable for Docker Compose projects ([#1131](#1131)) - Prompt user to choose between docker compose and package.json when both are present ([#1120](#1120)) ### Patch Changes - Refactor docker compose support ([#1121](#1121)) - disable sentry in development mode ([#1143](#1143)) - **Security:** Restrict CORS origins for Sidecar to prevent unauthorized access ([#1138](#1138)) The Sidecar now only accepts requests from trusted origins: - `localhost` with any port or protocol (http/https) - `https://spotlightjs.com` and `https://*.spotlightjs.com` (HTTPS only, default port)⚠️ **Potentially Breaking:** If you were accessing the Sidecar from other origins (e.g., custom domains, non-HTTPS spotlightjs.com), those connections will now be rejected. This change improves security by preventing malicious websites from connecting to your local Sidecar instance. - Fix file capture error handling to log errors instead of crashing when SPOTLIGHT_CAPTURE is enabled ([#1142](#1142)) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Miguel Betegón <[email protected]>
Added new Spotlight SDK for helping others to build on top of Spotlight
Closes #1094.