From abe7c49b9969efb0d2dfedddb7bd35148baacf38 Mon Sep 17 00:00:00 2001 From: louisbuchbinder Date: Sat, 27 Apr 2019 18:36:26 -0700 Subject: [PATCH 1/2] do not throw on bad redirect in pipe --- .gitignore | 1 + src/node/index.js | 7 +++--- test/node/pipe.js | 57 +++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index ef4ee3814..1592c7754 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ coverage .nyc_output lib dist +*.swp diff --git a/src/node/index.js b/src/node/index.js index 3722da46b..5df4d2987 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -423,9 +423,10 @@ Request.prototype.pipe = function(stream, options) { Request.prototype._pipeContinue = function(stream, options) { this.req.once('response', res => { // redirect - const redirect = isRedirect(res.statusCode); - if (redirect && this._redirects++ !== this._maxRedirects) { - return this._redirect(res)._pipeContinue(stream, options); + if (isRedirect(res.statusCode) && this._redirects++ != this._maxRedirects) { + return this._redirect(res) === this + ? this._pipeContinue(stream, options) + : void 0; } this.res = res; diff --git a/test/node/pipe.js b/test/node/pipe.js index 23ca369ba..1ed2d612f 100644 --- a/test/node/pipe.js +++ b/test/node/pipe.js @@ -17,8 +17,16 @@ app.get('/', (req, res) => { fs.createReadStream('test/node/fixtures/user.json').pipe(res); }); -app.post('/', (req, res) => { - if (process.env.HTTP2_TEST) { +app.get("/redirect", (req, res) => { + res.set('Location', '/').sendStatus(302); +}); + +app.get("/badRedirectNoLocation", (req, res) => { + res.set('Location', '').sendStatus(302); +}); + +app.post("/", (req, res) => { + if (process.env.HTTP2_TEST){ // body-parser does not support http2 yet. // This section can be remove after body-parser supporting http2. res.set('content-type', 'application/json'); @@ -93,4 +101,49 @@ describe('request pipe', () => { }); req.pipe(stream); }); + + it("should follow redirects", done => { + const stream = fs.createWriteStream(destPath); + + let responseCalled = false; + const req = request.get(base + '/redirect'); + req.type("json"); + + req.on("response", res => { + res.status.should.eql(200); + responseCalled = true; + }); + stream.on("finish", () => { + JSON.parse(fs.readFileSync(destPath, "utf8")).should.eql({ + name: "tobi", + }); + responseCalled.should.be.true(); + done(); + }); + req.pipe(stream); + }); + + it("should not throw on bad redirects", done => { + const stream = fs.createWriteStream(destPath); + + let responseCalled = false; + let errorCalled = false; + const req = request.get(base + '/badRedirectNoLocation'); + req.type("json"); + + req.on("response", res => { + responseCalled = true; + }); + req.on("error", err => { + err.message.should.eql("No location header for redirect"); + errorCalled = true; + stream.end(); + }); + stream.on("finish", () => { + responseCalled.should.be.false(); + errorCalled.should.be.true(); + done(); + }); + req.pipe(stream); + }); }); From f45aae84fee272b3cce5a975f0c57fee1cd9aea5 Mon Sep 17 00:00:00 2001 From: louisbuchbinder Date: Sat, 27 Apr 2019 19:00:18 -0700 Subject: [PATCH 2/2] linting --- src/node/index.js | 7 +++++-- test/node/pipe.js | 32 ++++++++++++++++---------------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/node/index.js b/src/node/index.js index 5df4d2987..23e478272 100644 --- a/src/node/index.js +++ b/src/node/index.js @@ -423,10 +423,13 @@ Request.prototype.pipe = function(stream, options) { Request.prototype._pipeContinue = function(stream, options) { this.req.once('response', res => { // redirect - if (isRedirect(res.statusCode) && this._redirects++ != this._maxRedirects) { + if ( + isRedirect(res.statusCode) && + this._redirects++ !== this._maxRedirects + ) { return this._redirect(res) === this ? this._pipeContinue(stream, options) - : void 0; + : undefined; } this.res = res; diff --git a/test/node/pipe.js b/test/node/pipe.js index 1ed2d612f..08bcbdc2c 100644 --- a/test/node/pipe.js +++ b/test/node/pipe.js @@ -17,16 +17,16 @@ app.get('/', (req, res) => { fs.createReadStream('test/node/fixtures/user.json').pipe(res); }); -app.get("/redirect", (req, res) => { +app.get('/redirect', (req, res) => { res.set('Location', '/').sendStatus(302); }); -app.get("/badRedirectNoLocation", (req, res) => { +app.get('/badRedirectNoLocation', (req, res) => { res.set('Location', '').sendStatus(302); }); -app.post("/", (req, res) => { - if (process.env.HTTP2_TEST){ +app.post('/', (req, res) => { + if (process.env.HTTP2_TEST) { // body-parser does not support http2 yet. // This section can be remove after body-parser supporting http2. res.set('content-type', 'application/json'); @@ -102,20 +102,20 @@ describe('request pipe', () => { req.pipe(stream); }); - it("should follow redirects", done => { + it('should follow redirects', done => { const stream = fs.createWriteStream(destPath); let responseCalled = false; const req = request.get(base + '/redirect'); - req.type("json"); + req.type('json'); - req.on("response", res => { + req.on('response', res => { res.status.should.eql(200); responseCalled = true; }); - stream.on("finish", () => { - JSON.parse(fs.readFileSync(destPath, "utf8")).should.eql({ - name: "tobi", + stream.on('finish', () => { + JSON.parse(fs.readFileSync(destPath, 'utf8')).should.eql({ + name: 'tobi' }); responseCalled.should.be.true(); done(); @@ -123,23 +123,23 @@ describe('request pipe', () => { req.pipe(stream); }); - it("should not throw on bad redirects", done => { + it('should not throw on bad redirects', done => { const stream = fs.createWriteStream(destPath); let responseCalled = false; let errorCalled = false; const req = request.get(base + '/badRedirectNoLocation'); - req.type("json"); + req.type('json'); - req.on("response", res => { + req.on('response', res => { responseCalled = true; }); - req.on("error", err => { - err.message.should.eql("No location header for redirect"); + req.on('error', err => { + err.message.should.eql('No location header for redirect'); errorCalled = true; stream.end(); }); - stream.on("finish", () => { + stream.on('finish', () => { responseCalled.should.be.false(); errorCalled.should.be.true(); done();