Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Catch error when unmarshaling instead of crashing #113

Merged
merged 7 commits into from
Dec 1, 2017
Merged

Conversation

mkg20001
Copy link
Member

Related #112

@mkg20001
Copy link
Member Author

mkg20001 commented Nov 27, 2017

weird test results
just noticed that the rsa key.verify function sometimes doesn't return an error even when the key is invalid. is this intended?

src/keys/rsa.js Outdated
}
})
} catch (err) {
callback(new Error('Key is invalid!'))
Copy link
Member

Choose a reason for hiding this comment

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

needs a return

kty: key.kty,
n: key.n,
e: key.e
}
Copy link
Member

Choose a reason for hiding this comment

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

Where would an exception be thrown in this operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Never underestimate the power of undefined: TypeError: Cannot read property 'e' of undefined

Copy link
Member

Choose a reason for hiding this comment

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

True, but in this case, it would never through that error, only if you did something like key.e.something. As long as if key exists and it is an object, key.e will never throw

Copy link

Choose a reason for hiding this comment

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

reverted this, but now inside a setImmediate.

Copy link
Member Author

Choose a reason for hiding this comment

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

And what about unmarshalPrivateKey(undefined, (err, res) => {}) ??

Copy link

Choose a reason for hiding this comment

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

@mkg20001 good point, fixing.

test/testCb.js Outdated
cb()
})

fnc(...args)
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use this operator for now (we are still avoiding some ES6 features)

@pgte pgte self-assigned this Nov 30, 2017
@pgte
Copy link

pgte commented Nov 30, 2017

Going to push some changes here to help this move along.

@pgte
Copy link

pgte commented Nov 30, 2017

@diasdavid should be go to go. Please tell if there's anything else you'd like to see addressed here.

@daviddias
Copy link
Member

Thanks @pgte :) @victorbjelkholm could you check what's wrong with Circle?

@daviddias
Copy link
Member

@victorbjelkholm seems that ci-sync never got here. Mind checking?

Going to merge for now.

@daviddias daviddias merged commit 7608fdd into master Dec 1, 2017
@daviddias daviddias deleted the fix/crash branch December 1, 2017 08:36
@ghost ghost removed the status/in-progress In progress label Dec 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants