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.

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

[BUG] X-Forwarded-For parsing is incorrect #326

Closed
andreitoupinets opened this issue Apr 13, 2023 · 4 comments · Fixed by #327
Closed

[BUG] X-Forwarded-For parsing is incorrect #326

andreitoupinets opened this issue Apr 13, 2023 · 4 comments · Fixed by #327
Assignees
Labels

Comments

@andreitoupinets
Copy link

Describe the bug
Chproxy use ", " (comma with space) as a separator for elements in header. In result, when chproxy receives request from GCP HTTP load balancer, it ignores header at all, because GCP LB use only comma symbol (space is optional by RFC7239).

To Reproduce
curl -i -H "x-forwarded-for: 1.1.1.1,2.2.2.2" asdf@127.0.0.1:8123/?query=select%201

Expected behavior
Removing of space fix this issue:

+++ b/proxy_handler.go
@@ -74,7 +74,7 @@ func extractFirstMatchFromIPList(ipList string) string {
 	if ipList == "" {
 		return ""
 	}
-	s := strings.Index(ipList, ", ")
+	s := strings.Index(ipList, ",")
 	if s == -1 {
 		s = len(ipList)
 	}
@@ -91,7 +91,7 @@ func parseForwardedHeader(fwd string) string {
 	for _, split := range splits {
 		trimmed := strings.TrimSpace(split)
 		if strings.HasPrefix(strings.ToLower(trimmed), "for=") {
-			forSplits := strings.Split(trimmed, ", ")
+			forSplits := strings.Split(trimmed, ",")
 			if len(forSplits) == 0 {
 				return ""
 			}
@mga-chka
Copy link
Collaborator

Hi, thanks for the issue. We will check if your proposal doesn't create new issue then make a fix.

@Blokje5
Copy link
Collaborator

Blokje5 commented Apr 18, 2023

I should be able to look into this this week, I will pick it up.

@Blokje5 Blokje5 self-assigned this Apr 18, 2023
@mga-chka
Copy link
Collaborator

mga-chka commented Apr 18, 2023

@Blokje5 my 2 cents since I started to look.
It seems to be a missing case when we implemented the feature since in the RFC 7239, this case case happen

   identifiers, and the list may be split over multiple header fields.
   As an example, the header field

       Forwarded: for=192.0.2.43,for="[2001:db8:cafe::17]",for=unknown

   is equivalent to the header field

       Forwarded: for=192.0.2.43, for="[2001:db8:cafe::17]", for=unknown

I quickly looked at the code and the fix doesn't seem dangerous. I let you have a second look then apply the fix (or not).

@mga-chka mga-chka added the bug label Apr 19, 2023
Blokje5 added a commit to Blokje5/chproxy that referenced this issue Apr 20, 2023
Having spaces or no spaces results in equivalent headers in RFC 7239:
   As an example, the header field

       Forwarded: for=192.0.2.43,for="[2001:db8:cafe::17]",for=unknown

   is equivalent to the header field

       Forwarded: for=192.0.2.43, for="[2001:db8:cafe::17]", for=unknown

This PR fixes ContentSquare#326 because we treated spaces as mandatory in the Forwarded Header.

Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
Blokje5 added a commit that referenced this issue Apr 20, 2023
Having spaces or no spaces results in equivalent headers in RFC 7239:
   As an example, the header field

       Forwarded: for=192.0.2.43,for="[2001:db8:cafe::17]",for=unknown

   is equivalent to the header field

       Forwarded: for=192.0.2.43, for="[2001:db8:cafe::17]", for=unknown

This PR fixes #326 because we treated spaces as mandatory in the Forwarded Header.

Signed-off-by: Lennard Eijsackers <lennardeijsackers92@gmail.com>
@mga-chka
Copy link
Collaborator

FYI the version containing the fix should be released btw 1 or 3 weeks

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

Successfully merging a pull request may close this issue.

3 participants