-
-
Notifications
You must be signed in to change notification settings - Fork 13
1.0.6 - Fixed baseUrl events check #58
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
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.
Pull Request Overview
A version bump to 1.0.6 that fixes the logic for checking allowed event origins when a custom baseUrl
is provided, adds a new Storybook story to demonstrate baseUrl
, and updates the changelog.
- Refactored
handleEvent
to correctly allow events from abaseUrl
containing the origin - Added a
BaseUrl
story inDiagramsEmbed.stories.tsx
- Bumped package version and updated
CHANGELOG.md
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/utils/handleEvent.ts | Enhanced origin check to include cases where baseUrl contains the origin |
stories/DiagramsEmbed.stories.tsx | Added BaseUrl story demonstrating custom baseUrl usage |
package.json | Updated version to 1.0.6 |
CHANGELOG.md | Added entry for version 1.0.6 |
Comments suppressed due to low confidence (1)
src/utils/handleEvent.ts:14
- Add unit tests for
handleEvent
to cover scenarios whereevent.origin
matches the default embed origin, matches viabaseUrl.includes(event.origin)
, and is correctly rejected otherwise.
if (
!event.origin.includes('embed.diagrams.net') && | ||
baseUrl && | ||
!baseUrl.includes(event.origin) && | ||
!event.origin.includes(baseUrl) |
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.
Rather than using string includes
for URL matching, parse baseUrl
with the URL
API (e.g. new URL(baseUrl).origin
) and compare origins directly for reliability.
!event.origin.includes('embed.diagrams.net') && | |
baseUrl && | |
!baseUrl.includes(event.origin) && | |
!event.origin.includes(baseUrl) | |
new URL(event.origin).origin !== 'https://embed.diagrams.net' && | |
baseUrl && | |
new URL(baseUrl).origin !== new URL(event.origin).origin && | |
new URL(event.origin).origin !== new URL(baseUrl).origin |
Copilot uses AI. Check for mistakes.
export function handleEvent( | ||
event: MessageEvent, | ||
handlers: EventHandler, | ||
baseUrl?: string | ||
) { | ||
if ( | ||
!event.origin.includes('embed.diagrams.net') && | ||
baseUrl && | ||
!baseUrl.includes(event.origin) && | ||
!event.origin.includes(baseUrl) | ||
) { |
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.
[nitpick] The compound conditional for origin checks is complex—consider extracting it into a descriptive helper function (e.g. isAllowedOrigin(event.origin, baseUrl)
) to improve readability.
export function handleEvent( | |
event: MessageEvent, | |
handlers: EventHandler, | |
baseUrl?: string | |
) { | |
if ( | |
!event.origin.includes('embed.diagrams.net') && | |
baseUrl && | |
!baseUrl.includes(event.origin) && | |
!event.origin.includes(baseUrl) | |
) { | |
function isAllowedOrigin(origin: string, baseUrl?: string): boolean { | |
return ( | |
origin.includes('embed.diagrams.net') || | |
(baseUrl && | |
(baseUrl.includes(origin) || origin.includes(baseUrl))) | |
); | |
} | |
export function handleEvent( | |
event: MessageEvent, | |
handlers: EventHandler, | |
baseUrl?: string | |
) { | |
if (!isAllowedOrigin(event.origin, baseUrl)) { |
Copilot uses AI. Check for mistakes.
No description provided.