Skip to content

Conversation

@albdima83
Copy link
Contributor

@albdima83 albdima83 commented Jun 25, 2024

When an external image resource is loaded from https, the function convertUrlToAbsolute inside the utils.ts doesn't check if it is a network or internal resource.

// handle local file imports if the url isn't remote resource or data blob
if (
self.location.protocol === 'file:' &&
!EXTERNAL_REGEX_URL.test(url) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all cases could be a single regex, if you just test /^(data|https?|ftps?):/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And TBH, you could simplify further by just checking whether it looks like {protocol}:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elsassph Thanks, I fixed it! Simplify as {protocol}:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I thought it might be simpler/safer to not test the protocol itself, e.g. any ^[a-z]+: is a protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elsassph I prefer to have a more specific check based on protocol resources.
IMO your suggestion is too generic.
What happen if you have an local image with filename like aaa:2222.png?

Copy link
Contributor

@wouterlucas wouterlucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 😅 thanks! 😄

@frank-weindel frank-weindel added this pull request to the merge queue Jun 26, 2024
Merged via the queue into lightning-js:main with commit 632ffd9 Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: wrong load external resources when the local protocol is file://

4 participants