refactor: internalEvent.url is a full URL #752

vicb · 2025-02-25T10:05:59Z

fixes #719

edit: One thing I need to check is the base path, we should probably add it when constructing URLs (i.e. 404, 500)

@conico974 could you please review and maybe run the aws integration tests when this looks ok?
cf tests are green with this PR.
Thanks!

changeset-bot · 2025-02-25T10:06:04Z

🦋 Changeset detected

Latest commit: 9ffa41a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@opennextjs/aws	Patch
app-pages-router	Patch
app-router	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

pkg-pr-new · 2025-02-25T10:06:37Z

Open in Stackblitz

pnpm add https://pkg.pr.new/@opennextjs/aws@752

commit: 9ffa41a

github-actions · 2025-02-25T10:07:02Z

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	24.19%	2012 / 8316
🔵	Statements	24.19%	2012 / 8316
🔵	Functions	51.78%	116 / 224
🔵	Branches	71.82%	497 / 692

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
packages/open-next/src/adapters/edge-adapter.ts	0%	0%	0%	0%	1-75
packages/open-next/src/adapters/middleware.ts	0%	0%	0%	0%	1-120
packages/open-next/src/core/requestHandler.ts	0%	0%	0%	0%	1-265
packages/open-next/src/core/routingHandler.ts	0%	0%	0%	0%	1-227
packages/open-next/src/core/routing/matcher.ts	77.68%	74.15%	91.66%	77.68%	37-39, 48-52, 57-59, 62, 101, 103, 106, 109, 119-120, 162-164, 275-277, 279-292, 311-312, 377-416
packages/open-next/src/core/routing/middleware.ts	96.49%	85.18%	50%	96.49%	34-37, 113
packages/open-next/src/core/routing/util.ts	87.15%	88.76%	90%	87.15%	87-90, 223-224, 294-296, 374-397
packages/open-next/src/overrides/converters/aws-apigw-v1.ts	96.87%	82.6%	100%	96.87%	94-96
packages/open-next/src/overrides/converters/aws-apigw-v2.ts	91.45%	83.33%	100%	91.45%	45-46, 50-54, 114-116
packages/open-next/src/overrides/converters/aws-cloudfront.ts	78.45%	80.76%	100%	78.45%	110-113, 158-198
packages/open-next/src/overrides/converters/edge.ts	0%	0%	0%	0%	1-102
packages/open-next/src/overrides/converters/node.ts	92.3%	78.57%	66.66%	92.3%	52-55
packages/open-next/src/overrides/converters/utils.ts	100%	100%	100%	100%
packages/open-next/src/overrides/wrappers/cloudflare-node.ts	0%	0%	0%	0%	1-85
packages/open-next/src/types/open-next.ts	0%	0%	0%	0%

Generated in workflow #1011 for commit 9ffa41a by the Vitest Coverage Report Action

conico974

All internal redirect are failing in e2e because of the incorrect url created.

One thing I need to check is the base path, we should probably add it when constructing URLs (i.e. 404, 500)

Yeah good point, we should probably create a helper functions for these cases

conico974 · 2025-02-25T11:08:38Z

