From 5967391be00fbbbab8a59026114b419a8859fba9 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Feb 28 2022 22:49:19 +0000
Subject: [PATCH 1/3] return 400 codes when client fails to send a full request


Fixes: https://pagure.io/koji/issue/3268

---

diff --git a/hub/kojixmlrpc.py b/hub/kojixmlrpc.py
index 98d4c26..53c0c85 100644
--- a/hub/kojixmlrpc.py
+++ b/hub/kojixmlrpc.py
@@ -38,9 +38,18 @@ import koji.policy
 import koji.util
 from koji.context import context
 # import xmlrpclib functions from koji to use tweaked Marshaller
+from koji.server import ServerError
 from koji.xmlrpcplus import ExtendedMarshaller, Fault, dumps, getparser
 
 
+class BadRequest(ServerError):
+    """Used to trigger an http 400 error"""
+
+
+class RequestTimeout(ServerError):
+    """Used to trigger an http 408 error"""
+
+
 class Marshaller(ExtendedMarshaller):
 
     dispatch = ExtendedMarshaller.dispatch.copy()
@@ -213,7 +222,16 @@ class ModXMLRPCRequestHandler(object):
         rlen = 0
         maxlen = opts.get('MaxRequestLength', None)
         while True:
-            chunk = stream.read(8192)
+            try:
+                chunk = stream.read(8192)
+            except OSError as e:
+                str_e = str(e)
+                if 'timeout' in str_e:
+                    self.logger.exception("Timed out reading input stream")
+                    raise RequestTimeout(str_e)
+                else:
+                    self.logger.exception("Error reading input stream")
+                    raise BadRequest(str_e)
             if not chunk:
                 break
             rlen += len(chunk)
@@ -255,6 +273,9 @@ class ModXMLRPCRequestHandler(object):
             # wrap response in a singleton tuple
             response = (response,)
             response = dumps(response, methodresponse=1, marshaller=Marshaller)
+        except ServerError:
+            raise
+            # these are handled higher up
         except Fault as fault:
             self.traceback = True
             response = dumps(fault, marshaller=Marshaller)
@@ -382,6 +403,18 @@ def offline_reply(start_response, msg=None):
     return [response]
 
 
