Skip to content

Commit

Permalink
Fix date parsing with complex types
Browse files Browse the repository at this point in the history
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 flagging a date hash
as invalid if any of the values don't respond to `to_i`. We're always
expecting these values to be numeric so this feels like a valid
assumption to make.

The exception is raised in the constructor currently, where we call
`transform_values(&:to_i)`, which blows up if a value is, for example,
an array.

Instead we'll extend the `any_non_positive_values?` method to check that
the values both respond to `to_i` and are positive.
  • Loading branch information
jackbot committed Dec 23, 2024
1 parent fafe486 commit 19e0511
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 5 deletions.
9 changes: 4 additions & 5 deletions app/parsers/date_hash_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ def initialize(date_hash)
@date_hash = date_hash
.slice(:day, :month, :year)
.compact_blank
.transform_values(&:to_i)
end

def parse
Expand All @@ -43,19 +42,19 @@ def day_without_month?
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?)
!date_hash.values.all? { |value| value.respond_to?(:to_i) && value.to_i.positive? }
end

def day
date_hash.fetch(:day, 1)
date_hash.fetch(:day, 1).to_i
end

def month
date_hash.fetch(:month, 1)
date_hash.fetch(:month, 1).to_i
end

def year
value = date_hash.fetch(:year, Date.current.year)
value = date_hash.fetch(:year, Date.current.year).to_i

case value
when 0..MILLENNIUM_CUTOFF
Expand Down
6 changes: 6 additions & 0 deletions spec/parsers/date_hash_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@
it { is_expected.to be_nil }
end

context "with hash with a complex type value" do
let(:date_hash) { { day: [1], month: 12, year: 2023 } }

it { is_expected.to be_nil }
end

context "with hash with non-numeric year only" do
let(:date_hash) { { year: "baz" } }

Expand Down

0 comments on commit 19e0511

Please sign in to comment.