Set correct value of X-Forwarded-For header in PreAuthorize request

Closes #82 (closed)

This is my first "real" Go code . Shamelessly copied from https://github.com/golang/go/blob/5bfba30d3325d87ef89dd877f05e5d1e2d618bc3/src/net/http/httputil/reverseproxy.go#L193.

If you are curious why strings.Join(prior, ", ") is necessary:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

This probably needs some tests.

/cc @nick.thomas

mentioned in issue #82 (closed)

OK, I found the same snippet of code in internal/terminal/terminal.go .

Hmm if we have this in two place we should maybe reuse it :)

Something like helper.SetForwardedFor(newRequest, originalRequest *http.Request)? Just throw it in the internal/helper package.

assigned to @adamniedzielski

added 1 commit

• 4e88135b - Set correct value of X-Forwarded-For header in PreAuthorize request

Compare with previous version

assigned to @jacobvosmaer-gitlab

assigned to @adamniedzielski

Now that I look at it again, I think the two tests can be combined in one single table-driven test.

testCases := []struct{
remoteAddr string
header *http.Header
expected string
}{
{"8.8.8.8:3000", nil, "8.8.8.8"},
// etc.
}
for tc := range testCases {
  headers := http.Header{}
  originalRequest := http.Request{RemoteAddr: tc.remoteAddr}
  // etc.
}

added 1 commit

• 896251b1 - Set correct value of X-Forwarded-For header in PreAuthorize request

Compare with previous version

resolved all discussions

@jacobvosmaer-gitlab Thanks for the review. I followed your suggestion and added a third test case, because it was easier .

assigned to @jacobvosmaer-gitlab

Test table looks great, do have another question for you. :)

assigned to @adamniedzielski

@jacobvosmaer-gitlab The second test case checks a single X-Forwarded-For header with two IPs separated by comma ("138.124.33.63, 151.146.211.237"). The third test case checks two X-Forwarded-For headers with single IP in each ("8.154.76.107", "115.206.118.179"). It's because:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list

Visually the difference is small, but I think that it's nice to also check for this case.

Thanks, yes those are two different things and worth checking.

resolved all discussions

LGTM

assigned to @nick.thomas

mentioned in commit 9b0d5297

merged

Great job, nothing to add from me :)