Add Hawk support for authenticating requests to the sync storage server

VERIFIED FIXED in mozilla28

Status

Cloud Services
Firefox Sync: Backend
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: ckarlof, Assigned: warner)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

4 years ago
Also see: https://bugzilla.mozilla.org/show_bug.cgi?id=820643 This should be a minor change, since we already support MACAuth used in Oauth2.
Whiteboard: [qa+]
Blocks: 912219
(Assignee)

Comment 1

4 years ago
Created attachment 801865 [details] [diff] [review]
initial patch

Here's the first cut. Tests (with vectors copied from the node.js HAWK test suite) pass, but could use more coverage. I went with the node.js API to start, this may or may not fit into our style.

(it's also my first m-c patch.. I need to learn both the functionality and the style of this strange new neighborhood, so I'm eager for any and all feedback you can give).
Attachment #801865 - Flags: feedback?(rnewman)
Comment on attachment 801865 [details] [diff] [review]
initial patch

Review of attachment 801865 [details] [diff] [review]:
-----------------------------------------------------------------

I tried to be thorough to illustrate some style points. Looking good!

::: services/crypto/modules/utils.js
@@ +55,5 @@
> +   * Treat the given message as a bytes string and feed them into the given
> +   * hasher. Does not return a hash. This can be called multiple times with a
> +   * single hasher, but eventually you must extract the result yourself.
> +   */
> +  updateBytes: function digestBytes(message, hasher) {

Kill the function name; the debugger will pull it out of the object reference (and in any case, they disagree!).

@@ +57,5 @@
> +   * single hasher, but eventually you must extract the result yourself.
> +   */
> +  updateBytes: function digestBytes(message, hasher) {
> +    let bytes = [b.charCodeAt() for each (b in message)];
> +    hasher.update(bytes, bytes.length);

This doesn't seem particularly safe in the presence of non-ASCII:

[18:52:01.079] "fi".charCodeAt(0)
[18:52:01.082] 64257

nor efficient. Fortunately, this same file provides the idiom:

    let data = this._utf8Converter.convertToByteArray(message, {});
    hasher.update(data, data.length);

@@ +353,5 @@
>      return header += ', ext="' + ext +'"';
>    },
> +
> +  /**
> +   * Compute the HAWK header for an HTTP request.

This is really "HAWK header values", right?

@@ +372,5 @@
> +   *           nonce - (string) pre-calculated nonce. Should only be defined
> +   *             for testing as this function will generate a
> +   *             cryptographically secure random one if not defined.
> +   *           localtimeOffsetMsec - (number) local clock offset (vs server)
> +   *           payload - (string) payload to include in hash. ENCODING?

UTF-8.

@@ +373,5 @@
> +   *             for testing as this function will generate a
> +   *             cryptographically secure random one if not defined.
> +   *           localtimeOffsetMsec - (number) local clock offset (vs server)
> +   *           payload - (string) payload to include in hash. ENCODING?
> +   *           contentType - (string) payload Content-Type

Caller must include charset?

@@ +389,5 @@
> +   *             port - (number)
> +   *             hash - (string) payload hash
> +   *             ext - (string) app-specific data
> +   */
> +  computeHAWKHeader: function computeHAWKHeader(uri, method, options) {

Again, kill function name.

And probably

  computeHAWKHeaderValues

'cos you're not returning a string header, right?

@@ +391,5 @@
> +   *             ext - (string) app-specific data
> +   */
> +  computeHAWKHeader: function computeHAWKHeader(uri, method, options) {
> +    let credentials = options.credentials;
> +    let ts = options.ts || Math.floor((Date.now()+(options.localtimeOffsetMsec || 0)) / 1000);

Nit: spaces around arithmetic operators.

Another thing to consider: for testability, it's often convenient to be able to impose an instant in time. Think about whether you want to add

  , now=Date.now()

to the function signature.

@@ +409,5 @@
> +      ts: ts,
> +      nonce: options.nonce || btoa(CryptoUtils.generateRandomBytes(8)),
> +      method: method.toUpperCase(),
> +      resource: uri.path, // TODO: include .search ?
> +      host: uri.asciiHost.toLowerCase(),

Make sure this has test coverage.

@@ +416,5 @@
> +      ext: options.ext
> +    };
> +
> +    let algo;
> +    if (credentials.algorithm == "sha1")

Move this block right to the front -- fail early without generating bytes etc.

@@ +418,5 @@
> +
> +    let algo;
> +    if (credentials.algorithm == "sha1")
> +      algo = Ci.nsICryptoHMAC.SHA1;
> +    else if (credentials.algorithm == "sha256")

Always {
} else {
}

@@ +425,5 @@
> +      throw new Error("Unsupported algorithm: " + credentials.algorithm);
> +
> +    function hupdate(hasher, str) {
> +      hasher.update(str, str.length);
> +    }

This function isn't used.

@@ +429,5 @@
> +    }
> +
> +    let contentType = "";
> +    if (options.contentType)
> +      contentType = options.contentType.split(";")[0].trim().toLowerCase();

.split allocates a new array and two (or more) string references. indexOf and substring are 3x faster (http://jsperf.com/strip-trailing-params).

  contentType = options.contentType || "";
  if (options.contentType) {
    let i = contentType.indexOf(";");
    contentType = contentType.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase();
  }

If that's too messy, at least do:

  contentType.replace(/;.*$/, "");

@@ +433,5 @@
> +      contentType = options.contentType.split(";")[0].trim().toLowerCase();
> +
> +    if (!artifacts.hash && options.hasOwnProperty("payload")) {
> +      let hasher = Cc["@mozilla.org/security/hash;1"]
> +        .createInstance(Ci.nsICryptoHash);

Nit: align . with [

@@ +434,5 @@
> +
> +    if (!artifacts.hash && options.hasOwnProperty("payload")) {
> +      let hasher = Cc["@mozilla.org/security/hash;1"]
> +        .createInstance(Ci.nsICryptoHash);
> +      if (credentials.algorithm == "sha1")

{}

@@ +436,5 @@
> +      let hasher = Cc["@mozilla.org/security/hash;1"]
> +        .createInstance(Ci.nsICryptoHash);
> +      if (credentials.algorithm == "sha1")
> +        hasher.init(hasher.SHA1);
> +      else if (credentials.algorithm == "sha256")

Can we dispatch on algo instead?

@@ +456,5 @@
> +                         artifacts.host         + "\n" +
> +                         artifacts.port         + "\n" +
> +                         artifacts.hash         + "\n");
> +    if (artifacts.ext)
> +      requestString += artifacts.ext.replace("\\", "\\\\").replace("\n", "\\n");

{}

@@ +475,5 @@
> +                  (artifacts.ext ? ('ext="' + escape(artifacts.ext) + '", ') : '') +
> +                  'mac="' + mac_b64 + '"');
> +    return {
> +      artifacts: artifacts,
> +      field: header

Trailing comma.

::: services/crypto/tests/unit/test_utils_hawk.js
@@ +11,5 @@
> +  run_next_test();
> +}
> +
> +add_test(function test_hawk() {
> +  _("Ensure HAWK MAC generation works as expected.");

Don't really need this; seeing the test output show "test_hawk" should be enough, I think.

@@ +16,5 @@
> +
> +  let compute = CryptoUtils.computeHAWKHeader;
> +
> +  // vectors copied from the HAWK (node.js) tests
> +  var credentials_sha1 = {

s/var/let.

@@ +19,5 @@
> +  // vectors copied from the HAWK (node.js) tests
> +  var credentials_sha1 = {
> +    id: '123456',
> +    key: '2983d45yun89q',
> +    algorithm: 'sha1'

General point: we usually include a trailing comma on the last item, because it's syntactically valid and it makes diffs much more pleasant. (Throughout.)

@@ +35,5 @@
> +                                  nonce: nonce,
> +                                  payload: "something to write about"
> +                                 });
> +
> +  // note HAWK spec uses non-urlsafe base64 for its output MAC string

Nit: I tend to prefer full sentences (caps and period) for comments. (Throughout.)

@@ +51,5 @@
> +  // note: artifacts.hash is the *payload* hash, not the overall request MAC
> +  do_check_eq(result.artifacts.hash, "bsvY3IfUllw6V5rvk4tStEvpBhE=");
> +  do_check_eq(result.artifacts.ext, "Bazinga!");
> +
> +  var credentials_sha256 = {

Let.

@@ +58,5 @@
> +    algorithm: 'sha256'
> +  };
> +
> +  let uri2 = CommonUtils.makeURI("https://example.net/somewhere/over/the/rainbow");
> +  result = compute(uri2, method, { credentials: credentials_sha256,

Be consistent with whitespace (cf line 32).

@@ +102,5 @@
> +
> +/* Leaving optional fields out should work, although of course then we can't
> + * assert much about the resulting hashes. The resulting header should look
> + * roughly like:
> + * Hawk id="123456", ts="1378764955", nonce="QkynqsrS44M=", mac="/C5NsoAs2fVn+d/I5wMfwe2Gr1MZyAJ6pFyDHG4Gf9U=" */

Nit: */ on next line.

@@ +108,5 @@
> +  result = compute(uri2, method, { credentials: credentials_sha256 });
> +  let fields = result.field.split(" ");
> +  do_check_eq(fields[0], 'Hawk');
> +  do_check_eq(fields[1], 'id="123456",'); // from creds.id
> +  do_check_eq(fields[2].slice(0,4), 'ts="');

do_check_true(fields[2].startsWith("ts="));

@@ +113,5 @@
> +  /* the HAWK spec calls for seconds-since-epoch, not ms-since-epoch.
> +   * Warning: this test will fail in the year 33658, and for time travellers
> +   * who journey earlier than 2001. Please plan accordingly. */
> +  do_check_true(result.artifacts.ts > 1000*1000*1000);
> +  do_check_true(result.artifacts.ts < 1000*1000*1000*1000);

Generally we always put spaces around arithmetic operators. For unit-bumping like this, I don't care too much, so up to you.

@@ +114,5 @@
> +   * Warning: this test will fail in the year 33658, and for time travellers
> +   * who journey earlier than 2001. Please plan accordingly. */
> +  do_check_true(result.artifacts.ts > 1000*1000*1000);
> +  do_check_true(result.artifacts.ts < 1000*1000*1000*1000);
> +  do_check_eq(fields[3].slice(0,7), 'nonce="');

startsWith.

::: services/crypto/tests/unit/xpcshell.ini
@@ +13,4 @@
>  
>  [test_utils_hkdfExpand.js]
>  [test_utils_httpmac.js]
> +[test_utils_hawk.js]

Alphabetical order, plz.
Attachment #801865 - Flags: feedback?(rnewman) → feedback+
(Assignee)

Comment 3

4 years ago
Wow, thanks! This feedback is great.

> Kill the function name; the debugger will pull it out of the object
> reference (and in any case, they disagree!).

Will do. Should I strip them from all the pre-existing functions too
(consistency), or just from the ones I added (minimal patch)?

> > +    let bytes = [b.charCodeAt() for each (b in message)];
> 
> This doesn't seem particularly safe in the presence of non-ASCII:

> > +   *           payload - (string) payload to include in hash. ENCODING?
> 
> UTF-8.

These touch on a deeper issue. The caller gets to chose an arbitrary payload,
which can include arbitrary bytes (most higher-level protocols bend over
backwards to avoid that, with base64/etc, but I can't enforce that at this
layer). My sample case is a caller who sends a hash or some encrypted bytes
in their HTTP payload. And JS doesn't provide a single obvious way to manage
arbitrary bytes (I've seen array-of-small-numbers,
String-of-code-points-0x00-to-0xff, and more recently TypedArray/ArrayBuffer
objects).

So we need a contract for how that payload is given to computeHAWK() and then
hashed. I'm uncomfortable with having the caller pass down (unicode string,
encoding), because then the data gets encoded twice (once by computeHAWK()
before it feeds the hasher, once by the caller when they give it to the HTTP
client code). That makes it possible to do it wrong (if the encoders differ,
the MAC won't match, but only when you have non-ASCII in the payload, which
delays discovery and makes the bug more expensive).

So I prefer the API contract to call for computeHAWK(payload=bytes). Given
that, what's the best hacked-up JS-bytestring workaround to use? I went with
String-of-0x00-0xff-code-points, since the digestBytes() function already
existed and suggested it wasn't too crazy. But utils.js only uses that for
HKDF and PBKDF2, and doesn't seem to have any other functions that take HTTP
payloads, so that guidance is weak at best.

What does HTTP client code (XHR?) in this environment take?


> > +   *           contentType - (string) payload Content-Type
> 
> Caller must include charset?

Nope, but it (and any other attributes) will be stripped off. The contentType
is covered by the HMAC, but isn't used for anything else (in particular, it
doesn't affect interpretation of the payload, see above).

> And probably
> 
>   computeHAWKHeaderValues
> 
> 'cos you're not returning a string header, right?

True. I think I'll use "computeHAWK", to match "computeHTTPMACSHA1".

> Another thing to consider: for testability, it's often convenient to be able
> to impose an instant in time. Think about whether you want to add
> 
>   , now=Date.now()
> 
> to the function signature.

In addition to ts= ? Yeah, I guess that makes sense, to also test the offset
computation. Will do.

I still need to add tests for localtimeOffsetMsec.

> > +      ts: ts,
> > +      nonce: options.nonce || btoa(CryptoUtils.generateRandomBytes(8)),
> > +      method: method.toUpperCase(),
> > +      resource: uri.path, // TODO: include .search ?
> > +      host: uri.asciiHost.toLowerCase(),
> 
> Make sure this has test coverage.

Coverage added for .method and .host, and already there for .nonce . I'm
still working on .search (the node.js library's tests didn't cover that, so I
need to run some experiments to get the vectors).

> > +    let contentType = "";
> > +    if (options.contentType)
> > +      contentType = options.contentType.split(";")[0].trim().toLowerCase();
> 
> .split allocates a new array and two (or more) string references. indexOf
> and substring are 3x faster (http://jsperf.com/strip-trailing-params).

>   contentType = options.contentType || "";
>   if (options.contentType) {
>     let i = contentType.indexOf(";");
>     contentType = contentType.substring(0, (i >= 0) ? i :
> undefined).trim().toLowerCase();
>   }

How about this?:

    let contentType = options.contentType || "";
    let i = contentType.indexOf(";");
    contentType = contentType.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase();

(benefits: less messy, removes "if" statement from main path. problems:
small-named "i" lives in a larger scope than really necessary, executes
indexOf and substring even if options.contentType is undefined or "")

> > +      if (credentials.algorithm == "sha1")
> > +        hasher.init(hasher.SHA1);
> 
> Can we dispatch on algo instead?

Do you mean if (algo === Ci.nsICryptoHMAC.SHA1) ? I don't know if the
nsICryptoHash instance's .SHA1 property is the same as the nsICryptoHMAC.SHA1
property.

> ::: services/crypto/tests/unit/test_utils_hawk.js

> General point: we usually include a trailing comma on the last item,
> because it's syntactically valid and it makes diffs much more pleasant.
> (Throughout.)

Ah, that's nice. This is a mozilla-js -specific feature, right? I remember
web-page JS (and/or JSON parsers) rejecting those.

> do_check_true(fields[2].startsWith("ts="));

We have startsWith here? How civilized! :)

> > +  do_check_true(result.artifacts.ts < 1000*1000*1000*1000);
> 
> Generally we always put spaces around arithmetic operators. For
> unit-bumping like this, I don't care too much, so up to you.

Yeah, for this particular case, I think I prefer the compact form. Inserting
spaces would draw attention away from the comparison.

> ::: services/crypto/tests/unit/xpcshell.ini
> > +[test_utils_hawk.js]
> 
> Alphabetical order, plz.

Am I correct in assuming that the "skip-if" on line 12 is attached to the
[test_crypto_random.js] that precedes it? So my hawk test will still run on
android if I use?:

[test_crypto_random.js]
# Bug 676977: test hangs consistently on Android
skip-if = os == "android"

[test_utils_hawk.js]
[test_utils_hkdfExpand.js]
(Assignee)

Comment 4

4 years ago
Created attachment 802584 [details] [diff] [review]
second try at patch

This covers most (but not all) of rnewman's feedback. I'll post a third version later today with more improvements.
Attachment #801865 - Attachment is obsolete: true
(In reply to Brian Warner [:warner :bwarner] from comment #3)

> Will do. Should I strip them from all the pre-existing functions too
> (consistency), or just from the ones I added (minimal patch)?

Usually if I'm breaking nearby blame, I'll fix it, but generally err on the side of minimal.


> These touch on a deeper issue.

Yup, this is a sucky situation. If only we had HttpClient's Entity classes!

> 
> So we need a contract for how that payload is given to computeHAWK() and then
> hashed. I'm uncomfortable with having the caller pass down (unicode string,
> encoding), because then the data gets encoded twice (once by computeHAWK()
> before it feeds the hasher, once by the caller when they give it to the HTTP
> client code). That makes it possible to do it wrong (if the encoders differ,
> the MAC won't match, but only when you have non-ASCII in the payload, which
> delays discovery and makes the bug more expensive).
> 
> So I prefer the API contract to call for computeHAWK(payload=bytes). Given
> that, what's the best hacked-up JS-bytestring workaround to use? I went with
> String-of-0x00-0xff-code-points, since the digestBytes() function already
> existed and suggested it wasn't too crazy. But utils.js only uses that for
> HKDF and PBKDF2, and doesn't seem to have any other functions that take HTTP
> payloads, so that guidance is weak at best.
> 
> What does HTTP client code (XHR?) in this environment take?

Concerns we have here:

* Following existing code.
* Perf.
* Flexibility.

I suspect that the common case here will be "I have a small JS string, probably a JSON serialization of a compact object", so (a) let's decide whether that's true, and (b) let's bear that in mind when we think about perf.

Existing code here is twofold: there's the codebase you're in (which takes string input and does UTF-8 fiddling, as I mentioned in my feedback), and there's XHR, which goes for flexibility but is much more complicated:

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#send%28%29

If we go the obvious ArrayBuffer approach, we need to not only have the caller do the right thing prepping and retaining that buffer, but also we must make sure that downstream code is prepared to send that over the wire. (I doubt it is already.)

And doing that work when we'll only be hitting this code path for small JSON strings might be premature optimization. So I'd aim for getting this working with JSON strings first, and see where we go from there.


> > > +      host: uri.asciiHost.toLowerCase(),
> > 
> > Make sure this has test coverage.
> 
> Coverage added for .method and .host, and already there for .nonce .

I meant the asciiHost part. Make your hostname 例子.測試 and make sure this does the right thing!

(http://idn.icann.org/#Things_to_test)


> (benefits: less messy, removes "if" statement from main path. problems:
> small-named "i" lives in a larger scope than really necessary, executes
> indexOf and substring even if options.contentType is undefined or "")

*shrug*


> Ah, that's nice. This is a mozilla-js -specific feature, right? I remember
> web-page JS (and/or JSON parsers) rejecting those.

Basically IE drools in the back of the bus. Current spec includes them.

http://stackoverflow.com/questions/7246618/trailing-commas-in-javascript


> > do_check_true(fields[2].startsWith("ts="));
> 
> We have startsWith here? How civilized! :)

You wait until you get to maps, sets, and fat arrows!


> Am I correct in assuming that the "skip-if" on line 12 is attached to the
> [test_crypto_random.js] that precedes it? So my hawk test will still run on
> android if I use?:

skip-if applies to the previous entry.

https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests#Adding_conditions_to_a_test
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Created attachment 802650 [details] [diff] [review]
third try

Relative to the previous patch, this adds documentation for the (lack of) payload encoding, tests and fixes payload=undefined, and adds tests for "now", "localtimeOffsetMsec", search/queryargs, payload hashing (with binary strings), and using "hash" to override the payload hash. It was written before rnewman's latest comment, so probably doesn't address anything therein.
Attachment #802584 - Attachment is obsolete: true
bwarner: N.B., I'm waiting for an updated patch before I give this another run through, in the expectation that you omitted the flags for that reason :)
(Assignee)

Comment 8

4 years ago
Created attachment 803226 [details] [diff] [review]
implement HAWK header creation

Ok, I think this should address everything.
Attachment #802650 - Attachment is obsolete: true
Attachment #803226 - Flags: review?(rnewman)
(Assignee)

Comment 9

4 years ago
Created attachment 803248 [details] [diff] [review]
implement HAWK header creation

found one more test I could add.. now it's ready for review.
Attachment #803226 - Attachment is obsolete: true
Attachment #803226 - Flags: review?(rnewman)
Attachment #803248 - Flags: review?(rnewman)
Comment on attachment 803248 [details] [diff] [review]
implement HAWK header creation

Review of attachment 803248 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/crypto/modules/utils.js
@@ +425,5 @@
> +    }
> +
> +    let port;
> +    if (uri.port != -1) {
> +      port = uri.port;

This will be a `long`, not a string. Make the others numbers to match?

@@ +447,5 @@
> +    };
> +
> +    let contentType = options.contentType || "";
> +    let i = contentType.indexOf(";");
> +    contentType = contentType.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase();

On reflection, split this out as a standalone method ("stripParameters"?) so we can test it.

@@ +483,5 @@
> +    // The output MAC uses '+' and '/', and padded== .
> +
> +    function escape(attribute) {
> +      // This is used for "x=y" attributes inside HTTP headers.
> +      return attribute.replace(/\\/g, '\\\\').replace(/\"/g, '\\"');

N.B., for future reference: we use double quotes for all strings, excepting those that contain lots of double quotes.

::: services/crypto/tests/unit/test_utils_hawk.js
@@ +242,5 @@
> +
> +  run_next_test();
> +});
> +
> +/*

No commented-out tests, plz.
Attachment #803248 - Flags: review?(rnewman) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 803840 [details] [diff] [review]
6db05f8.diff

Incorporated latest feedback: double-quotes, integer portnum, split out stripHeaderAttributes() and added tests.

One quirk: the node.js HAWK library (specifically node's URL parser) appears to leave the port as a string, but our parser gets an integer. So node.js will treat http://example.net:012345/ differently than http://example.net:12345/ , but we'll treat them identically. I think we can argue that HAWK should settle on integer ports.
Attachment #803248 - Attachment is obsolete: true
Attachment #803840 - Flags: review?(rnewman)
Attachment #803840 - Flags: review?(rnewman) → review+
(Assignee)

Comment 12

4 years ago
Landed, in http://hg.mozilla.org/projects/elm/rev/3ff599ec9e3c . Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
oops, closed this prematurely, it's only landed to the "elm" branch, not to m-c, and also awaits qa+.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Marking [qa-], 'cos there's nothing here that a human being can test…
Status: REOPENED → ASSIGNED
Whiteboard: [qa+] → [qa-][fixed in elm]
Not even an automated human being?
;-)

(fine with me. one less qa+)
(Reporter)

Updated

4 years ago
Blocks: 911378
No longer blocks: 907420
(In reply to Richard Newman [:rnewman] from comment #14)
> Marking [qa-], 'cos there's nothing here that a human being can test…

How can we land this on m-c? This would be useful for FxAccts on Firefox OS.
Flags: needinfo?(rnewman)
:rnewman
bump

See Comment 16
(In reply to James Bonacci [:jbonacci] from comment #17)
> :rnewman
> bump
> 
> See Comment 16

Hey, I was away for three days! :D


(In reply to Zachary Carter [:zaach] from comment #16)
> How can we land this on m-c? This would be useful for FxAccts on Firefox OS.

If you're happy with the level of security oversight that this patch has received -- i.e., does it need a brief sec code review? -- then just make sure the tests are passing on Elm, and land it.

I don't know if you need to land on another branch for FxOS; one of those folks will need to chime in for that.
Flags: needinfo?(rnewman)
Blocks: 930553
No longer depends on: 930553
Blocks: 930598
(Assignee)

Comment 19

4 years ago
Created attachment 824786 [details] [diff] [review]
undefined-payload.diff

small extra patch to tolerate options={payload:undefined}

Comment 20

4 years ago
Created attachment 824918 [details] [diff] [review]
911384-test-empty-payload.patch

This patch adds a little test to :warner 's previous enabling of present-but-empty payload field in the computeHawk's options parameter. We would like to land these three patches in mozilla-central.
Attachment #824918 - Flags: review?(rnewman)
Comment on attachment 824918 [details] [diff] [review]
911384-test-empty-payload.patch

Review of attachment 824918 [details] [diff] [review]:
-----------------------------------------------------------------

Repetitive, but I doubt these'll be changing much.

Don't forget to set a commit message!
Attachment #824918 - Flags: review?(rnewman) → review+
Comment on attachment 824786 [details] [diff] [review]
undefined-payload.diff

Review of attachment 824786 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/crypto/modules/utils.js
@@ +489,5 @@
>  
>      let contentType = CryptoUtils.stripHeaderAttributes(options.contentType);
>  
> +    if (!artifacts.hash && options.hasOwnProperty("payload")
> +        && options.payload) {

if (!... &&
      ... &&
      ...) {
Attachment #824786 - Flags: review+

Comment 23

4 years ago
Created attachment 824968 [details] [diff] [review]
911384-test-empty-payload.patch

Add test for empty options.payload parameter to computeHawk; this time with commit message and whitespace cleanup.
Attachment #824918 - Attachment is obsolete: true

Comment 24

4 years ago
Created attachment 824975 [details] [diff] [review]
911384-computeHawk-options-payload-undefined.patch

Whitespace tweak to  "824786: undefined-payload.diff" per rnewman feedback.
Attachment #824786 - Attachment is obsolete: true
Created attachment 825000 [details] [diff] [review]
Follow-up, consolidated.

Will check these in as soon as trees reopen. (Bug 932781.)
Attachment #824968 - Attachment is obsolete: true
Attachment #824975 - Attachment is obsolete: true
Attachment #825000 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/30ad8d901361
https://hg.mozilla.org/mozilla-central/rev/b7bfd927abc0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Cleaning up Resolved/Fixed bugs from December's first release.
Verified that we now have a working first-release of FxA to Desktop/Android Nightly.
Re-open as needed.
Status: RESOLVED → VERIFIED
Blocks: 1055643
You need to log in before you can comment on or make changes to this bug.