V3: valid timestamps → subset of ISO 8601

Timestamps are typically used when querying audit_events, e.g. `GET /v3/audit_events?created_at=2020-06-30T23:49:04Z` We enforce an opinionated subset of ISO 8601: - must match `YYYY-MM-DDThh:mm:ssZ` - no subseconds - no local timezones We updated our tests to accommodate the "no local timezones" stricture (we used `Time.now.utc` to force UTC). We did a simple regex to guard against bad timestamps. It is admittedly ugly, but late is the hour. [#173542711] Authored-by: Brian Cunnie <[email protected]>
univ0298 · Jul 1, 2020 · db63d9b · db63d9b
1 parent 2e946eb
commit db63d9b
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 19 deletions.
diff --git a/app/messages/events_list_message.rb b/app/messages/events_list_message.rb
@@ -27,9 +27,11 @@ def validate(record)
             timestamp = record.created_at.values[0]
           end
           begin
+            raise ArgumentError.new('invalid date') unless timestamp =~ /\A\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z\Z/
+
             Time.iso8601(timestamp)
           rescue
-            record.errors[:created_at] << "Invalid timestamp format: '#{timestamp}'"
+            record.errors[:created_at] << "has an invalid timestamp format. Timestamps should be formatted as 'YYYY-MM-DDThh:mm:ssZ'"
             return
           end
         end

diff --git a/spec/request/events_spec.rb b/spec/request/events_spec.rb
@@ -359,6 +359,28 @@
           expect(last_response).to have_error_message("Invalid comparison operator: 'goat'")
         end
       end
+
+      context 'using an invalid timestamp (with fractional seconds)' do
+        let(:fractional_second_timestamp) { '2020-06-30T23:45:67.890Z' }
+        it 'returns a useful error' do
+          get "/v3/audit_events?created_at[lt]=#{fractional_second_timestamp}", nil, admin_header
+
+          expect(last_response).to have_status_code(400)
+          expect(last_response).to have_error_message(
+            "The query parameter is invalid: Created at has an invalid timestamp format. Timestamps should be formatted as 'YYYY-MM-DDThh:mm:ssZ'")
+        end
+      end
+
+      context 'using an invalid timestamp (local time zone)' do
+        let(:local_timezone_timestamp) { '2020-06-30T23:45:67-0700' }
+        it 'returns a useful error' do
+          get "/v3/audit_events?created_at[lt]=#{local_timezone_timestamp}", nil, admin_header
+
+          expect(last_response).to have_status_code(400)
+          expect(last_response).to have_error_message(
+            "The query parameter is invalid: Created at has an invalid timestamp format. Timestamps should be formatted as 'YYYY-MM-DDThh:mm:ssZ'")
+        end
+      end
     end
 
     context 'filtering by organization_guid' do

diff --git a/spec/unit/messages/events_list_message_spec.rb b/spec/unit/messages/events_list_message_spec.rb
@@ -27,7 +27,7 @@ module VCAP::CloudController
           target_guids: ['guid1', 'guid2'],
           space_guids: ['guid3', 'guid4'],
           organization_guids: ['guid5', 'guid6'],
-          created_at: { lt: Time.now.iso8601 },
+          created_at: { lt: Time.now.utc.iso8601 },
         })
         expect(message).to be_valid
       end
@@ -66,52 +66,58 @@ module VCAP::CloudController
 
         context 'validates the created_at filter' do
           it 'requires a hash or a timestamp' do
-            message = EventsListMessage.from_params({ created_at: [Time.now.iso8601] })
+            message = EventsListMessage.from_params({ created_at: [Time.now.utc.iso8601] })
             expect(message).not_to be_valid
             expect(message.errors[:created_at]).to include('comparison operator and timestamp must be specified')
           end
 
           it 'requires a valid comparison operator' do
-            message = EventsListMessage.from_params({ created_at: { garbage: Time.now.iso8601 } })
+            message = EventsListMessage.from_params({ created_at: { garbage: Time.now.utc.iso8601 } })
             expect(message).not_to be_valid
             expect(message.errors[:created_at]).to include("Invalid comparison operator: 'garbage'")
           end
 
-          it 'requires a valid comparison operator' do
-            message = EventsListMessage.from_params({ created_at: { garbage: Time.now.iso8601 } })
-            expect(message).not_to be_valid
-            expect(message.errors[:created_at]).to include("Invalid comparison operator: 'garbage'")
-          end
-
-          it 'requires a valid timestamp' do
-            message = EventsListMessage.from_params({ created_at: { gt: 123 } })
-            expect(message).not_to be_valid
-            expect(message.errors[:created_at]).to include("Invalid timestamp format: '123'")
+          context 'requires a valid timestamp' do
+            it "won't accept garbage" do
+              message = EventsListMessage.from_params({ created_at: { gt: 123 } })
+              expect(message).not_to be_valid
+              expect(message.errors[:created_at]).to include("has an invalid timestamp format. Timestamps should be formatted as 'YYYY-MM-DDThh:mm:ssZ'")
+            end
+            it "won't accept fractional seconds even though it's ISO 8601-compliant" do
+              message = EventsListMessage.from_params({ created_at: { gt: '2020-06-30T12:34:56.78Z' } })
+              expect(message).not_to be_valid
+              expect(message.errors[:created_at]).to include("has an invalid timestamp format. Timestamps should be formatted as 'YYYY-MM-DDThh:mm:ssZ'")
+            end
+            it "won't accept local time zones even though it's ISO 8601-compliant" do
+              message = EventsListMessage.from_params({ created_at: { gt: '2020-06-30T12:34:56.78-0700' } })
+              expect(message).not_to be_valid
+              expect(message.errors[:created_at]).to include("has an invalid timestamp format. Timestamps should be formatted as 'YYYY-MM-DDThh:mm:ssZ'")
+            end
           end
 
           it 'allows the lt operator' do
-            message = EventsListMessage.from_params({ created_at: { lt: Time.now.iso8601 } })
+            message = EventsListMessage.from_params({ created_at: { lt: Time.now.utc.iso8601 } })
             expect(message).to be_valid
           end
 
           it 'allows the lte operator' do
-            message = EventsListMessage.from_params({ created_at: { lte: Time.now.iso8601 } })
+            message = EventsListMessage.from_params({ created_at: { lte: Time.now.utc.iso8601 } })
             expect(message).to be_valid
           end
 
           it 'allows the gt operator' do
-            message = EventsListMessage.from_params({ created_at: { gt: Time.now.iso8601 } })
+            message = EventsListMessage.from_params({ created_at: { gt: Time.now.utc.iso8601 } })
             expect(message).to be_valid
           end
 
           it 'allows the gte operator' do
-            message = EventsListMessage.from_params({ created_at: { gte: Time.now.iso8601 } })
+            message = EventsListMessage.from_params({ created_at: { gte: Time.now.utc.iso8601 } })
             expect(message).to be_valid
           end
 
           context 'when the operator is an equals operator' do
             it 'allows the equals operator' do
-              message = EventsListMessage.from_params({ created_at: Time.now.iso8601 })
+              message = EventsListMessage.from_params({ created_at: Time.now.utc.iso8601 })
               expect(message).to be_valid
             end