Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid redirect in pipe #1471

Merged
merged 2 commits into from
Apr 28, 2019
Merged

Invalid redirect in pipe #1471

merged 2 commits into from
Apr 28, 2019

Conversation

louisbuchbinder
Copy link
Contributor

@louisbuchbinder louisbuchbinder commented Mar 23, 2019

There is a bug in redirects during a pipe. If the request responds with a redirect status, but an empty Location header then superagent will throw Uncaught TypeError: Cannot read property '_pipeContinue' of undefined.

To reproduce you can checkout my tests (without the patch) and run them. You will see:

 330 passing (8s)                               
  5 pending                                
  1 failing                                      
                                             
  1) request pipe should not throw on bad redirects:          
     Uncaught TypeError: Cannot read property '_pipeContinue' of undefined
      at ClientRequest.req.once.res (lib/node/index.js:414:33)
      at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:543:21)
      at HTTPParser.parserOnHeadersComplete (_http_common.js:112:17)
      at Socket.socketOnData (_http_client.js:440:20)
      at addChunk (_stream_readable.js:263:12)
      at readableAddChunk (_stream_readable.js:250:11)
      at Socket.Readable.push (_stream_readable.js:208:10)
      at TCP.onread (net.js:597:20)  

or you can reproduce with by hand with something like:

http.createServer((req, res) => {
  res.setHeader('Location', '');
  res.statusCode = 302;
  res.end();
}).listen(8000);

require('superagent').get('localhost:8000/').pipe(require('fs').createWriteStream('./temp'));

@@ -92,4 +100,48 @@ describe("request pipe", () => {
});
req.pipe(stream);
});

it("should follow redirects", done => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test for pipes with a good redirect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now there is already a test for a good redirect in ./test/node/pipe-redirect.js

req.pipe(stream);
});

it("should not throw on bad redirects", done => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test to prove that redirects with a missing Location will not throw and error.

package-lock.json
*.swp
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added *.swp to the gitignore for vim files

@louisbuchbinder
Copy link
Contributor Author

louisbuchbinder commented Mar 23, 2019

failing tests in node10 and 11... updating

Edit: Tests now passing in Travis for all node versions 6, 8, 10, and 11.

if (redirect && this._redirects++ != this._maxRedirects) {
if (!res.headers.location) {
this.callback(new Error('No location header for redirect'), res);
stream.end();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, superagent should not be responsible for ending the client stream if there is an error. Instead the client should listen for the error event and then handle their stream on their own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please advise...

@niftylettuce
Copy link
Collaborator

@louisbuchbinder could you please refactor/pull latest from master? may be easiest to start fresh. would love to get this added to v5.x release that just came out.

@louisbuchbinder
Copy link
Contributor Author

@niftylettuce I rebased and update for the linting rules

@niftylettuce niftylettuce merged commit 7d0ea4b into ladjs:master Apr 28, 2019
@niftylettuce
Copy link
Collaborator

@louisbuchbinder v5.0.4 is released, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants