SECURITY: XSS in routes

SECURITY: XSS in routes

Co-authored-by: Guo Xiang Tan tgx_world@hotmail.com Co-authored-by: David Taylor david@taylorhq.com

diff --git a/app/assets/javascripts/discourse/adapters/rest.js.es6 b/app/assets/javascripts/discourse/adapters/rest.js.es6
index 92d98ba..b5ffc0f 100644
--- a/app/assets/javascripts/discourse/adapters/rest.js.es6
+++ b/app/assets/javascripts/discourse/adapters/rest.js.es6
@@ -54,7 +54,9 @@ export default Ember.Object.extend({
         }
       } else {
         // It's serializable as a string if not an object
-        return `${path}/${findArgs}${extension ? extension : ""}`;
+        return `${path}/${encodeURIComponent(findArgs)}${
+          extension ? extension : ""
+        }`;
       }
     }
     return path;
diff --git a/app/assets/javascripts/discourse/routes/new-message.js.es6 b/app/assets/javascripts/discourse/routes/new-message.js.es6
index bf52392..42b664f 100644
--- a/app/assets/javascripts/discourse/routes/new-message.js.es6
+++ b/app/assets/javascripts/discourse/routes/new-message.js.es6
@@ -11,7 +11,7 @@ export default Discourse.Route.extend({
       this.replaceWith("discovery.latest").then(e => {
         if (params.username) {
           // send a message to a user
-          User.findByUsername(params.username)
+          User.findByUsername(encodeURIComponent(params.username))
             .then(user => {
               if (user.can_send_private_message_to_user) {
                 Ember.run.next(() =>
diff --git a/app/assets/javascripts/discourse/routes/user.js.es6 b/app/assets/javascripts/discourse/routes/user.js.es6
index 97d9bff..4eaaa4c9 100644
--- a/app/assets/javascripts/discourse/routes/user.js.es6
+++ b/app/assets/javascripts/discourse/routes/user.js.es6
@@ -39,7 +39,9 @@ export default Discourse.Route.extend({
       return this.currentUser;
     }
 
-    return Discourse.User.create({ username: params.username });
+    return Discourse.User.create({
+      username: encodeURIComponent(params.username)
+    });
   },
 
   afterModel() {
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index 8b7ddfa..4b780b5 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -69,6 +69,9 @@ class UploadsController < ApplicationController
   end
 
   def show
+    # do not serve uploads requested via XHR to prevent XSS
+    return render_404 if request.xhr?
+
     return render_404 if !RailsMultisite::ConnectionManagement.has_db?(params[:site])
 
     RailsMultisite::ConnectionManagement.with_connection(params[:site]) do |db|
@@ -88,6 +91,9 @@ class UploadsController < ApplicationController
   end
 
   def show_short
+    # do not serve uploads requested via XHR to prevent XSS
+    return render_404 if request.xhr?
+
     if SiteSetting.prevent_anons_from_downloading_files && current_user.nil?
       return render_404
     end
diff --git a/test/javascripts/acceptance/user-test.js.es6 b/test/javascripts/acceptance/user-test.js.es6
index ad6aeaa..ff93b54 100644
--- a/test/javascripts/acceptance/user-test.js.es6
+++ b/test/javascripts/acceptance/user-test.js.es6
@@ -2,6 +2,30 @@ import { acceptance } from "helpers/qunit-helpers";
 
 acceptance("User", { loggedIn: true });
 
+QUnit.test("Invalid usernames", async assert => {
+  // prettier-ignore
+  server.get("/u/eviltrout%2F..%2F..%2F.json", () => { // eslint-disable-line no-undef
+    return [
+      404,
+      { "Content-Type": "application/json" },
+      {
+        errors: ["The requested URL or resource could not be found."],
+        error_type: "not_found"
+      }
+    ];
+  });
+
+  await visit("/u/eviltrout%2F..%2F..%2F/summary");
+
+  assert.equal(currentPath(), "exception-unknown");
+});
+
+QUnit.test("Unicode usernames", async assert => {
+  await visit("/u/%E3%83%A9%E3%82%A4%E3%82%AA%E3%83%B3/summary");
+
+  assert.equal(currentPath(), "user.summary");
+});
+
 QUnit.test("Invites", async assert => {
   await visit("/u/eviltrout/invited/pending");
   assert.ok($("body.user-invites-page").length, "has the body class");
diff --git a/test/javascripts/fixtures/user_fixtures.js.es6 b/test/javascripts/fixtures/user_fixtures.js.es6
index 954fb14..4505893 100644
--- a/test/javascripts/fixtures/user_fixtures.js.es6
+++ b/test/javascripts/fixtures/user_fixtures.js.es6
@@ -2601,5 +2601,205 @@ export default {
       ],
       top_categories: []
     }
+  },
+  "/u/%E3%83%A9%E3%82%A4%E3%82%AA%E3%83%B3.json": {
+    user_badges: [],
+    user: {
+      id: 2,
+      username: "ライオン",
+      name: null,
+      avatar_template:
+        "/letter_avatar_proxy/v4/letter/%E3%83%A9/d9b06d/{size}.png",
+      email: "lion@example.com",
+      last_posted_at: null,
+      last_seen_at: "2019-06-26T09:29:56.044Z",
+      bio_raw: "this is my bio",
+      bio_cooked: "\u003cp\u003ethis is my bio\u003c/p\u003e",
+      created_at: "2019-06-26T08:40:40.964Z",
+      can_edit: true,
+      can_edit_username: true,
+      can_edit_email: true,
+      can_edit_name: true,
+      ignored: false,
+      muted: false,
+      can_ignore_user: false,
+      can_mute_user: false,
+      can_send_private_messages: true,
+      can_send_private_message_to_user: false,
+      bio_excerpt: "this is my bio",
+      trust_level: 1,
+      moderator: false,
+      admin: false,
+      title: null,
+      uploaded_avatar_id: null,
+      badge_count: 0,
+      has_title_badges: false,
+      custom_fields: {},
+      pending_count: 0,
+      profile_view_count: 1,
+      time_read: 0,
+      recent_time_read: 0,
+      primary_group_name: null,
+      primary_group_flair_url: null,
+      primary_group_flair_bg_color: null,
+      primary_group_flair_color: null,
+      second_factor_enabled: false,
+      second_factor_backup_enabled: false,
+      associated_accounts: [],
+      locale: "en_US",
+      muted_category_ids: [],
+      watched_tags: [],
+      watching_first_post_tags: [],
+      tracked_tags: [],
+      muted_tags: [],
+      tracked_category_ids: [],
+      watched_category_ids: [],
+      watched_first_post_category_ids: [],
+      system_avatar_upload_id: null,
+      system_avatar_template:
+        "/letter_avatar_proxy/v4/letter/%E3%83%A9/d9b06d/{size}.png",
+      muted_usernames: [],
+      ignored_usernames: [],
+      mailing_list_posts_per_day: 0,
+      can_change_bio: true,
+      user_api_keys: null,
+      user_auth_tokens: [
+        {
+          id: 2,
+          client_ip: "127.0.0.1",
+          location: "unknown",
+          browser: "Google Chrome",
+          device: "GNU/Linux Computer",
+          os: "Linux",
+          icon: "fab-linux",
+          created_at: "2019-06-26T08:41:18.436Z",
+          seen_at: "2019-06-26T09:24:24.683Z",
+          is_active: true
+        }
+      ],
+      user_auth_token_logs: [
+        {
+          id: 4,
+          client_ip: "127.0.0.1",
+          location: "unknown",
+          browser: "Google Chrome",
+          device: "GNU/Linux Computer",
+          os: "Linux",
+          icon: "fab-linux",
+          created_at: "2019-06-26T08:41:18.448Z",
+          action: "Log In"
+        }
+      ],
+      invited_by: null,
+      groups: [
+        {
+          id: 10,
+          automatic: true,
+          name: "trust_level_0",
+          display_name: "trust_level_0",
+          user_count: 2,
+          mentionable_level: 0,
+          messageable_level: 0,
+          visibility_level: 0,
+          primary_group: false,
+          title: null,
+          grant_trust_level: null,
+          has_messages: false,
+          flair_url: null,
+          flair_bg_color: null,
+          flair_color: null,
+          bio_cooked: null,
+          bio_excerpt: null,
+          public_admission: false,
+          public_exit: false,
+          allow_membership_requests: false,
+          full_name: null,
+          default_notification_level: 3,
+          membership_request_template: null
+        },
+        {
+          id: 11,
+          automatic: true,
+          name: "trust_level_1",
+          display_name: "trust_level_1",
+          user_count: 2,

[... diff too long, it was truncated ...]

GitHub sha: 13f38055

1 Like

Does this also affect 2.3?

Yes. I backported it to beta and stable.

1 Like

Thanks!

Shouldn’t this be a 400 and not a 404 for xhr?

2 Likes

Yes, an error 400 would have been a better choice. I’ll change it.

3 Likes

Cool be sure to backport.

2 Likes

DEV: Respond with error 400 to uploads requested via XHR