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

Wrapped parse call into try/catch block #875

Closed
wants to merge 4 commits into from

Conversation

yunda
Copy link

@yunda yunda commented Jan 28, 2016

This commit resolves situation when server response holds something like "Forbidden".

@focusaurus
Copy link
Contributor

Could you add a unit test, please? Otherwise seems reasonable to me.

@yunda
Copy link
Author

yunda commented Jan 29, 2016

This pull-request supposed to fix #675

@kornelski
Copy link
Contributor

This is a breaking change, and it has inconsistent behaviour between client and the server.

@kornelski
Copy link
Contributor

Returning of unparsed string is dangerous, e.g. let's say the server used to return {accessDenied: false} or Forbidden!

.end(err, res) {
   if (err || res.body.accessDenied) return;
   allowAccess();
}

with this change res.body would be "Forbidden!" and res.body.accessDenied would be undefined and evaluate to falsy value, causing a security issue.

@yunda
Copy link
Author

yunda commented Jan 29, 2016

@pornel The idea is to return a response even if it fails to parse body. Otherwise we cannot read status codes or any other necessary information form the response object.

@kornelski
Copy link
Contributor

@yunda you can from the error object, that's what rawResponse was for. Other properties can be added to the error.

@kornelski
Copy link
Contributor

@yunda now there's err.statusCode. Do you need to read other properties during an error?

@kornelski
Copy link
Contributor

I'm closing in favor of err.rawResponse and err.statusCode, which I think are safer and more backwards-compatible.

@kornelski kornelski closed this Feb 26, 2016
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.

3 participants