From 9361d9a58721d2dce377efe00d2f400ae4953569 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Mon, 20 Jun 2022 16:57:46 +1000
Subject: [PATCH] FIX: stop logging blank and invalid CSP reports (#17144)

Certain rogue bots such as Yandex may send across invalid CSP reports
when CSP report collection is enabled.

This ensures that invalid reports will not cause log floods and simply
returns a 422 error.

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
---
 app/controllers/csp_reports_controller.rb    | 49 +++++++++++++-------
 spec/requests/csp_reports_controller_spec.rb | 12 +++++
 2 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/app/controllers/csp_reports_controller.rb b/app/controllers/csp_reports_controller.rb
index 30824e3536d..d18fd7d1c2c 100644
--- a/app/controllers/csp_reports_controller.rb
+++ b/app/controllers/csp_reports_controller.rb
@@ -5,28 +5,43 @@ class CspReportsController < ApplicationController
   def create
     raise Discourse::NotFound unless report_collection_enabled?
 
-    Logster.add_to_env(request.env, 'CSP Report', report)
-    Rails.logger.warn("CSP Violation: '#{report['blocked-uri']}' \n\n#{report['script-sample']}")
+    report = parse_report
 
-    head :ok
+    if report.blank?
+      render_json_error("empty CSP report", status: 422)
+    else
+      Logster.add_to_env(request.env, 'CSP Report', report)
+      Rails.logger.warn("CSP Violation: '#{report['blocked-uri']}' \n\n#{report['script-sample']}")
+
+      head :ok
+    end
+
+  rescue JSON::ParserError
+    render_json_error("invalid CSP report", status: 422)
   end
 
   private
 
-  def report
-    @report ||= JSON.parse(request.body.read)['csp-report'].slice(
-      'blocked-uri',
-      'disposition',
-      'document-uri',
-      'effective-directive',
-      'original-policy',
-      'referrer',
-      'script-sample',
-      'status-code',
-      'violated-directive',
-      'line-number',
-      'source-file'
-    )
+  def parse_report
+    obj = JSON.parse(request.body.read)
+    if Hash === obj
+      obj = obj['csp-report']
+      if Hash === obj
+        obj.slice(
+          'blocked-uri',
+          'disposition',
+          'document-uri',
+          'effective-directive',
+          'original-policy',
+          'referrer',
+          'script-sample',
+          'status-code',
+          'violated-directive',
+          'line-number',
+          'source-file'
+        )
+      end
+    end
   end
 
   def report_collection_enabled?
diff --git a/spec/requests/csp_reports_controller_spec.rb b/spec/requests/csp_reports_controller_spec.rb
index 87ecbcd6daa..911c30f71aa 100644
--- a/spec/requests/csp_reports_controller_spec.rb
+++ b/spec/requests/csp_reports_controller_spec.rb
@@ -32,6 +32,18 @@ describe CspReportsController do
       }.to_json, headers: { "Content-Type": "application/csp-report" }
     end
 
+    it 'returns an error for invalid reports' do
+      SiteSetting.content_security_policy_collect_reports = true
+
+      post '/csp_reports', params: "[ not-json", headers: { "Content-Type": "application/csp-report" }
+
+      expect(response.status).to eq(422)
+
+      post '/csp_reports', params: ["yes json"].to_json, headers: { "Content-Type": "application/csp-report" }
+
+      expect(response.status).to eq(422)
+    end
+
     it 'is enabled by SiteSetting' do
       SiteSetting.content_security_policy = false
       SiteSetting.content_security_policy_report_only = false