Fix cloudflare env #514

conico974 · 2024-09-12T16:51:24Z

This PR fix an issue with cloudflare env variable being undefined if the middleware does not run.
This was breaking enableCacheInterception with the middleware in cloudflare.

This PR also fix some issue with cookies being not properly set when set both in the routing layer (i.e. middleware or next.config) and the route itself.
It also fix an issue with cookies and the node wrapper

changeset-bot · 2024-09-12T16:51:28Z

⚠️ No Changeset found

Latest commit: 88a8675

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

vercel · 2024-09-12T16:51:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
open-next	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 17, 2024 11:22am

khuezy · 2024-09-12T16:56:46Z

packages/open-next/src/wrappers/cloudflare.ts

  async (event: Request, env: Record<string, string>): Promise<Response> => {
-    //@ts-expect-error - process is not defined in cloudflare workers
-    globalThis.process = { env };
+    globalThis.process = process;


Where is the process variable coming from? Is that specific to cloudflare?
Oh nvm, it's the global.process... got confused w/ the globalThis

I added this as a js banner in the esbuild that i did for the cloudflare worker.
Now that i think about it we should probably do this esbuild part inside of OpenNext as well. We'd get a single file out of it which is easier for cloudflare worker.

I'll publish a sample repo later with SST v3 with different way to deploy OpenNext on different target.
Right now i have EC2, ECS and lambda with either cloudfront or cloudflare in front of it ( and with cloudflare the middleware runs in the worker )
Maybe that's something we could put in the new org as well

khuezy · 2024-09-12T17:20:03Z

packages/open-next/src/wrappers/node.ts

    const internalEvent = await converter.convertFrom(req);
    const _res: StreamCreator = {
      writeHeaders: (prelude) => {
+        res.setHeader("Set-Cookie", prelude.cookies);


I don't think Set-Cookie allows for setting multiple cookies at once (could be wrong). Need to split the cookies and res.addHeader if so.

Actually it does ( I thought the same thing as well at first ).

That's really weird, according to the spec: https://httpwg.org/specs/rfc6265.html#overview it shouldn't. Since the comma character may also be part of the cookie value, eg Expires=Wed, 09 Jun 2021 10:18:14 GMT

I wonder what the prelude.cookies is, is it an array? Does it get parsed out and handle somewhere else in the stack, or does res.setHeader here ultimately go back to the client.

Edit: I tried doing comma separated Set-Cookies in my server and it doesn't work. Only the first cookie is set. But doing multiple Set-Cookie for each cookie works as expected.

prelude.cookies is an array, it's from here
https://github.com/sst/open-next/blob/faefb33a2a485c571c2cc43680f1b702a03b58c7/packages/open-next/src/http/openNextResponse.ts#L187

It will work with an array: https://nodejs.org/api/http.html#http_response_setheader_name_value

Ah ok, I missed that the node response setHeader is doing the heavy lifting.

Actually there was an issue here ( i forgot to remove the custom wrapper on my test 🙄 ).
We needed to remove the set-cookie header from the headers, otherwise the broken one would take precedence.
There was also another issue with the set-cookie that affected everything. If cookies were set both in the middleware and in a route, only the middleware ones would be applied

khuezy · 2024-09-13T15:15:37Z

packages/open-next/src/http/openNextResponse.ts

+      const initialCookies = parseCookies(
+        (this.initialHeaders[SET_COOKIE_HEADER] as string | string[]) ?? [],
+      ) as string[];
+      //Do we want to filter out the cookies that are already set?


Suggested change

//Do we want to filter out the cookies that are already set?

// Do we want to filter out the cookies that are already set?

khuezy · 2024-09-13T15:15:46Z

packages/open-next/src/http/openNextResponse.ts

+        (this.initialHeaders[SET_COOKIE_HEADER] as string | string[]) ?? [],
+      ) as string[];
+      //Do we want to filter out the cookies that are already set?
+      // At the moment cookies from the middlewware will override the ones set in the route


Suggested change

// At the moment cookies from the middlewware will override the ones set in the route

// At the moment cookies from the middleware will override the ones set in the route

khuezy · 2024-09-13T15:17:25Z

packages/open-next/src/http/openNextResponse.ts

+      ) as string[];
+      //Do we want to filter out the cookies that are already set?
+      // At the moment cookies from the middlewware will override the ones set in the route
+      this._cookies = [...this._cookies, ...initialCookies];


Good question, we should ask the community? In terms of flow, middleware comes before the route so I'd assume the route should have the final say in the cookies. Just my pov, but you could argue the other way depending on your use case.

Yeah we could ask the community.
Right now for headers, middleware will override route, it makes sense that it's the same for cookies. The thing with the route having the final say is that in some case ( app router for example ) there is no way to modify the headers others than by having the middleware take precedence.

There is also another option here, we could use an env variable (or an option in the config) to decide which should take priority.

Yeah we could ask the community. Right now for headers, middleware will override route, it makes sense that it's the same for cookies. The thing with the route having the final say is that in some case ( app router for example ) there is no way to modify the headers others than by having the middleware take precedence.

There is also another option here, we could use an env variable (or an option in the config) to decide which should take priority.

I like this idea, especially in the config. However, I think that the middleware should override both cookies and headers by default

conico974 added 2 commits September 12, 2024 11:43

fix cookies improperly set on node

d97633d

fix issue with env in cloudflare

08fa891

vercel bot deployed to Preview September 12, 2024 16:52 View deployment

khuezy reviewed Sep 12, 2024

View reviewed changes

khuezy approved these changes Sep 12, 2024

View reviewed changes

conico974 added 2 commits September 13, 2024 11:31

fix cookies not properly set when set both in the middleware and a route

4a47d5a

bundle everything for edge into one file

69c71d3

vercel bot deployed to Preview September 13, 2024 10:29 View deployment

avoid bundling in one file when middleware is inside server function

7c2277a

vercel bot deployed to Preview September 13, 2024 11:09 View deployment

conico974 mentioned this pull request Sep 13, 2024

fix cookies improperly set on node #506

Closed

khuezy reviewed Sep 13, 2024

View reviewed changes

added option for headers priority

88a8675

vercel bot deployed to Preview September 17, 2024 11:22 View deployment

sommeeeer mentioned this pull request Sep 17, 2024

docs: add headers and cookies priority #519

Closed

Create weak-hotels-thank.md

b8197ce

conico974 merged commit 50703a3 into opennextjs:main Oct 3, 2024
1 check passed

conico974 deleted the fix/cloudflare-env branch October 29, 2024 10:41

	//Do we want to filter out the cookies that are already set?
	// Do we want to filter out the cookies that are already set?

	// At the moment cookies from the middlewware will override the ones set in the route
	// At the moment cookies from the middleware will override the ones set in the route

Fix cloudflare env #514

Fix cloudflare env #514

Uh oh!

Conversation

conico974 commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

vercel bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khuezy Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khuezy Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sommeeeer Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

conico974 commented Sep 12, 2024 •

edited

Loading

changeset-bot bot commented Sep 12, 2024 •

edited

Loading

vercel bot commented Sep 12, 2024 •

edited

Loading

khuezy Sep 12, 2024 •

edited

Loading

khuezy Sep 12, 2024 •

edited

Loading

sommeeeer Sep 12, 2024 •

edited

Loading