Skip to content

Commit

Permalink
Avoid parsing plain-text errors as JSON
Browse files Browse the repository at this point in the history 
It's somewhat unhelpful to spam over the actual error from the reverse-proxy or
whatever with a SyntaxError.
  • Loading branch information
@richvdh
richvdh committed Jul 3, 2017
1 parent cc16cb9 commit 5f6e4bd
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 31 deletions.
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
13 changes: 6 additions & 7 deletions spec/integ/matrix-client-methods.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
26 changes: 21 additions & 5 deletions spec/mock-request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down Expand Up @@ -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,
};
},

Expand Down Expand Up @@ -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;
},
Expand Down
120 changes: 101 additions & 19 deletions src/http-api.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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,
Expand All @@ -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);
},
Expand Down Expand Up @@ -718,35 +740,31 @@ 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() {};

return function(err, response, body) {
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}`);
}
}

Expand All @@ -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,
};
Expand All @@ -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.
Expand Down

0 comments on commit 5f6e4bd

Please sign in to comment.