From d43e25f79e51eb8574e7f953812735b641f506f6 Mon Sep 17 00:00:00 2001
From: Andy Soiron <asoiron@gitlab.com>
Date: Tue, 29 Nov 2022 10:03:28 +0000
Subject: [PATCH] Remove custom CORS controller for JiraConnect
This goes back to use Rack Cors instead of the custom cors
controller for jira_connect routes.
The customer controller was added to be able to add origins dynamically
based on the jira_connect_proxy_url setting. I learned that this can be
done by passing a block to origins using Rack Cors.
The custom controller also had the problem that the OPTIONS
request was intercepted by Rack Cors and never reached the controller.
Changelog: fixed
---
.../jira_connect/application_controller.rb | 27 --------
.../cors_preflight_checks_controller.rb | 16 -----
.../oauth_application_ids_controller.rb | 1 -
.../jira_connect/subscriptions_controller.rb | 1 -
config/application.rb | 15 +++++
config/routes.rb | 7 +-
.../cors_preflight_checks_controller_spec.rb | 66 -------------------
.../oauth_application_ids_controller_spec.rb | 19 +++++-
.../subscriptions_controller_spec.rb | 36 +++++++++-
9 files changed, 70 insertions(+), 118 deletions(-)
delete mode 100644 app/controllers/jira_connect/cors_preflight_checks_controller.rb
delete mode 100644 spec/requests/jira_connect/cors_preflight_checks_controller_spec.rb
diff --git a/app/controllers/jira_connect/application_controller.rb b/app/controllers/jira_connect/application_controller.rb
index 9e55aace7832..e26d69314cda 100644
--- a/app/controllers/jira_connect/application_controller.rb
+++ b/app/controllers/jira_connect/application_controller.rb
@@ -3,12 +3,6 @@
class JiraConnect::ApplicationController < ApplicationController
include Gitlab::Utils::StrongMemoize
- CORS_ALLOWED_METHODS = {
- '/-/jira_connect/oauth_application_id' => %i[GET OPTIONS],
- '/-/jira_connect/subscriptions.json' => %i[GET OPTIONS],
- '/-/jira_connect/subscriptions/*' => %i[DELETE OPTIONS]
- }.freeze
-
skip_before_action :authenticate_user!
skip_before_action :verify_authenticity_token
before_action :verify_atlassian_jwt!
@@ -66,25 +60,4 @@ def jwt
def auth_token
params[:jwt] || request.headers['Authorization']&.split(' ', 2)&.last
end
-
- def cors_allowed_methods
- CORS_ALLOWED_METHODS[resource]
- end
-
- def resource
- request.path.gsub(%r{/\d+$}, '/*')
- end
-
- def set_cors_headers
- return unless allow_cors_request?
-
- response.set_header('Access-Control-Allow-Origin', Gitlab::CurrentSettings.jira_connect_proxy_url)
- response.set_header('Access-Control-Allow-Methods', cors_allowed_methods.join(', '))
- end
-
- def allow_cors_request?
- return false if cors_allowed_methods.nil?
-
- !Gitlab.com? && Gitlab::CurrentSettings.jira_connect_proxy_url.present?
- end
end
diff --git a/app/controllers/jira_connect/cors_preflight_checks_controller.rb b/app/controllers/jira_connect/cors_preflight_checks_controller.rb
deleted file mode 100644
index 3f30c1e04df4..000000000000
--- a/app/controllers/jira_connect/cors_preflight_checks_controller.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# frozen_string_literal: true
-
-module JiraConnect
- class CorsPreflightChecksController < ApplicationController
- feature_category :integrations
-
- skip_before_action :verify_atlassian_jwt!
- before_action :set_cors_headers
-
- def index
- return render_404 unless allow_cors_request?
-
- render plain: '', content_type: 'text/plain'
- end
- end
-end
diff --git a/app/controllers/jira_connect/oauth_application_ids_controller.rb b/app/controllers/jira_connect/oauth_application_ids_controller.rb
index 48827785e56b..de520337af3f 100644
--- a/app/controllers/jira_connect/oauth_application_ids_controller.rb
+++ b/app/controllers/jira_connect/oauth_application_ids_controller.rb
@@ -5,7 +5,6 @@ class OauthApplicationIdsController < ApplicationController
feature_category :integrations
skip_before_action :verify_atlassian_jwt!
- before_action :set_cors_headers
def show
if show_application_id?
diff --git a/app/controllers/jira_connect/subscriptions_controller.rb b/app/controllers/jira_connect/subscriptions_controller.rb
index da4b25943f73..30cfeb563eb9 100644
--- a/app/controllers/jira_connect/subscriptions_controller.rb
+++ b/app/controllers/jira_connect/subscriptions_controller.rb
@@ -23,7 +23,6 @@ class JiraConnect::SubscriptionsController < JiraConnect::ApplicationController
push_frontend_feature_flag(:jira_connect_oauth_self_managed, @user)
end
- before_action :set_cors_headers
before_action :allow_rendering_in_iframe, only: :index
before_action :verify_qsh_claim!, only: :index
before_action :allow_self_managed_content_security_policy, only: :index
diff --git a/config/application.rb b/config/application.rb
index 249db9c6a67b..56eabc59815c 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -417,6 +417,21 @@ class Application < Rails::Application
expose: headers_to_expose
end
+ allow do
+ origins { |source, env| source == Gitlab::CurrentSettings.jira_connect_proxy_url }
+ resource '/-/jira_connect/oauth_application_id', headers: :any, credentials: false, methods: %i(get options)
+ end
+
+ allow do
+ origins { |source, env| source == Gitlab::CurrentSettings.jira_connect_proxy_url }
+ resource '/-/jira_connect/subscriptions.json', headers: :any, credentials: false, methods: %i(get options)
+ end
+
+ allow do
+ origins { |source, env| source == Gitlab::CurrentSettings.jira_connect_proxy_url }
+ resource '/-/jira_connect/subscriptions/*', headers: :any, credentials: false, methods: %i(delete options)
+ end
+
# Cross-origin requests must be enabled for the Authorization code with PKCE OAuth flow when used from a browser.
%w(/oauth/token /oauth/revoke).each do |oauth_path|
allow do
diff --git a/config/routes.rb b/config/routes.rb
index e218099ec4a2..3482b0566ce8 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -55,10 +55,9 @@
match '/oauth/token' => 'oauth/tokens#create', via: :options
match '/oauth/revoke' => 'oauth/tokens#revoke', via: :options
- match '/-/jira_connect/oauth_application_id' => 'jira_connect/cors_preflight_checks#index', via: :options
- match '/-/jira_connect/subscriptions.json' => 'jira_connect/cors_preflight_checks#index', via: :options
- match '/-/jira_connect/subscriptions/:id' => 'jira_connect/cors_preflight_checks#index', via: :options
- match '/-/jira_connect/installations' => 'jira_connect/cors_preflight_checks#index', via: :options
+ match '/-/jira_connect/oauth_application_id' => 'jira_connect/oauth_application_ids#show', via: :options
+ match '/-/jira_connect/subscriptions(.:format)' => 'jira_connect/subscriptions#index', via: :options
+ match '/-/jira_connect/subscriptions/:id' => 'jira_connect/subscriptions#delete', via: :options
# Sign up
scope path: '/users/sign_up', module: :registrations, as: :users_sign_up do
diff --git a/spec/requests/jira_connect/cors_preflight_checks_controller_spec.rb b/spec/requests/jira_connect/cors_preflight_checks_controller_spec.rb
deleted file mode 100644
index e356aaa37965..000000000000
--- a/spec/requests/jira_connect/cors_preflight_checks_controller_spec.rb
+++ /dev/null
@@ -1,66 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe JiraConnect::CorsPreflightChecksController do
- shared_examples 'allows cross-origin requests on self managed' do
- it 'renders not found' do
- options path
-
- expect(response).to have_gitlab_http_status(:not_found)
- expect(response.headers['Access-Control-Allow-Origin']).to be_nil
- end
-
- context 'with jira_connect_proxy_url setting' do
- before do
- stub_application_setting(jira_connect_proxy_url: 'https://gitlab.com')
-
- options path, headers: { 'Origin' => 'http://notgitlab.com' }
- end
-
- it 'returns 200' do
- expect(response).to have_gitlab_http_status(:ok)
- end
-
- it 'responds with access-control-allow headers', :aggregate_failures do
- expect(response.headers['Access-Control-Allow-Origin']).to eq 'https://gitlab.com'
- expect(response.headers['Access-Control-Allow-Methods']).to eq allowed_methods
- expect(response.headers['Access-Control-Allow-Credentials']).to be_nil
- end
-
- context 'when on GitLab.com' do
- before do
- allow(Gitlab).to receive(:com?).and_return(true)
- end
-
- it 'renders not found' do
- options path
-
- expect(response).to have_gitlab_http_status(:not_found)
- expect(response.headers['Access-Control-Allow-Origin']).to be_nil
- end
- end
- end
- end
-
- describe 'OPTIONS /-/jira_connect/oauth_application_id' do
- let(:allowed_methods) { 'GET, OPTIONS' }
- let(:path) { '/-/jira_connect/oauth_application_id' }
-
- it_behaves_like 'allows cross-origin requests on self managed'
- end
-
- describe 'OPTIONS /-/jira_connect/subscriptions.json' do
- let(:allowed_methods) { 'GET, OPTIONS' }
- let(:path) { '/-/jira_connect/subscriptions.json' }
-
- it_behaves_like 'allows cross-origin requests on self managed'
- end
-
- describe 'OPTIONS /-/jira_connect/subscriptions/:id' do
- let(:allowed_methods) { 'DELETE, OPTIONS' }
- let(:path) { '/-/jira_connect/subscriptions/123' }
-
- it_behaves_like 'allows cross-origin requests on self managed'
- end
-end
diff --git a/spec/requests/jira_connect/oauth_application_ids_controller_spec.rb b/spec/requests/jira_connect/oauth_application_ids_controller_spec.rb
index 4b3467059ab5..ed619bc50a77 100644
--- a/spec/requests/jira_connect/oauth_application_ids_controller_spec.rb
+++ b/spec/requests/jira_connect/oauth_application_ids_controller_spec.rb
@@ -4,7 +4,7 @@
RSpec.describe JiraConnect::OauthApplicationIdsController do
describe 'GET /-/jira_connect/oauth_application_id' do
- let(:cors_request_headers) { { 'Origin' => 'http://notgitlab.com' } }
+ let(:cors_request_headers) { { 'Origin' => 'https://gitlab.com' } }
before do
stub_application_setting(jira_connect_application_key: '123456')
@@ -50,4 +50,21 @@
end
end
end
+
+ describe 'OPTIONS /-/jira_connect/oauth_application_id' do
+ let(:cors_request_headers) { { 'Origin' => 'https://gitlab.com', 'access-control-request-method' => 'GET' } }
+
+ before do
+ stub_application_setting(jira_connect_application_key: '123456')
+ stub_application_setting(jira_connect_proxy_url: 'https://gitlab.com')
+ end
+
+ it 'allows cross-origin requests', :aggregate_failures do
+ options '/-/jira_connect/oauth_application_id', headers: cors_request_headers
+
+ expect(response.headers['Access-Control-Allow-Origin']).to eq 'https://gitlab.com'
+ expect(response.headers['Access-Control-Allow-Methods']).to eq 'GET, OPTIONS'
+ expect(response.headers['Access-Control-Allow-Credentials']).to be_nil
+ end
+ end
end
diff --git a/spec/requests/jira_connect/subscriptions_controller_spec.rb b/spec/requests/jira_connect/subscriptions_controller_spec.rb
index ba195471a12c..73a825d471ae 100644
--- a/spec/requests/jira_connect/subscriptions_controller_spec.rb
+++ b/spec/requests/jira_connect/subscriptions_controller_spec.rb
@@ -10,7 +10,7 @@
end
let(:jwt) { Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret) }
- let(:cors_request_headers) { { 'Origin' => 'http://notgitlab.com' } }
+ let(:cors_request_headers) { { 'Origin' => 'https://gitlab.com' } }
let(:path) { '/-/jira_connect/subscriptions' }
let(:params) { { jwt: jwt } }
@@ -49,6 +49,38 @@
end
end
+ describe 'OPTIONS /-/jira_connect/subscriptions' do
+ let(:cors_request_headers) { { 'Origin' => 'https://gitlab.com', 'access-control-request-method' => 'GET' } }
+
+ before do
+ stub_application_setting(jira_connect_proxy_url: 'https://gitlab.com')
+ end
+
+ it 'allows cross-origin requests', :aggregate_failures do
+ options '/-/jira_connect/subscriptions.json', headers: cors_request_headers
+
+ expect(response.headers['Access-Control-Allow-Origin']).to eq 'https://gitlab.com'
+ expect(response.headers['Access-Control-Allow-Methods']).to eq 'GET, OPTIONS'
+ expect(response.headers['Access-Control-Allow-Credentials']).to be_nil
+ end
+ end
+
+ describe 'OPTIONS /-/jira_connect/subscriptions/:id' do
+ let(:cors_request_headers) { { 'Origin' => 'https://gitlab.com', 'access-control-request-method' => 'DELETE' } }
+
+ before do
+ stub_application_setting(jira_connect_proxy_url: 'https://gitlab.com')
+ end
+
+ it 'allows cross-origin requests', :aggregate_failures do
+ options '/-/jira_connect/subscriptions/1', headers: cors_request_headers
+
+ expect(response.headers['Access-Control-Allow-Origin']).to eq 'https://gitlab.com'
+ expect(response.headers['Access-Control-Allow-Methods']).to eq 'DELETE, OPTIONS'
+ expect(response.headers['Access-Control-Allow-Credentials']).to be_nil
+ end
+ end
+
describe 'DELETE /-/jira_connect/subscriptions/:id' do
let_it_be(:installation) { create(:jira_connect_installation, instance_url: 'http://self-managed-gitlab.com') }
let_it_be(:subscription) { create(:jira_connect_subscription, installation: installation) }
@@ -58,7 +90,7 @@
end
let(:jwt) { Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret) }
- let(:cors_request_headers) { { 'Origin' => 'http://notgitlab.com' } }
+ let(:cors_request_headers) { { 'Origin' => 'https://gitlab.com' } }
let(:params) { { jwt: jwt, format: :json } }
before do
--
GitLab