Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RealIP sets r.RemoteAddr without port #453

Open
Dirbaio opened this issue Sep 30, 2019 · 7 comments · May be fixed by #518
Open

RealIP sets r.RemoteAddr without port #453

Dirbaio opened this issue Sep 30, 2019 · 7 comments · May be fixed by #518
Labels
v6

Comments

@Dirbaio
Copy link

Dirbaio commented Sep 30, 2019 •
edited
Loading

The Go documentation states the following on http.Request.RemoteAddr:

The HTTP server in this package sets RemoteAddr to an "IP:port" address before invoking a handler.

Therefore, you could expect that a code like this is correct:

package main

import (
	"fmt"
	"net"
	"net/http"

	"github.com/go-chi/chi"
	"github.com/go-chi/chi/middleware"
)

func main() {
	r := chi.NewMux()
	r.Use(middleware.RealIP)
	r.Get("/test", func(rw http.ResponseWriter, r *http.Request) {
		host, port, err := net.SplitHostPort(r.RemoteAddr)
		if err != nil {
			fmt.Fprintf(rw, "Error: %v\n", err)
		} else {
			fmt.Fprintf(rw, "Host: %s\nPort: %s\n", host, port)
		}
	})
	http.ListenAndServe(":8000", r)
}

However, the RealIP middleware is just copying the X-Forwarded-For value into r.RemoteAddr, which usually does not contain a port, making the code fail:

[dirbaio@mars]$ curl localhost:8000/test
Host: ::1
Port: 40188
[dirbaio@mars]$ curl localhost:8000/test -H 'X-Forwarded-For: 1.2.3.4'
Error: address 1.2.3.4: missing port in address
[dirbaio@mars]$ curl localhost:8000/test -H 'X-Forwarded-For: ::1'
Error: address ::1: too many colons in address

Perhaps RealIP should try to parse X-Forwarded-For for a host:port, and if it isn't, add a port? Maybe 0 to denote the port is unknown, like 1.2.3.4 -> 1.2.3.4:0?

This is particularly frustrating with IPv6 addresses because they have colons but they're not the host:port colon, making it hard to parse the RemoteAddr in user code.
@rof20004
Copy link

rof20004 commented Nov 24, 2019 •
edited
Loading

@Dirbaio

From Go pkg net: https://golang.org/pkg/net/#SplitHostPort

func SplitHostPort(hostport string) (host, port string, err error)

SplitHostPort splits a network address of the form "host:port", "host%zone:port", "[host]:port" or "[host%zone]:port" into host or host%zone and port.

A literal IPv6 address in hostport must be enclosed in square brackets, as in "[::1]:80", "[::1%lo0]:80".

A literal IPv6 address in hostport must be enclosed in square brackets, as in "[::1]:80", "[::1%lo0]:80

Your IPv6 need to follow these recomendations.

@moorereason
Copy link

@Dirbaio is correct. The RealIP middleware is breaking the rules here. Three options:

  1. Stop using the RealIP middleware since it's fairly trivial.
  2. Fork a local copy.
  3. Change the RealIP API to accept a port (see Hearbeat middleware as an example).

Maybe changing the RealIP API is just what we need to force a v5.0.0 (#462). 😄

@Dirbaio
Copy link
Author

Dirbaio commented Dec 22, 2019

I'm using this in my code in place of Chi's RealIP. It sets the port to 0. Not pretty but at least the code consuming RemoteAddr doesn't need hacks to accept both ip and ip:port values.

func RealIP(h http.Handler) http.Handler {
	fn := func(w http.ResponseWriter, r *http.Request) {
		if rip := realIP(r); rip != "" {
			r.RemoteAddr = net.JoinHostPort(rip, "0")
		}
		h.ServeHTTP(w, r)
	}

	return http.HandlerFunc(fn)
}

This would be a breaking change, of course.

@pkieltyka
Copy link
Member

this is an interesting point. any thoughts on the ideal solution? r.RemoteAddr = net.JoinHostPort(rip, "0") might work, but it provides a confusing port, but at least would be valid to parse

This was referenced Jun 11, 2020
Closed
Open
@lrstanley
Copy link

Maybe you could take the port from the existing r.RemoteAddr, and essentially just replace the IP?

@pkieltyka
Copy link
Member

alternatively, RealIP can change so it doesn't override r.RemoteAddr at all, and instead it will set a "RealIP" on the request context, I think that would be cleaner

@jawnsy jawnsy mentioned this issue Feb 21, 2022
Open
@VojtechVitek VojtechVitek mentioned this issue Dec 15, 2024
Open
@VojtechVitek
Copy link
Contributor

Please, have a look at this proposal: #967

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
v6
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add port number to RealIP middleware
6 participants
@pkieltyka @moorereason @VojtechVitek @rof20004 @Dirbaio @lrstanley