+def error_reply(start_response, status, response, extra_headers=None):
+        response = response.encode()
+        headers = [
+            ('Content-Length', str(len(response))),
+            ('Content-Type', "text/plain"),
+        ]
+        if extra_headers:
+            headers.extend(extra_headers)
+        start_response(status, headers)
+        return [response]
+
+
 def load_config(environ):
     """Load configuration options
 
@@ -744,18 +777,12 @@ def application(environ, start_response):
                 firstcall = False
     # XMLRPC uses POST only. Reject anything else
     if environ['REQUEST_METHOD'] != 'POST':
-        headers = [
+        extra_headers = [
             ('Allow', 'POST'),
         ]
-        start_response('405 Method Not Allowed', headers)
         response = "Method Not Allowed\n" \
-                   "This is an XML-RPC server. Only POST requests are accepted."
-        response = response.encode()
-        headers = [
-            ('Content-Length', str(len(response))),
-            ('Content-Type', "text/plain"),
-        ]
-        return [response]
+                   "This is an XML-RPC server. Only POST requests are accepted.\n"
+        return error_reply(start_response, '405 Method Not Allowed', response, extra_headers)
     if opts.get('ServerOffline'):
         return offline_reply(start_response, msg=opts.get("OfflineMessage", None))
     # XXX check request length
@@ -776,10 +803,15 @@ def application(environ, start_response):
             except Exception:
                 return offline_reply(start_response, msg="database outage")
             h = ModXMLRPCRequestHandler(registry)
-            if environ.get('CONTENT_TYPE') == 'application/octet-stream':
-                response = h._wrap_handler(h.handle_upload, environ)
-            else:
-                response = h._wrap_handler(h.handle_rpc, environ)
+            try:
+                if environ.get('CONTENT_TYPE') == 'application/octet-stream':
+                    response = h._wrap_handler(h.handle_upload, environ)
+                else:
+                    response = h._wrap_handler(h.handle_rpc, environ)
+            except BadRequest as e:
+                return error_reply(start_response, '400 Bad Request', str(e) + '\n')
+            except RequestTimeout as e:
+                return error_reply(start_response, '408 Request Timeout', str(e) + '\n')
             response = response.encode()
             headers = [
                 ('Content-Length', str(len(response))),

From f51c5025d4d2ea775f924a226d85b7799a84a426 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Feb 28 2022 23:51:24 +0000
Subject: [PATCH 2/3] fix indent


---

diff --git a/hub/kojixmlrpc.py b/hub/kojixmlrpc.py
index 53c0c85..07a537a 100644
--- a/hub/kojixmlrpc.py
+++ b/hub/kojixmlrpc.py
@@ -404,15 +404,15 @@ def offline_reply(start_response, msg=None):
 
 
 def error_reply(start_response, status, response, extra_headers=None):
-        response = response.encode()
-        headers = [
-            ('Content-Length', str(len(response))),
-            ('Content-Type', "text/plain"),
-        ]
-        if extra_headers:
-            headers.extend(extra_headers)
-        start_response(status, headers)
-        return [response]
+    response = response.encode()
+    headers = [
+        ('Content-Length', str(len(response))),
+        ('Content-Type', "text/plain"),
+    ]
+    if extra_headers:
+        headers.extend(extra_headers)
+    start_response(status, headers)
+    return [response]
 
 
 def load_config(environ):

From 323bb2f86bbd6a80cae721ab8ea7aec6da800550 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 02 2022 00:23:37 +0000
Subject: [PATCH 3/3] also raise 400 errors when we can't read the client upload stream


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index 81cdbc6..7a44818 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -62,6 +62,7 @@ from koji.tasks import parse_task_params
 import koji.xmlrpcplus
 from koji.context import context
 from koji.daemon import SCM
+from koji.server import BadRequest, RequestTimeout
 from koji.util import (
     base64encode,
     decode_bytes,
@@ -15406,7 +15407,17 @@ def handle_upload(environ):
             os.ftruncate(fd, offset)
             os.lseek(fd, offset, 0)
         while True:
-            chunk = inf.read(65536)
+            try:
+                chunk = inf.read(65536)
+            except OSError as e:
+                str_e = str(e)
+                logger.error(f"Error reading upload. Offset {offset}+{size}, path {fn}")
+                if 'timeout' in str_e:
+                    logger.exception("Timed out reading input stream")
+                    raise RequestTimeout(str_e)
+                else:
+                    logger.exception("Error reading input stream")
+                    raise BadRequest(str_e)
             if not chunk:
                 break
             size += len(chunk)
diff --git a/hub/kojixmlrpc.py b/hub/kojixmlrpc.py
index 07a537a..f7aaf3d 100644
--- a/hub/kojixmlrpc.py
+++ b/hub/kojixmlrpc.py
@@ -38,18 +38,10 @@ import koji.policy
 import koji.util
 from koji.context import context
 # import xmlrpclib functions from koji to use tweaked Marshaller
-from koji.server import ServerError
+from koji.server import ServerError, BadRequest, RequestTimeout
 from koji.xmlrpcplus import ExtendedMarshaller, Fault, dumps, getparser
 
 
-class BadRequest(ServerError):
-    """Used to trigger an http 400 error"""
-
-
-class RequestTimeout(ServerError):
-    """Used to trigger an http 408 error"""
-
-
 class Marshaller(ExtendedMarshaller):
 
     dispatch = ExtendedMarshaller.dispatch.copy()
diff --git a/koji/server.py b/koji/server.py
index 06291d5..da5b2ee 100644
--- a/koji/server.py
+++ b/koji/server.py
@@ -26,3 +26,11 @@ class ServerError(Exception):
 
 class ServerRedirect(ServerError):
     """Used to handle redirects"""
+
+
+class BadRequest(ServerError):
+    """Used to trigger an http 400 error"""
+
+
+class RequestTimeout(ServerError):
+    """Used to trigger an http 408 error"""

