From 2603373b860acb555062c01a5bb434d6c712aa3e Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 15 Jul 2019 07:03:32 -0400 Subject: [PATCH] Additional protections for HTTP endpoints Reject browser-based requests that don't require a CORS preflight request [1] if they don't come from the connector or include Zotero-Allowed-Request: 1 [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Simple_requests --- .../xpcom/connector/server_connector.js | 4 - chrome/content/zotero/xpcom/server.js | 81 +++++++++++---- test/tests/server_connectorTest.js | 99 ++++++++++++++++--- 3 files changed, 150 insertions(+), 34 deletions(-) diff --git a/chrome/content/zotero/xpcom/connector/server_connector.js b/chrome/content/zotero/xpcom/connector/server_connector.js index 68576ae7ce..c80293b25a 100644 --- a/chrome/content/zotero/xpcom/connector/server_connector.js +++ b/chrome/content/zotero/xpcom/connector/server_connector.js @@ -1135,10 +1135,6 @@ Zotero.Server.Connector.Import.prototype = { permitBookmarklet: false, init: async function (requestData) { - if (!('X-Zotero-Connector-API-Version' in requestData.headers)) { - return 400; - } - let translate = new Zotero.Translate.Import(); translate.setString(requestData.data); let translators = await translate.getTranslators(); diff --git a/chrome/content/zotero/xpcom/server.js b/chrome/content/zotero/xpcom/server.js index bc8fbc547d..b91576389c 100755 --- a/chrome/content/zotero/xpcom/server.js +++ b/chrome/content/zotero/xpcom/server.js @@ -31,6 +31,7 @@ Zotero.Server = new function() { 204:"No Content", 300:"Multiple Choices", 400:"Bad Request", + 403:"Forbidden", 404:"Not Found", 409:"Conflict", 412:"Precondition Failed", @@ -239,35 +240,44 @@ Zotero.Server.DataListener.prototype._headerFinished = function() { Zotero.debug(this.header, 5); - const methodRe = /^([A-Z]+) ([^ \r\n?]+)(\?[^ \r\n]+)?/; - const hostRe = /[\r\n]Host: *(localhost|127\.0\.0\.1)(:[0-9]+)?[\r\n]/i; - const contentTypeRe = /[\r\n]Content-Type: *([^ \r\n]+)/i; - - const originRe = /[\r\n]Origin: *([^ \r\n]+)/i; - var m = originRe.exec(this.header); - if (m) { - this.origin = m[1]; + // Parse headers into this.headers with lowercase names + this.headers = {}; + var headerLines = this.header.trim().split(/\r\n/); + for (let line of headerLines) { + line = line.trim(); + let pos = line.indexOf(':'); + if (pos == -1) { + continue; + } + let k = line.substr(0, pos).toLowerCase(); + let v = line.substr(pos + 1).trim(); + this.headers[k] = v; } - else { - const bookmarkletRe = /[\r\n]Zotero-Bookmarklet: *([^ \r\n]+)/i; - var m = bookmarkletRe.exec(this.header); - if (m) this.origin = "https://www.zotero.org"; + + if (this.headers.origin) { + this.origin = this.headers.origin; + } + else if (this.headers['zotero-bookmarklet']) { + this.origin = "https://www.zotero.org"; } if (!Zotero.isServer) { // Make sure the Host header is set to localhost/127.0.0.1 to prevent DNS rebinding attacks - if (!hostRe.exec(this.header)) { + const hostRe = /^(localhost|127\.0\.0\.1)(:[0-9]+)?$/i; + if (!hostRe.test(this.headers.host)) { this._requestFinished(this._generateResponse(400, "text/plain", "Invalid Host header\n")); return; } } // get first line of request + const methodRe = /^([A-Z]+) ([^ \r\n?]+)(\?[^ \r\n]+)?/; var method = methodRe.exec(this.header); + // get content-type - var contentType = contentTypeRe.exec(this.header); - if(contentType) { - var splitContentType = contentType[1].split(/\s*;/); + var contentType = this.headers['content-type']; + if (contentType) { + let splitContentType = contentType.split(/\s*;/); this.contentType = splitContentType[0]; } @@ -288,10 +298,10 @@ Zotero.Server.DataListener.prototype._headerFinished = function() { } else if(method[1] == "GET") { this._processEndpoint("GET", null); // async } else if(method[1] == "POST") { - const contentLengthRe = /[\r\n]Content-Length: +([0-9]+)/i; + const contentLengthRe = /^([0-9]+)$/; // parse content length - var m = contentLengthRe.exec(this.header); + var m = contentLengthRe.exec(this.headers['content-length']); if(!m) { this._requestFinished(this._generateResponse(400, "text/plain", "Content-length not provided\n")); return; @@ -408,13 +418,46 @@ Zotero.Server.DataListener.prototype._processEndpoint = Zotero.Promise.coroutine } } + + // Reject browser-based requests that don't require a CORS preflight request [1] if they + // don't come from the connector or include Zotero-Allowed-Request + // + // Endpoints that can be triggered with a simple request can be whitelisted if they don't + // trigger any actions + // + // [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Simple_requests + var whitelistedEndpoints = [ + '/test/translate/test.html', + '/test/translate/test.pdf', + '/test/translate/does_not_exist.html', + ]; + var simpleRequestContentTypes = [ + 'application/x-www-form-urlencoded', + 'multipart/form-data', + 'text/plain' + ]; + var isBrowser = this.headers['user-agent'].startsWith('Mozilla/') + // Origin isn't sent via fetch() for HEAD/GET, but for crazy UA strings, protecting + // POST requests is better than nothing + || 'origin' in this.headers; + if (isBrowser + && !this.headers['x-zotero-connector-api-version'] + && !this.headers['zotero-allowed-request'] + && (!endpoint.supportedDataTypes + || endpoint.supportedDataTypes == '*' + || endpoint.supportedDataTypes.some(type => simpleRequestContentTypes.includes(type))) + && !whitelistedEndpoints.includes(this.pathname) + && !(this.contentType && !simpleRequestContentTypes.includes(this.contentType))) { + this._requestFinished(this._generateResponse(403, "text/plain", "Request not allowed\n")); + return; + } + var decodedData = null; if(postData && this.contentType) { // check that endpoint supports contentType var supportedDataTypes = endpoint.supportedDataTypes; if(supportedDataTypes && supportedDataTypes != '*' && supportedDataTypes.indexOf(this.contentType) === -1) { - this._requestFinished(this._generateResponse(400, "text/plain", "Endpoint does not support content-type\n")); return; } diff --git a/test/tests/server_connectorTest.js b/test/tests/server_connectorTest.js index 93a97205e8..66a6fcda83 100644 --- a/test/tests/server_connectorTest.js +++ b/test/tests/server_connectorTest.js @@ -1323,9 +1323,19 @@ describe("Connector Server", function () { describe('/connector/installStyle', function() { var endpoint; + var style; before(function() { endpoint = connectorServerPath + "/connector/installStyle"; + style = ` + +`; }); it('should reject styles with invalid text', function* () { @@ -1358,15 +1368,6 @@ describe("Connector Server", function () { }); }); - var style = ` - -`; var response = yield Zotero.HTTP.request( 'POST', endpoint, @@ -1374,11 +1375,73 @@ describe("Connector Server", function () { headers: { "Content-Type": "application/vnd.citationstyles.style+xml" }, body: style } - ); + ); assert.equal(response.status, 201); assert.equal(response.response, JSON.stringify({name: 'Test1'})); Zotero.Styles.install.restore(); }); + + it('should accept text/plain request with X-Zotero-Connector-API-Version or Zotero-Allowed-Request', async function () { + sinon.stub(Zotero.Styles, 'install').callsFake(function(style) { + var parser = Components.classes["@mozilla.org/xmlextras/domparser;1"] + .createInstance(Components.interfaces.nsIDOMParser), + doc = parser.parseFromString(style, "application/xml"); + + return Zotero.Promise.resolve({ + styleTitle: Zotero.Utilities.xpathText( + doc, '/csl:style/csl:info[1]/csl:title[1]', Zotero.Styles.ns + ), + styleID: Zotero.Utilities.xpathText( + doc, '/csl:style/csl:info[1]/csl:id[1]', Zotero.Styles.ns + ) + }); + }); + + // X-Zotero-Connector-API-Version + var response = await Zotero.HTTP.request( + 'POST', + endpoint, + { + headers: { + "Content-Type": "text/plain", + "X-Zotero-Connector-API-Version": "2" + }, + body: style + } + ); + assert.equal(response.status, 201); + + // Zotero-Allowed-Request + response = await Zotero.HTTP.request( + 'POST', + endpoint, + { + headers: { + "Content-Type": "text/plain", + "Zotero-Allowed-Request": "1" + }, + body: style + } + ); + assert.equal(response.status, 201); + + Zotero.Styles.install.restore(); + }); + + it('should reject text/plain request without X-Zotero-Connector-API-Version', async function () { + var req = await Zotero.HTTP.request( + 'POST', + endpoint, + { + headers: { + "Content-Type": "text/plain" + }, + body: style, + successCodes: [403] + } + ); + assert.equal(req.status, 403); + }); }); describe('/connector/import', function() { @@ -1404,6 +1467,20 @@ describe("Connector Server", function () { assert.equal(error.xmlhttp.status, 400); }); + it('should reject requests without X-Zotero-Connector-API-Version', async function () { + var req = await Zotero.HTTP.request( + 'POST', + endpoint, + { + headers: { + "Content-Type": "text/plain" + }, + successCodes: [403] + } + ); + assert.equal(req.status, 403); + }); + it('should import resources (BibTeX) into selected collection', function* () { var collection = yield createDataObject('collection'); yield waitForItemsLoad(win); @@ -1427,7 +1504,7 @@ describe("Connector Server", function () { }, body: resource } - ); + ); assert.equal(req.status, 201); assert.equal(JSON.parse(req.responseText)[0].title, 'Test1');