packages/open-next/src/overrides/converters/aws-apigw-v1.ts

  return {
    type: "core",
    method: httpMethod,
    rawPath: path,
-    url: path + normalizeAPIGatewayProxyEventQueryParams(event),
+    url: `https://${headers.host ?? "on"}${path}${normalizeAPIGatewayProxyEventQueryParams(event)}`,


For all aws converters, the host will not be correct here.
The correct host is in the x-forwarded-host header, we apply it but only after the converter has already run :

opennextjs-aws/packages/open-next/src/core/requestHandler.ts

Lines 42 to 44 in 674dce6

if (initialHeaders["x-forwarded-host"]) {

initialHeaders.host = initialHeaders["x-forwarded-host"];

}

Ok I'll apply the change in aws converters.
Could the code in request handler (your link) be deleted then?

Honestly i'm not sure how to handle this one properly. We need the headers to be updated, but only if there is a x-forwarded-host

This could be done in the converter as it is aws specific, no?

Not entirely, this is kind of a standard https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host.
And this will likely cause trouble when used with the node converter and wrapper as well.
Maybe a better solution would be to modify the url in the request handler as well. If we have a x-forwarded-host we change both the host header and the url to use this header.
We may have to move this to the routing handler though (in case of an external middleware)

conico974 · 2025-02-25T11:43:01Z

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

-      url: `${rewrittenUrl}${convertToQueryString(finalQuery)}`,
+      url: new URL(
+        `${rewrittenUrl}${convertToQueryString(finalQuery)}`,
+        new URL(event.url),


Here it fails in e2e because event.url is incorrect.

Do you know what test fails?

opennextjs-aws/packages/tests-e2e/tests/appRouter/config.redirect.test.ts

Line 17 in 674dce6

test("Missing cookies", async ({ page }) => {

opennextjs-aws/packages/tests-e2e/tests/appRouter/config.redirect.test.ts

Line 40 in 674dce6

test("Has cookies", async ({ page }) => {

opennextjs-aws/packages/tests-e2e/tests/appRouter/config.redirect.test.ts

Line 51 in 674dce6

test("Has cookies with value", async ({ page }) => {

And these 2

opennextjs-aws/packages/tests-e2e/tests/pagesRouter/redirect.test.ts

Lines 11 to 25 in 674dce6

test("Redirect with default locale support", async ({ page }) => {

await page.goto("/redirect-with-locale/");

await page.waitForURL("/ssr/");

const el = page.getByText("SSR");

await expect(el).toBeVisible();

});

test("Redirect with locale support", async ({ page }) => {

await page.goto("/nl/redirect-with-locale/");

await page.waitForURL("/nl/ssr/");

const el = page.getByText("SSR");

await expect(el).toBeVisible();

});

conico974 · 2025-02-25T12:48:50Z

packages/open-next/src/overrides/converters/aws-apigw-v1.ts

@@ -62,7 +62,7 @@ async function convertFromAPIGatewayProxyEvent(
    type: "core",
    method: httpMethod,
    rawPath: path,
-    url: `https://${headers.host ?? "on"}${path}${normalizeAPIGatewayProxyEventQueryParams(event)}`,
+    url: `https://${headers["x-forwarded-host"] ?? "on"}${path}${normalizeAPIGatewayProxyEventQueryParams(event)}`,


This is not 100% correct either, we could create an helper function that would retrieve the host from the headers.
If this header is not set (which can be the case), we should fallback to the host header

Do you mean an helper to headers["x-forwarded-host"] ?? headers.on ?? "on" ?
For all aws converters?

That was my original idea, but i think there is a better solution (#752 (comment)). In the converters we keep headers.host ?? "on", but in the routing handler if we have a x-forwarded-host header, we change both the host header and the internalEvent.url

vicb · 2025-02-25T15:06:03Z

I have pushed some commits to integrate the feedback.

@conico974 Could you please run the test in debug mode and report the rewrittenUrl is they are still failing?

debug("rewrittenUrl", { rewrittenUrl, finalQuery, isUsingParams });

Thanks!

conico974

I've run the e2e test and everything is working as expected.
LGTM Thanks

vicb · 2025-02-25T16:00:51Z

Thanks a lot @conico974, I will double check the basePath thing before merging.

vicb · 2025-02-26T08:34:02Z

I tested using a basePath on cloudflare and it works the same as with next dev, merging

refactor: internalEvent.url is a full URL

918e651

vicb force-pushed the url branch from a3d1482 to 918e651 Compare February 25, 2025 10:06

vicb requested a review from conico974 February 25, 2025 10:34

fixup! add changeset

ea73fc6

conico974 reviewed Feb 25, 2025

View reviewed changes

fixup! aws converter host

3f36c06

conico974 reviewed Feb 25, 2025

View reviewed changes

vicb added 2 commits February 25, 2025 14:52

fixup! review feedback

c2d5d99

fixup! helper for host

0e20584

vicb added 2 commits February 25, 2025 16:08

fixup!

e01ef8d

fixup! build

9ffa41a

conico974 approved these changes Feb 25, 2025

View reviewed changes

vicb merged commit e48951f into main Feb 26, 2025
3 checks passed

vicb deleted the url branch February 26, 2025 08:34

github-actions bot mentioned this pull request Feb 24, 2025

Version Packages #729

Merged

This was referenced Feb 26, 2025

How to handle host headers #753

Open

Fix i18n for domain #759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: internalEvent.url is a full URL #752

refactor: internalEvent.url is a full URL #752

vicb commented Feb 25, 2025 •

edited

Loading

changeset-bot bot commented Feb 25, 2025 •

edited

Loading

pkg-pr-new bot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

conico974 left a comment

conico974 Feb 25, 2025

vicb Feb 25, 2025

conico974 Feb 25, 2025

vicb Feb 25, 2025

conico974 Feb 25, 2025

conico974 Feb 25, 2025

vicb Feb 25, 2025

conico974 Feb 25, 2025

conico974 Feb 25, 2025

vicb Feb 25, 2025 •

edited

Loading

conico974 Feb 25, 2025

vicb commented Feb 25, 2025

conico974 left a comment

vicb commented Feb 25, 2025

vicb commented Feb 26, 2025

	if (initialHeaders["x-forwarded-host"]) {
	initialHeaders.host = initialHeaders["x-forwarded-host"];
	}

	test("Redirect with default locale support", async ({ page }) => {
	await page.goto("/redirect-with-locale/");

	await page.waitForURL("/ssr/");
	const el = page.getByText("SSR");
	await expect(el).toBeVisible();
	});

	test("Redirect with locale support", async ({ page }) => {
	await page.goto("/nl/redirect-with-locale/");

	await page.waitForURL("/nl/ssr/");
	const el = page.getByText("SSR");
	await expect(el).toBeVisible();
	});

refactor: internalEvent.url is a full URL #752

refactor: internalEvent.url is a full URL #752

Conversation

vicb commented Feb 25, 2025 • edited Loading

changeset-bot bot commented Feb 25, 2025 • edited Loading

🦋 Changeset detected

pkg-pr-new bot commented Feb 25, 2025 • edited Loading

github-actions bot commented Feb 25, 2025 • edited Loading

Coverage Report

conico974 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Feb 25, 2025

conico974 left a comment

Choose a reason for hiding this comment

vicb commented Feb 25, 2025

vicb commented Feb 26, 2025

vicb commented Feb 25, 2025 •

edited

Loading

changeset-bot bot commented Feb 25, 2025 •

edited

Loading

pkg-pr-new bot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

vicb Feb 25, 2025 •

edited

Loading