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(Redirect): Better handle wrong redirect header in a response #1387

Merged
merged 8 commits into from Nov 26, 2021

Conversation

tasinet
Copy link
Contributor

@tasinet tasinet commented Nov 18, 2021

Purpose

Fixes crash when a Location header point to an invalid URL

Changes

Just wrapping new URL() in a try/catch and returning a sensible (?) error message

Additional information

Test included


  • I updated ./docs/CHANGELOG.md with a link to this PR or Issue
  • I added unit test(s)

src/index.js Outdated Show resolved Hide resolved
LinusU
LinusU approved these changes Nov 18, 2021
Copy link
Member

@LinusU LinusU left a comment

LGTM 👍

src/index.js Outdated Show resolved Hide resolved
@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Nov 19, 2021

Just thought of something... and i tested this in chrome...

if you have redirect: 'manual' then it shouldn't follow the redirect header and try to fetch it... then you may think that it should not throw any error for having invalid headers but it dose...

fetch(/redirect/301/invalid', { redirect: 'manual' })

so i tested it in firefox, and safari also

// FF OK
fetch('/redirect/301/invalid', { redirect: 'manual' }).then(console.log, console.error)
// Safari OK
fetch('/redirect/301/invalid', { redirect: 'manual' }).then(console.log, console.error)
// Chrome Throws
fetch('/redirect/301/invalid', { redirect: 'manual' }).then(console.log, console.error)

We should have a test case for both.
would you mind

  • Adding unit test for when redirect is set to manual and dose not throw
  • Resolve the conflicts

Should also report this to Chrome / WPT

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Nov 19, 2021

Reported it to chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1271920

Copy link
Collaborator

@jimmywarting jimmywarting left a comment

need a redirect=manual test that don't throw

@jimmywarting jimmywarting changed the title Fixed crash when an invalid Location URL is returned from a redirect.… fix(Redirect): Better handle wrong redirect header in a response Nov 19, 2021
tasinet and others added 3 commits Nov 22, 2021
@tasinet
Copy link
Contributor Author

@tasinet tasinet commented Nov 22, 2021

Made some changes

  • linting catch(_error) { -> catch {
  • Does not throw when options.redirect == manual
  • Added test for above
  • Resolved CHANGELOG conflict

@tasinet
Copy link
Contributor Author

@tasinet tasinet commented Nov 25, 2021

Do you need anything else from my end here?

LinusU
LinusU approved these changes Nov 26, 2021
Copy link
Member

@LinusU LinusU left a comment

LGTM

@jimmywarting jimmywarting merged commit 6e4c1e4 into node-fetch:main Nov 26, 2021
8 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.

3 participants