sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
7.01k stars 435 forks source link

Different query parsing result on server and on client (polka bug) #1743

Closed istarkov closed 3 years ago

istarkov commented 3 years ago

Having url like ?prop=a+b we following page.query have in preload function.

At server: { prop: 'a+b' } At client: { prop: 'a b' }

The issue that polka has bug in query string parsing, and using req.query in such case is not safe.

Added issue to polka https://github.com/lukeed/polka/issues/150#issuecomment-796226741 but looks like polka is dead

antony commented 3 years ago

If the issue is in Polka, please raise it only there. Even if it was dead (which it isn't), it still doesn't mean you should raise it here. Polka isn't dead - it's important to differentiate between "dead" and "feature complete".

You can switch to express to confirm your issue, to be 100% certain that this is Polka specific.

But also please use the issue template if you want to raise an issue, rather than deleting it. It's helpful for us who have to fix those issues.

istarkov commented 3 years ago

Im 100% certain that its polka specific, btw the issue on what side to implement query parsing having that URLSearchParams can be used in browser. Isnt it better to share client code to be sure that query parsing is same on both sides. Also I can do PRs too, and I did even in supper, so the reason of opening issue is discussion, as the final bug I see in sapper.

antony commented 3 years ago

I think what might be worthwhile is to determine if this issue exists within svelte-kit (when released) and deal with it there. I don't feel it will, since svelte kit uses something more akin to what you're saying, and doesn't rely on Polka.

However - if as you say this bug exists in Polka then we will wait for an upstream fix there and merge it into Sapper as and when it is available, if this is deemed critical. Sapper isn't at a point in its lifecycle where we would re-archictect the way query parsing happens, though.

istarkov commented 3 years ago

The issue that sveltekit source is not available to community and until then we use sapper :-)

antony commented 3 years ago

I too use Sapper :) It'll be available soon. I'm 100% confident this issue doesn't exist there, though.

benmccann commented 3 years ago

Actually, SvelteKit's adapter-node uses Polka, so if there's a bug in query parsing there's a good chance it might affect SvelteKit

lukeed commented 3 years ago

Lol dead.

You can always override the Polka parser if you're not happy w/ it or can't wait for me to get to it:

const app = polka();
app.parse = require('parseurl');
//=> Done!

As @antony mentioned, there's a difference between dead & feature complete. There's intentionally a distinction between the stable Polka packages and the @next versions. The latter is collecting feedback/use cases before a 1.0 is finalized. But thank you for your addl bug report & your diagnosis.

istarkov commented 3 years ago

Ok not dead ;-)

Finally sveltekit gives page query as URLSearchParams { 'prop' => 'a b' } so should not be an issue there

benmccann commented 3 years ago

Finally sveltekit gives page query as URLSearchParams { 'prop' => 'a b' } so should not be an issue there

Just to make sure, did you test with @sveltejs/adapter-node specified in svelte.config.js and package.json, do npm run build, and then node build when testing? That's the only way you'd actually test the node adapter where Polka is used

istarkov commented 3 years ago

Yes, I did. One issue with node-adapter that it uses deprecated node api for url parsing url.parse https://nodejs.org/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost

lukeed commented 3 years ago

Just to follow up, this was fixed on Polka's side in the next.15 releases.