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

fix: make byteArray return empty array on failure instead of fatal error #53

Merged
merged 3 commits into from
Apr 1, 2019

Conversation

Andrew-Lees11
Copy link
Contributor

Description

This PR changes byteArray so that on failure it returns an empty array instead of throwing a fatal error.

Motivation and Context

This is required because you make pass user code into the function (such as in Kitura-Session where a users Cookie is decoded). If it throws a fatal error on failure then a user can crash the system. If it returns an empty array the application can handle this in the correct way.

How Has This Been Tested?

This has been tested within Kitura-Session. Using this code makes the session reject the cookie instead of throwing a fatal error and crashing the server.

Checklist:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.

@ianpartridge
Copy link
Contributor

Looks good to me 🙂 Would be nice to have a test.

@billabt
Copy link
Collaborator

billabt commented Mar 29, 2019

I understand the rationale behind the PR but I just want make sure you know that it’s a potential source breaking change. The unexpected empty array could cause problems that could be difficult to detect and find. Have you considered this? @ianpartridge, you ok with this?

@ianpartridge
Copy link
Contributor

Yeah I think we should make this change, the current behaviour is dangerous.

@ianpartridge ianpartridge requested review from ianpartridge and removed request for djones6 April 1, 2019 09:55
@Andrew-Lees11
Copy link
Contributor Author

So the only way this impacts existing applications is that if it was previously throwing a fatal error. This makes it unlikely that this will change expected behavior. In the next major release of BlueCryptor this function should probably throw but I think this fix will solve the current issue in a non breaking way.

Copy link
Collaborator

@billabt billabt left a comment

Choose a reason for hiding this comment

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

👍

@billabt billabt merged commit 7843b9c into master Apr 1, 2019
@ianpartridge ianpartridge deleted the byteArray branch April 2, 2019 08:59
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