CVE-2023-38684 (Ruby, published 2023-07-28T16:15Z)
Description: value: Discourse is an open source discussion platform. Prior to version 3.0.6 of the `stable` branch and version 3.1.0.beta7 of the `beta` and `tests-passed` branches, in multiple controller actions, Discourse accepts limit params but does not impose any upper bound on the values being accepted. Without an upper bound, the software may allow arbitrary users to generate DB queries which may end up exhausting the resources on the server. The issue is patched in version 3.0.6 of the `stable` branch and version 3.1.0.beta7 of the `beta` and `tests-passed` branches. There are no known workarounds for this vulnerability.
Severity: nan (CVSS v3 base score: 7.5)
Weakness: CWE-770 \u2014 Allocation of Resources Without Limits or Throttling
The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
Repository: https://github.com/discourse/discourse
Files changed:
app/controllers/admin/api_controller.rb
app/controllers/admin/reports_controller.rb
app/controllers/admin/staff_action_logs_controller.rb
app/controllers/application_controller.rb
app/controllers/directory_items_controller.rb
app/controllers/drafts_controller.rb
app/controllers/groups_controller.rb
app/controllers/notifications_controller.rb
app/controllers/post_action_users_controller.rb
app/controllers/posts_controller.rb
app/controllers/tag_groups_controller.rb
app/controllers/tags_controller.rb
app/controllers/users_controller.rb
app/models/user_bookmark_list.rb
lib/bookmark_query.rb
plugins/chat/app/controllers/chat/api/channels_memberships_controller.rb
spec/lib/bookmark_query_spec.rb
spec/models/user_bookmark_list_spec.rb
spec/rails_helper.rb
spec/requests/admin/api_controller_spec.rb
spec/requests/admin/reports_controller_spec.rb
spec/requests/admin/staff_action_logs_controller_spec.rb
spec/requests/directory_items_controller_spec.rb
spec/requests/drafts_controller_spec.rb
spec/requests/examples/invalid_limit_params.rb
spec/requests/notifications_controller_spec.rb
spec/requests/post_action_users_controller_spec.rb
spec/requests/posts_controller_spec.rb
spec/requests/tag_groups_controller_spec.rb
spec/requests/tags_controller_spec.rb
spec/requests/users_controller_spec.rb
spec/serializers/user_bookmark_list_serializer_spec.rb
Commit message: SECURITY: Impose a upper bound on limit params in various controllers

What is the problem here?

In multiple controllers, we are accepting a `limit` params but do not
impose any upper bound on the values being accepted. Without an upper
bound, we may be allowing arbituary users from generating DB queries
which may end up exhausing the resources on the server.

What is the fix here?

A new `fetch_limit_from_params` helper method is introduced in
`ApplicationController` that can be used by controller actions to safely
get the limit from the params as a default limit and maximum limit has
to be set. When an invalid limit params is encountered, the server will
respond with the 400 response code.


