From 1299c94a5282693e57661f156b4072e3c890d7cc Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Wed, 15 May 2019 09:55:31 +0100
Subject: [PATCH] FIX: Make serverside and clientside omniauth origin redirects
 consistent

Previously external domains were allowed in the client-side redirects, but not the server-side redirects. Now the behavior is to only allow local origins.
---
 .../users/omniauth_callbacks_controller.rb       | 12 ++++++------
 .../omniauth_callbacks_controller_spec.rb        | 16 ++++++++++++++--
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index f96b046d0ac..d7fda5e304d 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/app/controllers/users/omniauth_callbacks_controller.rb
@@ -44,18 +44,18 @@ class Users::OmniauthCallbacksController < ApplicationController
       @auth_result = authenticator.after_authenticate(auth)
     end
 
-    origin = request.env['omniauth.origin']
+    preferred_origin = request.env['omniauth.origin']
 
     if SiteSetting.enable_sso_provider && payload = cookies.delete(:sso_payload)
-      origin = session_sso_provider_url + "?" + payload
+      preferred_origin = session_sso_provider_url + "?" + payload
     elsif cookies[:destination_url].present?
-      origin = cookies[:destination_url]
+      preferred_origin = cookies[:destination_url]
       cookies.delete(:destination_url)
     end
 
-    if origin.present?
+    if preferred_origin.present?
       parsed = begin
-        URI.parse(origin)
+        URI.parse(preferred_origin)
       rescue URI::Error
       end
 
@@ -69,7 +69,7 @@ class Users::OmniauthCallbacksController < ApplicationController
       @origin = Discourse.base_uri("/")
     end
 
-    @auth_result.destination_url = origin
+    @auth_result.destination_url = @origin
 
     if @auth_result.failed?
       flash[:error] = @auth_result.failed_reason.html_safe
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index d830d999e48..0cb2d54a400 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -119,7 +119,7 @@ RSpec.describe Users::OmniauthCallbacksController do
       end
 
       it 'should return the right response' do
-        destination_url = 'http://thisisasite.com/somepath'
+        destination_url = '/somepath'
         Rails.application.env_config["omniauth.origin"] = destination_url
 
         get "/auth/google_oauth2/callback.json"
@@ -138,7 +138,7 @@ RSpec.describe Users::OmniauthCallbacksController do
       end
 
       it 'should include destination url in response' do
-        destination_url = 'http://thisisasite.com/somepath'
+        destination_url = '/cookiepath'
         cookies[:destination_url] = destination_url
 
         get "/auth/google_oauth2/callback.json"
@@ -353,6 +353,9 @@ RSpec.describe Users::OmniauthCallbacksController do
 
           expect(response.status).to eq 302
           expect(response.location).to eq "http://test.localhost/"
+
+          cookie_data = JSON.parse(response.cookies['authentication_data'])
+          expect(cookie_data["destination_url"]).to eq('/')
         end
 
         it "redirects to internal origin" do
@@ -361,6 +364,9 @@ RSpec.describe Users::OmniauthCallbacksController do
 
           expect(response.status).to eq 302
           expect(response.location).to eq "http://test.localhost/t/123"
+
+          cookie_data = JSON.parse(response.cookies['authentication_data'])
+          expect(cookie_data["destination_url"]).to eq('/t/123')
         end
 
         it "redirects to relative origin" do
@@ -369,6 +375,9 @@ RSpec.describe Users::OmniauthCallbacksController do
 
           expect(response.status).to eq 302
           expect(response.location).to eq "http://test.localhost/t/123"
+
+          cookie_data = JSON.parse(response.cookies['authentication_data'])
+          expect(cookie_data["destination_url"]).to eq('/t/123')
         end
 
         it "redirects with query" do
@@ -377,6 +386,9 @@ RSpec.describe Users::OmniauthCallbacksController do
 
           expect(response.status).to eq 302
           expect(response.location).to eq "http://test.localhost/t/123?foo=bar"
+
+          cookie_data = JSON.parse(response.cookies['authentication_data'])
+          expect(cookie_data["destination_url"]).to eq('/t/123?foo=bar')
         end
 
         it "removes authentication_data cookie on logout" do