fix: return a 400 for malformed URIs

[flakey5]

See https://nodejs-org.sentry.io/issues/4622681006/events/3a4e26a6669c46ffb84f20369e90ac8c/

[ovflowd]

Quick question: Why does it crash there and not here? https://github.com/nodejs/release-cloudflare-worker/blob/main/src/handlers/get.ts#L29

[flakey5]

Quick question: Why does it crash there and not here? https://github.com/nodejs/release-cloudflare-worker/blob/main/src/handlers/get.ts#L29

Honestly I don't know, my best guess is that calling new URL doesn't validate the path and instead focuses just on the structure of the url itself?

[ovflowd]

Quick question: Why does it crash there and not here? https://github.com/nodejs/release-cloudflare-worker/blob/main/src/handlers/get.ts#L29

Honestly I don't know, my best guess is that calling new URL doesn't validate the path and instead focuses just on the structure of the url itself?

Could also be lazy construction/validation

[targos]

I'm not sure you're supposed to call decodeURIComponent on the pathname. The URL API already decodes automatically what should be decoded.

[ovflowd]

I'm not sure you're supposed to call decodeURIComponent on the pathname. The URL API already decodes automatically what should be decoded.

Right, I think it is indeed unnecessary as URL already does the sanitizations that are needed on the pathname property.

Off-topic: @flakey5 I believe

release-cloudflare-worker/src/util.ts

Lines 53 to 61 in b854489

	const urlToBucketPathMap: Record<string, string> = {
	dist: DIST_PATH_PREFIX + (url.pathname.substring('/dist'.length) || '/'),
	download:
	DOWNLOAD_PATH_PREFIX +
	(url.pathname.substring('/download'.length) || '/'),
	docs: DOCS_PATH_PREFIX + (url.pathname.substring('/docs'.length) || '/'),
	api: API_PATH_PREFIX + (url.pathname.substring('/api'.length) || '/'),
	metrics: url.pathname.substring(1), // substring to cut off the /
	};

things like these, should be in the constants file. We should have all sorts of mappings like these in dedicated constants and explain them. The code is already becoming complex enough that often it's hard to find our way in some places.

Like I didn'te ven realise mapUrlPathToBucketPath was doing so much.

[flakey5]

The call to decodeURIComponent isn't necessarily to validate the path, it's to decode the path from say /a%20b%20c to /a b c. From my testing in both workers and node the URL object seemingly doesn't decode the path for this automatically.

We should have all sorts of mappings like these in dedicated constants and explain them

Yeah that sounds like a good idea as well

[targos]

The call to decodeURIComponent isn't necessarily to validate the path, it's to decode the path from say /a%20b%20c to /a b c. From my testing in both workers and node the URL object seemingly doesn't decode the path for this automatically.

Yes, but why do we need it? There are no legitimate URLs that need percent encoding in the pathname.

[targos]

Another approach could be to call decodeURI on the entire URL before passing it to the URL constructor?

[ovflowd]

Another approach could be to call decodeURI on the entire URL before passing it to the URL constructor?

That sounds the best approach here, and to catch the broken URL directly when calling `new UR. (inside the parseURL) (with try/catch) so we return undefined -> BAD_REQUEST directly from there

[flakey5]

If we don't need to validate the uri I think we can just go without it, if it becomes a need later on (e.g. file names with spaces) we can add it back

[ovflowd]

If we don't need to validate the uri I think we can just go without it, if it becomes a need later on (e.g. file names with spaces) we can add it back

I think the idea was to validate the url within parseUrl when we create new URL and have try/catch there to then return undefined.

But I guess we can also simply do it like this for now, and be reactive if errors arise.