Vulnerable code:
    limit = (params[:limit] || 50).to_i.clamp(1, 50)
    limit = nil
    if report_params.has_key?(:limit) && report_params[:limit].to_i > 0
      limit = report_params[:limit].to_i
    end
    page_size = (params[:limit] || 200).to_i.clamp(1, 200)
    limit = [params[:limit].to_i, PAGE_SIZE].min if params[:limit].to_i > 0
    limit ||= PAGE_SIZE
    params.permit(:limit)
    stream = Draft.stream(user: current_user, offset: params[:offset], limit: params[:limit])
    limit = (params[:limit] || 50).to_i
    raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > 1000
      limit = (params[:limit] || 15).to_i
      limit = 50 if limit > 50
    page_size = (params[:limit] || 200).to_i
    limit = [(params[:limit] || 60).to_i, 100].min
    matches = matches.order("name").limit(params[:limit] || 5)
    if params[:limit]
      begin
        filter_params[:limit] = Integer(params[:limit])
        raise Discourse::InvalidParameters.new(:limit) if !filter_params[:limit].positive?
      rescue ArgumentError
        raise Discourse::InvalidParameters.new(:limit)
      end
    if params[:limit].present?
      options[:limit] = params[:limit].to_i
      raise Discourse::InvalidParameters.new(:limit) if options[:limit] <= 0
        bookmark_list = UserBookmarkList.new(user: user, guardian: guardian, params: params)
          params: {
            per_page: USER_MENU_LIST_LIMIT - reminder_notifications.size,
          },
  def initialize(user:, guardian:, params:)
    @params = params
    @params.merge!(per_page: PER_PAGE) if params[:per_page].blank?
    @params[:per_page] = PER_PAGE if @params[:per_page] > PER_PAGE
    query = BookmarkQuery.new(user: @user, guardian: @guardian, params: @params)
    @has_more = (@params[:page].to_i + 1) * @params[:per_page] < query.count

  def per_page
    @per_page ||= @params[:per_page]
  end
  def initialize(user:, guardian: nil, params: {})
    @params = params
    @page = @params[:page].to_i
    @limit = @params[:limit].present? ? @params[:limit].to_i : @params[:per_page]
    search_term = @params[:q]
    ts_query = search_term.present? ? Search.ts_query(term: search_term) : nil
    search_term_wildcard = search_term.present? ? "%#{search_term}%" : nil
          if search_term.present?
    results = results.offset(@page * @params[:per_page]) if @page.positive?
    results = results.limit(@limit).to_a
    limit = (params[:limit] || 50).to_i.clamp(1, 50)
  let(:params) { {} }
  def bookmark_query(user: nil, params: nil)
    BookmarkQuery.new(user: user || self.user, params: params || self.params)
    context "when q param is provided" do
        bookmarks = bookmark_query(params: { q: "check" }).list_all
        bookmarks = bookmark_query(params: { q: "content" }).list_all
        bookmarks = bookmark_query(params: { q: "bugfix" }).list_all
          bookmarks = bookmark_query(params: { q: "bookmarkk" }).list_all
    context "when the limit param is provided" do
      let(:params) { { limit: 1 } }
        expect(bookmark_query.list_all.count).to eq(1)
  let(:params) { {} }
  let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) }
    let(:params) { { per_page: 1000 } }

    it "does not exceed PAGE_SIZE if limit parameter is more than PAGE_SIZE" do
      large_limit = DirectoryItemsController::PAGE_SIZE + 10
      get "/directory_items.json", params: { period: "all", limit: large_limit }
      expect(response.status).to eq(200)
      json = response.parsed_body

      expect(json["directory_items"].length).to eq(DirectoryItemsController::PAGE_SIZE)
    end

    it "handles invalid limit parameters gracefully" do
      get "/directory_items.json", params: { period: "all", limit: "invalid_limit" }
      expect(response.status).to eq(200)
      json = response.parsed_body

      expect(json["directory_items"]).not_to be_empty
    end
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      user = sign_in(Fabricate(:user))
      it "returns error 400 for negative limit" do
        get "/tags/filter/search.json", params: { q: "", limit: -1 }

        expect(response.status).to eq(400)
        expect(response.parsed_body["errors"].first).to eq(
          I18n.t("invalid_params", message: "limit"),
        )
      end

      it "returns error 400 for suspicious limit" do
        get "/tags/filter/search.json", params: { q: "", limit: "1; SELECT 1" }

        expect(response.status).to eq(400)
        expect(response.parsed_body["errors"].first).to eq(
          I18n.t("invalid_params", message: "limit"),
        )
  fab!(:user1) { Fabricate(:user) }
    context "with limit" do
      it "returns an error if value is invalid" do
        get "/u/search/users.json", params: { limit: "-1" }
        expect(response.status).to eq(400)
      end
      bookmark_list = UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: {})

Fixed code:
  INDEX_LIMIT = 50

    limit = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
  REPORTS_LIMIT = 50

    limit = fetch_limit_from_params(params: report_params, default: nil, max: REPORTS_LIMIT)
  INDEX_LIMIT = 200

    page_size = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)

  def fetch_limit_from_params(params: self.params, default:, max:)
    raise "default limit cannot be greater than max limit" if default.present? && default > max

    if params.has_key?(:limit)
      limit =
        begin
          Integer(params[:limit])
        rescue ArgumentError
          raise Discourse::InvalidParameters.new(:limit)
        end

      raise Discourse::InvalidParameters.new(:limit) if limit < 0 || limit > max
      limit
    else
      default
    end
  end
    limit = fetch_limit_from_params(default: PAGE_SIZE, max: PAGE_SIZE)
  INDEX_LIMIT = 50

    stream =
      Draft.stream(
        user: current_user,
        offset: params[:offset],
        limit: fetch_limit_from_params(default: nil, max: INDEX_LIMIT),
      )
  MEMBERS_LIMIT = 1_000

    limit = fetch_limit_from_params(default: 50, max: MEMBERS_LIMIT)
  INDEX_LIMIT = 50

      limit = fetch_limit_from_params(default: 15, max: INDEX_LIMIT)
  INDEX_LIMIT = 200

    page_size = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
  DELETED_POSTS_MAX_LIMIT = 100

    limit = fetch_limit_from_params(default: 60, max: DELETED_POSTS_MAX_LIMIT)
    matches =
      matches.order("name").limit(
        fetch_limit_from_params(default: 5, max: SiteSetting.max_tag_search_results),
      )
    if limit = fetch_limit_from_params(default: nil, max: SiteSetting.max_tag_search_results)
      filter_params[:limit] = limit
  SEARCH_USERS_LIMIT = 50


    if limit = fetch_limit_from_params(default: nil, max: SEARCH_USERS_LIMIT)
      options[:limit] = limit

  BOOKMARKS_LIMIT = 20

        bookmark_list =
          UserBookmarkList.new(
            user: user,
            guardian: guardian,
            search_term: params[:q],
            page: params[:page],
            per_page: fetch_limit_from_params(default: nil, max: BOOKMARKS_LIMIT),
          )

          per_page: USER_MENU_LIST_LIMIT - reminder_notifications.size,

  def initialize(user:, guardian:, search_term: nil, per_page: nil, page: 0)
    @per_page = per_page || PER_PAGE
    @per_page = PER_PAGE if @per_page > PER_PAGE

    @search_term = search_term
    @page = page.to_i
    query =
      BookmarkQuery.new(
        user: @user,
        guardian: @guardian,
        search_term: @search_term,
        page: @page,
        per_page: @per_page,
      )

    @has_more = (@page.to_i + 1) * @per_page < query.count
  def initialize(user:, guardian: nil, search_term: nil, page: nil, per_page: nil)
    @search_term = search_term
    @page = page ? page.to_i : 0
    @per_page = per_page ? per_page.to_i : 20
    ts_query = @search_term.present? ? Search.ts_query(term: @search_term) : nil
    search_term_wildcard = @search_term.present? ? "%#{@search_term}%" : nil
          if @search_term.present?
    results = results.offset(@page * @per_page) if @page.positive?
    results = results.limit(@per_page).to_a

  INDEX_LIMIT = 50

    limit = fetch_limit_from_params(default: INDEX_LIMIT, max: INDEX_LIMIT)
  def bookmark_query(user: nil, search_term: nil, per_page: nil)
    BookmarkQuery.new(user: user || self.user, search_term:, per_page:)
    context "when search_term is provided" do
        bookmarks = bookmark_query(search_term: "check").list_all
        bookmarks = bookmark_query(search_term: "content").list_all
        bookmarks = bookmark_query(search_term: "bugfix").list_all
          bookmarks = bookmark_query(search_term: "bookmarkk").list_all
    context "when the per_page is provided" do
        expect(bookmark_query(per_page: 1).list_all.count).to eq(1)
  let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user)) }



      list = UserBookmarkList.new(user: user, guardian: Guardian.new(user), per_page: 1000)

