General comments on the code about http-hmac-javascript HOT 6 CLOSED

Hey @nickveenhof, thank you very much for your detailed code review! Both immediate implementable items are marked "implemented as explained" and done here, others are commented as follows. I didn't get to the bottom and will continue tomorrow. =)

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L19
Why is the nonce filled in here? A nonce is always randomly generated, so it's not really configurable. It's to make sure that the same request in the same second can still be uniquely identified.

Shay's comment:
Implemented as explained.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L22
Default content type? Why is this necessary? This has nothing to do with the signing library. The implementation should set this for each request, not as a global config. And the library should actually take this from the headers, not from the config.

Shay's comment:
`How the code works here is as layers of fallbacks:

1. JSON as the "default content type" that is likely the library's most use cases (if not all, in here).
2. Moreover, in instance constructor, the developer gets to set the instance's default content type.
3. Furturemore, when actually signing a request, the developer has the option of defaulting to the instance's format, or specifying a new format.`

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L17
client_id is not the right word here, it's plain id or public_key. Client id is application specific. The application implementing this library can choose to add a client_id query param but that's completely up to the implementation.

Shay's comment:
Good to know. Implemented with public_key instead. Do we also want to rename "client ID" wording the specification?

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L30
Any reason why we would support CUSTOM? Also, all methods should be supported. There is nothing in the specification that limits any HTTP Verb. The only difference is if there is a body to be submitted or not.

Shay's comment:
Modern browsers supports XMLHttpRequest with only subset of all methods (as listed), and not others, due to insecurity. There is no reason why CUSTOM should not be supported by this JS library; there are only reasons why CUSTOM is not supported at the actual server implementation.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L116
additional headers should only be added to the signature when they exist.

Shay's comment:
`You probably meant one of the two things, and answers are:

1. I cannot verify against the current XMLHttpRequest because it only supports setRequestHeader() - and no getRequestHeader().
2. That string is empty if there is no additional signed header. And skipping the "\n" will cause the proxy to fail.`

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L118
content_type should only be added when there is a body in the request

Shay's comment:
I hear ya. Unfortunately, acquia-auth-proxy rejects when content_type is omitted, and the proxy is my only testing method. Are we able to fix acquia-auth-proxy? I can do it if you are not free.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L119
x_authorization_content_sha256 when there is a body in the request

Shay's comment:
That variable is an empty string when body is empty. And skipping the "\n" will cause the proxy to fail.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L123
signed headers should not be added to the authorization string when they do not exist.

Shay's comment:
When there is no signed header, the signed_headers_string is an empty string. Also, I can't skip the headers="" because the proxy will fail.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L106
We do not care about the method here. Only the body length. This way someone can't fool the algorithm by sending a GET with a body. It should not happen, but we should also not expect it won't happen.

Shay's comment:
When a XMLHttpRequest is opened at "GET" method, its send() method skips body, silently. That condition checking keeps the code's behavior in line with XMLHttpRequest.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L111
method has to be uppercase, I don't see you checking this.

Shay's comment:
To find the checking, go to #L54.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L112
hostname needs to be lowercase

Shay's comment:
To find the parsing to lowercase, go to #L105.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L107
The secret key needs to be decoded from base64 encoding first before using it.

Shay's comment:
Too late to respond to this today. Will fill in tomorrow. =)

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L146
The secret key needs to be decoded from base64 encoding first before using it.

Shay's comment:
Too late to respond to this today. Will fill in tomorrow. =)

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L115
I don't see it being urlencoded. Most likely that doesn't matter much but it is in the specification.

Shay's comment:
Too late to respond to this today. Will fill in tomorrow. =)

from http-hmac-javascript.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L17
client_id is not the right word here, it's plain id or public_key. Client id is application specific. The application implementing this library can choose to add a client_id query param but that's completely up to the implementation.

Shay's comment:
Good to know. Implemented with public_key instead. Do we also want to rename "client ID" wording the specification?

Nick's Reply:
I don't think we have the wording client_id in the spec of http-hmac-spec 2.0? We do have ID which is good. In the decision api we use the public key as the identifier, but it can very well be something else. id/public_key != client_id.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L30
Any reason why we would support CUSTOM? Also, all methods should be supported. There is nothing in the specification that limits any HTTP Verb. The only difference is if there is a body to be submitted or not.

Shay's comment:
Modern browsers supports XMLHttpRequest with only subset of all methods (as listed), and not others, due to insecurity. There is no reason why CUSTOM should not be supported by this JS library; there are only reasons why CUSTOM is not supported at the actual server implementation.

Nick's comment:
I'm fine with a supported subset of http verbs as long as you are not using them for checking if we need to validate the post body. All http verbs can contain that and have the additional checks accordingly.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L116
additional headers should only be added to the signature when they exist.

Shay's comment:
You probably meant one of the two things, and answers are: 1) I cannot verify against the current XMLHttpRequest because it only supports setRequestHeader() - and no getRequestHeader(). 2) That string is empty if there is no additional signed header. And skipping the "\n" will cause the proxy to fail.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L118
content_type should only be added when there is a body in the request

