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

fix: handle relative and invalid URLs in redirects when passing URL to loadImage() #866

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

lostfictions
Copy link
Contributor

@lostfictions lostfictions commented Aug 9, 2024

Fixes #865, see the description of the original issue there.

This PR does two things:

  • One, this wraps the http.get()/https.get() callback in loadImage() in a try...catch block so that exceptions are properly handled and passed to callers instead of crashing the Node process.

  • Two, this passes url.origin as the second parameter to new URL() to ensure that "relative" redirects (where the location header is something like /some/resource.jpg instead of https://site.com/some/resource.jpg) do not cause the constructor to throw an exception.

    Redirects to another domain (for example, a location header to https://other.com from the origin https://site.com) are not affected by adding the second parameter. Only relative URLs are affected -- the kind that currently cause crashes in user code.

Without this fix, passing URLs to loadImage() does not reliably work when the URL has a redirect, and will instead crash the Node process without a full stack trace, even if the call to loadImage() is in a try block.

@lostfictions
Copy link
Contributor Author

The CI failure seems unrelated. I've also added more details to the PR description. @Brooooooklyn, do you have any feedback?

@Brooooooklyn Brooooooklyn changed the title Handle relative and invalid URLs in redirects when passing URL to loadImage() fix: handle relative and invalid URLs in redirects when passing URL to loadImage() Aug 14, 2024
@Brooooooklyn Brooooooklyn merged commit 1666a33 into Brooooooklyn:main Aug 14, 2024
29 checks passed
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.

loadImage() does not handle exceptions properly, causing calling code to crash ("floating" async code)
2 participants