createSign doesn't emit an error when the secret is undefined

[zbinlin]

jws.createSign({
  header: { alg: 'HS256' },
  secret: undefined,
  payload: 'hhhh'
}).on('error', function (err) {
  console.error(err);
}).on('done', function(signature) {
  console.log(signature);
});

[cbaron]

I am experiencing the same thing. Please contact me for more information. I thought you addressed this months ago?

[robertrossmann]

I found the root cause, but I am currently too busy providing a PR with a fix (you know, Christmas & all that stuff).

When you provide a secret which is null or undefined (falsy value), it gets passed in here to the DataStream: https://github.com/brianloveswords/node-jws/blob/master/lib/sign-stream.js#L29

Later on, the SignStream attaches a single "close" event listener to the DataStream object which, when fired, will mark the SignStream as finished: https://github.com/brianloveswords/node-jws/blob/master/lib/sign-stream.js#L35

The problem: The DataStream implementation checks for the secret to be truthy, but if it is not, it will simply return itself and will not emit any event: https://github.com/brianloveswords/node-jws/blob/master/lib/data-stream.js#L12

This leaves the SignStream hanging and patiently waiting for the "close" event to be fired, but at this point it is quite certain it will never happen.

I am not certain what the correct behaviour should be in this case:

• emit the "close" event even when no input has been provided
• emit an "error" event
• throw an error

If someone can make a decision what the correct behaviour is I will attempt to fix this and send a PR next year. 😄 (I mean January, of course). Unless someone beats me to it, now that you know how to fix it...

Thanks for your input and help!

[willm]

Hi, I just hunted down a bug due to this. I think the safest thing to do here is to emit an error event. It seems to me that attempting to sign with an empty key is a programmer error. I think there is also an issue where the DataStream constructor ends synchronously in this case, but async in the others (by registering process.nextTick or piping the stream. This is causing the callback in the calling package I'm using (jsonwebtoken) to never get called.

[borromeotlhs]

I made pr #65 , but have to work to make it emit an error in order to satisfy the automated build tests. . .

[willm]

I feel like the only way to change this is to make a breaking change to not allow a DataStream to be created without any data. I'm not sure what implications that has though?

[borromeotlhs]

I don't think so, I'm trying to create a test for failures during signing (without verifying), to enable testing of signing with empty strings while also failing on undefined strings.

Data should be allowed to be empty, especially streams, but the secret signing using a data stream may need to get its own data stream constructor ( maybe) to prevent these types of breaking changes at worst. I'm hoping to create a change in sign stream to deal with invalid secrets, while keeping data streams intact. . .

On Thu, May 4, 2017, 15:59 Will Munn notifications@github.com wrote:

I feel like the only way to change this is to make a breaking change to not allow a DataStream to be created without any data. I'm not sure what implications that has though?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/brianloveswords/node-jws/issues/62#issuecomment-299292510, or mute the thread https://github.com/notifications/unsubscribe-auth/ABYaerIxzCCkUEaTkQN8YmUXeNI8IoXTks5r2i4CgaJpZM4KxStv .