Dir[Rails.root.join("spec/requests/examples/*.rb")].each { |f| require f }
      describe "when limit params is invalid" do
        include_examples "invalid limit params",
                         "/admin/api/keys.json",
                         described_class::INDEX_LIMIT
      end

        context "when limit param is invalid" do
          include_examples "invalid limit params",
                           "/admin/reports/topics.json",
                           described_class::REPORTS_LIMIT
        end


        context "when limit param is invalid" do
          include_examples "invalid limit params",
                           "/admin/reports/topics.json",
                           described_class::REPORTS_LIMIT
        end

      describe "when limit params is invalid" do
        include_examples "invalid limit params",
                         "/admin/logs/staff_action_logs.json",
                         described_class::INDEX_LIMIT
      end
    include_examples "invalid limit params", "/directory_items.json", described_class::PAGE_SIZE
  fab!(:user) { Fabricate(:user) }

    describe "when limit params is invalid" do
      before { sign_in(user) }

      include_examples "invalid limit params", "/drafts.json", described_class::INDEX_LIMIT
    end

      sign_in(user)
      sign_in(user)
      sign_in(user)
      sign_in(user)
      sign_in(user)
      sign_in(user)
      sign_in(user)
      sign_in(user)
      sign_in(user)
# frozen_string_literal: true

RSpec.shared_examples "invalid limit params" do |endpoint, max_limit, extra_params|
  let(:params) { extra_params&.dig(:params) || {} }

  it "returns 400 response code when limit params is negative" do
    get endpoint, params: { limit: -1 }.merge(params)

    expect(response.status).to eq(400)
  end

  it "returns 400 response code when limit params is suspicious" do
    get endpoint, params: { limit: "1; DROP TABLE users" }.merge(params)

    expect(response.status).to eq(400)
  end

  it "returns 400 response code when limit params exceeds the max limit" do
    get endpoint, params: { limit: max_limit + 1 }.merge(params)

    expect(response.status).to eq(400)
  end
end
        describe "when limit params is invalid" do
          include_examples "invalid limit params",
                           "/notifications.json",
                           described_class::INDEX_LIMIT,
                           params: {
                             recent: true,
                           }
        end

  describe "index" do
    describe "when limit params is invalid" do
      include_examples "invalid limit params",
                       "/post_action_users.json",
                       described_class::INDEX_LIMIT
    end
  end

      describe "when limit params is invalid" do
        before { sign_in(moderator) }

        include_examples "invalid limit params",
                         "/posts/system/deleted.json",
                         described_class::DELETED_POSTS_MAX_LIMIT
      end

    describe "when limit params is invalid" do
      include_examples "invalid limit params",
                       "/tag_groups/filter/search.json",
                       SiteSetting.max_tag_search_results
    end

      describe "when limit params is invalid" do
        include_examples "invalid limit params",
                         "/tags/filter/search.json",
                         SiteSetting.max_tag_search_results
  fab!(:user1) { Fabricate(:user, username: "someusername") }
    describe "when limit params is invalid" do
      include_examples "invalid limit params",
                       "/u/search/users.json",
                       described_class::SEARCH_USERS_LIMIT

    describe "when limit params is invalid" do
      before { sign_in(user1) }

      include_examples "invalid limit params",
                       "/u/someusername/bookmarks.json",
                       described_class::BOOKMARKS_LIMIT
    end
      bookmark_list = UserBookmarkList.new(user: user, guardian: Guardian.new(user))