svelte-adapter-express icon indicating copy to clipboard operation
svelte-adapter-express copied to clipboard

Quick review

Open Prinzhorn opened this issue 4 years ago • 3 comments

I've skimmed over server.js and noticed some issues / possible improvements:

  1. AFAIK Express 4 does not support async routes. That means a promise rejection in your async handler will likely take down the whole server via unhandledRejection
  2. I'd rather const { render } = await import("./app.js"); once outside the middleware. Yes, the import is cached but I think it'd be better to import it outside so that if there is an issue with the import the server won't even start instead of waiting for the first request to make it crash.
  3. You are passing host to render. The official node adapter does not. I can't find adapter docs, so I'm not sure if that's needed.
  4. URL does not have a query property, only with the legacy url API. So no need to explicitly use new URLSearchParams, there already is a searchParams property
  5. Worse than 3. the host is built using x-forwarded-proto which is (malicious) user provided. That means a malicious user can inject a malicious host into it which can lead to fun vulnerabilities (if I send a x-forwarded-proto: https://evil.com? header you will set host to https://evil.com?://${request.headers.host}). E.g. if a route uses it to point to a resource or generate an e-mail such as "Forgotten password". For parsing out pathname and query a bogus host can be used to make the URL constructor happy. As for using host itself it is common practice to not rely on x-* headers to build absolute URLs and instead configure a fixed host. This could be done via an env variable.

I can't test any of these claims because

svelte.config.cjs should be renamed to svelte.config.js and converted to an ES module. See https://kit.svelte.dev/docs#configuration for an example

> Using @mankins/svelte-adapter-express
> Cannot read property 'minor' of undefined

Prinzhorn avatar May 31 '21 14:05 Prinzhorn

Hi, thanks for looking. As you've discovered this is very much a piece of research. I namespaced this to my username to indicate it's not an official sveltekit adapter.

With that said it was lifted from the node adapter as a way to work with middleware for development. And worked for the project I was working on.

I'm not sure I'd use this in production for some of the reasons you mentioned above.

SvelteKit is moving very rapidly (the whole svelte.config.cjs error) so hopefully this can help out in a similar way.

Of course: PRs accepted for some of the issues above.

mankins avatar Jun 02 '21 16:06 mankins

I was curious how an Express adapter could look like and I couldn't ignore the issues I saw, that's the only reason I opened this. Some might blindly copy the code and actually use it in production, that's just how things go :smile:

Prinzhorn avatar Jun 03 '21 08:06 Prinzhorn

True, I should put some warnings on this.

I have to think that the sveltekit team has intentionally not done something with express/hiding the serving complexity.

mankins avatar Jun 03 '21 11:06 mankins