fix/internal/requestclient: read all instances of x-forwarded-for header, not just the first

[ggilmore]

Closes https://linear.app/sourcegraph/issue/SRC-454/extract-and-propagate-user-ip-address-throughout-the-request-lifecycle

According to HTTP1.1/RFC 2616: Headers may be repeated, and any comma-separated list-headers (like X-Forwarded-For) should be treated as a single value.

In section 4.2:

Multiple message-header fields with the same field-name MAY bepresent 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.

For Example:

For the following HTTP request, it's valid to have multiple instances of x-forwarded-for:

Header Name	Header Value
X-Forwarded-For	203.0.113.195, 70.41.3.18
X-Forwarded-For	150.172.238.178
X-Forwarded-For	123.45.67.89
...	...

That must be interpret-able as X-Forwarded-For: 203.0.113.195, 70.41.3.18, 150.172.238.178, 123.45.67.89

Previously, our code used http.Header.Get():

https://github.com/sourcegraph/sourcegraph/blob/9e26623d90461bf7df2e0f3e35561fecc0c35f3f/internal/requestclient/http.go#L81-L95

However, func (Header) Get only returns the first value of the header:

Get gets the first value associated with the given key. If there are no values associated with the key, Get returns "". ...

In our example, this means that our code would only get X-Forwarded-For: 203.0.113.195, 70.41.3.18, which is invalid according to RFC 2616.

Test plan

There were no unit tests, so I added some.

Changelog

[ggilmore]

• fix/internal/requestclient: read all instances of x-forwarded-for header, not just the first #64137 👈
• feat/worker/permission syncing: make sub repo permissions re-insertion fall back to original paths if ips not added yet #64086
• feature/worker/permission syncer: perforce: sync HOST field using IP addresses #64010
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @ggilmore and the rest of your teammates on Graphite

[ggilmore]

I got rid of external boolean here since we only ever took action if both it and useCloudflareHeaders were true. We simplify the logic here by just moving this logic to the Internal/External middleware constructors.

[eseliger]

I assume CF prevents anyone from adding this header, or it makes sure that the header they set is always first?

[ggilmore]

https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#x-forwarded-for

I haven't tested this personally, but the way I interpret their docs is that they assume responsibility for preventing anyone from being able to set this header:

CF-Connecting-IP provides the client IP address connecting to Cloudflare to the origin web server. This header will only be sent on the traffic from Cloudflare’s edge to your origin web server.

@eseliger

[eseliger]

thanks for tests!

[ggilmore]

Merge activity

• Jul 30, 10:40 AM EDT: @ggilmore started a stack merge that includes this pull request via Graphite.
• Jul 30, 11:10 AM EDT: Graphite rebased this pull request as part of a merge.
• Jul 30, 11:15 AM EDT: Graphite couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'buildkite/sourcegraph', 'check').

[github-actions]

Caution

License checking failed, please read: how to deal with third parties licensing.