DEV: Gracefully handle an array of IDs passed to Topics#show (#28631)

We're seeing a lot of log noise coming from unhandled exceptions stemming from requests to TopicsController#show where id is passed in as an array.

In the implementation of the method, we assume that if id is present it will be a string. This is because one of the routes to this action uses :id as a URL fragment, and so must be a string. However, there are other routes that go to this endpoint as well. Some of them don't have this URL fragment, so you can pass an arbitrary id query parameter.

Instead of a downstream unhandled exception, we raise a Discourse::InvalidParameters upfront.
This commit is contained in:
Ted Johansson 2024-08-29 14:22:42 +08:00 committed by GitHub
parent 0ee68b3583
commit bfad9a7170
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 10 additions and 0 deletions

View File

@ -51,6 +51,10 @@ class TopicsController < ApplicationController
end end
def show def show
if params[:id].is_a?(Array)
raise Discourse::InvalidParameters.new("Show only accepts a single ID")
end
flash["referer"] ||= request.referer[0..255] if request.referer flash["referer"] ||= request.referer[0..255] if request.referer
# TODO: We'd like to migrate the wordpress feed to another url. This keeps up backwards # TODO: We'd like to migrate the wordpress feed to another url. This keeps up backwards

View File

@ -2322,6 +2322,12 @@ RSpec.describe TopicsController do
expect(response).to redirect_to(topic.relative_url) expect(response).to redirect_to(topic.relative_url)
end end
it "does not raise an unhandled exception when receiving an array of IDs" do
get "/t/#{topic.id}/summary?id[]=a,b"
expect(response.status).to eq(400)
end
it "keeps the post_number parameter around when redirecting" do it "keeps the post_number parameter around when redirecting" do
get "/t/#{topic.slug}", params: { post_number: 42 } get "/t/#{topic.slug}", params: { post_number: 42 }
expect(response).to redirect_to(topic.relative_url + "/42") expect(response).to redirect_to(topic.relative_url + "/42")