{"action":"created","issue":{"url":"https://api.github.com/repos/amper5and/secrets.js/issues/8","labels_url":"https://api.github.com/repos/amper5and/secrets.js/issues/8/labels{/name}","comments_url":"https://api.github.com/repos/amper5and/secrets.js/issues/8/comments","events_url":"https://api.github.com/repos/amper5and/secrets.js/issues/8/events","html_url":"https://github.com/amper5and/secrets.js/issues/8","id":128800069,"number":8,"title":"Share hex length is (I think) always invalid (odd)","user":{"login":"jgeewax","id":112928,"avatar_url":"https://avatars.githubusercontent.com/u/112928?v=3","gravatar_id":"","url":"https://api.github.com/users/jgeewax","html_url":"https://github.com/jgeewax","followers_url":"https://api.github.com/users/jgeewax/followers","following_url":"https://api.github.com/users/jgeewax/following{/other_user}","gists_url":"https://api.github.com/users/jgeewax/gists{/gist_id}","starred_url":"https://api.github.com/users/jgeewax/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jgeewax/subscriptions","organizations_url":"https://api.github.com/users/jgeewax/orgs","repos_url":"https://api.github.com/users/jgeewax/repos","events_url":"https://api.github.com/users/jgeewax/events{/privacy}","received_events_url":"https://api.github.com/users/jgeewax/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2016-01-26T11:59:49Z","updated_at":"2016-01-29T00:22:57Z","closed_at":null,"body":"The issue looks to be with the prefixing, which always appends an \"8\" (https://github.com/amper5and/secrets.js/blob/master/secrets.js#L246)\r\n\r\nFrom what I can see, the following lines aren't quite right:\r\n\r\n```js\r\nfor(var i=0; i<numShares; i++){\r\n    x[i] = config.bits.toString(36).toUpperCase() + padLeft(x[i],padding) + bin2hex(y[i]);\r\n}\r\n```\r\n\r\n1. Why are we using base 36? For a larger number than 8, (ie, 200), there's no way that this will generate a valid hexadecimal value... (`(200).toString(36) == '5k'`)\r\n\r\n1. Assuming we meant this to be `16` instead of `36` (which makes much more sense), it still can result in an odd-length string, which is :-1: (`(8).toString(16) == '8'`)\r\n  - To illustrate why this is a bad time, here's a traceback:\r\n  ```js\r\n  > new Buffer((8).toString(16), 'hex');\r\nTypeError: Invalid hex string\r\n    at TypeError (native)\r\n    at Buffer.write (buffer.js:568:21)\r\n    at fromString (buffer.js:115:26)\r\n    at new Buffer (buffer.js:54:12)\r\n    at repl:1:1\r\n    at REPLServer.defaultEval (repl.js:248:27)\r\n    at bound (domain.js:287:14)\r\n    at REPLServer.runBound [as eval] (domain.js:300:12)\r\n    at REPLServer.<anonymous> (repl.js:412:12)\r\n    at emitOne (events.js:82:20)\r\n  ```\r\n\r\nI *think* we should do what you did a bit later in the line with `padLeft`:\r\n\r\n```js\r\nfor(var i = 0; i < numShares; i++) {\r\n    // x[i] is (even-length number of bits, any padding requested, and then the secret share)\r\n    var prefix = padLeft(config.bits.toString(16).toUpperCase(), 2),\r\n        padding = padLeft(x[i], padding);\r\n    x[i] = prefix + padding + bin2hex(y[i]);\r\n}\r\n```\r\n\r\nThe question then becomes, what corresponding changes do we need for the combining portion of code...?\r\n\r\nI think we would need to update the `processShare` method to make this work (https://github.com/amper5and/secrets.js/blob/master/secrets.js#L301)\r\n\r\n(the lines affected are `var bits = ...` and `var id = ...`)\r\n\r\n```js\r\nfunction processShare(share){\r\n\tvar bits = parseInt(share.substring(0, 2), 16);\r\n\tif(bits && (typeof bits !== 'number' || bits%1 !== 0 || bits<defaults.minBits || bits>defaults.maxBits)){\r\n\t\tthrow new Error('Number of bits must be an integer between ' + defaults.minBits + ' and ' + defaults.maxBits + ', inclusive.')\r\n\t}\r\n\t\r\n\tvar max = Math.pow(2, bits) - 1;\r\n\tvar idLength = max.toString(config.radix).length;\r\n\t\r\n\tvar id = parseInt(share.substr(2, idLength), config.radix);\r\n\tif(typeof id !== 'number' || id%1 !== 0 || id<1 || id>max){\r\n\t\tthrow new Error('Share id must be an integer between 1 and ' + config.max + ', inclusive.');\r\n\t}\r\n\tshare = share.substr(idLength + 1);\r\n\tif(!share.length){\r\n\t\tthrow new Error('Invalid share: zero-length share.')\r\n\t}\r\n\treturn {\r\n\t\t'bits': bits,\r\n\t\t'id': id,\r\n\t\t'value': share\r\n\t};\r\n};"},"comment":{"url":"https://api.github.com/repos/amper5and/secrets.js/issues/comments/176494757","html_url":"https://github.com/amper5and/secrets.js/issues/8#issuecomment-176494757","issue_url":"https://api.github.com/repos/amper5and/secrets.js/issues/8","id":176494757,"user":{"login":"grempe","id":117,"avatar_url":"https://avatars.githubusercontent.com/u/117?v=3","gravatar_id":"","url":"https://api.github.com/users/grempe","html_url":"https://github.com/grempe","followers_url":"https://api.github.com/users/grempe/followers","following_url":"https://api.github.com/users/grempe/following{/other_user}","gists_url":"https://api.github.com/users/grempe/gists{/gist_id}","starred_url":"https://api.github.com/users/grempe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/grempe/subscriptions","organizations_url":"https://api.github.com/users/grempe/orgs","repos_url":"https://api.github.com/users/grempe/repos","events_url":"https://api.github.com/users/grempe/events{/privacy}","received_events_url":"https://api.github.com/users/grempe/received_events","type":"User","site_admin":false},"created_at":"2016-01-29T00:22:57Z","updated_at":"2016-01-29T00:22:57Z","body":"@jgeewax I wonder if you would be interested in taking a look at my fork of this project and seeing if you have similar issues still there.  I don't think any changes have been accepted on this project for a couple of years. I did a pretty major revamp on the fork and perhaps it resolved your issues.  I would certainly take a look at pull requests if there is an issue you are still seeing.\r\n\r\nhttps://github.com/grempe/secrets.js\r\n\r\nGlenn"}}