Improve error tracking error handling

Instead of failing with 500 error when payload is invalid or
activerecord is invalid, we handle those scenarios and return a 400
error response to the client.

Changelog: changed

Signed-off-by: Dmytro Zaporozhets <dzaporozhets@gitlab.com>

app/validators/json_schemas/error_tracking_event_payload.json

@@ -1,7 +1,7 @@
 {
   "description": "Error tracking event payload",
   "type": "object",
-  "required": [],
+  "required": ["exception"],
   "properties": {
     "environment": {
       "type": "string"
@@ -14,7 +14,7 @@
     },
     "exception": {
       "type": "object",
-      "required": [],
+      "required": ["values"],
       "properties": {
         "values": {
           "type": "array",

lib/api/error_tracking/collector.rb

@@ -12,6 +12,10 @@ class ErrorTracking::Collector < ::API::Base
     content_type :txt, 'text/plain'
     default_format :envelope
 
+    rescue_from ActiveRecord::RecordInvalid do |e|
+      render_api_error!(e.message, 400)
+    end
+
     before do
       not_found!('Project') unless project
       not_found! unless feature_enabled?
@@ -50,6 +54,12 @@ def extract_public_key
           bad_request!('Failed to parse sentry request')
         end
       end
+
+      def validate_payload(payload)
+        unless ::ErrorTracking::Collector::PayloadValidator.new.valid?(payload)
+          bad_request!('Unsupported sentry payload')
+        end
+      end
     end
 
     desc 'Submit error tracking event to the project as envelope' do
@@ -88,6 +98,8 @@ def extract_public_key
       # We don't have use for transaction request yet,
       # so we record only event one.
       if type == 'event'
+        validate_payload(parsed_request[:event])
+
         ::ErrorTracking::CollectErrorService
           .new(project, nil, event: parsed_request[:event])
           .execute
@@ -125,6 +137,8 @@ def extract_public_key
         bad_request!('Failed to parse sentry request')
       end
 
+      validate_payload(parsed_body)
+
       ::ErrorTracking::CollectErrorService
         .new(project, nil, event: parsed_body)
         .execute

lib/error_tracking/collector/payload_validator.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module ErrorTracking
+  module Collector
+    class PayloadValidator
+      PAYLOAD_SCHEMA_PATH = Rails.root.join('app', 'validators', 'json_schemas', 'error_tracking_event_payload.json').to_s
+
+      def valid?(payload)
+        JSONSchemer.schema(Pathname.new(PAYLOAD_SCHEMA_PATH)).valid?(payload)
+      end
+    end
+  end
+end

spec/lib/error_tracking/collector/payload_validator_spec.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ErrorTracking::Collector::PayloadValidator do
+  describe '#valid?' do
+    RSpec.shared_examples 'valid payload' do
+      it 'returns true' do
+        expect(described_class.new.valid?(payload)).to be_truthy
+      end
+    end
+
+    RSpec.shared_examples 'invalid payload' do
+      it 'returns false' do
+        expect(described_class.new.valid?(payload)).to be_falsey
+      end
+    end
+
+    context 'ruby payload' do
+      let(:payload) { Gitlab::Json.parse(fixture_file('error_tracking/parsed_event.json')) }
+
+      it_behaves_like 'valid payload'
+    end
+
+    context 'python payload' do
+      let(:payload) { Gitlab::Json.parse(fixture_file('error_tracking/python_event.json')) }
+
+      it_behaves_like 'valid payload'
+    end
+
+    context 'browser payload' do
+      let(:payload) { Gitlab::Json.parse(fixture_file('error_tracking/browser_event.json')) }
+
+      it_behaves_like 'valid payload'
+    end
+
+    context 'empty payload' do
+      let(:payload) { '' }
+
+      it_behaves_like 'invalid payload'
+    end
+
+    context 'invalid payload' do
+      let(:payload) { { 'foo' => 'bar' } }
+
+      it_behaves_like 'invalid payload'
+    end
+  end
+end

spec/requests/api/error_tracking/collector_spec.rb

@@ -136,6 +136,21 @@
       it_behaves_like 'bad request'
     end
 
+    context 'body with string instead of json' do
+      let(:params) { '"********"' }
+
+      it_behaves_like 'bad request'
+    end
+
+    context 'collector fails with validation error' do
+      before do
+        allow(::ErrorTracking::CollectErrorService)
+          .to receive(:new).and_raise(ActiveRecord::RecordInvalid)
+      end
+
+      it_behaves_like 'bad request'
+    end
+
     context 'gzip body' do
       let(:headers) do
         {