From fa847f2b3aa2f4fc14fa3c77bf51ba8266a142b4 Mon Sep 17 00:00:00 2001 From: Jack Weeden Date: Mon, 23 Dec 2024 11:09:28 +0000 Subject: [PATCH] Fix date parsing with complex types The `DateHashParser` class is responsible for taking a hash containing keys of some combination of `day`, `month` and `year` and trying to return a full `Date` object. It is flexible enough to handle hashes where certain keys are missing, e.g. if the `year` key isn't present, it sets the value to be the current year. We've seen quite a few exceptions coming through from this class where it looks like somebody has been messing about with the query parameters in the URL, which are combined into a hash and sent through to this class for parsing. A (truncated) example request is: ``` /search/all?keywords=the&public_timestamp[from][day][]=17&public_timestamp[from][month]=7&public_timestamp[from][year]=1967 ``` Here we can see that `public_timestamp[from][day]` is an array value rather than a numeric value like `month` and `year`. This doesn't appear to be a legitimate bug caused by the frontend. Looking through the Sentry logs it's pretty clear somebody is manipulating the URL for some reason. So it makes sense to fix the bug in this class by parsing the input hash, trying to convert any non-integer values into an integer (without raising an exception), then selecting only the positive values. That way we can do as much as we can to coerce the input into a group of valid values, then carry on building the date as we did before. --- app/parsers/date_hash_parser.rb | 12 +++--------- spec/parsers/date_hash_parser_spec.rb | 18 ++++++++++++------ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/parsers/date_hash_parser.rb b/app/parsers/date_hash_parser.rb index 3cf2c14bb..5258bdcb5 100644 --- a/app/parsers/date_hash_parser.rb +++ b/app/parsers/date_hash_parser.rb @@ -20,12 +20,12 @@ class DateHashParser def initialize(date_hash) @date_hash = date_hash .slice(:day, :month, :year) - .compact_blank - .transform_values(&:to_i) + .transform_values { |value| Integer(value, exception: false) } + .select { |_, value| value&.positive? } end def parse - return if date_hash.empty? || any_non_positive_values? || day_without_month? + return if date_hash.empty? || day_without_month? Date.new(year, month, day) rescue Date::Error @@ -40,12 +40,6 @@ def day_without_month? date_hash.key?(:day) && !date_hash.key?(:month) end - def any_non_positive_values? - # If the user enters a zero or negative value, or a string that `#to_i` converts to zero, we - # consider the whole date invalid - !date_hash.values.all?(&:positive?) - end - def day date_hash.fetch(:day, 1) end diff --git a/spec/parsers/date_hash_parser_spec.rb b/spec/parsers/date_hash_parser_spec.rb index 43526a924..cf526d1bf 100644 --- a/spec/parsers/date_hash_parser_spec.rb +++ b/spec/parsers/date_hash_parser_spec.rb @@ -60,6 +60,18 @@ it { is_expected.to eq Date.new(2024, 12, 1) } end + + context "with hash with a complex type value" do + let(:date_hash) { { day: [1], month: 12, year: 2024 } } + + it { is_expected.to eq Date.new(2024, 12, 1) } + end + + context "with hash with any non-numeric values" do + let(:date_hash) { { day: 13, month: 12, year: "baz" } } + + it { is_expected.to eq Date.new(2024, 12, 13) } + end end describe "invalid input" do @@ -99,12 +111,6 @@ it { is_expected.to be_nil } end - context "with hash with any non-numeric values (even if others are good)" do - let(:date_hash) { { day: 13, month: 12, year: "baz" } } - - it { is_expected.to be_nil } - end - context "with hash with non-numeric year only" do let(:date_hash) { { year: "baz" } }