From 5f6e4bdfe91d7fe77e6b211d59d97a91f9ec52d4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 3 Jul 2017 18:29:24 +0100 Subject: [PATCH 1/2] Avoid parsing plain-text errors as JSON It's somewhat unhelpful to spam over the actual error from the reverse-proxy or whatever with a SyntaxError. --- package.json | 1 + spec/integ/matrix-client-methods.spec.js | 13 ++- spec/mock-request.js | 26 ++++- src/http-api.js | 120 +++++++++++++++++++---- 4 files changed, 129 insertions(+), 31 deletions(-) diff --git a/package.json b/package.json index 67420c22b58..1771b7502a0 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "dependencies": { "another-json": "^0.2.0", "browser-request": "^0.3.3", + "content-type": "^1.0.2", "q": "^1.4.1", "request": "^2.53.0" }, diff --git a/spec/integ/matrix-client-methods.spec.js b/spec/integ/matrix-client-methods.spec.js index 41806141227..b3de0f07e93 100644 --- a/spec/integ/matrix-client-methods.spec.js +++ b/spec/integ/matrix-client-methods.spec.js @@ -49,13 +49,13 @@ describe("MatrixClient", function() { httpBackend.when( "POST", "/_matrix/media/v1/upload", ).check(function(req) { - expect(req.data).toEqual(buf); + expect(req.rawData).toEqual(buf); expect(req.queryParams.filename).toEqual("hi.txt"); expect(req.queryParams.access_token).toEqual(accessToken); expect(req.headers["Content-Type"]).toEqual("text/plain"); expect(req.opts.json).toBeFalsy(); expect(req.opts.timeout).toBe(undefined); - }).respond(200, "content"); + }).respond(200, "content", true); const prom = client.uploadContent({ stream: buf, @@ -86,7 +86,7 @@ describe("MatrixClient", function() { "POST", "/_matrix/media/v1/upload", ).check(function(req) { expect(req.opts.json).toBeFalsy(); - }).respond(200, JSON.stringify({ "content_uri": "uri" })); + }).respond(200, { "content_uri": "uri" }); client.uploadContent({ stream: buf, @@ -102,16 +102,15 @@ describe("MatrixClient", function() { }); it("should parse errors into a MatrixError", function(done) { - // opts.json is false, so request returns unparsed json. httpBackend.when( "POST", "/_matrix/media/v1/upload", ).check(function(req) { - expect(req.data).toEqual(buf); + expect(req.rawData).toEqual(buf); expect(req.opts.json).toBeFalsy(); - }).respond(400, JSON.stringify({ + }).respond(400, { "errcode": "M_SNAFU", "error": "broken", - })); + }); client.uploadContent({ stream: buf, diff --git a/spec/mock-request.js b/spec/mock-request.js index e4a929a6157..4ace2a776e9 100644 --- a/spec/mock-request.js +++ b/spec/mock-request.js @@ -126,10 +126,15 @@ HttpBackend.prototype = { } testResponse = matchingReq.response; console.log(" responding to %s", matchingReq.path); + let body = testResponse.body; if (Object.prototype.toString.call(body) == "[object Function]") { body = body(req.path, req.data); } + + if (!testResponse.rawBody) { + body = JSON.stringify(body); + } req.callback( testResponse.err, testResponse.response, body, ); @@ -210,17 +215,22 @@ ExpectedRequest.prototype = { /** * Respond with the given data when this request is satisfied. * @param {Number} code The HTTP status code. - * @param {Object|Function} data The HTTP JSON body. If this is a function, - * it will be invoked when the JSON body is required (which should be returned). + * @param {Object|Function?} data The response body object. If this is a function, + * it will be invoked when the response body is required (which should be returned). + * @param {Boolean} rawBody true if the response should be returned directly rather + * than json-stringifying it first. */ - respond: function(code, data) { + respond: function(code, data, rawBody) { this.response = { response: { statusCode: code, - headers: {}, + headers: { + 'content-type': 'application/json', + }, }, - body: data, + body: data || "", err: null, + rawBody: rawBody || false, }; }, @@ -265,6 +275,12 @@ function Request(opts, callback) { }); Object.defineProperty(this, 'data', { + get: function() { + return opts.body ? JSON.parse(opts.body) : opts.body; + }, + }); + + Object.defineProperty(this, 'rawData', { get: function() { return opts.body; }, diff --git a/src/http-api.js b/src/http-api.js index de213cc43f2..69b66786b87 100644 --- a/src/http-api.js +++ b/src/http-api.js @@ -19,6 +19,8 @@ limitations under the License. * @module http-api */ const q = require("q"); +const parseContentType = require('content-type').parse; + const utils = require("./utils"); // we use our own implementation of setTimeout, so that if we get suspended in @@ -623,7 +625,31 @@ module.exports.MatrixHttpApi.prototype = { } } + const headers = utils.extend({}, opts.headers || {}); const json = opts.json === undefined ? true : opts.json; + let bodyParser = opts.bodyParser; + + // we handle the json encoding/decoding here, because request and + // browser-request make a mess of it. Specifically, they attempt to + // json-decode plain-text error responses, which in turn means that the + // actual error gets swallowed by a SyntaxError. + + if (json) { + if (data) { + data = JSON.stringify(data); + headers['content-type'] = 'application/json'; + } + + if (!headers['accept']) { + headers['accept'] = 'application/json'; + } + + if (bodyParser === undefined) { + bodyParser = function(rawBody) { + return JSON.parse(rawBody); + }; + } + } const defer = q.defer(); @@ -662,7 +688,7 @@ module.exports.MatrixHttpApi.prototype = { withCredentials: false, qs: queryParams, body: data, - json: json, + json: false, timeout: localTimeoutMs, headers: opts.headers || {}, _matrix_opts: this.opts, @@ -675,13 +701,9 @@ module.exports.MatrixHttpApi.prototype = { } } - // if json is falsy, we won't parse any error response, so need - // to do so before turning it into a MatrixError - const parseErrorJson = !json; const handlerFn = requestCallback( defer, callback, self.opts.onlyData, - parseErrorJson, - opts.bodyParser, + bodyParser, ); handlerFn(err, response, body); }, @@ -718,15 +740,18 @@ module.exports.MatrixHttpApi.prototype = { * that will either resolve or reject the given defer as well as invoke the * given userDefinedCallback (if any). * - * If onlyData is true, the defer/callback is invoked with the body of the - * response, otherwise the result code. + * HTTP errors are transformed into javascript errors and the deferred is rejected. * - * If parseErrorJson is true, we will JSON.parse the body if we get a 4xx error. + * If bodyParser is given, it is used to transform the body of the successful + * responses before passing to the defer/callback. + * + * If onlyData is true, the defer/callback is invoked with the body of the + * response, otherwise the result object (with `code` and `data` fields) * */ const requestCallback = function( defer, userDefinedCallback, onlyData, - parseErrorJson, bodyParser, + bodyParser, ) { userDefinedCallback = userDefinedCallback || function() {}; @@ -734,19 +759,12 @@ const requestCallback = function( if (!err) { try { if (response.statusCode >= 400) { - if (parseErrorJson) { - // we won't have json-decoded the response. - body = JSON.parse(body); - } - err = new module.exports.MatrixError(body); + err = parseErrorResponse(response, body); } else if (bodyParser) { body = bodyParser(body); } } catch (e) { - err = e; - } - if (err) { - err.httpStatus = response.statusCode; + err = new Error(`Error parsing server response: ${e}`); } } @@ -756,6 +774,9 @@ const requestCallback = function( } else { const res = { code: response.statusCode, + + // XXX: why do we bother with this? it doesn't work for + // XMLHttpRequest, so clearly we don't use it. headers: response.headers, data: body, }; @@ -765,6 +786,67 @@ const requestCallback = function( }; }; +/** + * Attempt to turn an HTTP error response into a Javascript Error. + * + * If it is a JSON response, we will parse it into a MatrixError. Otherwise + * we return a generic Error. + * + * @param {XMLHttpRequest|http.IncomingMessage} response response object + * @param {String} body raw body of the response + * @returns {Error} + */ +function parseErrorResponse(response, body) { + const httpStatus = response.statusCode; + const contentType = getResponseContentType(response); + + let err; + if (contentType) { + if (contentType.type === 'application/json') { + err = new module.exports.MatrixError(JSON.parse(body)); + } else if (contentType.type === 'text/plain') { + err = new Error(`Server returned ${httpStatus} error: ${body}`); + } + } + + if (!err) { + err = new Error(`Server returned ${httpStatus} error`); + } + err.httpStatus = httpStatus; + return err; +} + + +/** + * extract the Content-Type header from the response object, and + * parse it to a `{type, parameters}` object. + * + * returns null if no content-type header could be found. + * + * @param {XMLHttpRequest|http.IncomingMessage} response response object + * @returns {{type: String, parameters: Object}?} parsed content-type header, or null if not found + */ +function getResponseContentType(response) { + let contentType; + if (response.getResponseHeader) { + // XMLHttpRequest provides getResponseHeader + contentType = response.getResponseHeader("Content-Type"); + } else if (response.headers) { + // request provides http.IncomingMessage which has a message.headers map + contentType = response.headers['content-type'] || null; + } + + if (!contentType) { + return null; + } + + try { + return parseContentType(contentType); + } catch(e) { + throw new Error(`Error parsing Content-Type '${contentType}': ${e}`); + } +} + /** * Construct a Matrix error. This is a JavaScript Error with additional * information specific to the standard Matrix error response. From b0661bb586868966272f4ef4d84c6169a0d262cb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Tue, 4 Jul 2017 16:35:33 +0100 Subject: [PATCH 2/2] Update to matrix-mock-request 1.0 -- to pick up on the json parsing differences --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 6b1c1138c09..db623ae356d 100644 --- a/package.json +++ b/package.json @@ -66,7 +66,7 @@ "istanbul": "^0.4.5", "jsdoc": "^3.4.0", "lolex": "^1.5.2", - "matrix-mock-request": "^0.1.3", + "matrix-mock-request": "^1.0.0", "mocha": "^3.2.0", "mocha-jenkins-reporter": "^0.3.6", "rimraf": "^2.5.4",