From 8769ca08bbb31368f290f84aedb7d8a357f4a636 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Thu, 19 Mar 2020 10:59:32 +1000
Subject: [PATCH] SECURITY: Prevent access to other user's bookmark lists

---
 .../user-activity-bookmarks-with-reminders.js |  7 +++++--
 app/controllers/users_controller.rb           |  1 +
 config/locales/client.en.yml                  |  1 +
 spec/requests/users_controller_spec.rb        | 19 +++++++++++++++++++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js b/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js
index 49982b12449..ad8ab35a6b5 100644
--- a/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js
+++ b/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js
@@ -33,6 +33,9 @@ export default Controller.extend({
           this.content.pushObjects(bookmarks);
         }
       })
+      .catch(() => {
+        this.set("noResultsHelp", I18n.t("bookmarks.list_permission_denied"));
+      })
       .finally(() =>
         this.setProperties({
           loaded: true,
@@ -42,8 +45,8 @@ export default Controller.extend({
   },
 
   @discourseComputed("loaded", "content.length")
-  noContent(loaded, content) {
-    return loaded && content.length === 0;
+  noContent(loaded, contentLength) {
+    return loaded && contentLength === 0;
   },
 
   actions: {
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index d7cc5f3de33..6125bf98366 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -1397,6 +1397,7 @@ class UsersController < ApplicationController
 
   def bookmarks
     user = fetch_user_from_params
+    guardian.ensure_can_edit!(user)
 
     respond_to do |format|
       format.json do
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 9ab7d5c5d7b..ef241f4e166 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -313,6 +313,7 @@ en:
       save: "Save"
       no_timezone: 'You have not set a timezone yet. You will not be able to set reminders. Set one up <a href="%{basePath}/my/preferences/profile">in your profile</a>.'
       invalid_custom_datetime: "The date and time you provided is invalid, please try again."
+      list_permission_denied: "You do not have permission to view this user's bookmarks."
       reminders:
         at_desktop: "Next time I'm at my desktop"
         later_today: "Later today <br/>{{date}}"
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 43e41e55224..b2cc43fedee 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -4167,6 +4167,25 @@ describe UsersController do
     end
   end
 
+  describe "#bookmarks" do
+    let!(:bookmark1) { Fabricate(:bookmark, user: user) }
+    let!(:bookmark2) { Fabricate(:bookmark, user: user) }
+    let!(:bookmark3) { Fabricate(:bookmark) }
+
+    it "returns a list of serialized bookmarks for the user" do
+      sign_in(user)
+      get "/u/#{user.username}/bookmarks.json"
+      expect(response.status).to eq(200)
+      expect(JSON.parse(response.body)['bookmarks'].map { |b| b['id'] }).to match_array([bookmark1.id, bookmark2.id])
+    end
+
+    it "does not show another user's bookmarks" do
+      sign_in(user)
+      get "/u/#{bookmark3.user.username}/bookmarks.json"
+      expect(response.status).to eq(403)
+    end
+  end
+
   def create_second_factor_security_key
     sign_in(user)
     stub_secure_session_confirmed