Skip to content

fix(proxy): Resolve issue when proxying 204 #4045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

ErreMalote
Copy link

when proxying 204 the body is null since it has no content. this makes Hono interpret the response as a 200. However, it is a 204.

this is something I found while using Hono.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

when proxying 204 the body is null since it has no content. this makes Hono interpret the response as a 200. However, it is a 204.
@yusukebe yusukebe changed the title Resolve issue when proxying 204 fix(proxy): Resolve issue when proxying 204 Apr 3, 2025
@yusukebe
Copy link
Member

yusukebe commented Apr 7, 2025

Hi @ErreMalote

If the origin server returns a null body and sets the status code to the 204 property, the Proxy Helper returns the correct 204 response. So, the following test will pass. I think it's okay.

diff --git a/src/helper/proxy/index.test.ts b/src/helper/proxy/index.test.ts
index 7aeb1767..a3d195cd 100644
--- a/src/helper/proxy/index.test.ts
+++ b/src/helper/proxy/index.test.ts
@@ -58,6 +58,12 @@ describe('Proxy Middleware', () => {
               },
             })
           )
+        } else if (req.url === 'https://example.com/no-content') {
+          return Promise.resolve(
+            new Response(null, {
+              status: 204,
+            })
+          )
         }
         return Promise.resolve(new Response('not found', { status: 404 }))
       })
@@ -213,6 +219,14 @@ describe('Proxy Middleware', () => {
       expect(req.headers.get('Authorization')).toBeNull()
     })

+    it('no content', async () => {
+      const app = new Hono()
+      app.get('/proxy/:path', (c) => proxy(`https://example.com/${c.req.param('path')}`))
+      const res = await app.request('/proxy/no-content')
+      expect(res.status).toBe(204)
+      expect(res.body).toBe(null)
+    })
+
     it('client disconnect', async () => {
       const app = new Hono()
       const controller = new AbortController()

Or do you have any reason for adding this fix?

@ErreMalote
Copy link
Author

ErreMalote commented Apr 7, 2025

Hi @yusukebe

yea, I noticed that 204 calls in my server were being CORS blocked so I had to add this and that solved the issue.

uniquePaths.forEach((path) => {
	app.all(`${path}/*`, async (c) => {
		// Log all request parameters
		const queryParams = c.req.query();
		const queryString = new URLSearchParams(queryParams).toString();
		const fullPath = queryString
			? `${BACKEND_CLOUD}${c.req.path}?${queryString}`
			: `${BACKEND_CLOUD}${c.req.path}`;

		console.log(`Proxying ${c.req.method} request to ${fullPath}`);
		const response = await proxy(fullPath, {
			...c.req, // optional, specify only when forwarding all the request data (including credentials) is necessary.
			headers: {
				...c.req.header(),
				host: BACKEND_CLOUD?.substring(8),
			},
		});

		// Handle 204 responses explicitly
		if (response.status === 204) {
			return c.body(
				null, // No content
				204, // HTTP status code
				{
					...Object.fromEntries(response.headers.entries()), // Preserve original headers
				},
			);
		}

		// For other responses, return as-is
		return response;
	});
});

I didn't really tested the fix in Hono. However, it is needed. maybe not this middleware, but something is defaulting 204 to 200s.

for reference my backend is python's Fast API

@yusukebe
Copy link
Member

yusukebe commented Apr 8, 2025

@ErreMalote

Is the content in the response from the origin really blank? We can't review this PR without confirming that.

@ErreMalote
Copy link
Author

@ErreMalote

Is the content in the response from the origin really blank? We can't review this PR without confirming that.

@yusukebe I did more digging and Fast API was returning an empty stream object... so there is no need to have this PR.

Sorry for the confusion.

@ErreMalote ErreMalote closed this Apr 14, 2025
@yusukebe
Copy link
Member

@ErreMalote

No problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants