From d03b7bb1e024dcbf68b523686751d3a2025ed03c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 24 Feb 2019 08:45:00 -0800 Subject: [PATCH] Properly handle multiple X-Forwarded-For addresses in runner IP https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24624 extracted the X-Forwarded-For address directly, but this didn't consider the case where multiple proxies are in the chain. To fix this, we use the Rails implementation to filter trusted proxies, as documented by Grape: https://github.com/ruby-grape/grape#remote-ip Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/58103 --- changelogs/unreleased/sh-fix-issue-58103.yml | 5 +++++ lib/api/helpers/runner.rb | 2 +- spec/requests/api/runner_spec.rb | 9 +++++++++ 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-fix-issue-58103.yml diff --git a/changelogs/unreleased/sh-fix-issue-58103.yml b/changelogs/unreleased/sh-fix-issue-58103.yml new file mode 100644 index 00000000000..1599af23fed --- /dev/null +++ b/changelogs/unreleased/sh-fix-issue-58103.yml @@ -0,0 +1,5 @@ +--- +title: Properly handle multiple X-Forwarded-For addresses in runner IP +merge_request: 25511 +author: +type: fixed diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 16df8e830e1..ff73a49d5e8 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -26,7 +26,7 @@ module API end def get_runner_ip - { ip_address: request.env["HTTP_X_FORWARDED_FOR"] || request.ip } + { ip_address: env["action_dispatch.remote_ip"].to_s || request.ip } end def current_runner diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index d7ddd97e8c8..91981f7c56a 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -526,6 +526,15 @@ describe API::Runner, :clean_gitlab_redis_shared_state do expect(runner.reload.ip_address).to eq('123.222.123.222') end + it "handles multiple X-Forwarded-For addresses" do + post api('/jobs/request'), + params: { token: runner.token }, + headers: { 'User-Agent' => user_agent, 'X-Forwarded-For' => '123.222.123.222, 127.0.0.1' } + + expect(response).to have_gitlab_http_status 201 + expect(runner.reload.ip_address).to eq('123.222.123.222') + end + context 'when concurrently updating a job' do before do expect_any_instance_of(Ci::Build).to receive(:run!)