From dac0a6016dd20ad3dcb282d03fb7b2fc4504ec2f Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Mon, 2 Dec 2024 11:35:23 -0500 Subject: [PATCH 1/3] regression tests for existing kinds of bad params currently handled okay But in past versions of bl_range_limit i know from my own app resulted in uncaught exception 500s --- spec/requests/bad_param_requests_spec.rb | 46 ++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 spec/requests/bad_param_requests_spec.rb diff --git a/spec/requests/bad_param_requests_spec.rb b/spec/requests/bad_param_requests_spec.rb new file mode 100644 index 0000000..5e043a3 --- /dev/null +++ b/spec/requests/bad_param_requests_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' + +describe CatalogController, type: :request do + let(:range_facet_field) { "pub_date_si" } + + let(:parsed_body) { Nokogiri::HTML(response.body) } + + describe "bad params should not produce uncaught exception when" do + it "bad root range" do + get "/catalog?range=bad" + + expect(response.code).to eq("200") + expect(parsed_body.css("span.applied-filter")).not_to be_present + end + + it "facet params are ill structured" do + get "/catalog?#{ {"f" => { range_facet_field => [{"=Library&q="=>""}] } }.to_param }" + + expect(response.code).to eq("200") + expect(parsed_body.css("span.applied-filter")).not_to be_present + end + + it "newline in range facet does not interupt facet" do + get "/catalog?#{ {"range"=>{ range_facet_field => {"begin"=>"1588\n", "end"=>"2020\n"}}}.to_param }" + + expect(response.code).to eq("200") + expect(parsed_body.css("span.applied-filter")).to be_present + expect(parsed_body.css("span.applied-filter").collect(&:text)).to include(/1588.*to.*2020/) + end + + it "weird attack in range value is ignored" do + param_hash = {"range"=>{"year_facet_isim"=>{"begin"=>"1989',(;))#- --", "end"=>"1989',(;))#- --"}}} + get "/catalog?#{ param_hash.to_param }" + + expect(response.code).to eq("200") + expect(parsed_body.css("span.applied-filter")).not_to be_present + end + + it "empty range param is ignored" do + get "/catalog?#{ { "range" => { "year_facet_isim" => nil } }.to_param }" + + expect(response.code).to eq("200") + expect(parsed_body.css("span.applied-filter")).not_to be_present + end + end +end From 35646d74c257351d69b3419ce118167cdd05278b Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Mon, 2 Dec 2024 12:36:51 -0500 Subject: [PATCH 2/3] Test that out of bounds ranges do not result in error One test applying to formerly without tests #294. In process, I realize that the values displayed in the "constraint" aren't clamped, so that'd be an area of improvement -- but I'm not too worried about it, I don't personally have a use case for it. The main thing is ensuring this bad data doesn't result in an uncaught exception, which this regression test now does. --- spec/requests/bad_param_requests_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/requests/bad_param_requests_spec.rb b/spec/requests/bad_param_requests_spec.rb index 5e043a3..88fdd71 100644 --- a/spec/requests/bad_param_requests_spec.rb +++ b/spec/requests/bad_param_requests_spec.rb @@ -42,5 +42,20 @@ expect(response.code).to eq("200") expect(parsed_body.css("span.applied-filter")).not_to be_present end + + describe "out of bounds range config" do + let(:max) { BlacklightRangeLimit.default_range_config[:range_config][:max_value] } + let(:min) { BlacklightRangeLimit.default_range_config[:range_config][:min_value] } + + let(:too_high) { max.abs * 2 } + let(:too_low) { min.abs * -2 } + + it "does not error" do + get "/catalog?#{ {"range"=>{ range_facet_field => {"begin"=> too_low, "end"=> too_high }}}.to_param }" + + expect(response.code).to eq("200") + expect(parsed_body.css("span.applied-filter")).to be_present + end + end end end From 288c657534c5ff007225d91ebe7b3748913660eb Mon Sep 17 00:00:00 2001 From: Jonathan Rochkind Date: Mon, 2 Dec 2024 15:39:33 -0500 Subject: [PATCH 3/3] Correct bad non-hash range param for BL 7.x This branch correction is not necessary in BL 8, not sure why --- app/presenters/blacklight_range_limit/filter_field.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/presenters/blacklight_range_limit/filter_field.rb b/app/presenters/blacklight_range_limit/filter_field.rb index 48f7c42..a437b65 100644 --- a/app/presenters/blacklight_range_limit/filter_field.rb +++ b/app/presenters/blacklight_range_limit/filter_field.rb @@ -50,7 +50,13 @@ def remove(item) def values(except: []) params = search_state.params param_key = filters_key - range = if params.dig(param_key, config.key).is_a? Range + range = if !params.try(:dig, param_key).respond_to?(:dig) + # bad data, not a hash at all, correct it. Yes, it's bad form to mutate + # params here, but we found no better solution -- this only necessary in BL + # prior to 8.x, not sure why, but this branch can be omitted in BL 8. + params.delete(param_key) + nil + elsif params.dig(param_key, config.key).is_a? Range params.dig(param_key, config.key) elsif params.dig(param_key, config.key).is_a? Hash b_bound = params.dig(param_key, config.key, :begin).presence