Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Conversation

@jervtub
Copy link

@jervtub jervtub commented May 16, 2020

#1142
Route with address "/client/" is not working.

The first request to any route under "/client" - for instance "http://localhost/client/new" - would
incorrectly get past the filter function at req.start.startsWith(prefix).

Fix: add an extra test in the filter, whether there is a dot present in the
requested path.

Reasoning:
I assume all the content served by sapper has an extension, such as "/client/chunk_slug.js".
As well I assume there aren't requests to paths both startign with
"/client" and containing a dot, such as "http://localhost/client/new.123". That would still result in a 404.

#1142
Route with address "/client/" is not working.

The first request to any route under "/client" - for instance "http://localhost/client/new" - would
incorrectly get past the filter function at [`req.start.startsWith(prefix)`](https://github.com/sveltejs/sapper/blob/facbd96e0bd2539acf32c5ac14cd9478c7c635e5/runtime/src/server/middleware/index.ts#L104).

Fix: add an extra test in the filter, whether there is a dot present in the
requested path.

Reasoning:
I assume all the content served by sapper has an extension, such as "/client/chunk_slug.js".
As well I assume there aren't requests to paths both startign with
"/client" _and_ containing a dot, such as "http://localhost/client/new.123". That would still result in a 404.
@benmccann
Copy link
Member

benmccann commented May 23, 2020

@jervtub I don't quite understand how you're differentiating. Doesn't /client/chunk_slug.js have a dot in it just like client/new.123?

> /\..*$/.test('/client/chunk_slug.js')
true
> /\..*$/.test('/client/new.123')
true

I'm not sure it's a good idea to try to differentiate by file name. I prefer the approach that @Conduitry suggested #1142 (comment)

@benmccann
Copy link
Member

@jervtub just checking in on this PR. Are you still interested in pursuing it? Did you have any thoughts about my question?

@benmccann
Copy link
Member

Closing in favor of #1307

@benmccann benmccann closed this Jul 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants