Skip to content

fix: prevent dev server crashing on malformed URI (fix #6300)#6308

Merged
patak-cat merged 3 commits intovitejs:mainfrom
toSayNothing:main
Dec 31, 2021
Merged

fix: prevent dev server crashing on malformed URI (fix #6300)#6308
patak-cat merged 3 commits intovitejs:mainfrom
toSayNothing:main

Conversation

@toSayNothing
Copy link
Copy Markdown
Contributor

Description

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

when there is URIError, the dev server will crash because it didn't catch the error
reprodution : open url like http://localhost:3000/a%a

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 30, 2021
@Niputi Niputi linked an issue Dec 30, 2021 that may be closed by this pull request
7 tasks
bluwy
bluwy previously approved these changes Dec 30, 2021
@Niputi
Copy link
Copy Markdown
Contributor

Niputi commented Dec 30, 2021

would it be better to first try with decodeURI and then without?

decodeURI change was introduced in #4728, with other places than this so other places might need to be updated too

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Dec 30, 2021

I think that makes sense too. Like some sort of tryDecodeURI utility. The only other place I found which might hit the decode error was viteServeStaticMiddleware, but that middleware is instantiated after viteTransformMiddleware (where this PR fixes), so this PR would indirectly cover viteServeStaticMiddleware too

@toSayNothing
Copy link
Copy Markdown
Contributor Author

toSayNothing commented Dec 30, 2021

i just found that there's already a errorMiddleware which can handle the error and prevent crash🤣, whether in transformMiddleware or serveStaticMiddleware, looks like this:
fdbb681c19800864c901c3260b40039
so i correct the return with next(e) , so the error can be passed to the errorMiddleware to handle it:
image
after passing to errorMiddleware, therefor we don't need to specifically address this type of error in all places.

@Niputi
Copy link
Copy Markdown
Contributor

Niputi commented Dec 30, 2021

@bluwy @patak-dev what do you think is best fallback or showing error message?

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Dec 30, 2021

Looking back now, I think showing an error message may be safer, since having a fallback might imply that we support it, which we might not. So using next(e) looks good to me. Neat solution too.

@toSayNothing Re viteServeStaticMiddleware, I'm taking back what I said 😅 viteTransformMiddleware can be skipped, so we need to handle decodeURI for viteServeStaticMiddleware too:

const url = decodeURI(req.url!)

@toSayNothing
Copy link
Copy Markdown
Contributor Author

toSayNothing commented Dec 30, 2021

@bluwy 🤣i have debugged and found that :
connect can handle the Synchronization function 's error , but not the Asynchronous function .
viteServeStaticMiddleware is sync, so it can handle by the connect itself(call try catch)
but the async viteTransformMiddleware will crash the server if not handly by catch(e){return next(e)}
image
online connect repo : https://stackblitz.com/edit/node-w8ukhk

@bluwy
Copy link
Copy Markdown
Member

bluwy commented Dec 31, 2021

Nice observation @toSayNothing. That's an interesting behaviour from connect. It would sure be nice if it handles promises too, but for now this looks good to me.

@patak-cat patak-cat merged commit a49d723 into vitejs:main Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Vite dev server crashes on malformed URI

5 participants