Copy array buffer data used by WebCrypto in the order expected by draft specification.

Copy array buffer data used by WebCrypto in the order expected by draft specification.

* Most data buffers to crypto.subtle.* are copied prior to algorithm normalization.

* Data buffers that are part of algorithm dictionaries are copied and read in the declaration order of the properties.

BUG=626948
Committed: https://crrev.com/923aa7b0442d331e07b1853acd4f40d3a99a35ce
Cr-Commit-Position: refs/heads/master@{#406170}

[eroman, 2016-07-12 01:44:45]

eroman@chromium.org changed reviewers:
+ jochen@chromium.org

[eroman, 2016-07-12 01:44:45]

@jochen could you suggest a reviewer for this? Thanks!

(I would normally try jww@, but he is out right now)

Some questions for the reviewer:

 * I wasn't sure what error type this should be, so chose TypeError. Alternately
I could treat these as empty buffers but that seems somewhat unexpected.

 * I wrote tests using the old harness not W3C one in order to match the
existing tests, and write in familiar format. I can convert these in a follow-up
change.

[jochen (gone - plz use gerrit), 2016-07-12 15:13:13]

jochen@chromium.org changed reviewers:
+ foolip@chromium.org, haraken@chromium.org

[jochen (gone - plz use gerrit), 2016-07-12 15:13:13]

haraken@ for the actual code changes
foolip@ for what exception to throw

[haraken, 2016-07-13 01:18:17]

On 2016/07/12 15:13:13, jochen wrote:
> haraken@ for the actual code changes
> foolip@ for what exception to throw

The actual code change LGTM.

[foolip, 2016-07-13 14:27:39]

https://codereview.chromium.org/2141843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/2141843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp:155: // The spec
doesn't specifically say what error to return when the
Looks like spec always uses
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#concept-...,
which is defined in terms of ArrayBuffer.prototype.slice.

https://www.khronos.org/registry/typedarray/specs/latest/ doesn't seem to
actually say what should happen, but it talks about "computed length" which is
never defined as a concept. If the computed length should be zero when neutered,
then there would be no special exception to throw.

Rejecting for a neutered array buffer makes more sense than just behavior as if
it was empty, so I guess talk to Ryan Sleevi about fixing this in the spec?

[foolip, 2016-07-13 14:28:19]

https://codereview.chromium.org/2141843002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/2141843002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp:155: // The spec
doesn't specifically say what error to return when the
On 2016/07/13 14:27:39, foolip wrote:
> Looks like spec always uses
>
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#concept-...,
> which is defined in terms of ArrayBuffer.prototype.slice.
> 
> https://www.khronos.org/registry/typedarray/specs/latest/ doesn't seem to
> actually say what should happen, but it talks about "computed length" which is
> never defined as a concept. If the computed length should be zero when
neutered,
> then there would be no special exception to throw.
> 
> Rejecting for a neutered array buffer makes more sense than just behavior as
if
> it was empty, so I guess talk to Ryan Sleevi about fixing this in the spec?

s/behavior/behaving/

[eroman, 2016-07-13 17:51:56]

Thanks Philip!

Note, the current WebCrypto spec is actually this URL:
https://w3c.github.io/webcrypto/Overview.html

It refers to "getting a copy of the bytes" [1] [2], and my confusion stems from
the linked WebIDL spec [3] saying that getting bytes from a detached buffer
result in implementation-specific error.

OK so I will proceed as follows:

(1) I will update this CL shortly to *not* throw an error, but instead consider
these successful reads of 0 bytes. This is more consistent with how other code
handles neutered array buffers in Blink today. But in doing so I will obviously
address the particular implementation bug in WebCrypto's code that I am trying
to fix.

(2) I will follow-up with the WebCrypto spec (note that rsleevi is no longer an
editor) and raise the question if we should special-case reading from a neutered
buffers to throw an exception in the spec. The argument for doing so would be
that it is potentially concerning to use an unexpectedly truncated buffer for a
security-oriented API, rather than just failing.

(3) I suggest adding clarity to the WebIDL/TypedArray specifications around
these cases. Which by my reading at least seem ambiguous today.

[1]
https://w3c.github.io/webcrypto/Overview.html#dfn-SubtleCrypto-method-encrypt

[2]
https://w3c.github.io/webcrypto/Overview.html#algorithm-normalization-normali...

[eroman, 2016-07-14 00:51:27]

Description was changed from

==========
Reject WebCrypto (crypto.subtle.*) calls with a TypeError when given
a neutered BufferSource.

BUG=626948
==========

to

==========
Copy array buffer data used by WebCrypto in the order expected by draft
specification.

* Most data buffers to crypto.subtle.* are copied prior to algorithm
normalization.

* Data buffers that are part of algorithm dictionaries are copied and read in
the declaration order of the properties.

BUG=626948
==========

[eroman, 2016-07-14 01:36:03]

PTAL, I have updated the change to longer throw an exception.

[eroman, 2016-07-18 18:54:06]

ping

[haraken, 2016-07-18 22:41:57]

Actual code change still LGTM

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp:49: // FIXME:
Change the interface for constructing WebCryptoAlgorithmParams

TODO(your name)

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp:47: // FIXME: Change
the public blink::WebCrypto interface to allow transferring

TODO(your name)

[eroman, 2016-07-18 23:15:25]

Thanks, I will go ahead and land.

@foolip: If I have failed to address your feedback, I will correct that in a
follow-up CL.

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp:49: // FIXME:
Change the interface for constructing WebCryptoAlgorithmParams
On 2016/07/18 22:41:57, haraken wrote:
> 
> TODO(your name)

Done.

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp (right):

https://codereview.chromium.org/2141843002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/crypto/SubtleCrypto.cpp:47: // FIXME: Change
the public blink::WebCrypto interface to allow transferring
On 2016/07/18 22:41:57, haraken wrote:
> 
> TODO(your name)

Done.

[eroman, 2016-07-18 23:18:39]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-07-18 23:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2141843002/#ps80001
(title: "Change FIXME --> TODO(eroman)")

[commit-bot: I haz the power, 2016-07-18 23:19:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 01:51:05]

Description was changed from

==========
Copy array buffer data used by WebCrypto in the order expected by draft
specification.

* Most data buffers to crypto.subtle.* are copied prior to algorithm
normalization.

* Data buffers that are part of algorithm dictionaries are copied and read in
the declaration order of the properties.

BUG=626948
==========

to

==========
Copy array buffer data used by WebCrypto in the order expected by draft
specification.

* Most data buffers to crypto.subtle.* are copied prior to algorithm
normalization.

* Data buffers that are part of algorithm dictionaries are copied and read in
the declaration order of the properties.

BUG=626948
==========

[commit-bot: I haz the power, 2016-07-19 01:51:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-19 01:51:15]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 01:52:51]

Description was changed from

==========
Copy array buffer data used by WebCrypto in the order expected by draft
specification.

* Most data buffers to crypto.subtle.* are copied prior to algorithm
normalization.

* Data buffers that are part of algorithm dictionaries are copied and read in
the declaration order of the properties.

BUG=626948
==========

to

==========
Copy array buffer data used by WebCrypto in the order expected by draft
specification.

* Most data buffers to crypto.subtle.* are copied prior to algorithm
normalization.

* Data buffers that are part of algorithm dictionaries are copied and read in
the declaration order of the properties.

BUG=626948

Committed: https://crrev.com/923aa7b0442d331e07b1853acd4f40d3a99a35ce
Cr-Commit-Position: refs/heads/master@{#406170}
==========

[commit-bot: I haz the power, 2016-07-19 01:52:52]

Patchset 5 (id:??) landed as
https://crrev.com/923aa7b0442d331e07b1853acd4f40d3a99a35ce
Cr-Commit-Position: refs/heads/master@{#406170}