fix: handle request paths starting with // #6
Conversation
Calling `new URL('//foo', 'http://127.0.0.1')` produces href
`http://foo/`. As a result, we were treating all requests for
paths starting with double slash as if no path was specified.
Signed-off-by: Miroslav Bajtoš <[email protected]>
| @@ -32,7 +32,9 @@ export const createHandler = ({ | |||
| * @param {import('pg').Pool} pgPool | |||
| */ | |||
| const handler = async (req, res, pgPool) => { | |||
| const { pathname, searchParams } = new URL(req.url, 'http://127.0.0.1') | |||
| // Fix the edge case: new URL('//foo', 'http://127.0.0.1') produces href "http://foo/" | |||
| const reqUrl = req.url.replace(/^\/+/, '/') | |||
There was a problem hiding this comment.
I disagree with this change. The url ${HOST}// is invalid and doesn't need to be supported.
There was a problem hiding this comment.
According to https://stackoverflow.com/a/20524044/69868, which links to several other resources including RFC 3986 - Uniform Resource Identifier (URI): Generic Syntax, a path starting with a double slash is a valid one.
Consensus: browsers will do the request as-is, they will not alter the request. The / character is the path separator, but as path segments are defined as:
path-abempty = *( "/" segment ) segment = *pcharMeans the slash after
http://example.com/can directly be followed by another slash, ad infinitum. Servers might ignore it, but browsers don't, as you have figured out.
TBH, I would rather start using a framework like Fastify that would handle these low-level details for us.
There was a problem hiding this comment.
The source says that URLs like this can be valid, but that the server can handle it as it wish. We can be strict and just disallow it. If we want to keep allowing it then I agree that a framework would be great, as what we have now with this PR feels a bit like a hack.
I propose to revert this PR and keep the server simple and strict. Or do you have a use-case for supporting double slashes?
There was a problem hiding this comment.
Well, I was surprised to learn that a request starting with double slashes was treated the same way as a request starting with /. That's a bug I want to fix, to avoid further surprises.
An alternative solution to this PR is to return 404 when request.url starts with //. Is such a solution in line with your proposal to be strict and just disallow it?
if (req.url.startsWith('//')) {
return notFound(res)
}To be clear, I am happy to change this, but I want to find a solution that behaves as one would expect.
Another option that comes to my mind, I would need to double-check that it works as I think it should work.
- new URL(req.url, 'http://127.0.0.1')
+ new URL(`http://127.0.0.1${req.url}`)There was a problem hiding this comment.
An alternative solution to this PR is to return 404 when request.url starts with //. Is such a solution in line with your proposal to be strict and just disallow it?
I would like not to mention // at all. It's a pity that the URL constructor allows the relative first argument to go "above" the provided base URL. Maybe yeah new URL(http://127.0.0.1${req.url}`)` is a simple improvement 👍
There was a problem hiding this comment.
FWIW, http://api.filspark.com//rounds/current handles double-slash as a single slash. I want our services to behave consistently.
I am going to use http://127.0.0.1${req.url} approach.
Calling
new URL('//foo', 'http://127.0.0.1')produces hrefhttp://foo/. As a result, we were treating all requests for paths starting with double slash as if no path was specified.Links: