Skip to content
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

Throw error if fetching from relative URL on the server #203

Closed
shuding opened this issue Oct 14, 2022 · 9 comments
Closed

Throw error if fetching from relative URL on the server #203

shuding opened this issue Oct 14, 2022 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@shuding
Copy link
Member

shuding commented Oct 14, 2022

Feature Request

Description

If running on the server, you cannot use <img src="/my-img.png"/> because there is no domain provided. We should throw a specific error message in that case.

@styfle styfle added the enhancement New feature or request label Oct 15, 2022
@wong2
Copy link
Contributor

wong2 commented Oct 18, 2022

node-fetch already throw such an error: Can't load image /aaa.jpg: Only absolute URLs are supported

Node.js 18: Can't load image /aaa.jpg: Failed to parse URL from /aaa.jpg

@shuding
Copy link
Member Author

shuding commented Oct 18, 2022

We need to unify that error message, also I think some fetch polyfills won't throw in that case.

@KiwiKilian
Copy link

Not quite exactly on the topic, but is there any other option to fix this error other than inlining the image?

@shuding
Copy link
Member Author

shuding commented Jan 7, 2023

You can fetch the image with an absolute URL such as <img src="https://example.com/my-img.png"/>.

@shuding shuding added the good first issue Good for newcomers label Jan 7, 2023
@KiwiKilian
Copy link

KiwiKilian commented Jan 7, 2023

Thanks for the answer, I should have been more clear. I was thinking about other solutions to local usage on the server. Is there currently any, aside of inlining?

If not, would it be in scope for satori to extend towards this sever usage? For example a resolveResource function one could pass as an option? I guess it would be called at the same place this error occurs.

@multipletwigs
Copy link
Contributor

Hey! I'd love to take this issue (for my first OSS contribution) if it hasn't been assigned yet 🥳

@multipletwigs
Copy link
Contributor

multipletwigs commented Jan 16, 2023

Hey there, spent some time tinkering around the codebase, and I was wondering if the enhancement is as simple as checking the image src with the following code. First checking if the code is running on a server environment, then checking if it's using an absolute url.

// In src/handler/image.ts

// Check if on server
  if (typeof window === 'undefined') {
    // Check if src is absolute url
    if (!src.startsWith('http') && !src.startsWith('data:')) {
      throw new Error(`Image source must be an absolute URL: ${src}`)
    }
  }

@shuding
Copy link
Member Author

shuding commented Apr 14, 2023

@multipletwigs that fix makes sense to me! Feel free to open a PR :)

shuding pushed a commit that referenced this issue Apr 16, 2023
Addresses #203 by placing a check on absolute url patterns by checking
if the URL begins with 'http' or 'data:' format. Also added in a test to
check for relative urls.
@shuding
Copy link
Member Author

shuding commented Apr 16, 2023

Fixed with #447.

@shuding shuding closed this as completed Apr 16, 2023
sahithyandev pushed a commit to sahithyandev/satori that referenced this issue Apr 26, 2024
Addresses vercel#203 by placing a check on absolute url patterns by checking
if the URL begins with 'http' or 'data:' format. Also added in a test to
check for relative urls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants