Skip to content

extractIP may return empty IP if RemoteAddr has no port ( SplitHostPort fallback suggestion) #2757

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

Open
convto opened this issue Mar 21, 2025 · 3 comments

Comments

@convto
Copy link

convto commented Mar 21, 2025

The extractIP function currently uses net.SplitHostPort to parse http.Request.RemoteAddr and extract the IP address.

This works well when RemoteAddr is in the form "host:port", but SplitHostPort returns an error (and an empty host) if the port is missing — which is an intentional design choice in Go. In such cases, the extractIP function ends up returning an empty string.

Relevant code:
https://github.com/labstack/echo/blob/master/ip.go#L221-L224
Introduced in:
124825e

This behavior can lead to issues in environments where RemoteAddr does not include a port such as "192.0.2.10". In such cases, extractIP returns an empty string, which causes functions like RealIP() or ExtractIPFromXForwardedFor() to behave unexpectedly (e.g. returning an empty IP or skipping IP trust checks).

Suggested improvement:

Instead of returning an empty string when SplitHostPort fails, we propose falling back to the original RemoteAddr value — possibly with a simple validation using net.ParseIP.

func extractIP(req *http.Request) string {
    host, _, err := net.SplitHostPort(req.RemoteAddr)
    if err != nil {
        if net.ParseIP(req.RemoteAddr) != nil {
            return req.RemoteAddr
        }
        return ""
    }
    return host
}

This approach improves robustness when RemoteAddr lacks a port. Alternatively, using a regular expression to extract the IP part may also work, but parsing it with net.ParseIP is likely sufficient.

Let me know if this makes sense — happy to submit a PR if it would be helpful.

@convto convto changed the title extractIP may return empty IP if RemoteAddr has no port (SplitHostPort fallback suggestion) extractIP may return empty IP if RemoteAddr has no port ( SplitHostPort fallback suggestion) Mar 21, 2025
@Merlinux-source
Copy link

I have a issue with the ip extractor that looks verry much like this issue. However it behaves differently on ARM vs on X86_64
I solved this by using a custom function for X86_64 binaries:

func getIpFromContext(c echo.Context) net.IP {
	contextIp := c.RealIP()
	if strings.Contains(contextIp, "[") {
		endIndex := strings.Index(contextIp, "]:")
		return net.ParseIP(contextIp[1:endIndex])
	}
	if strings.Contains(contextIp, ":") {
		endIndex := strings.Index(contextIp, ":")
		return net.ParseIP(contextIp[:endIndex])
	}
	return net.ParseIP(contextIp)
}

This is used in production like this:

	var requesterIpAddress string
	frameworkExtractedIP := c.RealIP()
	customIpExtractor := getIpFromContext(c).String()

	// this is required due to differences with X86_64 and ARM implementations of the echo frameworks ip extractor.
	if ip := net.ParseIP(frameworkExtractedIP); ip != nil {
		requesterIpAddress = ip.String()
	}

	// this is required due to differences with X86_64 and ARM implementations of the echo frameworks ip extractor.
	if ip := net.ParseIP(customIpExtractor); ip != nil {
		requesterIpAddress = ip.String()
	}

To be honest this was a dirty work around but it works and it was only an issue as I tried to support ARM

@aldas
Copy link
Contributor

aldas commented Mar 24, 2025

Echo does not differentiate any CPU architecture from each other. Echo is built on Go standard library and if there are differences - it has to originate from there.


about

This works well when RemoteAddr is in the form "host:port", but SplitHostPort returns an error (and an empty host) if the port is missing — which is an intentional design choice in Go. In such cases, the extractIP function ends up returning an empty string.

http.Request.RemoteAddr documentation says it is set to IP:port

	// RemoteAddr allows HTTP servers and other software to record
	// the network address that sent the request, usually for
	// logging. This field is not filled in by ReadRequest and
	// has no defined format. The HTTP server in this package
	// sets RemoteAddr to an "IP:port" address before invoking a
	// handler.
	// This field is ignored by the HTTP client.
	RemoteAddr string

In which situation/environment/OS RemoteAddr is not in that format? We can not just start to make workarounds for problems what I am having trouble to reproduce.

I am testing with this trivial example:

package main

import (
	"fmt"
	"github.com/labstack/echo/v4"
	"log/slog"
	"net/http"
)

func main() {
	e := echo.New()

	//_, trusted, _ := net.ParseCIDR("127.0.0.1/32")
	//e.IPExtractor = echo.ExtractIPFromRealIPHeader(
	//	echo.TrustIPRange(trusted),
	//)

	e.GET("/", func(c echo.Context) error {
		return c.String(http.StatusOK, fmt.Sprintf("RemoteAddr: %s\n", c.Request().RemoteAddr))
	})

	if err := e.Start(":8080"); err != nil {
		slog.Error("start server error: %v", err)
	}
}

@Merlinux-source
Copy link

I'm sorry for jumping to conclusions to quickly, the issue I'm experiencing may not be related to this issue at all...
I'm trying to reproduce my issue and managed to do it once so far, but after trying to make the setup easier to reproduce, the issue didn't occur again.
I used a docker from scratch container, with SSL certificates and users copied from the go build container based on alpine.
I used Caddy as a reverse proxy and enabled ipv6 support for docker. Then under some conditions c.RealIp() returns host with port. I'm still trying to debug this further, but debugging inside of a docker container is not that easy for me. if this seems related or relevant to you, you can see my progress here

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

No branches or pull requests

3 participants