CVE-2019-1020017: Insufficient Information (NVD-CWE-noinfo)
value: Discourse before 2.3.0 and 2.4.x before 2.4.0.beta3 lacks a confirmation screen when logging in via a user-api OTP.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 5.3
Published: 2019-07-29T13:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/session_controller.rb, app/views/session/one_time_password.html.erb, config/locales/server.en.yml, config/routes.rb, spec/requests/session_controller_spec.rb
Commit date: 2019-06-17 16:18:44 +0100
Commit message: SECURITY: Add confirmation screen when logging in via user-api OTP

Diff: diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index 617130453a..b48ee8a8f1 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -363,16 +363,23 @@ class SessionController < ApplicationController
 
     return render json: { error: I18n.t('email_login.invalid_token') }
   end
 
   def one_time_password
-    otp_username = $redis.get "otp_#{params[:token]}"
+    @otp_username = otp_username = $redis.get "otp_#{params[:token]}"
 
     if otp_username && user = User.find_by_username(otp_username)
-      log_on_user(user)
-      $redis.del "otp_#{params[:token]}"
-      return redirect_to path("/")
+      if current_user&.username == otp_username
+        $redis.del "otp_#{params[:token]}"
+        return redirect_to path("/")
+      elsif request.post?
+        log_on_user(user)
+        $redis.del "otp_#{params[:token]}"
+        return redirect_to path("/")
+      else
+        # Display the form
+      end
     else
       @error = I18n.t('user_api_key.invalid_token')
     end
 
     render layout: 'no_ember'
diff --git a/app/views/session/one_time_password.html.erb b/app/views/session/one_time_password.html.erb
index a0faef79d9..e86e371298 100644
--- a/app/views/session/one_time_password.html.erb
+++ b/app/views/session/one_time_password.html.erb
@@ -1,5 +1,11 @@
 <%if @error%>
   <div class='alert alert-error'>
     <%= @error %>
   </div>
+<%else%>
+  <%= form_tag do%>
+    <h2><%= t("user_api_key.otp_confirmation.confirm_title", site_name: SiteSetting.title) %></h2>
+    <p><%= t("user_api_key.otp_confirmation.logging_in_as", username: @otp_username) %></p>
+    <%= submit_tag(t("user_api_key.otp_confirmation.confirm_button"), class: "btn btn-primary") %>
+  <%end%>
 <%end%>
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 04de4f106b..2140b1f407 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -937,10 +937,14 @@ en:
     read: "read"
     read_write: "read/write"
     description: '"%{application_name}" is requesting the following access to your account:'
     instructions: 'We just generated a new user API key for you to use with "%{application_name}", please paste the following key into your application:'
     otp_description: 'Would you like to allow "%{application_name}" to access this site?'
+    otp_confirmation: 
+      confirm_title: Continue to %{site_name}
+      logging_in_as: Logging in as %{username}
+      confirm_button: Finish Login
     no_trust_level: "Sorry, you do not have the required trust level to access the user API"
     generic_error: "Sorry, we are unable to issue user API keys, this feature may be disabled by the site admin"
     scopes:
       message_bus: "Live updates"
       notifications: "Read and clear notifications"
diff --git a/config/routes.rb b/config/routes.rb
index f9e57a6650..d6132b9da2 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -338,10 +338,11 @@ Discourse::Application.routes.draw do
   get "session/current" => "session#current"
   get "session/csrf" => "session#csrf"
   get "session/email-login/:token" => "session#email_login_info"
   post "session/email-login/:token" => "session#email_login"
   get "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ }
+  post "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ }
   get "composer_messages" => "composer_messages#index"
   post "composer/parse_html" => "composer#parse_html"
 
   resources :static
   post "login" => "static#enter", constraints: { format: /(json|html)/ }
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index e329b5e77e..f3491ef5bd 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -1367,23 +1367,54 @@ RSpec.describe SessionController do
     context 'invalid token' do
       it 'returns the right response' do
         get "/session/otp/asd1231dasd123"
 
         expect(response.status).to eq(404)
+
+        post "/session/otp/asd1231dasd123"
+
+        expect(response.status).to eq(404)
       end
 
       context 'when token is valid' do
+        it "should display the form for GET" do
+          token = SecureRandom.hex
+          $redis.setex "otp_#{token}", 10.minutes, user.username
+
+          get "/session/otp/#{token}"
+
+          expect(response.status).to eq(200)
+          expect(response.body).to include(
+            I18n.t("user_api_key.otp_confirmation.logging_in_as", username: user.username)
+          )
+          expect($redis.get("otp_#{token}")).to eq(user.username)
+
+          expect(session[:current_user_id]).to eq(nil)
+        end
+
+        it "should redirect on GET if already logged in" do
+          sign_in(user)
+          token = SecureRandom.hex
+          $redis.setex "otp_#{token}", 10.minutes, user.username
+
+          get "/session/otp/#{token}"
+          expect(response.status).to eq(302)
+
+          expect($redis.get("otp_#{token}")).to eq(nil)
+          expect(session[:current_user_id]).to eq(user.id)
+        end
+
         it 'should authenticate user and delete token' do
           user = Fabricate(:user)
 
           get "/session/current.json"
           expect(response.status).to eq(404)
 
           token = SecureRandom.hex
           $redis.setex "otp_#{token}", 10.minutes, user.username
 
-          get "/session/otp/#{token}"
+          post "/session/otp/#{token}"
 
           expect(response.status).to eq(302)
           expect(response).to redirect_to("/")
           expect($redis.get("otp_#{token}")).to eq(nil)
 

Vulnerable code:     otp_username = $redis.get "otp_#{params[:token]}"
      log_on_user(user)
      $redis.del "otp_#{params[:token]}"
      return redirect_to path("/")
          get "/session/otp/#{token}"
Fixed code:     @otp_username = otp_username = $redis.get "otp_#{params[:token]}"
      if current_user&.username == otp_username
        $redis.del "otp_#{params[:token]}"
        return redirect_to path("/")
      elsif request.post?
        log_on_user(user)
        $redis.del "otp_#{params[:token]}"
        return redirect_to path("/")
      else
        # Display the form
      end
<%else%>
  <%= form_tag do%>
    <h2><%= t("user_api_key.otp_confirmation.confirm_title", site_name: SiteSetting.title) %></h2>
    <p><%= t("user_api_key.otp_confirmation.logging_in_as", username: @otp_username) %></p>
    <%= submit_tag(t("user_api_key.otp_confirmation.confirm_button"), class: "btn btn-primary") %>
  <%end%>
    otp_confirmation: 
      confirm_title: Continue to %{site_name}
      logging_in_as: Logging in as %{username}
      confirm_button: Finish Login
  post "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ }

        post "/session/otp/asd1231dasd123"

        expect(response.status).to eq(404)
        it "should display the form for GET" do
          token = SecureRandom.hex
          $redis.setex "otp_#{token}", 10.minutes, user.username

          get "/session/otp/#{token}"

          expect(response.status).to eq(200)
          expect(response.body).to include(
            I18n.t("user_api_key.otp_confirmation.logging_in_as", username: user.username)
          )
          expect($redis.get("otp_#{token}")).to eq(user.username)

          expect(session[:current_user_id]).to eq(nil)
        end

        it "should redirect on GET if already logged in" do
          sign_in(user)
          token = SecureRandom.hex
          $redis.setex "otp_#{token}", 10.minutes, user.username

          get "/session/otp/#{token}"
          expect(response.status).to eq(302)

          expect($redis.get("otp_#{token}")).to eq(nil)
          expect(session[:current_user_id]).to eq(user.id)
        end

          post "/session/otp/#{token}"