FIX: Fix routes ending in `:username` for usernames containing periods (#6660)

FIX: Fix routes ending in :username for usernames containing periods (#6660)

From a3ed570124ac2e61e919fa2e0bb1d1f2e91b25ae Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Fri, 23 Nov 2018 17:41:41 +0000
Subject: [PATCH] FIX: Fix routes ending in `:username` for usernames
 containing periods (#6660)


diff --git a/config/routes.rb b/config/routes.rb
index 0a8263b..501d016 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -386,7 +386,7 @@ Discourse::Application.routes.draw do
     get "#{root_path}/:username/messages/tags/:tag_id" => "user_actions#private_messages", constraints: StaffConstraint.new
     get "#{root_path}/:username.json" => "users#show", constraints: { username: RouteFormat.username }, defaults: { format: :json }
     get({ "#{root_path}/:username" => "users#show", constraints: { username: RouteFormat.username, format: /(json|html)/ } }.merge(index == 1 ? { as: 'user' } : {}))
-    put "#{root_path}/:username" => "users#update", constraints: { username: RouteFormat.username }, defaults: { format: :json }
+    put "#{root_path}/:username" => "users#update", constraints: { username: RouteFormat.username, format: /(json|html)/ }, defaults: { format: :json }
     get "#{root_path}/:username/emails" => "users#check_emails", constraints: { username: RouteFormat.username }
     get({ "#{root_path}/:username/preferences" => "users#preferences", constraints: { username: RouteFormat.username } }.merge(index == 1 ? { as: :email_preferences } : {}))
     get "#{root_path}/:username/preferences/email" => "users_email#index", constraints: { username: RouteFormat.username }
@@ -427,7 +427,7 @@ Discourse::Application.routes.draw do
     get "#{root_path}/:username/notifications" => "users#show", constraints: { username: RouteFormat.username }
     get "#{root_path}/:username/notifications/:filter" => "users#show", constraints: { username: RouteFormat.username }
     get "#{root_path}/:username/activity/pending" => "users#show", constraints: { username: RouteFormat.username }
-    delete "#{root_path}/:username" => "users#destroy", constraints: { username: RouteFormat.username }
+    delete "#{root_path}/:username" => "users#destroy", constraints: { username: RouteFormat.username, format: /(json|html)/ }
     get "#{root_path}/by-external/:external_id" => "users#show", constraints: { external_id: /[^\/]+/ }
     get "#{root_path}/:username/flagged-posts" => "users#show", constraints: { username: RouteFormat.username }
     get "#{root_path}/:username/deleted-posts" => "users#show", constraints: { username: RouteFormat.username }
@@ -436,7 +436,7 @@ Discourse::Application.routes.draw do
   end
 
   get "user-badges/:username.json" => "user_badges#username", constraints: { username: RouteFormat.username }, defaults: { format: :json }
-  get "user-badges/:username" => "user_badges#username", constraints: { username: RouteFormat.username }
+  get "user-badges/:username" => "user_badges#username", constraints: { username: RouteFormat.username, format: /(json|html)/ }
 
   post "user_avatar/:username/refresh_gravatar" => "user_avatars#refresh_gravatar", constraints: { username: RouteFormat.username }
   get "letter_avatar/:username/:size/:version.png" => "user_avatars#show_letter", format: false, constraints: { hostname: /[\w\.-]+/, size: /\d+/, username: RouteFormat.username }
@@ -641,11 +641,11 @@ Discourse::Application.routes.draw do
   get "topics/feature_stats"
 
   scope "/topics", username: RouteFormat.username do
-    get "created-by/:username" => "list#topics_by", as: "topics_by"
-    get "private-messages/:username" => "list#private_messages", as: "topics_private_messages"
-    get "private-messages-sent/:username" => "list#private_messages_sent", as: "topics_private_messages_sent"
-    get "private-messages-archive/:username" => "list#private_messages_archive", as: "topics_private_messages_archive"
-    get "private-messages-unread/:username" => "list#private_messages_unread", as: "topics_private_messages_unread"
+    get "created-by/:username" => "list#topics_by", as: "topics_by", constraints: { format: /(json|html)/ }, defaults: { format: :json }
+    get "private-messages/:username" => "list#private_messages", as: "topics_private_messages", constraints: { format: /(json|html)/ }, defaults: { format: :json }
+    get "private-messages-sent/:username" => "list#private_messages_sent", as: "topics_private_messages_sent", constraints: { format: /(json|html)/ }, defaults: { format: :json }
+    get "private-messages-archive/:username" => "list#private_messages_archive", as: "topics_private_messages_archive", constraints: { format: /(json|html)/ }, defaults: { format: :json }
+    get "private-messages-unread/:username" => "list#private_messages_unread", as: "topics_private_messages_unread", constraints: { format: /(json|html)/ }, defaults: { format: :json }
     get "private-messages-tags/:username/:tag_id.json" => "list#private_messages_tag", as: "topics_private_messages_tag", constraints: StaffConstraint.new
     get "groups/:group_name" => "list#group_topics", as: "group_topics", group_name: RouteFormat.username
 
diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb
index 733ece5..d7b0ea2 100644
--- a/spec/requests/list_controller_spec.rb
+++ b/spec/requests/list_controller_spec.rb
@@ -466,6 +466,14 @@ RSpec.describe ListController do
       json = JSON.parse(response.body)
       expect(json["topic_list"]["topics"].size).to eq(1)
     end
+
+    it "should work with period in username" do
+      user.update!(username: "myname.test")
+      get "/topics/created-by/#{user.username}", xhr: true
+      expect(response.status).to eq(200)
+      json = JSON.parse(response.body)
+      expect(json["topic_list"]["topics"].size).to eq(1)
+    end
   end
 
   describe "private_messages" do
diff --git a/spec/requests/user_badges_controller_spec.rb b/spec/requests/user_badges_controller_spec.rb
index f565caa..ae8199d 100644
--- a/spec/requests/user_badges_controller_spec.rb
+++ b/spec/requests/user_badges_controller_spec.rb
@@ -42,6 +42,15 @@ describe UserBadgesController do
       expect(parsed["user_badges"].length).to eq(1)
     end
 
+    it 'returns user_badges for a user with period in username' do
+      user.update!(username: "myname.test")
+      get "/user-badges/#{user.username}", xhr: true
+
+      expect(response.status).to eq(200)
+      parsed = JSON.parse(response.body)
+      expect(parsed["user_badges"].length).to eq(1)
+    end
+
     it 'returns user_badges for a badge' do
       get "/user_badges.json", params: { badge_id: badge.id }
 
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index f6f1caf..84b232b 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -1429,21 +1429,14 @@ describe UsersController do
       before do
         sign_in(user)
       end
-      let(:user) { Fabricate(:user) }
+      let(:user) { Fabricate(:user, username: 'test.test', name: "Test User") }
 
       it "should be able to update a user" do
-        put "/u/#{user.username}.json", params: { name: 'test.test' }
+        put "/u/#{user.username}", params: { name: 'test.test' }
 
         expect(response.status).to eq(200)
         expect(user.reload.name).to eq('test.test')
       end
-
-      it "should be able to update a user" do
-        put "/u/#{user.username}.json", params: { name: 'testing123' }
-
-        expect(response.status).to eq(200)
-        expect(user.reload.name).to eq('testing123')
-      end
     end
 
     context "as a staff user" do
@@ -2027,6 +2020,17 @@ describe UsersController do
     end
   end
 
+  describe "for user with period in username" do
+    let(:user_with_period) { Fabricate(:user, username: "myname.test") }
+
+    it "still works" do
+      sign_in(user_with_period)
+      UserDestroyer.any_instance.expects(:destroy).with(user_with_period, anything).returns(user_with_period)
+      delete "/u/#{user_with_period.username}", xhr: true
+      

GitHub

Just to clarify, this fixes usernames such as bob.json? Sam.saffron used to work afaik

this fixes usernames such as bob.json? Sam.saffron used to work afaik

No, the reverse. Usernames ending .json are not allowed (and haven’t been for years).

Before this fix, you could have a user sam and sam.saffron. Sending this request

PUT /u/sam.saffron

would actually update the sam user, NOT the sam.saffron user. (because our router interpreted .saffron as the format).

In our app we tend to do

PUT /u/sam.saffron.json

which was working correctly.

Does that make sense?

2 Likes

Hmm do the tag routes or any other routes have similar problems, so we want to reverse the approach in routes.rb so we have a whitelist default to json and then force explicit routes to expand?

Category slugs don’t allow periods, and neither do tag names. Group names can have periods, so we should check that.

A general solution would probably be better :+1:

3 Likes

so we have a whitelist default to json

@SamSaffron did you have an idea of to do this? I came up with one solution - enclosing the entire routes.rb file with

scope path: nil, constraints: { format: /(json|html)/ } do
    # Then you can override the constraint for a specific route
    get "somepath" => "controller#action", constraints: {format: /(xml)/}
end

That won’t apply to plugins (e.g. polls) which append routes at runtime, but it is still better than what we have now.

What do you think, is there a tidier solution you were thinking of?

1 Like

@SamSaffron what do you think about this method?