Shay's comment:
I hear ya. Unfortunately, acquia-auth-proxy rejects when content_type is omitted, and the proxy is my only testing method. Are we able to fix acquia-auth-proxy? I can do it if you are not free.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L119
x_authorization_content_sha256 when there is a body in the request

Shay's comment:
That variable is an empty string when body is empty. And skipping the "\n" will cause the proxy to fail.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L123
signed headers should not be added to the authorization string when they do not exist.

Shay's comment:
When there is no signed header, the signed_headers_string is an empty string. Also, I can't skip the headers="" because the proxy will fail.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L106
We do not care about the method here. Only the body length. This way someone can't fool the algorithm by sending a GET with a body. It should not happen, but we should also not expect it won't happen.

Shay's comment:
When a XMLHttpRequest is opened at "GET" method, its send() method skips body, silently. That condition checking keeps the code's behavior in line with XMLHttpRequest.

Nick's Comment:
Sure, that works for me, but just make sure you don't rely too much on assumptions of browsers, that's all.

from http-hmac-javascript.

3 of them are very similar and are a change in the auth-proxy that happened recently. The auth proxy is backwards compatible but the output should tell you whether the hash you sent to it is a legacy hash or regular one.

from http-hmac-javascript.

All applicable changes are implemented in 7fc21a6. All changes should be using non-legacy path.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L17
client_id is not the right word here, it's plain id or public_key. Client id is application specific. The application implementing this library can choose to add a client_id query param but that's completely up to the implementation.

Shay's comment:
Good to know. Implemented with public_key instead. Do we also want to rename "client ID" wording the specification?

Nick's Reply:
I don't think we have the wording client_id in the spec of http-hmac-spec 2.0? We do have ID which is good. In the decision api we use the public key as the identifier, but it can very well be something else. id/public_key != client_id.

Shay's comment:
If you search for "client ID" in the spec, you will find the one that I am talking about. My personal preference here is that for the purpose of disambiguation, a spec should mention only one name for each concept. (e.g. use "personalization" and never use "campaign"). "public_key" is probably the most widely accepted therefore most suitable name in this context, but, I think sticking with one name like "id" in the spec is good enough.

This is not about blaming the spec, though.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L116
additional headers should only be added to the signature when they exist.

Shay's comment:
You probably meant one of the two things, and answers are: 1) I cannot verify against the current XMLHttpRequest because it only supports setRequestHeader() - and no getRequestHeader(). 2) That string is empty if there is no additional signed header. And skipping the "\n" will cause the proxy to fail.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

Shay's comment:
I see the "added signed headers" support is turned off at the proxy. Implemented to match the latest proxy behavior.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L118
content_type should only be added when there is a body in the request

Shay's comment:
I hear ya. Unfortunately, acquia-auth-proxy rejects when content_type is omitted, and the proxy is my only testing method. Are we able to fix acquia-auth-proxy? I can do it if you are not free.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

Shay's comment:
Implemented to match the latest proxy behavior.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L119
x_authorization_content_sha256 when there is a body in the request

Shay's comment:
That variable is an empty string when body is empty. And skipping the "\n" will cause the proxy to fail.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

Shay's comment:
Implemented to match the latest proxy behavior.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L123
signed headers should not be added to the authorization string when they do not exist.

Shay's comment:
When there is no signed header, the signed_headers_string is an empty string. Also, I can't skip the headers="" because the proxy will fail.

Nick's comment:
Make sure you use the last version of the auth-proxy as it was adjusted to work in both cases. However, omitting the \n in case of an empty value is what is expected according to the latest specification. Reach out to me if you are having issues making this work. See LEX-150 for the changes we did to the Drupal module to make this work.

Shay's comment:
Implemented to match the latest proxy behavior.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L107
The secret key needs to be decoded from base64 encoding first before using it.

Shay's comment:
Implemented to match the latest proxy behavior.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L146
The secret key needs to be decoded from base64 encoding first before using it.

Shay's comment:
Implemented to match the latest proxy behavior.

https://github.com/acquia/http-hmac-javascript/blob/LEX-88/src/hmac.js#L115
I don't see it being urlencoded. Most likely that doesn't matter much but it is in the specification.

Shay's comment:
Sounds like the proxy is using all url.QueryEscape() but meant url.QueryUnescape()? May need a fix there. Headers and signature don't seem to be unescaped at proxy yet.

I also think we should support non-ASCII characters only for "signed headers", and none else. I can only identify insignificant (or no) value in introducing non-ASCII char support for id, nonce, realm, version, or signature, only encoding/decoding efforts.

from http-hmac-javascript.

Great - looks like the only confusion left is the url encoding. Imho we can wait until that becomes a problem. I don't think it's an issue right now.

from http-hmac-javascript.

*Thumbs

from http-hmac-javascript.