feat(request): support x-forwarded-proto header for protocol detection

[yusukebe]

Fixes #146 (comment)

[yusukebe]

Hi @usualoma !

I think the comment #146 (comment) makes sense. It has to look up the x-forwarded-proto header. What do you think of it? If this is okay, can you review this PR?

[usualoma]

Hi @yusukebe, thank you for creating the pull request.

As mentioned in #146 (comment), I believe that caution should be exercised when referencing x-forwarded-proto by default at the server layer.

koa

In koa, x-forwarded-proto is only referenced when the user explicitly specifies the proxy option.

https://github.com/koajs/koa/blob/cb22d8dcdeb7c0e4e1bdee5eaa5be183ef23a1f2/lib/request.js#L388-L405

https://koajs.com/

Plack::Middleware::ReverseProxy

Plack has dedicated middleware for performing such processing. This kind of processing is not only necessary for Node.js, but also for bun and deno in some cases, so we may need a middleware called reverseProxy.

My opinion on this pull request

I don't believe it's a good idea to check x-forwarded-proto by default, but I think it's helpful to have an opt-in option like koa to enable this behavior in this PR.

[yusukebe]

Hi @usualoma

Thank you for the understandable explanation! As you mentioned, I reconsider that we should handle x-forwarded-proto at the Hono application layer.

But I also think it's hard for users if they have to write the logic by themselves, like this #146 (comment)

So, is it a good idea to implement middleware like Plack::Middleware::ReverseProxy? (For Hono, I sometimes confuse whether the Hono app is a reverse proxy or backed for a reverse proxy since I often use the Hono app as a reverse proxy on Cloudflare/CDN, so the naming should be considered.) If so, it can support x-forwarded-for, x-forwarded-host, etc, not only x-forwarded-proto. On the other side, it may not be good to rewrite the c.req.url at the middleware. What do you think of it?

[yusukebe]

Ah, but there is room for discussion as to whether this should be done in Hono's application layer.

It may be good that the URL in Request should be set to reflect headers such as x-forwarded-proto before Hono's fetch. In this case, the Node.js server should do it. In the case of Deno or Bun, they should do it.

[usualoma]

I also think there is room for consideration as to whether standard middleware is appropriate.
I believe the following points should be specified in order to determine the appropriate design.

• Should c.req.raw.url return the value before or after x-forwarded-* is reflected?
• Which value is passed to getPath?
  • https://github.com/honojs/hono/blob/92846abd1bcd44471f17135d60495190eaf069c8/src/hono-base.ts#L405-L406

[usualoma]

If we solve this on the application side, I think the following is one way to do it.
honojs/hono#4336

[yusukebe]

@usualoma

Ah, creating Reverse Proxy Helper is interesting! I'll consider it. Thanks.

[yusukebe]

Adding getRequest options honojs/hono#4336 is super cool. I think it's the best API to resolve the problem in "100% Hono framework-side".

There is another way to handle the reverse proxy-specific headers. You can write it before Hono's fetch like this:

// Cloudflare Workers / Deno / Bun
import { Hono } from 'hono'

const app = new Hono()

//...

export default {
  fetch: (req: Request) => {
    const url = new URL(req.url)
    url.protocol = req.headers.get('x-forwarded-proto') ?? url.protocol
    return app.fetch(new Request(url, req))
  }
}

// Node.js
import { Hono } from 'hono'
import { serve } from '@hono/node-server'

const app = new Hono()

serve({
  fetch: (req) => {
    const url = new URL(req.url)
    url.protocol = req.headers.get('x-forwarded-proto') ?? url.protocol
    return app.fetch(new Request(url, req))
  }
})

Or, it may be too much, but we can make a helper:

import { Hono } from 'hono'
import { handleRequest } from 'hono/reverse-proxy'

const app = new Hono()

export default {
  fetch: (req: Request) => {
    return app.fetch(handleRequest(req))
  }
}

In the getPath option case, it should be inside hono-base.ts, but for getRequest, it is not 100% needed in hono-base.ts.

So, honojs/hono#4336 is a great idea, but we may not accept it and we may not be in a hurry.

[usualoma]

That's true, I agree with that opinion.

In the getPath option case, it should be inside hono-base.ts, but for getRequest, it is not 100% needed in hono-base.ts.

[pthieu]

Does this preserve other parts of the request?
I tried the middleware and it broke the vite dev server middleware (i'm using react-router v7):

if (IS_DEVELOPMENT) {
  console.log('Starting development server');
  const viteDevServer = await import('vite').then((vite) =>
    vite.createServer({
      server: { middlewareMode: true },
    }),
  );

  app.use('*', async (c, next) => {
    return new Promise((resolve, reject) => {
      const incoming = /** @type {any} */ (c.env).incoming;
      const outgoing = /** @type {any} */ (c.env).outgoing;
      viteDevServer.middlewares(
        incoming,
        outgoing,
        /** @param {any} err */ (err) => {
          if (err) {
            reject(err);
          } else {
            next().then(resolve).catch(reject);
          }
        },
      );
    });
  });

  app.use('*', async (c, next) => {
    try {
      const source = await viteDevServer.ssrLoadModule('./src/server.ts');
      const response = await source.app.fetch(c.req.raw, c.env);
      return response;
    } catch (error) {
      if (typeof error === 'object' && error instanceof Error) {
        viteDevServer.ssrFixStacktrace(error);
      }
      return next();
    }
  });
} else {
  console.log('Starting production server');
  const { serveStatic } = await import('@hono/node-server/serve-static');

  app.use(
    '/assets/*',
    serveStatic({
      root: './build/client',
      rewriteRequestPath: (path) => path,
    }),
  );
  app.use('*', serveStatic({ root: './build/client' }));

  const BUILD_PATH = './build/server/index.js';
  const { app: serverApp } = await import(BUILD_PATH);
  app.route('/', serverApp);
}

serve(
  {
    fetch: (req) => {
      const forwardedProto = req.headers.get('x-forwarded-proto');

      if (forwardedProto) {   
        console.log('Forwarded proto', forwardedProto);
        const url = new URL(req.url);
        url.protocol = forwardedProto;
        return app.fetch(new Request(url, req));
      }
      
      return app.fetch(req);
    },
    port: PORT,
  },
  () => {
    console.log(`Server is running on http://localhost:${PORT}`);
  },
);

but get this error:

Server is running on http://localhost:9000
TypeError: Cannot read properties of undefined (reading 'incoming')
    at file:///Users/pthieu/www/lab46/p-stack/index.js:28:51
    at new Promise (<anonymous>)
    at file:///Users/pthieu/www/lab46/p-stack/index.js:27:12

Seems like just doing the return app.fetch(app) will break it.

---

UPDATE:

took a look at the fetch param and had to pull in the env:

fetch: (req, env) => {
  const forwardedProto = req.headers.get('x-forwarded-proto');

  if (forwardedProto) {
    const url = new URL(req.url);
    url.protocol = forwardedProto === 'https' ? 'https:' : 'http:';
    return app.fetch(new Request(url, req), env);
  }

  return app.fetch(req, env);
},