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

raise error on undefined secrets #65

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

borromeotlhs
Copy link

Change to SignStream class as it uses DataStream objects to create secret and payload streams. This error should then be caught by jwsSign to adequately fail users downstream instead of failing silently.

Change to SignStream class as it uses DataStream objects to create secret and payload streams.  This error should then be caught by jwsSign to adequately fail users downstream instead of failing silently.
All tests expect an undefined to be returned on failure, so using a naked return back to jwsSign rather than raising an explicit ReferenceError.
@willm
Copy link

willm commented May 4, 2017

You need to handle the case where SignStream takes a stream. Maybe the check in dataStream could be pushed higher up?

@borromeotlhs
Copy link
Author

Yeah, I'm failing the streaming signing of RSA tests specifically because of this. Trying to figure out a way to do just that, it just sucks that I can't run automated build and tests without creating PRs (failing in public is fun, right?)

@willm
Copy link

willm commented May 4, 2017

You can, just run npm test

@borromeotlhs
Copy link
Author

I don't have a node server at work, so I edit in atom directly and then create PRs. I'm not even using this library, yet, but got down this rabbit hole when investigating using jwts for authentication on an app I'm working on in cloud9, then saw this issue. . . ;)

undefined keys should raise no value, so that errors during jws.Sign can emit an error.
added tests to ensure the same are check upon HMAC testing
@borromeotlhs
Copy link
Author

There, I added the test reported from the issue that raised this.

I did a naked return whenever a secret is undefined to make an undefined value propagate up through to jws.Sign, at which point, an 'error' should emit.

@willm , does this address your concern without having to change DataStream directly for streams? I'd love a suggestion on how to write a test to catch the emit('error') functions though, for completeness.

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