From 8c6a42c589cbcbbc1f8c3dd4d13b7674f2661eb2 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Fri, 5 Jun 2020 18:31:58 +0200 Subject: [PATCH] FIX: Redirects containing Unicode usernames didn't work --- app/controllers/application_controller.rb | 2 +- app/controllers/email_controller.rb | 2 +- app/controllers/user_avatars_controller.rb | 4 +-- app/controllers/users_controller.rb | 4 +-- app/models/user.rb | 4 +++ spec/fabricators/user_fabricator.rb | 4 +++ spec/models/user_spec.rb | 15 +++++++++ spec/requests/application_controller_spec.rb | 9 ++++++ spec/requests/email_controller_spec.rb | 12 +++++-- spec/requests/user_avatars_controller_spec.rb | 26 ++++++++++++++-- spec/requests/users_controller_spec.rb | 31 ++++++++++++++----- spec/support/integration_helpers.rb | 2 +- 12 files changed, 96 insertions(+), 19 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2ce9f875ffd..c59266e1305 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -760,7 +760,7 @@ class ApplicationController < ActionController::Base return if !current_user return if !should_enforce_2fa? - redirect_path = "#{GlobalSetting.relative_url_root}/u/#{current_user.username}/preferences/second-factor" + redirect_path = path("/u/#{current_user.encoded_username}/preferences/second-factor") if !request.fullpath.start_with?(redirect_path) redirect_to path(redirect_path) nil diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index 5e8b8d63afb..a7d283a6b2c 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -7,7 +7,7 @@ class EmailController < ApplicationController before_action :ensure_logged_in, only: :preferences_redirect def preferences_redirect - redirect_to(email_preferences_path(current_user.username_lower)) + redirect_to path("/u/#{current_user.encoded_username}/preferences/emails") end def unsubscribe diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb index 13fc1f8cd93..3b0dd7099ce 100644 --- a/app/controllers/user_avatars_controller.rb +++ b/app/controllers/user_avatars_controller.rb @@ -109,7 +109,7 @@ class UserAvatarsController < ApplicationController if !Discourse.avatar_sizes.include?(size) && Discourse.store.external? closest = Discourse.avatar_sizes.to_a.min { |a, b| (size - a).abs <=> (size - b).abs } - avatar_url = UserAvatar.local_avatar_url(hostname, user.username_lower, upload_id, closest) + avatar_url = UserAvatar.local_avatar_url(hostname, user.encoded_username(lower: true), upload_id, closest) return redirect_to cdn_path(avatar_url) end @@ -117,7 +117,7 @@ class UserAvatarsController < ApplicationController upload ||= user.uploaded_avatar if user.uploaded_avatar_id == upload_id if user.uploaded_avatar && !upload - avatar_url = UserAvatar.local_avatar_url(hostname, user.username_lower, user.uploaded_avatar_id, size) + avatar_url = UserAvatar.local_avatar_url(hostname, user.encoded_username(lower: true), user.uploaded_avatar_id, size) return redirect_to cdn_path(avatar_url) elsif upload && optimized = get_optimized_image(upload, size) if optimized.local? diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 12d9643b571..42303354429 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -130,7 +130,7 @@ class UsersController < ApplicationController end def user_preferences_redirect - redirect_to email_preferences_path(current_user.username_lower) + redirect_to path("/u/#{current_user.encoded_username}/preferences") end def update @@ -273,7 +273,7 @@ class UsersController < ApplicationController cookies[:destination_url] = path("/my/#{params[:path]}") redirect_to path("/login-preferences") else - redirect_to(path("/u/#{current_user.username}/#{params[:path]}")) + redirect_to(path("/u/#{current_user.encoded_username}/#{params[:path]}")) end end diff --git a/app/models/user.rb b/app/models/user.rb index 0147c62b1bb..fdade36364c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1302,6 +1302,10 @@ class User < ActiveRecord::Base .pluck(:credential_id) end + def encoded_username(lower: false) + UrlHelper.encode_component(lower ? username_lower : username) + end + protected def badge_grant diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index 6e6e4624f9c..759a5a43d3e 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -112,3 +112,7 @@ end Fabricator(:staged, from: :user) do staged true end + +Fabricator(:unicode_user, from: :user) do + username { sequence(:username) { |i| "Löwe#{i}" } } +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4d2592c788f..2a50cd82c17 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2373,4 +2373,19 @@ describe User do def reset_last_seen_cache!(user) Discourse.redis.del("user:#{user.id}:#{Time.zone.now.to_date}") end + + describe ".encoded_username" do + it "doesn't encoded ASCII usernames" do + user = Fabricate(:user, username: "John") + expect(user.encoded_username).to eq("John") + expect(user.encoded_username(lower: true)).to eq("john") + end + + it "encodes Unicode characters" do + SiteSetting.unicode_usernames = true + user = Fabricate(:user, username: "Löwe") + expect(user.encoded_username).to eq("L%C3%B6we") + expect(user.encoded_username(lower: true)).to eq("l%C3%B6we") + end + end end diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 8e4dc439d87..0e3fe9a9c6f 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -162,6 +162,15 @@ RSpec.describe ApplicationController do expect(response.status).to eq(200) end + it "correctly redirects for Unicode usernames" do + SiteSetting.enforce_second_factor = "all" + SiteSetting.unicode_usernames = true + user = sign_in(Fabricate(:unicode_user)) + + get "/" + expect(response).to redirect_to("/u/#{user.encoded_username}/preferences/second-factor") + end + context "when enforcing second factor for staff" do before do SiteSetting.enforce_second_factor = "staff" diff --git a/spec/requests/email_controller_spec.rb b/spec/requests/email_controller_spec.rb index ec60265f667..9c9ab972325 100644 --- a/spec/requests/email_controller_spec.rb +++ b/spec/requests/email_controller_spec.rb @@ -176,11 +176,17 @@ RSpec.describe EmailController do end context 'when logged in' do - let!(:user) { sign_in(Fabricate(:user)) } - it 'redirects to your user preferences' do + user = sign_in(Fabricate(:user)) get "/email_preferences.json" - expect(response).to redirect_to("/u/#{user.username}/preferences") + expect(response).to redirect_to("/u/#{user.username}/preferences/emails") + end + + it "correctly redirects for Unicode usernames" do + SiteSetting.unicode_usernames = true + user = sign_in(Fabricate(:unicode_user)) + get "/email_preferences.json" + expect(response).to redirect_to("/u/#{user.encoded_username}/preferences/emails") end end end diff --git a/spec/requests/user_avatars_controller_spec.rb b/spec/requests/user_avatars_controller_spec.rb index 9772fe4b2d2..4f639fd15ba 100644 --- a/spec/requests/user_avatars_controller_spec.rb +++ b/spec/requests/user_avatars_controller_spec.rb @@ -75,10 +75,10 @@ describe UserAvatarsController do SiteSetting.s3_secret_access_key = "XXX" SiteSetting.s3_upload_bucket = "test" SiteSetting.s3_cdn_url = "http://cdn.com" + SiteSetting.unicode_usernames = true stub_request(:get, "http://cdn.com/something/else").to_return(body: 'image') - - GlobalSetting.stubs(:cdn_url).returns("http://awesome.com/boom") + set_cdn_url("http://awesome.com/boom") upload = Fabricate(:upload, url: "//test.s3.dualstack.us-east-1.amazonaws.com/something") @@ -103,6 +103,11 @@ describe UserAvatarsController do expect(response.body).to eq("image") expect(response.headers["Cache-Control"]).to eq('max-age=31556952, public, immutable') expect(response.headers["Last-Modified"]).to eq(optimized_image.upload.created_at.httpdate) + + user.update!(username: "Löwe") + + get "/user_avatar/default/#{user.encoded_username}/97/#{upload.id}.png" + expect(response).to redirect_to("http://awesome.com/boom/user_avatar/default/#{user.encoded_username(lower: true)}/98/#{upload.id}_#{OptimizedImage::VERSION}.png") end it 'serves new version for old urls' do @@ -149,5 +154,22 @@ describe UserAvatarsController do expect(response.status).to eq(200) end + + it "serves the correct image when the upload id changed" do + SiteSetting.avatar_sizes = "50" + SiteSetting.unicode_usernames = true + + upload = Fabricate(:upload) + another_upload = Fabricate(:upload) + user = Fabricate(:user, uploaded_avatar_id: upload.id) + + get "/user_avatar/default/#{user.username}/50/#{another_upload.id}.png" + expect(response).to redirect_to("http://test.localhost/user_avatar/default/#{user.username_lower}/50/#{upload.id}_#{OptimizedImage::VERSION}.png") + + user.update!(username: "Löwe") + + get "/user_avatar/default/#{user.encoded_username}/50/#{another_upload.id}.png" + expect(response).to redirect_to("http://test.localhost/user_avatar/default/#{user.encoded_username(lower: true)}/50/#{upload.id}_#{OptimizedImage::VERSION}.png") + end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 5d3e0bc0de0..9ec957d1c9c 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -2433,8 +2433,10 @@ describe UsersController do describe '#my_redirect' do it "redirects if the user is not logged in" do - get "/my/wat.json" - expect(response).to be_redirect + get "/my/wat" + expect(response).to redirect_to("/login-preferences") + expect(response.cookies).to have_key("destination_url") + expect(response.cookies["destination_url"]).to eq("/my/wat") expect(response.headers['X-Robots-Tag']).to eq('noindex') end @@ -2447,13 +2449,21 @@ describe UsersController do end it "will redirect to an valid path" do - get "/my/preferences.json" - expect(response).to be_redirect + get "/my/preferences" + expect(response).to redirect_to("/u/#{user.username}/preferences") end it "permits forward slashes" do - get "/my/activity/posts.json" - expect(response).to be_redirect + get "/my/activity/posts" + expect(response).to redirect_to("/u/#{user.username}/activity/posts") + end + + it "correctly redirects for Unicode usernames" do + SiteSetting.unicode_usernames = true + user = sign_in(Fabricate(:unicode_user)) + + get "/my/preferences" + expect(response).to redirect_to("/u/#{user.encoded_username}/preferences") end end end @@ -3495,7 +3505,14 @@ describe UsersController do it "redirects to their profile when logged in" do sign_in(user) get '/user_preferences' - expect(response).to redirect_to("/u/#{user.username_lower}/preferences") + expect(response).to redirect_to("/u/#{user.username}/preferences") + end + + it "correctly redirects for Unicode usernames" do + SiteSetting.unicode_usernames = true + user = sign_in(Fabricate(:unicode_user)) + get '/user_preferences' + expect(response).to redirect_to("/u/#{user.encoded_username}/preferences") end end diff --git a/spec/support/integration_helpers.rb b/spec/support/integration_helpers.rb index 8155ef4d2c0..6cb93acbdf3 100644 --- a/spec/support/integration_helpers.rb +++ b/spec/support/integration_helpers.rb @@ -26,7 +26,7 @@ module IntegrationHelpers end def sign_in(user) - get "/session/#{user.username}/become" + get "/session/#{user.encoded_username}/become" user end