From 170ccc9de5c09a543a60a7d9eada2e02ba9c9980 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 13 Mar 2017 13:50:16 +0000 Subject: Fix routing loop when fetching remote media When we proxy a media request to a remote server, add a query-param, which will tell the remote server to 404 if it doesn't recognise the server_name. This should fix a routing loop where the server keeps forwarding back to itself. Also improves the error handling on remote media fetches, so that we don't always return a rather obscure 502. --- synapse/api/errors.py | 59 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 7 deletions(-) (limited to 'synapse/api/errors.py') diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 921c457738..014bd60b9d 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -15,6 +15,7 @@ """Contains exceptions and error codes.""" +import json import logging logger = logging.getLogger(__name__) @@ -50,12 +51,15 @@ class Codes(object): class CodeMessageException(RuntimeError): - """An exception with integer code and message string attributes.""" + """An exception with integer code and message string attributes. - def __init__(self, code, msg): - super(CodeMessageException, self).__init__("%d: %s" % (code, msg)) + Attributes: + code (int): HTTP error code + response_code_message (str): HTTP reason phrase. None for the default. + """ + def __init__(self, code): + super(CodeMessageException, self).__init__("%d" % code) self.code = code - self.msg = msg self.response_code_message = None def error_dict(self): @@ -70,17 +74,44 @@ class SynapseError(CodeMessageException): Args: code (int): The integer error code (an HTTP response code) msg (str): The human-readable error message. - err (str): The error code e.g 'M_FORBIDDEN' + errcode (str): The synapse error code e.g 'M_FORBIDDEN' """ - super(SynapseError, self).__init__(code, msg) + super(SynapseError, self).__init__(code) + self.msg = msg self.errcode = errcode + def __str__(self): + return "%d: %s %s" % (self.code, self.errcode, self.msg) + def error_dict(self): return cs_error( self.msg, self.errcode, ) + @classmethod + def from_http_response_exception(cls, err): + """Make a SynapseError based on an HTTPResponseException + + Args: + err (HttpResponseException): + + Returns: + SynapseError: + """ + # try to parse the body as json, to get better errcode/msg, but + # default to M_UNKNOWN with the HTTP status as the error text + try: + j = json.loads(err.response) + except ValueError: + j = {} + errcode = j.get('errcode', Codes.UNKNOWN) + errmsg = j.get('error', err.response_code_message) + + res = SynapseError(err.code, errmsg, errcode) + res.response_code_message = err.response_code_message + return res + class RegistrationError(SynapseError): """An error raised when a registration event fails.""" @@ -243,6 +274,20 @@ class FederationError(RuntimeError): class HttpResponseException(CodeMessageException): + """ + Represents an HTTP-level failure of an outbound request + + Attributes: + response (str): body of response + """ def __init__(self, code, msg, response): + """ + + Args: + code (int): HTTP status code + msg (str): reason phrase from HTTP response status line + response (str): body of response + """ + super(HttpResponseException, self).__init__(code) + self.response_code_message = msg self.response = response - super(HttpResponseException, self).__init__(code, msg) -- cgit 1.4.1 From 7f237800e91c4640b79fceb645e07feb837ee4ef Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 14 Mar 2017 12:36:50 +0000 Subject: re-refactor exception heirarchy Give CodeMessageException back its `msg` attribute, and use that to hold the HTTP status message for HttpResponseException. --- synapse/api/errors.py | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) (limited to 'synapse/api/errors.py') diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 014bd60b9d..d5391a80cd 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -55,34 +55,35 @@ class CodeMessageException(RuntimeError): Attributes: code (int): HTTP error code - response_code_message (str): HTTP reason phrase. None for the default. + msg (str): string describing the error """ - def __init__(self, code): - super(CodeMessageException, self).__init__("%d" % code) + def __init__(self, code, msg): + super(CodeMessageException, self).__init__("%d: %s" % (code, msg)) self.code = code - self.response_code_message = None + self.msg = msg def error_dict(self): return cs_error(self.msg) class SynapseError(CodeMessageException): - """A base error which can be caught for all synapse events.""" + """A base exception type for matrix errors which have an errcode and error + message (as well as an HTTP status code). + + Attributes: + errcode (str): Matrix error code e.g 'M_FORBIDDEN' + """ def __init__(self, code, msg, errcode=Codes.UNKNOWN): """Constructs a synapse error. Args: code (int): The integer error code (an HTTP response code) msg (str): The human-readable error message. - errcode (str): The synapse error code e.g 'M_FORBIDDEN' + errcode (str): The matrix error code e.g 'M_FORBIDDEN' """ - super(SynapseError, self).__init__(code) - self.msg = msg + super(SynapseError, self).__init__(code, msg) self.errcode = errcode - def __str__(self): - return "%d: %s %s" % (self.code, self.errcode, self.msg) - def error_dict(self): return cs_error( self.msg, @@ -106,10 +107,9 @@ class SynapseError(CodeMessageException): except ValueError: j = {} errcode = j.get('errcode', Codes.UNKNOWN) - errmsg = j.get('error', err.response_code_message) + errmsg = j.get('error', err.msg) res = SynapseError(err.code, errmsg, errcode) - res.response_code_message = err.response_code_message return res @@ -204,7 +204,6 @@ class LimitExceededError(SynapseError): errcode=Codes.LIMIT_EXCEEDED): super(LimitExceededError, self).__init__(code, msg, errcode) self.retry_after_ms = retry_after_ms - self.response_code_message = "Too Many Requests" def error_dict(self): return cs_error( @@ -288,6 +287,5 @@ class HttpResponseException(CodeMessageException): msg (str): reason phrase from HTTP response status line response (str): body of response """ - super(HttpResponseException, self).__init__(code) - self.response_code_message = msg + super(HttpResponseException, self).__init__(code, msg) self.response = response -- cgit 1.4.1 From 1d09586599a495e01bfb6b887b1a59419673600a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 14 Mar 2017 13:36:06 +0000 Subject: Address review comments - don't blindly proxy all HTTPRequestExceptions - log unexpected exceptions at error - avoid `isinstance` - improve docs on `from_http_response_exception` --- synapse/api/errors.py | 19 +++++++++++++----- synapse/rest/media/v1/media_repository.py | 32 ++++++++++++++++--------------- 2 files changed, 31 insertions(+), 20 deletions(-) (limited to 'synapse/api/errors.py') diff --git a/synapse/api/errors.py b/synapse/api/errors.py index d5391a80cd..6fbd5d6876 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -94,6 +94,17 @@ class SynapseError(CodeMessageException): def from_http_response_exception(cls, err): """Make a SynapseError based on an HTTPResponseException + This is useful when a proxied request has failed, and we need to + decide how to map the failure onto a matrix error to send back to the + client. + + An attempt is made to parse the body of the http response as a matrix + error. If that succeeds, the errcode and error message from the body + are used as the errcode and error message in the new synapse error. + + Otherwise, the errcode is set to M_UNKNOWN, and the error message is + set to the reason code from the HTTP response. + Args: err (HttpResponseException): @@ -137,13 +148,11 @@ class UnrecognizedRequestError(SynapseError): class NotFoundError(SynapseError): """An error indicating we can't find the thing you asked for""" - def __init__(self, *args, **kwargs): - if "errcode" not in kwargs: - kwargs["errcode"] = Codes.NOT_FOUND + def __init__(self, msg="Not found", errcode=Codes.NOT_FOUND): super(NotFoundError, self).__init__( 404, - "Not found", - **kwargs + msg, + errcode=errcode ) diff --git a/synapse/rest/media/v1/media_repository.py b/synapse/rest/media/v1/media_repository.py index 66464cfe66..c43b185e08 100644 --- a/synapse/rest/media/v1/media_repository.py +++ b/synapse/rest/media/v1/media_repository.py @@ -12,7 +12,11 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + +from twisted.internet import defer, threads import twisted.internet.error +import twisted.web.http +from twisted.web.resource import Resource from .upload_resource import UploadResource from .download_resource import DownloadResource @@ -20,9 +24,6 @@ from .thumbnail_resource import ThumbnailResource from .identicon_resource import IdenticonResource from .preview_url_resource import PreviewUrlResource from .filepath import MediaFilePaths - -from twisted.web.resource import Resource - from .thumbnailer import Thumbnailer from synapse.http.matrixfederationclient import MatrixFederationHttpClient @@ -30,8 +31,6 @@ from synapse.util.stringutils import random_string from synapse.api.errors import SynapseError, HttpResponseException, \ NotFoundError -from twisted.internet import defer, threads - from synapse.util.async import Linearizer from synapse.util.stringutils import is_ascii from synapse.util.logcontext import preserve_context_over_fn @@ -174,16 +173,19 @@ class MediaRepository(object): except HttpResponseException as e: logger.warn("HTTP error fetching remote media %s/%s: %s", server_name, media_id, e.response) - raise SynapseError.from_http_response_exception(e) - - except Exception as e: - logger.warn("Failed to fetch remote media %s/%s", - server_name, media_id, - exc_info=True) - if isinstance(e, SynapseError): - raise e - else: - raise SynapseError(502, "Failed to fetch remote media") + if e.code == twisted.web.http.NOT_FOUND: + raise SynapseError.from_http_response_exception(e) + raise SynapseError(502, "Failed to fetch remote media") + + except SynapseError: + logger.exception("Failed to fetch remote media %s/%s", + server_name, media_id) + raise + + except Exception: + logger.exception("Failed to fetch remote media %s/%s", + server_name, media_id) + raise SynapseError(502, "Failed to fetch remote media") media_type = headers["Content-Type"][0] time_now_ms = self.clock.time_msec() -- cgit 1.4.1