Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Updating methods to use buffers, adding tests, addressing feedback #164

Merged
merged 1 commit into from Sep 3, 2013

Conversation

vladikoff
Copy link
Contributor

No description provided.

@vladikoff
Copy link
Contributor Author

@warner r?

* @param {String} url scrypt helper server url
* @returns {Object} d.promise Deferred promise
*/
function hash(input, salt, url) {
var p
var payload = {
salt: salt,
salt: salt.toString('utf8'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should treat the salt as an arbitrary binary string. So encode it as hex when we send it to the remote helper, and "encode" it as utf-8 only for the (hopefully temporary) case where we're passing it to a node-scrypt module that doesn't accept a Buffer.

Does the remote scrypt helper expect a regular JSON string as the salt? If so, we should fix that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah to get that fixed we need to update the helper I think
//cc @zaach

@vladikoff
Copy link
Contributor Author

@warner I've fixed a few of these. For 3 of the issues we need to update the sjcl bundle, I talked to @zaach about this and he said he can take care of this. This should be good to merge unless there are other issues.

@zaach
Copy link
Contributor

zaach commented Sep 3, 2013

👍 I'll take a look at getting around sjcl's missing bytes codec.

zaach added a commit that referenced this pull request Sep 3, 2013
Updating methods to use buffers, adding tests, addressing feedback
@zaach zaach merged commit 8379e21 into mozilla:master Sep 3, 2013
@ckarlof
Copy link
Contributor

ckarlof commented Sep 4, 2013

I'll take a look at getting around sjcl's missing bytes codec.

@zaach, please create an issue for this if you haven't already. thanks!

@zaach zaach mentioned this pull request Sep 4, 2013
rfk pushed a commit that referenced this pull request Oct 24, 2018
The AppError constructor accepts a `stack` property in the options,
but previously always assigned `this.stack` to it, leading to Errors
with no stack trace. This was confusing our logging infrastructure, which
tries to improve the format of the `stack` property.

Closes #164
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.

None yet

4 participants