Skip to content

When callback is passed to sign(), if typeof payload is a String, incorrectly stringifies payoad, resulting in extra "" marks #224

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

Closed
shady-gabal opened this issue Jul 1, 2016 · 9 comments

Comments

@shady-gabal
Copy link

When a callback is passed to sign(), if a string is passed in as the payload, it stringifies the String anyway, resulting in extra quotation marks.

in sign.js, around line 126:

  if(typeof callback === 'function') {
    jws.createSign({
      header: header,
      privateKey: secretOrPrivateKey,
      payload: JSON.stringify(payload),//problem is here
      encoding: encoding
    })

should be something like:

  if(typeof callback === 'function') {
var modifiedPayload = (typeof payload === 'string') ? payload : JSON.stringify(payload);

    jws.createSign({
      header: header,
      privateKey: secretOrPrivateKey,
      payload: modifiedPayload,
      encoding: encoding
    })

Haven't spent too much time looking at the implementation, but you might also not need to even stringify since in the corresponding else block, you don't bother with stringifying at all:

 } else {
    return jws.sign({header: header, payload: payload, secret: secretOrPrivateKey, encoding: encoding});
  }
@shady-gabal
Copy link
Author

To anyone wondering how to avoid this issue: just run it synchronously instead (without a callback) and you should be good.

@jfromaniello
Copy link
Member

You are right, I am going to fix this now.

@jfromaniello
Copy link
Member

published as v7.1.3. Thank you for reporting this

@zbjornson
Copy link

zbjornson commented Jul 14, 2016

This fix seems to have broken signing of objects.

test.js:

require("jsonwebtoken").sign({abc: 1}, "secret", {}, function (err, res) {
    console.log(err, res);
});

with 7.1.1

> node test.js
null 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhYmMiOjEsImlhdCI6MTQ2ODUyMzYxOH0.Hc8FzmdQhMwVGg0w5MQCNQGGvKHCaNpj_aG0nKt149I'

with 7.1.3 (7.1.2 does not exist):

> node test.js
buffer.js:287
  throw new TypeError(kFromErrorMsg);
  ^

TypeError: First argument must be a string, Buffer, ArrayBuffer, Array, or array-like object.
    at fromObject (buffer.js:287:9)
    at Function.Buffer.from (buffer.js:133:10)
    at Buffer (buffer.js:112:17)
    at new DataStream (C:\Users\usr\Documents\GitHub\test\node_modules\jws\lib\data-stream.js:7:17)
    at new SignStream (C:\Users\usr\Documents\GitHub\test\node_modules\jws\lib\sign-stream.js:34:18)
    at Object.createSign (C:\Users\usr\Documents\GitHub\test\node_modules\jws\index.js:17:10)
    at Object.module.exports [as sign] (C:\Users\usr\Documents\GitHub\test\node_modules\jsonwebtoken\sign.js:127:9)
    at Object.<anonymous> (C:\Users\usr\Documents\GitHub\test\test.js:3:5)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)

@jfromaniello
Copy link
Member

jfromaniello commented Jul 14, 2016

@zbjornson I can't reproduce this behaviour on master...I think you might be using a different version of JWS than the one pointed by this module. I notice in your stack trace that you have jsonwebtoken and jws at the same level.

@zbjornson
Copy link

zbjornson commented Jul 14, 2016

@jfromaniello (They're at the same level because we're using npm 3.) Ah, this is an incompatibility between [email protected] and jws@<3.1.1.

jsonwebtoken jws status?
7.1.1 3.0.0 pass
7.1.1 3.1.3 pass
7.1.3 3.0.0 fail
7.1.3 3.1.0 fail
7.1.3 3.1.1 pass
7.1.3 3.1.2 pass
7.1.3 3.1.3 pass

This commit fixed 7.1.3+3.1.1: auth0/node-jws@7682e03

Updating your package.json to something like "3.1.1 - 3.1.999" would fix this then. :) (There's no other way to spec "any 3.1.x patch level greater than 3.1.1" with semver that I know of.)

@jfromaniello
Copy link
Member

@zbjornson done! Thanks for testing this. I just fixed at v7.1.5

@zbjornson
Copy link

Thanks for the prompt fix!

(I didn't realize ^ matches "at least this patch level" :-p)

@jfromaniello
Copy link
Member

^3.1.3 matches starting from 3.1.3 all minor/patches of 3

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

No branches or pull requests

3 participants