diff --git a/src/node/index.js b/src/node/index.js index c21231ad..a568a896 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -1146,15 +1146,15 @@ Request.prototype._end = function () { res.on('data', (buf) => { responseBytesLeft -= buf.byteLength || buf.length > 0 ? buf.length : 0; if (responseBytesLeft < 0) { - // This will propagate through error event const error = new Error('Maximum response size reached'); error.code = 'ETOOLARGE'; // Parsers aren't required to observe error event, // so would incorrectly report success parserHandlesEnd = false; - // Will not emit error event - res.destroy(error); - // so we do callback now + // Destroy without an error: IncomingMessage.destroy(err) emits + // 'error', and the parser may also fail, each of which would + // call callback() again ("superagent: double callback bug"). + res.destroy(); this.callback(error, null); } }); @@ -1167,8 +1167,8 @@ Request.prototype._end = function () { parserHandlesEnd = buffer; parser(res, (error, object, files) => { - if (this.timedout) { - // Timeout has already handled all callbacks + if (this.timedout || this.called) { + // Timeout or maxResponseSize has already handled all callbacks return; } diff --git a/test/node/image.js b/test/node/image.js index 2b66b499..ffe3f5f2 100644 --- a/test/node/image.js +++ b/test/node/image.js @@ -1,5 +1,6 @@ 'use strict'; +const assert = require('assert'); const fs = require('fs'); const request = require('../support/client'); const getSetup = require('../support/setup'); @@ -24,6 +25,39 @@ describe('res.body', () => { done(); }); }); + + it('should not double-callback when maxResponseSize is exceeded', (done) => { + const warns = []; + const { warn } = console; + console.warn = function (...args) { + warns.push(args.join(' ')); + }; + + request + .get(`${base}/image`) + .maxResponseSize(1) + .end((error) => { + // Late parser/error events fire after the first callback. + setImmediate(() => { + console.warn = warn; + try { + assert.equal( + error && error.message, + 'Maximum response size reached' + ); + assert.equal(error && error.code, 'ETOOLARGE'); + assert.equal( + warns.some((line) => /double callback/.test(line)), + false, + `unexpected warning(s): ${warns.join('; ')}` + ); + done(); + } catch (err) { + done(err); + } + }); + }); + }); }); describe('application/octet-stream', () => { it('should parse the body', (done) => { diff --git a/test/node/inflate.js b/test/node/inflate.js index 7b48a1e0..4dfa5df2 100644 --- a/test/node/inflate.js +++ b/test/node/inflate.js @@ -121,17 +121,34 @@ describe('zlib', () => { }); it('should protect from zip bombs', (done) => { + const warns = []; + const { warn } = console; + console.warn = function (...args) { + warns.push(args.join(' ')); + }; + request .get(base) .buffer(true) .maxResponseSize(1) - .end((error, res) => { - try { - assert.equal('Maximum response size reached', error && error.message); - done(); - } catch (err) { - done(err); - } + .end((error) => { + setImmediate(() => { + console.warn = warn; + try { + assert.equal( + 'Maximum response size reached', + error && error.message + ); + assert.equal( + warns.some((line) => /double callback/.test(line)), + false, + `unexpected warning(s): ${warns.join('; ')}` + ); + done(); + } catch (err) { + done(err); + } + }); }); });