From 53e33770c2f82e0b6ebf07ff53fa705e0420aa04 Mon Sep 17 00:00:00 2001 From: Pavel Balashou Date: Tue, 21 Apr 2026 09:06:02 +0200 Subject: [PATCH] Use ssrf filtering in Jira Import. --- app/services/import/jira_client.rb | 94 ++++++++++--------- .../import/jira_import_projects_job.rb | 24 +++-- lib/open_project/ssrf_protection.rb | 50 +++++++++- .../outgoing/request_webhook_service_spec.rb | 2 +- .../workers/attachment_webhook_job_spec.rb | 2 +- .../spec/workers/project_webhook_job_spec.rb | 2 +- .../workers/time_entry_webhook_job_spec.rb | 2 +- .../work_package_comment_webhook_job_spec.rb | 2 +- .../workers/work_package_webhook_job_spec.rb | 2 +- spec/services/import/jira_client_spec.rb | 78 +++++++++++++++ ...rf_webhook_stubs.rb => with_ssrf_stubs.rb} | 8 +- .../import/jira_import_projects_job_spec.rb | 2 + 12 files changed, 200 insertions(+), 68 deletions(-) create mode 100644 spec/services/import/jira_client_spec.rb rename spec/support/shared/{with_ssrf_webhook_stubs.rb => with_ssrf_stubs.rb} (91%) diff --git a/app/services/import/jira_client.rb b/app/services/import/jira_client.rb index a37da18f54f..540b11b2e1f 100644 --- a/app/services/import/jira_client.rb +++ b/app/services/import/jira_client.rb @@ -46,23 +46,19 @@ module Import end end + HTTP_OPTIONS = { + open_timeout: 30, + read_timeout: 30 + }.freeze + def initialize(url:, personal_access_token:) raise ApiError.new(I18n.t(:"admin.jira.test.token_error")) if personal_access_token.nil? - @httpx = OpenProject - .httpx - .plugin(:auth) - .bearer_auth(personal_access_token) - .with( - headers: { "accept" => "application/json" }, - timeout: { - connect_timeout: 30, - operation_timeout: 30, - request_timeout: 30, - read_timeout: 30 - } - ) @url = url.chomp("/") + @headers = { + "Accept" => "application/json", + "Authorization" => "Bearer #{personal_access_token}" + } end def mypermissions @@ -124,7 +120,7 @@ module Import def issue_types_count response = get_response("/rest/api/2/issuetype/page", params: { maxResults: 0 }) - if response.status == 200 + if response.is_a?(Net::HTTPSuccess) parse_json(response)["total"] else issue_types.count @@ -149,7 +145,7 @@ module Import def statuses_count response = get_response("/rest/api/2/status/search", params: { maxResults: 0 }) - if response.status == 200 + if response.is_a?(Net::HTTPSuccess) parse_json(response)["total"] else statuses.count @@ -223,24 +219,34 @@ module Import }) end - def download_attachment(content_url) - case (response = @httpx.get(content_url)) - in { status: 200..299 } - response.body - in { status: 300..399 } - case (redirect_response = @httpx.get(response.headers["location"])) - in { status: 200..299 } - redirect_response.body - in { status: 300.. } - raise "BAD RESPONSE: #{redirect_response.status}, #{redirect_response.body}" - in { error: error } - raise error + ## + # Downloads a file from the given URL and saves it to a temporary file. + # + # @param content_url [String] The URL to download the attachment from + # @param filename [String] The name to use for the temporary file + # @return [Tempfile] The temporary file containing the downloaded content + # @raise [ConnectionError] If SSRF protection blocks the request or connection fails + # @raise [ApiError] If the server returns a non-success response + # @note The caller is responsible for removing the temporary file after use, + # for example with +File.unlink+ + def download_attachment(content_url, filename) # rubocop:disable Metrics/AbcSize + tempfile = nil + OpenProject::SsrfProtection.get(content_url, headers: @headers, http_options: HTTP_OPTIONS, max_redirects: 1) do |response| + case response + when Net::HTTPSuccess + tempfile = Tempfile.create(filename, binmode: true) + response.read_body do |chunk| + tempfile.write chunk + end + else + raise ApiError.new(I18n.t("admin.jira.client.api_error"), status: response.code.to_i, response_body: response.body) end - in { status: 400.. } - raise "BAD RESPONSE: #{response}" - in { error: error } - raise error end + tempfile + rescue SsrfFilter::Error => e + raise ConnectionError, I18n.t("admin.jira.client.connection_error", message: e.message) + rescue Timeout::Error => e + raise ConnectionError, I18n.t("admin.jira.client.connection_timeout", message: e.message) end private @@ -251,34 +257,34 @@ module Import end def get_response(path, params: {}) - response = @httpx.get("#{@url}#{path}", params:) - - if response.is_a?(HTTPX::ErrorResponse) - raise ConnectionError, I18n.t("admin.jira.client.connection_error", message: response.error.message) - end - - response - rescue HTTPX::ConnectionError, SocketError, Errno::ECONNREFUSED, Errno::EHOSTUNREACH => e + OpenProject::SsrfProtection.get( + "#{@url}#{path}", + headers: @headers, + params:, + http_options: HTTP_OPTIONS + ) + rescue SsrfFilter::Error, SocketError, Errno::ECONNREFUSED, Errno::EHOSTUNREACH => e raise ConnectionError, I18n.t("admin.jira.client.connection_error", message: e.message) rescue Timeout::Error => e raise ConnectionError, I18n.t("admin.jira.client.connection_timeout", message: e.message) end def handle_response(response) - if response.status >= 200 && response.status < 300 + status = response.code.to_i + if response.is_a?(Net::HTTPSuccess) parse_json(response) else raise ApiError.new( - I18n.t("admin.jira.client.#{response.status}_error", status: response.status, default: :"admin.jira.client.api_error"), - status: response.status, + I18n.t("admin.jira.client.#{status}_error", status:, default: :"admin.jira.client.api_error"), + status:, response_body: response.body.to_s ) end end def parse_json(response) - response.json - rescue JSON::ParserError, HTTPX::Error => e + JSON.parse(response.body) + rescue JSON::ParserError => e raise ParseError, I18n.t("admin.jira.client.parse_error", message: e.message) end end diff --git a/app/workers/import/jira_import_projects_job.rb b/app/workers/import/jira_import_projects_job.rb index aea7b64f492..ad407091f0d 100644 --- a/app/workers/import/jira_import_projects_job.rb +++ b/app/workers/import/jira_import_projects_job.rb @@ -286,21 +286,19 @@ module Import content_url = attachment["content"] mime_type = attachment["mimeType"] size = attachment["size"] - response_body = jira_client.download_attachment(content_url) + tempfile = jira_client.download_attachment(content_url, filename) - Tempfile.create(filename, binmode: true) do |tempfile| - response_body.copy_to(tempfile) - tempfile.rewind - tempfile.define_singleton_method(:original_filename) { filename } - tempfile.define_singleton_method(:content_type) { mime_type } - tempfile.define_singleton_method(:size) { size } - call = Attachments::CreateService - .new(user: author, contract_class: EmptyContract) - .call(container: work_package, filename:, file: tempfile) + tempfile.rewind + tempfile.define_singleton_method(:original_filename) { filename } + tempfile.define_singleton_method(:content_type) { mime_type } + tempfile.define_singleton_method(:size) { size } + call = Attachments::CreateService + .new(user: author, contract_class: EmptyContract) + .call(container: work_package, filename:, file: tempfile) - call.on_failure do - raise call.message - end + File.unlink(tempfile) + call.on_failure do + raise call.message end end # rubocop:enable Metrics/AbcSize diff --git a/lib/open_project/ssrf_protection.rb b/lib/open_project/ssrf_protection.rb index c9e786c2129..93d32bc9e0a 100644 --- a/lib/open_project/ssrf_protection.rb +++ b/lib/open_project/ssrf_protection.rb @@ -47,7 +47,7 @@ module OpenProject # @option options [Integer] :max_redirects Maximum number of redirects to follow (default: 10) # @option options [Hash] :http_options Options passed directly to Net::HTTP.start (e.g. read_timeout:, open_timeout:) # @option options [Proc] :resolver Custom DNS resolver; receives a hostname and returns an array of IPAddr objects - # @yield [Net::HTTP::Post] Optional block to further configure the request object before it is sent + # @yield [Net::HTTPResponse] Optional block to handle response object # @return [Net::HTTPResponse] The HTTP response # @raise [SsrfFilter::InvalidUriScheme] If the URI scheme is not in the whitelist # @raise [SsrfFilter::UnresolvedHostname] If the hostname cannot be resolved @@ -78,6 +78,54 @@ module OpenProject super(url, { max_redirects: 0, resolver: resolver }.merge(options), &) end + ## + # Performs an SSRF-safe HTTP GET request to the given URL. + # + # Resolves the hostname and blocks requests to private/reserved IP ranges + # (loopback, link-local, RFC 1918, etc.) to prevent server-side request forgery. + # Use OPENPROJECT_SSRF_PROTECTION_IP_ALLOWLIST to explicitly permit specific private IPs. + # + # @param url [String, URI] The URL to GET from (must use http or https) + # @param options [Hash] Request options + # @option options [Hash] :headers Additional HTTP headers, e.g. { "Authorization" => "Bearer token" } + # @option options [Hash] :params Query parameters to merge into the URL + # @option options [Array] :scheme_whitelist Allowed URI schemes (default: ["http", "https"]) + # @option options [Integer] :max_redirects Maximum number of redirects to follow (default: 10) + # @option options [Hash] :http_options Options passed directly to Net::HTTP.start (e.g. read_timeout:, open_timeout:) + # @option options [Proc] :resolver Custom DNS resolver; receives a hostname and returns an array of IPAddr objects + # @yield [Net::HTTPResponse] Optional block to handle response object + # @return [Net::HTTPResponse] The HTTP response + # @raise [SsrfFilter::InvalidUriScheme] If the URI scheme is not in the whitelist + # @raise [SsrfFilter::UnresolvedHostname] If the hostname cannot be resolved + # @raise [SsrfFilter::PrivateIPAddress] If all resolved IPs are private/blocked + # @raise [SsrfFilter::CRLFInjection] If CRLF characters are detected in headers + # @raise [SsrfFilter::TooManyRedirects] If the redirect limit is exceeded + # + # @example Simple GET with authorization + # response = OpenProject::SsrfProtection.get( + # "https://example.com/api/data", + # headers: { "Authorization" => "Bearer token123" } + # ) + # + # @example GET with custom timeout + # response = OpenProject::SsrfProtection.get( + # "https://example.com/api/resource", + # http_options: { open_timeout: 5, read_timeout: 10 } + # ) + # + # @example GET with streamed response + # response = OpenProject::SsrfProtection.get( + # "https://example.com/api/resource", + # http_options: { open_timeout: 5, read_timeout: 10 } + # ) do |response| + # response.read_body do |chunk| + # print chunk + # end + # end + def get(url, options = {}, &) + super(url, { resolver: resolver }.merge(options), &) + end + ## # Given a hostname or IP address, returns the first one which is safe to use # for triggering a user initiated request. diff --git a/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb b/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb index 1d826303b63..7a1b58c1c90 100644 --- a/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb +++ b/modules/webhooks/spec/services/webhooks/outgoing/request_webhook_service_spec.rb @@ -31,7 +31,7 @@ require "spec_helper" RSpec.describe Webhooks::Outgoing::RequestWebhookService, :webmock, type: :model do - include_context "with ssrf webhook stubs" + include_context "with ssrf stubs" let(:user) { build_stubbed(:user) } let(:instance) { described_class.new(webhook, event_name: :created, current_user: user) } diff --git a/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb b/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb index a0a5dce1ad2..dcb7c0c668c 100644 --- a/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/attachment_webhook_job_spec.rb @@ -29,7 +29,7 @@ require "spec_helper" RSpec.describe AttachmentWebhookJob, :webmock, type: :job do - include_context "with ssrf webhook stubs" + include_context "with ssrf stubs" shared_let(:user) { create(:admin) } shared_let(:request_url) { "http://example.net/test/42" } diff --git a/modules/webhooks/spec/workers/project_webhook_job_spec.rb b/modules/webhooks/spec/workers/project_webhook_job_spec.rb index 6b89510bdc0..47463e657bc 100644 --- a/modules/webhooks/spec/workers/project_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/project_webhook_job_spec.rb @@ -29,7 +29,7 @@ require "spec_helper" RSpec.describe ProjectWebhookJob, :webmock, type: :job do - include_context "with ssrf webhook stubs" + include_context "with ssrf stubs" shared_let(:request_url) { "http://example.net/test/42" } shared_let(:project) { create(:project, name: "Foo Bar") } diff --git a/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb b/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb index 17304244085..7e0b0d90893 100644 --- a/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/time_entry_webhook_job_spec.rb @@ -29,7 +29,7 @@ require "spec_helper" RSpec.describe TimeEntryWebhookJob, :webmock, type: :job do - include_context "with ssrf webhook stubs" + include_context "with ssrf stubs" shared_let(:user) { create(:admin) } shared_let(:request_url) { "http://example.net/test/42" } diff --git a/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb b/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb index 8a97a093c87..c1bcc286e69 100644 --- a/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/work_package_comment_webhook_job_spec.rb @@ -31,7 +31,7 @@ require "spec_helper" RSpec.describe WorkPackageCommentWebhookJob, :webmock, type: :model do - include_context "with ssrf webhook stubs" + include_context "with ssrf stubs" let(:user) { create(:admin) } let(:request_url) { "http://example.net/test/42" } diff --git a/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb b/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb index 90866e42691..903561001d3 100644 --- a/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb +++ b/modules/webhooks/spec/workers/work_package_webhook_job_spec.rb @@ -31,7 +31,7 @@ require "spec_helper" RSpec.describe WorkPackageWebhookJob, :webmock, type: :model do - include_context "with ssrf webhook stubs" + include_context "with ssrf stubs" shared_let(:user) { create(:admin) } shared_let(:title) { "Some workpackage subject" } diff --git a/spec/services/import/jira_client_spec.rb b/spec/services/import/jira_client_spec.rb new file mode 100644 index 00000000000..62d428ff8dc --- /dev/null +++ b/spec/services/import/jira_client_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe Import::JiraClient do + subject(:client) { described_class.new(url:, personal_access_token:) } + + let(:url) { "https://jira.example.com" } + let(:personal_access_token) { "test-token" } + + describe "#initialize" do + context "when personal_access_token is nil" do + let(:personal_access_token) { nil } + + it "raises ApiError" do + expect { client }.to raise_error(Import::JiraClient::ApiError) + end + end + end + + describe "SSRF protection" do + context "when using a loopback address" do + let(:url) { "http://127.0.0.1" } + + it "raises ConnectionError for API requests" do + expect { client.server_info } + .to raise_error(Import::JiraClient::ConnectionError) + end + + it "raises ConnectionError for download_attachment" do + expect { client.download_attachment("#{url}/attachment/123", "filename") } + .to raise_error(Import::JiraClient::ConnectionError) + end + end + + context "when using a private network address (10.x.x.x)" do + let(:url) { "http://10.0.0.1" } + + it "raises ConnectionError for API requests" do + expect { client.projects } + .to raise_error(Import::JiraClient::ConnectionError) + end + + it "raises ConnectionError for download_attachment" do + expect { client.download_attachment("#{url}/attachment/123", "filename") } + .to raise_error(Import::JiraClient::ConnectionError) + end + end + end +end diff --git a/spec/support/shared/with_ssrf_webhook_stubs.rb b/spec/support/shared/with_ssrf_stubs.rb similarity index 91% rename from spec/support/shared/with_ssrf_webhook_stubs.rb rename to spec/support/shared/with_ssrf_stubs.rb index 23bb7590060..3ca8eb74342 100644 --- a/spec/support/shared/with_ssrf_webhook_stubs.rb +++ b/spec/support/shared/with_ssrf_stubs.rb @@ -26,7 +26,7 @@ # # See COPYRIGHT and LICENSE files for more details. -module WithSsrfWebhookStubsMixin +module WithSsrfStubsMixin ## # A safe public IP returned by the stubbed resolver for any hostname. # It is not in SsrfFilter's private-address blocklist, so SSRF validation passes. @@ -39,11 +39,11 @@ module WithSsrfWebhookStubsMixin end end -RSpec.shared_context "with ssrf webhook stubs" do - include WithSsrfWebhookStubsMixin +RSpec.shared_context "with ssrf stubs" do + include WithSsrfStubsMixin before do - safe_ip = IPAddr.new(WithSsrfWebhookStubsMixin::SSRF_TEST_IP) + safe_ip = IPAddr.new(WithSsrfStubsMixin::SSRF_TEST_IP) allow(OpenProject::SsrfProtection).to receive(:resolver).and_return( ->(hostname) { ip_address?(hostname) ? [IPAddr.new(hostname)] : [safe_ip] } ) diff --git a/spec/workers/import/jira_import_projects_job_spec.rb b/spec/workers/import/jira_import_projects_job_spec.rb index 3887c343b54..a19a59d5ede 100644 --- a/spec/workers/import/jira_import_projects_job_spec.rb +++ b/spec/workers/import/jira_import_projects_job_spec.rb @@ -118,6 +118,8 @@ RSpec.describe Import::JiraImportProjectsJob, :webmock do let(:attachment_content) { Rails.root.join("spec/fixtures/files/image.png").binread } + include_context "with ssrf stubs" + before do stub_request(:get, "https://jira-software.local/secure/attachment/10000/solid-color-image.png") .to_return(status: 200, body: attachment_content, headers: { "Content-Type" => "image/png" })