[ARVADOS] updated: 1.3.0-187-geae02bfd9

[Git user]

Summary of changes:
 services/api/app/controllers/user_sessions_controller.rb    |  8 +++++++-
 .../api/test/functional/user_sessions_controller_test.rb    |  8 ++++++++
 services/api/test/integration/user_sessions_test.rb         | 13 +++++++++++--
 3 files changed, 26 insertions(+), 3 deletions(-)

       via  eae02bfd9730fa968865ab1cce65666e2653957b (commit)
      from  0d1836a8d4d5a0c0802881c2878a35f611e09e1f (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit eae02bfd9730fa968865ab1cce65666e2653957b
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Mon Jan 28 18:58:06 2019 -0300

    14718: Validates remote cluster id parameter on login endpoint & callback.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>

diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb
index 7f7b47fd9..1889d74ea 100644
--- a/services/api/app/controllers/user_sessions_controller.rb
+++ b/services/api/app/controllers/user_sessions_controller.rb
@@ -99,6 +99,9 @@ class UserSessionsController < ApplicationController
       # encoding the remote=zbbbb parameter passed by a client asking for a salted
       # token.
       remote, return_to_url = params[:return_to].split(',', 2)
+      if remote !~ /^[0-9a-z]{5}$/ && remote != ""
+        return send_error 'Invalid remote cluster id', status: 400
+      end
       remote = nil if remote == ''
       return send_api_token_to(return_to_url, user, remote)
     end
@@ -124,6 +127,9 @@ class UserSessionsController < ApplicationController
   # to save the return_to parameter (if it exists; see the application
   # controller). /auth/joshid bypasses the application controller.
   def login
+    if params[:remote] !~ /^[0-9a-z]{5}$/ && !params[:remote].nil?
+      return send_error 'Invalid remote cluster id', status: 400
+    end
     if current_user and params[:return_to]
       # Already logged in; just need to send a token to the requesting
       # API client.
@@ -139,7 +145,7 @@ class UserSessionsController < ApplicationController
       # Encode remote param inside callback's return_to, so that we'll get it on
       # create() after login.
       remote_param = params[:remote].nil? ? '' : params[:remote]
-      p << "return_to=#{CGI.escape(remote_param)},#{CGI.escape(params[:return_to])}"
+      p << "return_to=#{CGI.escape(remote_param + ',' + params[:return_to])}"
     end
     redirect_to "/auth/joshid?#{p.join('&')}"
   end
diff --git a/services/api/test/functional/user_sessions_controller_test.rb b/services/api/test/functional/user_sessions_controller_test.rb
index 43872e1b8..e3048159f 100644
--- a/services/api/test/functional/user_sessions_controller_test.rb
+++ b/services/api/test/functional/user_sessions_controller_test.rb
@@ -27,4 +27,12 @@ class UserSessionsControllerTest < ActionController::TestCase
     assert_not_nil api_client_auth
     assert_includes(@response.redirect_url, 'api_token='+api_client_auth.salted_token(remote: remote_prefix))
   end
+
+  test "login with malformed remote param returns an error" do
+    authorize_with :inactive
+    api_client_page = 'http://client.example.com/home'
+    remote_prefix = 'invalid_cluster_id'
+    get :login, return_to: api_client_page, remote: remote_prefix
+    assert_response 400
+  end
 end
diff --git a/services/api/test/integration/user_sessions_test.rb b/services/api/test/integration/user_sessions_test.rb
index 7d1ed1873..f5085999e 100644
--- a/services/api/test/integration/user_sessions_test.rb
+++ b/services/api/test/integration/user_sessions_test.rb
@@ -13,7 +13,7 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
     url
   end
 
-  def mock_auth_with(email: nil, username: nil, identity_url: nil, remote: nil)
+  def mock_auth_with(email: nil, username: nil, identity_url: nil, remote: nil, expected_response: :redirect)
     mock = {
       'provider' => 'josh_id',
       'uid' => 'https://edward.example.com',
@@ -30,7 +30,12 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
     post('/auth/josh_id/callback',
          {return_to: client_url(remote: remote)},
          {'omniauth.auth' => mock})
-    assert_response :redirect, 'Did not redirect to client with token'
+
+    errors = {
+      :redirect => 'Did not redirect to client with token',
+      400 => 'Did not return Bad Request error',
+    }
+    assert_response expected_response, errors[expected_response]
   end
 
   test 'assign username from sso' do
@@ -80,6 +85,10 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
     assert_includes(@response.redirect_url, 'api_token=' + api_client_auth.salted_token(remote: 'zbbbb'))
   end
 
+  test 'error out from omniauth callback with invalid remote param' do
+    mock_auth_with(email: 'edward at example.com', remote: 'invalid_cluster_id', expected_response: 400)
+  end
+
   # Test various combinations of auto_setup configuration and email
   # address provided during a new user's first session setup.
   [{result: :nope, email: nil, cfg: {auto: true, repo: true, vm: true}},

-----------------------------------------------------------------------


hooks/post-receive
--