Make event.url consistent across platforms (URL vs path)

[vicb]

We have an issue with trailing slashes, see opennextjs/opennextjs-cloudflare#312

The reason why this doesn't work is this code:

opennextjs-aws/packages/open-next/src/core/routing/matcher.ts

Lines 258 to 269 in 14b8182

	const url = new URL(event.url, "http://localhost");
	const emptyBody = emptyReadableStream();
	if (
	// Someone is trying to redirect to a different origin, let's not do that
	url.host !== "localhost" ||
	NextConfig.skipTrailingSlashRedirect ||
	// We should not apply trailing slash redirect to API routes
	event.rawPath.startsWith("/api/")
	) {
	return false;
	}

in cloudflare, event.url is http://localhost:8787/... so url.host !== "localhost" is true (the host is localhost:8787).

I think that event.url is not usually an URL but a path in @opennextjs/aws.

However in @opennextjs/cloudflare, event.url is actually an URL.

See also this code in cloudflare-node.ts:

opennextjs-aws/packages/open-next/src/overrides/wrappers/cloudflare-node.ts

Lines 33 to 38 in 14b8182

	// TODO:
	// The edge converter populate event.url with the url including the origin.
	// This is required for middleware to keep track of the protocol (i.e. http with wrangler dev).
	// However the server expects that the origin is not included.
	const url = new URL(internalEvent.url);
	(internalEvent.url as string) = url.href.slice(url.origin.length);

@conico974 do you have an idea on how to best fix this?

Thanks!

[conico974]

Maybe just changing this line would be enough to fix it temporarily

opennextjs-aws/packages/open-next/src/core/routing/matcher.ts

Line 258 in 14b8182

const url = new URL(event.url, "http://localhost");

We don't really care about the actual value here, we could replace it with something like http://n (same way next is doing it in some places)

I think a more permanent fix would be to ensure that url always have a protocol and an host (that we could replace by dummy values if we can't infer them ). We probably need to change a bunch of things in the routing layer (and the converters) for this to work

[vicb]

IIUC, I don't think that would help?

[conico974]

Ho sorry, yeah, we'll need to change the if below as well (i.e. if url.host !== "n" instead of localhost)
The issue we had with that is that by using trailing slash redirect stuff, an attacker could trigger a redirect to an external url (because in lambda url has no protocol/host).

[vicb]

I don't think that would work as url.host is localhost:8787 on my example above :)

[conico974]

Ho yeah nevermind.
Replacing with event.rawPath in this same line should work though

[vicb]

Replacing with event.rawPath in this same line should work though

Would that defeat "The issue we had with that is that by using trailing slash redirect stuff, an attacker could trigger a redirect to an external url"? (i.e. make the test useless).

[vicb]

I don't really understand how an attacker would forge event.url?

[conico974]

It should still work as expected for this specific usecase, this is the original issue : #355

[vicb]

I think a more permanent fix would be to ensure that url always have a protocol and an host (that we could replace by dummy values if we can't infer them ). We probably need to change a bunch of things in the routing layer (and the converters) for this to work

Is probably the next step for this bug