tus/tus-node-server

Respect X-Forwarded-Port and X-Forwarded-Prefix as well

Closed this issue · 2 comments

olee commented

It seems that with respectForwardedHeaders enabled, only X-Forwarded-Host and X-Forwarded-Proto are handled.
However, if the proxy also uses a prefix or differnt port for the TUS server, it will fail because neither X-Forwarded-Prefix nor X-Forwarded-Port are respected here:

if (this.options.respectForwardedHeaders) {
const forwarded = req.headers.forwarded as string | undefined
if (forwarded) {
host ??= reForwardedHost.exec(forwarded)?.[1]
proto ??= reForwardedProto.exec(forwarded)?.[1]
}
const forwardHost = req.headers['x-forwarded-host']
const forwardProto = req.headers['x-forwarded-proto']
// @ts-expect-error we can pass undefined
if (['http', 'https'].includes(forwardProto)) {
proto ??= forwardProto as string
}
host ??= forwardHost
}

I would expect it to handle it more like this:

  protected extractHostAndProto(req: http.IncomingMessage) {
    let proto
    let host

    if (this.options.respectForwardedHeaders) {
      const forwarded = req.headers.forwarded as string | undefined
      if (forwarded) {
        host ??= reForwardedHost.exec(forwarded)?.[1]
        proto ??= reForwardedProto.exec(forwarded)?.[1]
      }

      const forwardHost = req.headers['x-forwarded-host']
      const forwardProto = req.headers['x-forwarded-proto']
      const forwardPort = req.headers['x-forwarded-port']
      const forwardPrefix = req.headers['x-forwarded-prefix']

      // @ts-expect-error we can pass undefined
      if (['http', 'https'].includes(forwardProto)) {
        proto ??= forwardProto as string
      }

      host ??= forwardHost + (forwardPort ? `:${forwardPort}` : '') + (forwardPrefix ? forwardPrefix.slice(1, -1) : '')
    }

    host ??= req.headers.host
    proto ??= 'http'

    return {host: host as string, proto}
  }

Funny thing is, with a Forwarded header it would be possible to pass a path and port as well, but not with the proper standard X-Forwarded headers.

fenos commented

Hi @olee
I think generally makes sense to have the port included if provided in the x-forwarded-port header, however, the x-forwarded-prefix header seems a less canonical header. Maybe some proxies might use it mostly they don't.

For these specific use cases where you need full control on how the generated url is constructed, you can use the generateUrl function on the server, for example:

const server = new Server({
  ...restOptions,
  generateUrl(req, {proto, host, path, id}) {
    const forwardPort = req.headers['x-forwarded-port']
    const forwardPrefix = req.headers['x-forwarded-prefix']
    
   host ??= forwardHost + (forwardPort ? `:${forwardPort}` : '') + (forwardPrefix ? forwardPrefix.slice(1, -1) : '')

    return `${proto}://${host}${path}/${id}`
  }
})

I agree with @fenos, I prefer giving inversion of control so people can do whatever they want over supporting all kinds of internal conditions (of non-standard headers).