DEV: Strictly filter tag search limit parameter input (#21524)

### What is the problem?

It is possible to pass an arbitrary value to the limit parameter in `TagsController#search`, and have it flow through `DiscourseTagging.filter_allowed_tags` where it will raise an error deep in the database driver. MiniSql ensures there's no injection happening, but that ultimately results in an invalid query.

### How does this fix it?

This change checks more strictly that the parameter can be cleanly converted to an integer by replacing the loose `#to_i` conversion semantics with the stronger `Kernel#Integer` ones.

**Example:**

```ruby
"1; SELECT 1".to_i
#=> 1

Integer("1; SELECT 1")
#=> ArgumentError
```

As part of the change, I also went ahead to disallow a limit of "0", as that doesn't seem to be a useful option. Previously only negative limits were disallowed.
This commit is contained in:
Ted Johansson 2023-05-12 16:49:14 +08:00 committed by GitHub
parent 59867cc091
commit 07f87ff7a8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 16 additions and 3 deletions

View File

@ -247,13 +247,17 @@ class TagsController < ::ApplicationController
filter_params = {
for_input: params[:filterForInput],
selected_tags: params[:selected_tags],
limit: params[:limit],
exclude_synonyms: params[:excludeSynonyms],
exclude_has_synonyms: params[:excludeHasSynonyms],
}
if filter_params[:limit] && filter_params[:limit].to_i < 0
raise Discourse::InvalidParameters.new(:limit)
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
end
filter_params[:category] = Category.find_by_id(params[:categoryId]) if params[:categoryId]

View File

@ -1112,6 +1112,15 @@ RSpec.describe TagsController do
)
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"),
)
end
it "includes required tag group information" do
tag1 = Fabricate(:tag)
tag2 = Fabricate(:tag)